Re: [PATCH] sparc64: fix OF path names for sun4v systems

[Eric Snowberg]

> On Dec 18, 2017, at 8:22 AM, Daniel Kiper <address@hidden> 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 <address@hidden>
>> ---
>> 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 ("/address@hidden,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, "/address@hidden,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), "/address@hidden,%x", 
>> disk_name,
>> +                          sas_address, lun);
>> +      else
>> +        optr += snprintf (disk, sizeof (disk), "/address@hidden,%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.