Re: [PATCH v2] ieee1275: obdisk driver

[Daniel Kiper]

On Wed, May 30, 2018 at 04:55:22PM -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 current IEEE1275 ofdisk driver may be suitable for PPC and x86, it
> 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>
> ---
> Changes from v1:
> - Addressed various coding style changes requested by Daniel Kipper
> ---
>  grub-core/Makefile.core.def      |    1 +
>  grub-core/commands/nativedisk.c  |    1 +
>  grub-core/disk/ieee1275/obdisk.c | 1106 
> ++++++++++++++++++++++++++++++++++++++
>  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, 1150 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 fc4767f..ab84aa4 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..0bc37e6
> --- /dev/null
> +++ b/grub-core/disk/ieee1275/obdisk.c
> @@ -0,0 +1,1106 @@
> +/* 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>
> +
> +#define IEEE1275_DEV        "ieee1275/"
> +#define IEEE1275_DISK_ALIAS "/disk@"
> +
> +struct disk_dev
> +{
> +  struct disk_dev         *next;
> +  struct disk_dev         **prev;
> +  char                    *name;
> +  char                    *raw_name;
> +  char                    *grub_devpath;
> +  char                    *grub_alias_devpath;
> +  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;
> +  char                    *name;
> +  char                    *type;
> +  grub_ieee1275_ihandle_t ihandle;
> +  grub_uint32_t           address_cells;
> +};
> +
> +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;

I would drop all these 0/NULL initializations.
Compiler will do work for you. I asked about
that in earlier comments.

> +static const char *block_blacklist[] = {
> +  /* Requires addition work in grub before being able to be used. */

s/addition/additional/?

> +  "/iscsi-hba",
> +  /* This block device should never be used by grub. */
> +  "/address@hidden",
> +  0
> +};
> +
> +#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)

I saw that you have decided to use "src == NULL". Great! So, I would
expect that in cases like that you use "sptr != NULL". Could you fix
that here and below. Same applies to 0 comparison.

> +    *sptr = '\0';
> +
> +  return path;
> +}
> +
> +static char *
> +replace_escaped_commas (char *src)
> +{
> +  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)

I am confused by this play with commas. Could explain somewhere
where this commas are needed unescaped, escaped, replaced, etc.
Could not we simplify this somehow?

If all of this functions are really needed I would put them in
that order in the file: escape_commas(), replace_escaped_commas(),
and finally count_commas().

> +{
> +  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 == NULL)
> +    return NULL;
> +
> +  /* Un-escape commas. */

Ugh... Another play with commas...

> +  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)
> +    return NULL;
> +
> +  encoding = grub_malloc (sizeof (IEEE1275_DEV) + count_commas (path) +
> +                          grub_strlen (path) + 1);
> +
> +  if (encoding == NULL)
> +    {
> +      grub_print_error ();
> +      return NULL;
> +    }
> +
> +  optr = grub_stpcpy (encoding, IEEE1275_DEV);
> +  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, IEEE1275_DISK_ALIAS);
> +
> +  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);

I have a feeling that you assume that grub_ieee1275_get_integer_property()
does not touch address_cells if it fails. I think that it is dangerous. Hence,
I would check for error and if it happens then assign 2 to address_cells.

> +  grub_ieee1275_get_property (phandle, "device_type", op->type,
> +                              IEEE1275_MAX_PROP_LEN, &actual);

s/&actual/NULL/?

> +  op->ihandle = ihandle;
> +  op->address_cells = address_cells;
> +  return op;
> +}
> +
> +static struct parent_dev *
> +open_parent (const char *parent)
> +{
> +  struct parent_dev *op;
> +
> +  op = grub_named_list_find (GRUB_AS_NAMED_LIST (parent_devs), parent);
> +
> +  if (op == NULL)
> +    {
> +      op = open_new_parent (parent);
> +
> +      if (op)
> +        grub_list_push (GRUB_AS_LIST_P (&parent_devs), GRUB_AS_LIST (op));
> +    }
> +
> +  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);
> +    }
> +
> +  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 == 0) && (phy_hi != 0xffffffff))
> +    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;
> +      grub_size_t real_unit_str_len;
> +
> +      unit_address = grub_strstr (canon, IEEE1275_DISK_ALIAS);
> +      unit_address += grub_strlen (IEEE1275_DISK_ALIAS);
> +
> +      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_unit_str_len = grub_strlen (op->name) + sizeof 
> (IEEE1275_DISK_ALIAS)
> +                          + grub_strlen (real_unit_address);
> +
> +      real_canon = grub_malloc (real_unit_str_len);
> +
> +      grub_snprintf (real_canon, real_unit_str_len, "%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 == NULL)
> +    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:
> +  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 ret = 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)
> +        ret = grub_error (GRUB_ERR_OUT_OF_MEMORY, "failure to add disk");
> +    }
> +  else if (dev)
> +    dev->valid = 1;

What will happen if canon == NULL?

> +  grub_free (canon);
> +  return (ret);
> +}
> +
> +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 ret = GRUB_ERR_NONE;
> +  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))
> +    {
> +      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 ret;
> +}
> +
> +static void
> +grub_obdisk_close (grub_disk_t disk)

s/grub_obdisk_close/grub_obdisk_clear/?

> +{
> +  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)

What is exp? And why exp <= 0x100; exp+=0x100? Could you add
a comment here or use constants?

> +

Could you drop this empty line?

> +    for (phy = 0; phy < 0x20; phy++)

Why 0x20? Constant? Or comment at least?

> +      {
> +        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)

Why?

In general I am happy with the changes. However, some
my comments for earlier version are still not addressed.
Please take a look at it and incorporate them. If you do
not agree with something just drop me a line.

Daniel