All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ieee1275: obdisk driver
@ 2018-04-05 17:53 Eric Snowberg
  2018-04-11 10:29 ` Daniel Kiper
  0 siblings, 1 reply; 3+ messages in thread
From: Eric Snowberg @ 2018-04-05 17:53 UTC (permalink / raw)
  To: grub-devel; +Cc: daniel.kiper, eric.snowberg, glaubitz

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
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:

/pci@302/pci@2/pci@0/pci@17/LSI,sas@0/disk@w5000cca02f037d6d,0

It can also be referenced by its PHY identifier using this form:

/pci@302/pci@2/pci@0/pci@17/LSI,sas@0/disk@p0

It can also be referenced by its Target identifier using this form:

/pci@302/pci@2/pci@0/pci@17/LSI,sas@0/disk@0

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 <eric.snowberg@oracle.com>
---
 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;
+};
+
+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. */
+  "/reboot-memory@0",
+  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)
+    *sptr = '\0';
+
+  return path;
+}
+
+static char *
+remove_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)
+{
+  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)
+    return NULL;
+
+  /* Un-escape 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/") + count_commas (path) +
+                          grub_strlen (path) + 1);
+
+  if (encoding == NULL)
+    {
+      grub_print_error ();
+      return NULL;
+    }
+
+  optr = grub_stpcpy (encoding, "ieee1275/");
+  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@");
+
+  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)
+  {
+     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) && (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;
+
+      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));
+
+      grub_snprintf (real_canon, grub_strlen (op->name) + sizeof ("/disk@") +
+                     grub_strlen (real_unit_address), "%s/disk@%s",
+                     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:
+  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;
+  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 rval;
+}
+
+static void
+grub_obdisk_close (grub_disk_t disk)
+{
+  disk->data = NULL;
+  disk->id = 0;
+  disk->total_sectors = 0;
+  disk->log_sector_size = 0;
+}
+
+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/disk@0", 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/disk@1", 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/disk@%"
+                         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/disk@%s",
+                           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))
+    {
+      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                    /pci@301/pci@1/scsi@0/disk@p3
+     disk2                    /pci@301/pci@1/scsi@0/disk@p2
+     disk1                    /pci@301/pci@1/scsi@0/disk@p1
+     disk                     /pci@301/pci@1/scsi@0/disk@p0
+     disk0                    /pci@301/pci@1/scsi@0/disk@p0
+
+     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. */
+  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. */
+          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");
+    }
+
+  grub_printf ("-------------------------------------------------\n");
+}
+
+static void
+display_stats (void)
+{
+  const char *debug = grub_env_get ("debug");
+
+  if (! debug)
+    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
+  };
+
+void
+grub_obdisk_init (void)
+{
+  grub_disk_firmware_fini = grub_obdisk_fini;
+  add_bootpath ();
+  grub_disk_dev_register (&grub_obdisk_dev);
+}
+
+void
+grub_obdisk_fini (void)
+{
+  struct disk_dev *dev;
+
+  FOR_LIST_ELEMENTS (dev, disk_devs)
+    {
+      if (dev->opened)
+          grub_ieee1275_close (dev->ihandle);
+    }
+
+  grub_disk_dev_unregister (&grub_obdisk_dev);
+}
diff --git a/grub-core/kern/ieee1275/cmain.c b/grub-core/kern/ieee1275/cmain.c
index 3e12e6b..20cbbd7 100644
--- a/grub-core/kern/ieee1275/cmain.c
+++ b/grub-core/kern/ieee1275/cmain.c
@@ -108,6 +108,9 @@ grub_ieee1275_find_options (void)
   if (rc >= 0)
     {
       char *ptr;
+
+      if (grub_strncmp (tmp, "sun4v", 5) == 0)
+        grub_ieee1275_set_flag (GRUB_IEEE1275_FLAG_RAW_DEVNAMES);
       for (ptr = tmp; ptr - tmp < actual; ptr += grub_strlen (ptr) + 1)
 	{
 	  if (grub_memcmp (ptr, "MacRISC", sizeof ("MacRISC") - 1) == 0
diff --git a/grub-core/kern/ieee1275/init.c b/grub-core/kern/ieee1275/init.c
index 0d8ebf5..d483e35 100644
--- a/grub-core/kern/ieee1275/init.c
+++ b/grub-core/kern/ieee1275/init.c
@@ -30,6 +30,9 @@
 #include <grub/time.h>
 #include <grub/ieee1275/console.h>
 #include <grub/ieee1275/ofdisk.h>
+#ifdef __sparc__
+#include <grub/ieee1275/obdisk.h>
+#endif
 #include <grub/ieee1275/ieee1275.h>
 #include <grub/net.h>
 #include <grub/offsets.h>
@@ -280,8 +283,11 @@ grub_machine_init (void)
   grub_console_init_early ();
   grub_claim_heap ();
   grub_console_init_lately ();
+#ifdef __sparc__
+  grub_obdisk_init ();
+#else
   grub_ofdisk_init ();
-
+#endif
   grub_parse_cmdline ();
 
 #ifdef __i386__
@@ -296,7 +302,11 @@ grub_machine_fini (int flags)
 {
   if (flags & GRUB_LOADER_FLAG_NORETURN)
     {
+#ifdef __sparc__
+      grub_obdisk_fini ();
+#else
       grub_ofdisk_fini ();
+#endif
       grub_console_fini ();
     }
 }
diff --git a/include/grub/disk.h b/include/grub/disk.h
index b385af8..bd58b11 100644
--- a/include/grub/disk.h
+++ b/include/grub/disk.h
@@ -49,6 +49,7 @@ enum grub_disk_dev_id
     GRUB_DISK_DEVICE_CBFSDISK_ID,
     GRUB_DISK_DEVICE_UBOOTDISK_ID,
     GRUB_DISK_DEVICE_XEN,
+    GRUB_DISK_DEVICE_OBDISK_ID,
   };
 
 struct grub_disk;
diff --git a/include/grub/ieee1275/ieee1275.h b/include/grub/ieee1275/ieee1275.h
index 8868f3a..73e2f46 100644
--- a/include/grub/ieee1275/ieee1275.h
+++ b/include/grub/ieee1275/ieee1275.h
@@ -146,6 +146,8 @@ enum grub_ieee1275_flag
   GRUB_IEEE1275_FLAG_BROKEN_REPEAT,
 
   GRUB_IEEE1275_FLAG_CURSORONOFF_ANSI_BROKEN,
+
+  GRUB_IEEE1275_FLAG_RAW_DEVNAMES,
 };
 
 extern int EXPORT_FUNC(grub_ieee1275_test_flag) (enum grub_ieee1275_flag flag);
diff --git a/include/grub/ieee1275/obdisk.h b/include/grub/ieee1275/obdisk.h
new file mode 100644
index 0000000..2fbe934
--- /dev/null
+++ b/include/grub/ieee1275/obdisk.h
@@ -0,0 +1,25 @@
+/*
+ *  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/>.
+ */
+
+#ifndef GRUB_OBDISK_HEADER
+#define GRUB_OBDISK_HEADER	1
+
+extern void grub_obdisk_init (void);
+extern void grub_obdisk_fini (void);
+
+#endif
-- 
1.7.1



^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] ieee1275: obdisk driver
  2018-04-05 17:53 [PATCH] ieee1275: obdisk driver Eric Snowberg
@ 2018-04-11 10:29 ` Daniel Kiper
  2018-04-29 20:20   ` Eric Snowberg
  0 siblings, 1 reply; 3+ messages in thread
From: Daniel Kiper @ 2018-04-11 10:29 UTC (permalink / raw)
  To: Eric Snowberg; +Cc: grub-devel, glaubitz

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:
>
> /pci@302/pci@2/pci@0/pci@17/LSI,sas@0/disk@w5000cca02f037d6d,0
>
> It can also be referenced by its PHY identifier using this form:
>
> /pci@302/pci@2/pci@0/pci@17/LSI,sas@0/disk@p0
>
> It can also be referenced by its Target identifier using this form:
>
> /pci@302/pci@2/pci@0/pci@17/LSI,sas@0/disk@0
>
> 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 <eric.snowberg@oracle.com>
> ---
>  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. */
> +  "/reboot-memory@0",
> +  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/disk@%s",
> +                     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/disk@0", 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/disk@1", 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/disk@%"
> +                         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/disk@%s",
> +                           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                    /pci@301/pci@1/scsi@0/disk@p3
> +     disk2                    /pci@301/pci@1/scsi@0/disk@p2
> +     disk1                    /pci@301/pci@1/scsi@0/disk@p1
> +     disk                     /pci@301/pci@1/scsi@0/disk@p0
> +     disk0                    /pci@301/pci@1/scsi@0/disk@p0
> +
> +     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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] ieee1275: obdisk driver
  2018-04-11 10:29 ` Daniel Kiper
