> On Dec 18, 2017, at 8:22 AM, Daniel Kiper <daniel.ki...@oracle.com> wrote:
>
> 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.
What error do you see on a cross compile? I see __sparc__ used throughout the
code.
>
>> +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 ".”.
I’ll take care of all the extra space changes in V2.
>
>> + {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.
I’ll remove the initialization.
>
>> + 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?
This is duplicated since the result of p is modified ...
>
>> + ed = strstr (p, "host");
>> +
>> + if (!ed)
>> + goto out;
>> +
>> + *ed = '\0’;
right here. If I didn’t duplicate the string, it would corrupt the sysfs_path.
Also, sysfs_path is defined as const char *.
>> +
>> + 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?
It is being changed above? *ed is a pointer within 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);
I’ll make these goto changes.
>
>> 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.
>
I could look at doing that in the future. I always try to follow the coding
style within the file.
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
