Re: [PATCH] ieee1275: obdisk driver

[Daniel Kiper]

On Thu, Apr 05, 2018 at 10:53:55AM -0700, Eric Snowberg wrote:
> Add a new disk driver called obdisk for IEEE1275 platforms.  Currently
> the only platform using this disk driver is SPARC, however other IEEE1275
> platforms could start using it if they so choose.  While the functionality
> within the other IEEE1275 ofdisk driver may be suitable for PPC and x86, it

s/other/current/?

> presented too many problems on SPARC hardware.
>
> Within the old ofdisk, there is not a way to determine the true canonical
> name for the disk.  Within Open Boot, the same disk can have multiple names
> but all reference the same disk.  For example the same disk can be referenced
> by its SAS WWN, using this form:
>
> /address@hidden/address@hidden/address@hidden/address@hidden/LSI,address@hidden/address@hidden,0
>
> It can also be referenced by its PHY identifier using this form:
>
> /address@hidden/address@hidden/address@hidden/address@hidden/LSI,address@hidden/address@hidden
>
> It can also be referenced by its Target identifier using this form:
>
> /address@hidden/address@hidden/address@hidden/address@hidden/LSI,address@hidden/address@hidden
>
> Also, when the LUN=0, it is legal to omit the ,0 from the device name.  So 
> with
> the disk above, before taking into account the device aliases, there are 6 
> ways
> to reference the same disk.
>
> Then it is possible to have 0 .. n device aliases all representing the same 
> disk.
> Within this new driver the true canonical name is determined using the the
> IEEE1275 encode-unit and decode-unit commands when address_cells == 4.  This
> will determine the true single canonical name for the device so multiple 
> ihandles
> are not opened for the same device.  This is what frequently happens with the 
> old
> ofdisk driver.  With some devices when they are opened multiple times it 
> causes
> the entire system to hang.
>
> Another problem solved with this driver is devices that do not have a device
> alias can be booted and used within GRUB. Within the old ofdisk, this was not
> possible, unless it was the original boot device.  All devices behind a SAS
> or SCSI parent can be found.   Within the old ofdisk, finding these disks
> relied on there being an alias defined.  The alias requirement is not
> necessary with this new driver.  It can also find devices behind a parent
> after they have been hot-plugged.  This is something that is not possible
> with the old ofdisk driver.
>
> The old ofdisk driver also incorrectly assumes that the device pointing to by 
> a
> device alias is in its true canonical form. This assumption is never made with
> this new driver.
>
> Another issue solved with this driver is that it properly caches the ihandle
> for all open devices.  The old ofdisk tries to do this by caching the last
> opened ihandle.  However this does not work properly because the layer above
> does not use a consistent device name for the same disk when calling into the
> driver.  This is because the upper layer uses the bootpath value returned 
> within
> /chosen, other times it uses the device alias, and other times it uses the
> value within grub.cfg.  It does not have a way to figure out that these 
> devices
> are the same disk.  This is not a problem with this new driver.
>
> Due to the way GRUB repeatedly opens and closes the same disk. Caching the
> ihandle is important on SPARC.  Without caching, some SAS devices can take
> 15 - 20 minutes to get to the GRUB menu. This ihandle caching is not possible
> without correctly having the canonical disk name.
>
> When available, this driver also tries to use the deblocker #blocks and
> a way of determining the disk size.
>
> Finally and probably most importantly, this new driver is also capable of
> seeing all partitions on a GPT disk.  With the old driver, the GPT
> partition table can not be read and only the first partition on the disk
> can be seen.
>
> Signed-off-by: Eric Snowberg <address@hidden>
> ---
>  grub-core/Makefile.core.def      |    1 +
>  grub-core/commands/nativedisk.c  |    1 +
>  grub-core/disk/ieee1275/obdisk.c | 1093 
> ++++++++++++++++++++++++++++++++++++++
>  grub-core/kern/ieee1275/cmain.c  |    3 +
>  grub-core/kern/ieee1275/init.c   |   12 +-
>  include/grub/disk.h              |    1 +
>  include/grub/ieee1275/ieee1275.h |    2 +
>  include/grub/ieee1275/obdisk.h   |   25 +
>  8 files changed, 1137 insertions(+), 1 deletions(-)
>  create mode 100644 grub-core/disk/ieee1275/obdisk.c
>  create mode 100644 include/grub/ieee1275/obdisk.h
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 2c1d62c..14471c0 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -292,6 +292,7 @@ kernel = {
>    sparc64_ieee1275 = kern/sparc64/cache.S;
>    sparc64_ieee1275 = kern/sparc64/dl.c;
>    sparc64_ieee1275 = kern/sparc64/ieee1275/ieee1275.c;
> +  sparc64_ieee1275 = disk/ieee1275/obdisk.c;
>
>    arm = kern/arm/dl.c;
>    arm = kern/arm/dl_helper.c;
> diff --git a/grub-core/commands/nativedisk.c b/grub-core/commands/nativedisk.c
> index 2f56a87..2f2315d 100644
> --- a/grub-core/commands/nativedisk.c
> +++ b/grub-core/commands/nativedisk.c
> @@ -66,6 +66,7 @@ get_uuid (const char *name, char **uuid, int getnative)
>        /* Firmware disks.  */
>      case GRUB_DISK_DEVICE_BIOSDISK_ID:
>      case GRUB_DISK_DEVICE_OFDISK_ID:
> +    case GRUB_DISK_DEVICE_OBDISK_ID:
>      case GRUB_DISK_DEVICE_EFIDISK_ID:
>      case GRUB_DISK_DEVICE_NAND_ID:
>      case GRUB_DISK_DEVICE_ARCDISK_ID:
> diff --git a/grub-core/disk/ieee1275/obdisk.c 
> b/grub-core/disk/ieee1275/obdisk.c
> new file mode 100644
> index 0000000..1ebf237
> --- /dev/null
> +++ b/grub-core/disk/ieee1275/obdisk.c
> @@ -0,0 +1,1093 @@
> +/* obdisk.c - Open Boot disk access.  */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2018 Free Software Foundation, Inc.
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/disk.h>
> +#include <grub/env.h>
> +#include <grub/i18n.h>
> +#include <grub/kernel.h>
> +#include <grub/list.h>
> +#include <grub/misc.h>
> +#include <grub/mm.h>
> +#include <grub/scsicmd.h>
> +#include <grub/time.h>
> +#include <grub/ieee1275/ieee1275.h>
> +#include <grub/ieee1275/obdisk.h>
> +
> +struct disk_dev
> +{
> +  struct disk_dev *next;
> +  struct disk_dev **prev;
> +  /* open boot canonical name */
> +  char *name;
> +  /* open boot raw disk name to access entire disk */
> +  char *raw_name;
> +  /* grub encoded device name */
> +  char *grub_devpath;
> +  /* grub encoded alias name */
> +  char *grub_alias_devpath;
> +  /* grub unescaped name */
> +  char *grub_decoded_devpath;
> +  grub_ieee1275_ihandle_t ihandle;
> +  grub_uint32_t block_size;
> +  grub_uint64_t num_blocks;
> +  unsigned int log_sector_size;
> +  grub_uint32_t opened;
> +  grub_uint32_t valid;
> +  grub_uint32_t boot_dev;
> +};
> +
> +struct parent_dev
> +{
> +  struct parent_dev *next;
> +  struct parent_dev **prev;
> +  /* canonical parent name */
> +  char *name;
> +  char *type;
> +  grub_ieee1275_ihandle_t ihandle;
> +  grub_uint32_t address_cells;
> +};

Could you align all members names in one column for these structures?

> +static struct grub_scsi_test_unit_ready tur =
> +{
> +  .opcode = grub_scsi_cmd_test_unit_ready,
> +  .lun = 0,
> +  .reserved1 = 0,
> +  .reserved2 = 0,
> +  .reserved3 = 0,
> +  .control = 0,
> +};
> +
> +static int disks_enumerated = 0;
> +static struct disk_dev *disk_devs = NULL;
> +static struct parent_dev *parent_devs = NULL;
> +
> +static const char *block_blacklist[] = {
> +  /* Requires addition work in grub before being able to be used. */
> +  "/iscsi-hba",
> +  /* This block device should never be used by grub. */
> +  "/address@hidden",
> +  0
> +};

I do not think that you need to set values to 0/NULL above.
Compiler should do work for you.

> +#define STRCMP(a, b) ((a) && (b) && (grub_strcmp (a, b) == 0))
> +
> +static char *
> +strip_ob_partition (char *path)
> +{
> +  char *sptr;
> +
> +  sptr = grub_strstr (path, ":");
> +
> +  if (sptr)
> +    *sptr = '\0';
> +
> +  return path;
> +}
> +
> +static char *
> +remove_escaped_commas (char *src)

s/remove/replace/?

Hmmm... Why do not really remove them?

> +{
> +  char *iptr;
> +
> +  if (src == NULL)
> +    return NULL;
> +  for (iptr = src; *iptr; )
> +    {
> +      if ((*iptr == '\\') && (*(iptr + 1) == ','))
> +        {
> +          *iptr++ = '_';
> +          *iptr++ = '_';
> +        }
> +      iptr++;
> +    }
> +
> +  return src;
> +}
> +
> +static int
> +count_commas (const char *src)
> +{
> +  int count = 0;
> +
> +  for ( ; *src; src++)
> +    if (*src == ',')
> +      count++;
> +
> +  return count;
> +}
> +
> +static void
> +escape_commas (const char *src, char *dest)
> +{
> +  const char *iptr;
> +
> +  for (iptr = src; *iptr; )
> +    {
> +      if (*iptr == ',')
> +     *dest++ ='\\';
> +
> +      *dest++ = *iptr++;
> +    }
> +
> +  *dest = '\0';
> +}
> +
> +static char *
> +decode_grub_devname (const char *name)
> +{
> +  char *devpath = grub_malloc (grub_strlen (name) + 1);
> +  char *p, c;
> +
> +  if (!devpath)

Please be consistent. Use !devpath or devpath != NULL in entire file.
Same applies to comparison with 0.

> +    return NULL;
> +
> +  /* Un-escape commas. */

Is not it related to the issue which you have tried to
fix by the patch for shell parser?

> +  p = devpath;
> +  while ((c = *name++) != '\0')
> +    {
> +      if (c == '\\' && *name == ',')
> +     {
> +       *p++ = ',';
> +       name++;
> +     }
> +      else
> +     *p++ = c;
> +    }
> +
> +  *p++ = '\0';
> +
> +  return devpath;
> +}
> +
> +static char *
> +encode_grub_devname (const char *path)
> +{
> +  char *encoding, *optr;
> +
> +  if (path == NULL)

Yes, like this one please. If you like it/it is in line with other files of 
course...

> +    return NULL;
> +
> +  encoding = grub_malloc (sizeof ("ieee1275/") + count_commas (path) +
> +                          grub_strlen (path) + 1);
> +
> +  if (encoding == NULL)

Ditto.

> +    {
> +      grub_print_error ();
> +      return NULL;
> +    }
> +
> +  optr = grub_stpcpy (encoding, "ieee1275/");

You use "ieee1275/" in various places. Please define it as a constant.

> +  escape_commas (path, optr);
> +  return encoding;
> +}
> +
> +static char *
> +get_parent_devname (const char *devname)
> +{
> +  char *parent, *pptr;
> +
> +  parent = grub_strdup (devname);
> +
> +  if (parent == NULL)
> +    {
> +      grub_print_error ();
> +      return NULL;
> +    }
> +
> +  pptr = grub_strstr (parent, "/disk@");

A constant please...

> +
> +  if (pptr)
> +    *pptr = '\0';
> +
> +  return parent;
> +}
> +
> +static void
> +free_parent_dev (struct parent_dev *parent)
> +{
> +  if (parent)
> +    {
> +      grub_free (parent->name);
> +      grub_free (parent->type);
> +      grub_free (parent);
> +    }
> +}
> +
> +static struct parent_dev *
> +init_parent (const char *parent)
> +{
> +  struct parent_dev *op;
> +
> +  op = grub_zalloc (sizeof (struct parent_dev));
> +
> +  if (op == NULL)
> +    {
> +      grub_print_error ();
> +      return NULL;
> +    }
> +
> +  op->name = grub_strdup (parent);
> +  op->type = grub_malloc (IEEE1275_MAX_PROP_LEN);
> +
> +  if ((op->name == NULL) || (op->type == NULL))
> +    {
> +      grub_print_error ();
> +      free_parent_dev (op);
> +      return NULL;
> +    }
> +
> +  return op;
> +}
> +
> +static struct parent_dev *
> +open_new_parent (const char *parent)
> +{
> +  struct parent_dev *op = init_parent(parent);
> +  grub_ieee1275_ihandle_t ihandle;
> +  grub_ieee1275_phandle_t phandle;
> +  grub_uint32_t address_cells = 2;
> +  grub_ssize_t actual = 0;
> +
> +  if (op == NULL)
> +    return NULL;
> +
> +  grub_ieee1275_open (parent, &ihandle);
> +
> +  if (ihandle == 0)
> +    {
> +      grub_error (GRUB_ERR_BAD_DEVICE, "unable to open %s", parent);
> +      grub_print_error ();
> +      free_parent_dev (op);
> +      return NULL;
> +    }
> +
> +  if (grub_ieee1275_instance_to_package (ihandle, &phandle))
> +    {
> +      grub_error (GRUB_ERR_BAD_DEVICE, "unable to get parent %s", parent);
> +      grub_print_error ();
> +      free_parent_dev (op);
> +      return NULL;
> +    }
> +
> +  /* IEEE Std 1275-1994 page 110: A missing "address-cells" property
> +     signifies that the number of address cells is two. So ignore on error. 
> */
> +  grub_ieee1275_get_integer_property (phandle, "#address-cells", 
> &address_cells,
> +                                      sizeof (address_cells), 0);
> +
> +  grub_ieee1275_get_property (phandle, "device_type", op->type,
> +                              IEEE1275_MAX_PROP_LEN, &actual);
> +  op->ihandle = ihandle;
> +  op->address_cells = address_cells;
> +  return op;
> +}
> +
> +static struct parent_dev *
> +open_parent (const char *parent)
> +{
> +  struct parent_dev *op;
> +
> +  if ((op =
> +       grub_named_list_find (GRUB_AS_NAMED_LIST (parent_devs), parent)) == 
> NULL)

Oh no, please call grub_named_list_find() in separate line and then check op 
value.

> +  {
> +     op = open_new_parent (parent);
> +
> +    if (op)
> +      grub_list_push (GRUB_AS_LIST_P (&parent_devs), GRUB_AS_LIST (op));

Something is wrong with alignment.

> +  }
> +
> +  return op;
> +}
> +
> +static void
> +display_parents (void)
> +{
> +  struct parent_dev *parent;
> +
> +  grub_printf ("-------------------- PARENTS --------------------\n");
> +
> +  FOR_LIST_ELEMENTS (parent, parent_devs)
> +    {
> +      grub_printf ("name: %s\n", parent->name);
> +      grub_printf ("type: %s\n", parent->type);
> +      grub_printf ("address_cells %x\n", parent->address_cells);

Values in one column?

> +    }
> +
> +  grub_printf ("-------------------------------------------------\n");
> +}
> +
> +static char *
> +canonicalise_4cell_ua (grub_ieee1275_ihandle_t ihandle, char *unit_address)
> +{
> +  grub_uint32_t phy_lo, phy_hi, lun_lo, lun_hi;
> +  int valid_phy = 0;
> +  grub_size_t size;
> +  char *canon = NULL;
> +
> +  valid_phy = grub_ieee1275_decode_unit4 (ihandle, unit_address,
> +                                          grub_strlen (unit_address), 
> &phy_lo,
> +                                          &phy_hi, &lun_lo, &lun_hi);
> +
> +  if ((!valid_phy) && (phy_hi != 0xffffffff))

valid_phy == 0

> +    canon = grub_ieee1275_encode_uint4 (ihandle, phy_lo, phy_hi,
> +                                        lun_lo, lun_hi, &size);
> +
> +  return canon;
> +}
> +
> +static char *
> +canonicalise_disk (const char *devname)
> +{
> +  char *canon, *parent;
> +  struct parent_dev *op;
> +
> +  canon = grub_ieee1275_canonicalise_devname (devname);
> +
> +  if (canon == NULL)
> +    {
> +      /* This should not happen. */
> +      grub_error (GRUB_ERR_BAD_DEVICE, "canonicalise devname failed");
> +      grub_print_error ();
> +      return NULL;
> +    }
> +
> +  /* Don't try to open the parent of a virtual device. */
> +  if (grub_strstr (canon, "virtual-devices"))
> +    return canon;
> +
> +  parent = get_parent_devname (canon);
> +
> +  if (parent == NULL)
> +    return NULL;
> +
> +  op = open_parent (parent);
> +
> +  /* Devices with 4 address cells can have many different types of addressing
> +     (phy, wwn, and target lun). Use the parents encode-unit / decode-unit
> +     to find the true canonical name. */
> +  if ((op) && (op->address_cells == 4))
> +    {
> +      char *unit_address, *real_unit_address, *real_canon;
> +
> +      unit_address = grub_strstr (canon, "/disk@");
> +      unit_address += grub_strlen ("/disk@");
> +
> +      if (unit_address == NULL)
> +        {
> +          /* This should not be possible, but return the canonical name for
> +             the non-disk block device. */
> +          grub_free (parent);
> +          return (canon);
> +        }
> +
> +      real_unit_address = canonicalise_4cell_ua (op->ihandle, unit_address);
> +
> +      if (real_unit_address == NULL)
> +        {
> +          /* This is not an error, since this function could be called with 
> a devalias
> +             containing a drive that isn't installed in the system. */
> +          grub_free (parent);
> +          return NULL;
> +        }
> +
> +      real_canon = grub_malloc (grub_strlen (op->name) + sizeof ("/disk@") +
> +                                grub_strlen (real_unit_address));

Please calculate and assign the size in separate line and use it in
grub_malloc() and grub_snprintf().

> +
> +      grub_snprintf (real_canon, grub_strlen (op->name) + sizeof ("/disk@") +
> +                     grub_strlen (real_unit_address), "%s/address@hidden",
> +                     op->name, real_unit_address);
> +
> +      grub_free (canon);
> +      canon = real_canon;
> +    }
> +
> +  grub_free (parent);
> +  return (canon);
> +}
> +
> +static struct disk_dev *
> +add_canon_disk (const char *cname)
> +{
> +  struct disk_dev *dev;
> +
> +  dev = grub_zalloc (sizeof (struct disk_dev));
> +
> +  if (!dev)
> +    goto failed;
> +
> +  if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_RAW_DEVNAMES))
> +    {
> +    /* Append :nolabel to the end of all SPARC disks.
> +       nolabel is mutually exclusive with all other
> +       arguments and allows a client program to open
> +       the entire (raw) disk. Any disk label is ignored. */
> +
> +      dev->raw_name = grub_malloc (grub_strlen (cname) + sizeof 
> (":nolabel"));
> +
> +      if (dev->raw_name == NULL)
> +        goto failed;
> +
> +      grub_snprintf (dev->raw_name, grub_strlen (cname) + sizeof 
> (":nolabel"),
> +                     "%s:nolabel", cname);
> +    }
> +
> +  /* Don't use grub_ieee1275_encode_devname here, the devpath in grub.cfg 
> doesn't
> +     understand device aliases, which the layer above sometimes sends us. */
> +  dev->grub_devpath = encode_grub_devname(cname);
> +
> +  if (dev->grub_devpath == NULL)
> +    goto failed;
> +
> +  dev->name = grub_strdup (cname);
> +
> +  if (dev->name == NULL)
> +    goto failed;
> +
> +  dev->valid = 1;
> +  grub_list_push (GRUB_AS_LIST_P (&disk_devs), GRUB_AS_LIST (dev));
> +  return dev;
> +
> +failed:

