On Wed, Dec 06, 2017 at 03:53:30PM -0800, Eric Snowberg wrote: > Fix the Open Firmware (OF) path property for sun4v SPARC systems. > These platforms do not have a /sas/ within their path. Over time > different OF addressing schemes have been supported. There > is no generic addressing scheme that works across every HBA. > > Signed-off-by: Eric Snowberg <eric.snowb...@oracle.com> > --- > grub-core/osdep/linux/ofpath.c | 147 > ++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 144 insertions(+), 3 deletions(-) > > diff --git a/grub-core/osdep/linux/ofpath.c b/grub-core/osdep/linux/ofpath.c > index 3a8bc95a9..525a42ae6 100644 > --- a/grub-core/osdep/linux/ofpath.c > +++ b/grub-core/osdep/linux/ofpath.c > @@ -38,6 +38,44 @@ > #include <errno.h> > #include <ctype.h> > > +#ifdef __sparc__
This is not good. It will not work if you cross compile. > +typedef enum > + { > + GRUB_OFPATH_SPARC_WWN_ADDR = 1, > + GRUB_OFPATH_SPARC_TGT_LUN, > + } ofpath_sparc_addressing; > + > +struct ofpath_sparc_hba > +{ > + grub_uint32_t device_id; > + ofpath_sparc_addressing addressing; > +}; > + > +static struct ofpath_sparc_hba sparc_lsi_hba[] = { > + /* Rhea, Jasper 320, LSI53C1020/1030. */ Extra space after ".". > + {0x30, GRUB_OFPATH_SPARC_TGT_LUN}, > + /* SAS-1068E. */ Ditto and below... > + {0x50, GRUB_OFPATH_SPARC_TGT_LUN}, > + /* SAS-1064E. */ > + {0x56, GRUB_OFPATH_SPARC_TGT_LUN}, > + /* Pandora SAS-1068E. */ > + {0x58, GRUB_OFPATH_SPARC_TGT_LUN}, > + /* Aspen, Invader, LSI SAS-3108. */ > + {0x5d, GRUB_OFPATH_SPARC_TGT_LUN}, > + /* Niwot, SAS 2108. */ > + {0x79, GRUB_OFPATH_SPARC_TGT_LUN}, > + /* Erie, Falcon, LSI SAS 2008. */ > + {0x72, GRUB_OFPATH_SPARC_WWN_ADDR}, > + /* LSI WarpDrive 6203. */ > + {0x7e, GRUB_OFPATH_SPARC_WWN_ADDR}, > + /* LSI SAS 2308. */ > + {0x87, GRUB_OFPATH_SPARC_WWN_ADDR}, > + /* LSI SAS 3008. */ > + {0x97, GRUB_OFPATH_SPARC_WWN_ADDR}, > + {0, 0} > +}; > +#endif > + > #ifdef OFPATH_STANDALONE > #define xmalloc malloc > void > @@ -337,6 +375,66 @@ vendor_is_ATA(const char *path) > return (memcmp(bufcont, "ATA", 3) == 0); > } > > +#ifdef __sparc__ Ditto. > +static void > +check_hba_identifiers (const char *sysfs_path, int *vendor, int *device_id) > +{ > + char *ed = strstr (sysfs_path, "host"); > + size_t path_size; > + char *p = NULL, *path = NULL; I think that you do not need to initialize *p here. > + char buf[8]; > + int fd; > + > + if (!ed) > + return; > + > + p = xstrdup (sysfs_path); Why do you need to duplicate sysfs_path? I do not think it is needed. Just p = sysfs_path? > + ed = strstr (p, "host"); > + > + if (!ed) > + goto out; > + > + *ed = '\0'; > + > + path_size = (strlen (p) + sizeof ("vendor")); > + path = xmalloc (path_size); > + > + if (!path) > + goto out; > + > + snprintf (path, path_size, "%svendor", p); > + fd = open (path, O_RDONLY); > + > + if (fd < 0) > + goto out; > + > + memset (buf, 0, sizeof (buf)); > + > + if (read (fd, buf, sizeof (buf) - 1) < 0) > + goto out; > + > + close (fd); > + sscanf (buf, "%x", vendor); Please add empty line here. > + snprintf (path, path_size, "%sdevice", p); I have a feeling that p should be changed somehow here or to be precise a bit earlier... Should not it? > + fd = open (path, O_RDONLY); > + > + if (fd < 0) > + goto out; > + > + memset (buf, 0, sizeof (buf)); > + > + if (read (fd, buf, sizeof (buf) - 1) < 0) > + goto out; > + > + close (fd); > + sscanf (buf, "%x", device_id); > + > +out: err:? And please add one extra space before the label. > + free (path); > + free (p); > +} > +#endif > + > static void > check_sas (const char *sysfs_path, int *tgt, unsigned long int *sas_address) > { > @@ -398,7 +496,7 @@ of_path_of_scsi(const char *sys_devname > __attribute__((unused)), const char *dev > { > const char *p, *digit_string, *disk_name; > int host, bus, tgt, lun; > - unsigned long int sas_address; > + unsigned long int sas_address = 0; > char *sysfs_path, disk[MAX_DISK_CAT - sizeof ("/fp@0,0")]; > char *of_path; > > @@ -415,9 +513,11 @@ of_path_of_scsi(const char *sys_devname > __attribute__((unused)), const char *dev > } > > of_path = find_obppath(sysfs_path); > - free (sysfs_path); > if (!of_path) > - return NULL; goto err: > + { > + free (sysfs_path); > + return NULL; > + } Drop this change. > > if (strstr (of_path, "qlc")) > strcat (of_path, "/fp@0,0"); > @@ -446,6 +546,45 @@ of_path_of_scsi(const char *sys_devname > __attribute__((unused)), const char *dev > } > else > { > +#ifdef __sparc__ Ditto. > + ofpath_sparc_addressing addressing = GRUB_OFPATH_SPARC_TGT_LUN; > + int vendor = 0, device_id = 0; > + char *optr = disk; > + > + check_hba_identifiers (sysfs_path, &vendor, &device_id); > + > + /* LSI Logic Vendor ID */ > + if (vendor == 0x1000) Could you define a constant? > + { > + struct ofpath_sparc_hba *lsi_hba; > + > + /* Over time different OF addressing schemes have been supported. > + There is no generic addressing scheme that works across > + every HBA. */ > + for (lsi_hba = sparc_lsi_hba; lsi_hba->device_id; lsi_hba++) > + if (lsi_hba->device_id == device_id) > + { > + addressing = lsi_hba->addressing; > + break; > + } > + } > + > + if (addressing == GRUB_OFPATH_SPARC_WWN_ADDR) > + optr += snprintf (disk, sizeof (disk), "/%s@w%lx,%x", disk_name, > + sas_address, lun); > + else > + optr += snprintf (disk, sizeof (disk), "/%s@%x,%x", disk_name, tgt, > + lun); > + > + if (*digit_string != '\0') > + { > + int part; > + > + sscanf (digit_string, "%d", &part); > + snprintf (optr, sizeof (disk) - (optr - disk - 1), ":%c", 'a' > + + (part - 1)); > + } > +#else > if (lun == 0) > { > int sas_id = 0; > @@ -493,7 +632,9 @@ of_path_of_scsi(const char *sys_devname > __attribute__((unused)), const char *dev > } > free (lunstr); > } > +#endif > } > + free (sysfs_path); Drop this change. > strcat(of_path, disk); err: free (sysfs_path); > return of_path; > } In general I am not happy with sscanf() usage as a string to number converter. Especially without any error checking. However, as I can see, it is common here. So, I will accept fixed patch with sscanf() eventually but I would be happy if you replace it everywhere with something more robust in separate patch. Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel