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

[Daniel Kiper]

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.

> +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 ("/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);

>    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