Use one space before each label.

> +  grub_print_error ();
> +
> +  if (dev)
> +    {
> +      grub_free (dev->name);
> +      grub_free (dev->grub_devpath);
> +      grub_free (dev->raw_name);
> +    }
> +
> +  grub_free (dev);
> +  return NULL;
> +}
> +
> +static grub_err_t
> +add_disk (const char *path)
> +{
> +  grub_err_t rval = GRUB_ERR_NONE;
> +  struct disk_dev *dev;
> +  char *canon;
> +
> +  canon = canonicalise_disk (path);
> +  dev = grub_named_list_find (GRUB_AS_NAMED_LIST (disk_devs), canon);
> +
> +  if ((canon != NULL) && (dev == NULL))
> +    {
> +      struct disk_dev *ob_device;
> +
> +      ob_device = add_canon_disk (canon);
> +
> +      if (ob_device == NULL)
> +        rval = grub_error (GRUB_ERR_OUT_OF_MEMORY, "failure to add disk");
> +    }
> +  else if (dev)
> +    dev->valid = 1;
> +
> +  grub_free (canon);
> +  return (rval);
> +}
> +
> +static grub_err_t
> +grub_obdisk_read (grub_disk_t disk, grub_disk_addr_t sector,
> +               grub_size_t size, char *dest)
> +{
> +  grub_err_t rval = GRUB_ERR_NONE;

I would use ret instead of rval here and there.

> +  struct disk_dev *dev;
> +  unsigned long long pos;
> +  grub_ssize_t result = 0;
> +
> +  if (disk->data == NULL)
> +    return grub_error (GRUB_ERR_BAD_DEVICE, "invalid disk data");
> +
> +  dev = (struct disk_dev *)disk->data;
> +  pos = sector << disk->log_sector_size;
> +  grub_ieee1275_seek (dev->ihandle, pos, &result);
> +
> +  if (result < 0)
> +    {
> +      dev->opened = 0;
> +      return grub_error (GRUB_ERR_READ_ERROR, "seek error, can't seek block 
> %llu",
> +                         (long long) sector);
> +    }
> +
> +  grub_ieee1275_read (dev->ihandle, dest, size << disk->log_sector_size,
> +                      &result);
> +
> +  if (result != (grub_ssize_t) (size  << disk->log_sector_size))

s/  / /

> +    {
> +      dev->opened = 0;
> +      return grub_error (GRUB_ERR_READ_ERROR, N_("failure reading sector 
> 0x%llx "
> +                                                 "from `%s'"),
> +                         (unsigned long long) sector,
> +                         disk->name);
> +    }
> +  return rval;
> +}
> +
> +static void
> +grub_obdisk_close (grub_disk_t disk)

