> On Jun 15, 2018, at 6:02 AM, Daniel Kiper <daniel.ki...@oracle.com> 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:
>>
>> /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.snowb...@oracle.com>
>> ---
>> 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. */
>> + "/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)
>
> 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/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 == 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/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)
>
> 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/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)
>
> 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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
