grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2] ieee1275: obdisk driver


From: Eric Snowberg
Subject: Re: [PATCH v2] ieee1275: obdisk driver
Date: Fri, 15 Jun 2018 09:58:56 -0600

> On Jun 15, 2018, at 6:02 AM, Daniel Kiper <address@hidden> wrote:
> 
> 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.

I thought I changed everything I could without getting the warning Adrian 
found.  I’ll try to drop these.

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

ok

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

I missed that one, I’ll change it.

> 
>> +    *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?

I’m open for recommendations.

> 
> 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().

Ok, I’ll change the order in the file.


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

Ok, I’ll change this.

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

ok

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

dev will always be equal to NULL in that case so nothing would happen other 
than the error being printed.  But I supposed it would be better to have a 
final “else" after the "else if" and set ret = grub_error with 
GRUB_ERR_BAD_DEVICE. 

> 
>> +  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/?

It really is a close as far as the grub driver is concerned (grub_disk_dev). If 
you insist I’ll change it, but naming it clear doesn't make sense to me. 

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

SAS expander

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

It is for dual port SAS disks.  I’ll add a comment.

> 
>> +
> 
> Could you drop this empty line?

ok

> 
>> +    for (phy = 0; phy < 0x20; phy++)
> 
> Why 0x20? Constant? Or comment at least?

I’ll make this a constant since I suppose the max number of SAS phys could 
change in the future.

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

Many SPARC disks contain a comma within the name.  Code from various layers 
above this driver handle the comma differently.  At times they will strip 
everything to the right of the comma.  The solution I came up with here is self 
contained and will not impact generic grub2 code. So far it seems to work from 
all reports I've seen.

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]