s/grub_obdisk_close/grub_obdisk_clear/?

> +{
> +  disk->data = NULL;
> +  disk->id = 0;
> +  disk->total_sectors = 0;
> +  disk->log_sector_size = 0;

grub_memset (disk, 0, sizeof (*disk));?

> +}
> +
> +static void
> +scan_usb_disk (const char *parent)
> +{
> +  struct parent_dev *op;
> +  grub_ssize_t result;
> +
> +  op = open_parent (parent);
> +
> +  if (op == NULL)
> +    {
> +      grub_error (GRUB_ERR_BAD_DEVICE, "unable to open %s", parent);
> +      grub_print_error ();
> +      return;
> +    }
> +
> +  if ((grub_ieee1275_set_address (op->ihandle, 0, 0) == 0) &&
> +      (grub_ieee1275_no_data_command (op->ihandle, &tur, &result) == 0) &&
> +      (result == 0))
> +    {
> +      char *buf;
> +
> +      buf = grub_malloc (IEEE1275_MAX_PATH_LEN);
> +
> +      if (buf == NULL)
> +        {
> +          grub_error (GRUB_ERR_OUT_OF_MEMORY, "disk scan failure");
> +          grub_print_error ();
> +          return;
> +        }
> +
> +      grub_snprintf (buf, IEEE1275_MAX_PATH_LEN, "%s/address@hidden", 
> parent);
> +      add_disk (buf);
> +      grub_free (buf);
> +    }
> +}
> +
> +static void
> +scan_nvme_disk (const char *path)
> +{
> +  char *buf;
> +
> +  buf = grub_malloc (IEEE1275_MAX_PATH_LEN);
> +
> +  if (buf == NULL)
> +    {
> +      grub_error (GRUB_ERR_OUT_OF_MEMORY, "disk scan failure");
> +      grub_print_error ();
> +      return;
> +    }
> +
> +  grub_snprintf (buf, IEEE1275_MAX_PATH_LEN, "%s/address@hidden", path);
> +  add_disk (buf);
> +  grub_free (buf);
> +}
> +
> +static void
> +scan_sparc_sas_2cell (struct parent_dev *op)
> +{
> +  grub_ssize_t result;
> +  grub_uint8_t tgt;
> +  char *buf;
> +
> +  buf = grub_malloc (IEEE1275_MAX_PATH_LEN);
> +
> +  if (buf == NULL)
> +    {
> +      grub_error (GRUB_ERR_OUT_OF_MEMORY, "disk scan failure");
> +      grub_print_error ();
> +      return;
> +    }
> +
> +  for (tgt = 0; tgt < 0xf; tgt++)
> +    {
> +
> +      if ((grub_ieee1275_set_address(op->ihandle, tgt, 0) == 0) &&
> +          (grub_ieee1275_no_data_command (op->ihandle, &tur, &result) == 0) 
> &&
> +          (result == 0))
> +        {
> +
> +          grub_snprintf (buf, IEEE1275_MAX_PATH_LEN, "%s/address@hidden"
> +                         PRIxGRUB_UINT32_T, op->name, tgt);
> +
> +          add_disk (buf);
> +        }
> +    }
> +}
> +
> +static void
> +scan_sparc_sas_4cell (struct parent_dev *op)
> +{
> +  grub_uint16_t exp;
> +  grub_uint8_t phy;
> +  char *buf;
> +
> +  buf = grub_malloc (IEEE1275_MAX_PATH_LEN);
> +
> +  if (buf == NULL)
> +    {
> +      grub_error (GRUB_ERR_OUT_OF_MEMORY, "disk scan failure");
> +      grub_print_error ();
> +      return;
> +    }
> +
> +  for (exp = 0; exp <= 0x100; exp+=0x100)
> +
> +    for (phy = 0; phy < 0x20; phy++)
> +      {
> +        char *canon = NULL;
> +
> +        grub_snprintf (buf, IEEE1275_MAX_PATH_LEN, "p%" PRIxGRUB_UINT32_T 
> ",0",
> +                       exp | phy);
> +
> +        canon = canonicalise_4cell_ua (op->ihandle, buf);
> +
> +        if (canon)
> +          {
> +            grub_snprintf (buf, IEEE1275_MAX_PATH_LEN, "%s/address@hidden",
> +                           op->name, canon);
> +
> +            add_disk (buf);
> +            grub_free (canon);
> +          }
> +      }
> +
> +  grub_free (buf);
> +}
> +
> +static void
> +scan_sparc_sas_disk (const char *parent)
> +{
> +  struct parent_dev *op;
> +
> +  op = open_parent (parent);
> +
> +  if ((op) && (op->address_cells == 4))
> +    scan_sparc_sas_4cell (op);
> +  else if ((op) && (op->address_cells == 2))
> +    scan_sparc_sas_2cell (op);
> +}
> +
> +static void
> +iterate_devtree (const struct grub_ieee1275_devalias *alias)
> +{
> +  struct grub_ieee1275_devalias child;
> +
> +  if ((grub_strcmp (alias->type, "scsi-2") == 0) ||
> +      (grub_strcmp (alias->type, "scsi-sas") == 0))
> +    return scan_sparc_sas_disk (alias->path);
> +
> +  else if (grub_strcmp (alias->type, "nvme") == 0)
> +    return scan_nvme_disk (alias->path);
> +
> +  else if (grub_strcmp (alias->type, "scsi-usb") == 0)
> +    return scan_usb_disk (alias->path);
> +
> +  else if (grub_strcmp (alias->type, "block") == 0)
> +    {
> +      const char **bl = block_blacklist;
> +
> +      while (*bl != NULL)
> +        {
> +          if (grub_strstr (alias->path, *bl))
> +            return;
> +          bl++;
> +        }
> +
> +      add_disk (alias->path);
> +      return;
> +    }
> +
> +  FOR_IEEE1275_DEVCHILDREN (alias->path, child)
> +    iterate_devtree (&child);
> +}
> +
> +static void
> +unescape_devices (void)
> +{
> +  struct disk_dev *dev;
> +
> +  FOR_LIST_ELEMENTS (dev, disk_devs)
> +    {
> +      grub_free (dev->grub_decoded_devpath);
> +
> +      if ((dev->grub_alias_devpath) &&
> +        (grub_strcmp (dev->grub_alias_devpath, dev->grub_devpath) != 0))
> +        dev->grub_decoded_devpath =
> +          remove_escaped_commas (grub_strdup (dev->grub_alias_devpath));
> +      else
> +        dev->grub_decoded_devpath =
> +          remove_escaped_commas (grub_strdup (dev->grub_devpath));
> +    }
> +}
> +
> +static void
> +enumerate_disks (void)
> +{
> +  struct grub_ieee1275_devalias alias;
> +
> +  FOR_IEEE1275_DEVCHILDREN("/", alias)
> +    iterate_devtree (&alias);
> +}
> +
> +static grub_err_t
> +add_bootpath (void)
> +{
> +  struct disk_dev *ob_device;
> +  grub_err_t rval = GRUB_ERR_NONE;
> +  char *dev, *alias;
> +  char *type;
> +
> +  dev = grub_ieee1275_get_boot_dev ();
> +
> +  if (dev == NULL)
> +    return grub_error (GRUB_ERR_OUT_OF_MEMORY, "failure adding boot device");
> +
> +  type = grub_ieee1275_get_device_type (dev);
> +
> +  if (type == NULL)
> +    {
> +      grub_free (dev);
> +      return grub_error (GRUB_ERR_OUT_OF_MEMORY, "failure adding boot 
> device");
> +    }
> +
> +  alias = NULL;
> +
> +  if (!(grub_strcmp (type, "network") == 0))

grub_strcmp (type, "network") != 0

> +    {
> +      dev = strip_ob_partition (dev);
> +      ob_device = add_canon_disk (dev);
> +
> +      if (ob_device == NULL)
> +        rval =  grub_error (GRUB_ERR_OUT_OF_MEMORY, "failure adding boot 
> device");
> +
> +      ob_device->valid = 1;
> +
> +      alias = grub_ieee1275_get_devname (dev);
> +
> +      if (alias && grub_strcmp (alias, dev) != 0)
> +        ob_device->grub_alias_devpath = grub_ieee1275_encode_devname (dev);
> +
> +      ob_device->boot_dev = 1;
> +    }
> +
> +  grub_free (type);
> +  grub_free (dev);
> +  grub_free (alias);
> +  return rval;
> +}
> +
> +static void
> +enumerate_aliases (void)
> +{
> +  struct grub_ieee1275_devalias alias;
> +
> +  /* Some block device aliases are not in canonical form
> +
> +     For example:
> +
> +     disk3                    
> /address@hidden/address@hidden/address@hidden/address@hidden
> +     disk2                    
> /address@hidden/address@hidden/address@hidden/address@hidden
> +     disk1                    
> /address@hidden/address@hidden/address@hidden/address@hidden
> +     disk                     
> /address@hidden/address@hidden/address@hidden/address@hidden
> +     disk0                    
> /address@hidden/address@hidden/address@hidden/address@hidden
> +
> +     None of these devices are in canonical form.
> +
> +     Also, just because there is a devalias, doesn't mean there is a disk
> +     at that location.  And a valid boot block device doesn't have to have
> +     a devalias at all.
> +
> +     At this point, all valid disks have been found in the system
> +     and devaliases that point to canonical names are stored in the
> +     disk_devs list already. */

I do not like comments formated in that way because it is difficult to
differentiate code from comments at first sight. I know that coding style
says something different but I am going to change it. So, please adhere
to Linux kernel comments style. Here and there.

> +  FOR_IEEE1275_DEVALIASES (alias)
> +    {
> +      struct disk_dev *dev;
> +      char *canon;
> +
> +      if (grub_strcmp (alias.type, "block") != 0)
> +        continue;
> +
> +      canon = canonicalise_disk (alias.name);
> +
> +      if (canon == NULL)
> +        /* This is not an error, a devalias could point to a
> +           nonexistent disk */
> +        continue;
> +
> +      dev = grub_named_list_find (GRUB_AS_NAMED_LIST (disk_devs), canon);
> +
> +      if (dev)
> +        {
> +          /* If more than one alias points to the same device,
> +             remove the previous one unless it is the boot dev,
> +             since the upper level will use the first one. The reason
> +             all the others are redone is in the case of hot-plugging
> +             a disk.  If the boot disk gets hot-plugged, it will come
> +             thru here with a different name without the boot_dev flag
> +             set. */

Ditto...

> +          if ((dev->boot_dev) && (dev->grub_alias_devpath))
> +            continue;
> +
> +          grub_free (dev->grub_alias_devpath);
> +          dev->grub_alias_devpath = grub_ieee1275_encode_devname 
> (alias.path);
> +        }
> +      grub_free (canon);
> +    }
> +}
> +
> +static void
> +display_disks (void)
> +{
> +  struct disk_dev *dev;
> +
> +  grub_printf ("--------------------- DISKS ---------------------\n");
> +
> +  FOR_LIST_ELEMENTS (dev, disk_devs)
> +    {
> +      grub_printf ("name: %s\n", dev->name);
> +      grub_printf ("grub_devpath: %s\n", dev->grub_devpath);
> +      grub_printf ("grub_alias_devpath: %s\n", dev->grub_alias_devpath);
> +      grub_printf ("grub_decoded_devpath: %s\n", dev->grub_decoded_devpath);
> +      grub_printf ("valid: %s\n", (dev->valid) ? "yes" : "no");
> +      grub_printf ("boot_dev: %s\n", (dev->boot_dev) ? "yes" : "no");
> +      grub_printf ("opened: %s\n", (dev->ihandle) ? "yes" : "no");
> +      grub_printf ("block size: %" PRIuGRUB_UINT32_T "\n", dev->block_size);
> +      grub_printf ("num blocks: %" PRIuGRUB_UINT64_T "\n", dev->num_blocks);
> +      grub_printf ("log sector size: %" PRIuGRUB_UINT32_T "\n",
> +                   dev->log_sector_size);
> +      grub_printf ("\n");

Could not you put all values in one column?

> +    }
> +
> +  grub_printf ("-------------------------------------------------\n");
> +}
> +
> +static void
> +display_stats (void)
> +{
> +  const char *debug = grub_env_get ("debug");
> +
> +  if (! debug)

Please be consistent...

> +    return;
> +
> +  if (grub_strword (debug, "all") || grub_strword (debug, "obdisk"))
> +    {
> +      display_parents ();
> +      display_disks ();
> +    }
> +}
> +
> +static void
> +invalidate_all_disks (void)
> +{
> +  struct disk_dev *dev = NULL;
> +
> +  if (disks_enumerated)
> +    FOR_LIST_ELEMENTS (dev, disk_devs)
> +      dev->valid = 0;
> +}
> +
> +/* This is for backwards compatibility, since the path should be generated
> +   correctly now. */
> +static struct disk_dev *
> +find_legacy_grub_devpath (const char *name)
> +{
> +  struct disk_dev *dev = NULL;
> +  char *canon, *devpath = NULL;
> +
> +  devpath = decode_grub_devname (name + sizeof ("ieee1275"));
> +  canon = canonicalise_disk (devpath);
> +
> +  if (canon != NULL)
> +    dev = grub_named_list_find (GRUB_AS_NAMED_LIST (disk_devs), canon);
> +
> +  grub_free (devpath);
> +  grub_free (canon);
> +  return dev;
> +}
> +
> +static void
> +enumerate_devices (void)
> +{
> +  invalidate_all_disks ();
> +  enumerate_disks ();
> +  enumerate_aliases ();
> +  unescape_devices ();
> +  disks_enumerated = 1;
> +  display_stats ();
> +}
> +
> +static struct disk_dev *
> +find_grub_devpath_real (const char *name)
> +{
> +  struct disk_dev *dev = NULL;
> +
> +  FOR_LIST_ELEMENTS (dev, disk_devs)
> +    {
> +      if ((STRCMP (dev->grub_devpath, name))
> +        || (STRCMP (dev->grub_alias_devpath, name))
> +        || (STRCMP (dev->grub_decoded_devpath, name)))
> +        break;
> +    }
> +
> +  return dev;
> +}
> +
> +static struct disk_dev *
> +find_grub_devpath (const char *name)
> +{
> +  struct disk_dev *dev = NULL;
> +  int enumerated;
> +
> +  do {
> +    enumerated = disks_enumerated;
> +    dev = find_grub_devpath_real (name);
> +
> +    if (dev)
> +      break;
> +
> +    dev = find_legacy_grub_devpath (name);
> +
> +    if (dev)
> +      break;
> +
> +    enumerate_devices ();
> +  } while (enumerated == 0);
> +
> +  return dev;
> +}
> +
> +static int
> +grub_obdisk_iterate (grub_disk_dev_iterate_hook_t hook, void *hook_data,
> +                  grub_disk_pull_t pull)
> +{
> +  struct disk_dev *dev;
> +
> +  if (pull != GRUB_DISK_PULL_NONE)
> +    return 0;
> +
> +  enumerate_devices ();
> +
> +  FOR_LIST_ELEMENTS (dev, disk_devs)
> +    {
> +      if (dev->valid == 1)
> +        if (hook (dev->grub_decoded_devpath, hook_data))
> +          return 1;
> +    }
> +
> +  return 0;
> +}
> +
> +static grub_err_t
> +grub_obdisk_open (const char *name, grub_disk_t disk)
> +{
> +  grub_ieee1275_ihandle_t ihandle = 0;
> +  struct disk_dev *dev = NULL;
> +
> +  if (grub_strncmp (name, "ieee1275/", sizeof ("ieee1275/") - 1) != 0)
> +    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "not IEEE1275 device");
> +
> +  dev = find_grub_devpath (name);
> +
> +  if (dev == NULL)
> +    {
> +      grub_printf ("UNKNOWN DEVICE: %s\n", name);
> +      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "%s", name);
> +    }
> +
> +  if (dev->opened == 0)
> +    {
> +      if (dev->raw_name)
> +        grub_ieee1275_open (dev->raw_name, &ihandle);
> +      else
> +        grub_ieee1275_open (dev->name, &ihandle);
> +
> +      if (ihandle == 0)
> +        {
> +          grub_printf ("Can't open device %s\n", name);
> +          return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "can't open device 
> %s", name);
> +        }
> +
> +      dev->block_size = grub_ieee1275_get_block_size (ihandle);
> +      dev->num_blocks = grub_ieee1275_num_blocks (ihandle);
> +
> +      if (dev->num_blocks == 0)
> +        dev->num_blocks = grub_ieee1275_num_blocks64 (ihandle);
> +
> +      if (dev->num_blocks == 0)
> +        dev->num_blocks = GRUB_DISK_SIZE_UNKNOWN;
> +
> +      if (dev->block_size != 0)
> +        {
> +          for (dev->log_sector_size = 0;
> +               (1U << dev->log_sector_size) < dev->block_size;
> +               dev->log_sector_size++);
> +        }
> +      else
> +        dev->log_sector_size = 9;
> +
> +      dev->ihandle = ihandle;
> +      dev->opened = 1;
> +    }
> +
> +  disk->total_sectors = dev->num_blocks;
> +  disk->id = dev->ihandle;
> +  disk->data = dev;
> +  disk->log_sector_size = dev->log_sector_size;
> +  return GRUB_ERR_NONE;
> +}
> +
> +
> +static struct grub_disk_dev grub_obdisk_dev =
> +  {
> +    .name = "obdisk",
> +    .id = GRUB_DISK_DEVICE_OBDISK_ID,
> +    .iterate = grub_obdisk_iterate,
> +    .open = grub_obdisk_open,
> +    .close = grub_obdisk_close,
> +    .read = grub_obdisk_read,
> +    .next = 0

Assignments in one column please... And drop 0 assignment.
Compiler is your friend.

Daniel