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

[Daniel Kiper]

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.

> > If there are no objections I will commit this patch in a week or so.
>
> Thanks…
>
> Is there some reason this patch is being held up too?
>
> http://lists.gnu.org/archive/html/grub-devel/2017-05/msg00045.html
>
> This is where things last left off:
>
> http://lists.gnu.org/archive/html/grub-devel/2017-10/msg00046.html
>
> We really need minimal GPT support in there for many of the follow on patches 
> in my queue.

Oh, sorry, I have simply forgotten about that patch. I will commit
it with this one.

Daniel