@ 2018-04-29 20:20   ` Eric Snowberg
  0 siblings, 0 replies; 3+ messages in thread
From: Eric Snowberg @ 2018-04-29 20:20 UTC (permalink / raw)
  To: The development of GNU GRUB; +Cc: John Paul Adrian Glaubitz, Daniel Kiper


> On Apr 11, 2018, at 4:29 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> 
> 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/?
> 
>> +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. */
>> +  "/reboot-memory@0",
>> +  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?
> 
>> +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?

Yes. Doing it here does not impact generic GRUB2 code.

> 
>> +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.
> 
>> +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...
> 
>> +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.
> 
>> +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?
> 
>> +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
> 
>> +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().
> 
>> +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.
> 
>> +
>> +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 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
> 
>> +static void
>> +enumerate_aliases (void)
>> +{
>> +  struct grub_ieee1275_devalias alias;
>> +
>> +  /* Some block device aliases are not in canonical form
>> +
>> +     For example:
>> +
>> +     disk3                    /pci@301/pci@1/scsi@0/disk@p3
>> +     disk2                    /pci@301/pci@1/scsi@0/disk@p2
>> +     disk1                    /pci@301/pci@1/scsi@0/disk@p1
>> +     disk                     /pci@301/pci@1/scsi@0/disk@p0
>> +     disk0                    /pci@301/pci@1/scsi@0/disk@p0
>> +
>> +     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.

I have used the GNU GRUB2 coding style here.  Could the comment changes be done in a separate patch once the coding style has changed?  Possibly a script could be created to change them all from the old format to the new?

> 
>> +  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...
> 
>> +
>> +
>> +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.
> 

Thanks for your review, I’ll make all the other changes above in V2.




^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2018-04-29 20:21 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-05 17:53 [PATCH] ieee1275: obdisk driver Eric Snowberg
2018-04-11 10:29 ` Daniel Kiper
2018-04-29 20:20   ` Eric Snowberg

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.