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

[Daniel Kiper]

On Thu, Feb 08, 2018 at 08:37:27AM -0700, Eric Snowberg wrote:
> > On Feb 8, 2018, at 8:11 AM, Daniel Kiper <address@hidden> wrote:
> > On Wed, Feb 07, 2018 at 10:39:00AM -0700, Eric Snowberg wrote:
> >>> On Feb 7, 2018, at 4:53 AM, Daniel Kiper <address@hidden> wrote:
> >>> On Tue, Jan 30, 2018 at 08:49:48PM -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>
> >>>
> >>> In general Reviewed-by: Daniel Kiper <address@hidden>...
> >>>
> >>> Just a few comments below...
> >>>
> >>>> ---
> >>>> V2:
> >>>> - Requested coding style changes
> >>>> ---
> >>>> grub-core/osdep/linux/ofpath.c |  148 
> >>>> +++++++++++++++++++++++++++++++++++++++-
> >>>> 1 files changed, 145 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/grub-core/osdep/linux/ofpath.c 
> >>>> b/grub-core/osdep/linux/ofpath.c
> >>>> index dce4e59..5369508 100644
> >>>> --- a/grub-core/osdep/linux/ofpath.c
> >>>> +++ b/grub-core/osdep/linux/ofpath.c
> >>>> @@ -38,6 +38,46 @@
> >>>> #include <errno.h>
> >>>> #include <ctype.h>
> >>>>
> >>>> +#ifdef __sparc__
> >>>
> >>> I have discussed this with Vladimir. It looks that this will not
> >>> work if you try to cross-install SPARC GRUB2 binary using e.g.
> >>> x86 grub-install. By default it should work. However, we will also
> >>> have other issues here, like lack of access to OF firmware/paths,
> >>> which make such configs unusable anyway. So, I would leave this patch
> >>> as is for time being. If somebody cares then he/she should fix it.
> >>>
> >>> Anyway, I will add something like that into the commit message before 
> >>> committing.
> >>
> >> That is fine with me.
> >
> > Great! Thanks!
> >
> >>>> +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. */
> >>>> +  {0x30, GRUB_OFPATH_SPARC_TGT_LUN},
> >>>> +  /* SAS-1068E. */
> >>>> +  {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}
> >>>> +};
> >>>> +
> >>>> +static const int LSI_VENDOR_ID = 0x1000;
> >>>> +#endif
> >>>> +
> >>>> #ifdef OFPATH_STANDALONE
> >>>> #define xmalloc malloc
> >>>> void
> >>>> @@ -338,6 +378,67 @@ vendor_is_ATA(const char *path)
> >>>>  return (memcmp(bufcont, "ATA", 3) == 0);
> >>>> }
> >>>>
> >>>> +#ifdef __sparc__
> >>>> +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, *path;
> >>>> +  char buf[8];
> >>>> +  int fd;
> >>>> +
> >>>> +  if (!ed)
> >>>> +    return;
> >>>> +
> >>>> +  p = xstrdup (sysfs_path);
> >>>> +  ed = strstr (p, "host");
> >>>> +
> >>>> +  if (!ed)
> >>>> +    goto out;
> >>>
> >>> In normal circumstances this will never ever fail.
> >>> If you are OK I will drop this "if" before committing.
> >>
> >> I put that in for the case if the xstrdup above can not allocate memory.
> >> Then p will be null, the strstr will fail and then we should jump to out.
> >
> > If this would be the case then you should check p for NULL before
> > strstr() call. AIUI its behavior is not well defined if one of
> > arguments is NULL. Anyway, xstrdup() does not return if it fails.
> > So, we do not have such problem here.
>
> Ok, do you want to remove it?  Or should I send out an updated patch?

I will remove it before commit.

Daniel