Re: [PATCH] libndctl, dimm: Don't require an xlat function

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Wed, 2019-01-30 at 08:38 -0800, Dan Williams wrote:
> On Wed, Jan 30, 2019 at 2:27 AM Oliver O'Halloran <oohall@gmail.com> wrote:
> > commit 62bbfce3cb62 ("libndctl, intel: Add infrastructure for
> > firmware_status translation") has the unfortunate side effect of making
> > all NDCTL commands fail with -ENOMSG unless an xlat_firmware_status
> > function is defined for the DIMM family. This means that none of the
> > DIMM label manipulation commands work anymore, unless you happen to be
> > using an Intel DIMM.
> > 
> > Cc: Vishal Verma <vishal.l.verma@intel.com>
> > Fixes: 62bbfce3cb62 ("libndctl, intel: Add infrastructure for firmware_status translation")
> > Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> > ---
> >  ndctl/lib/libndctl.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> > index 06f835d76117..80d107394a74 100644
> > --- a/ndctl/lib/libndctl.c
> > +++ b/ndctl/lib/libndctl.c
> > @@ -2846,6 +2846,9 @@ NDCTL_EXPORT int ndctl_cmd_submit_xlat(struct ndctl_cmd *cmd)
> >          * useful), then the xlat function is available separately as well.
> >          */
> >         xlat_rc = ndctl_cmd_xlat_firmware_status(cmd);
> > +       if (xlat_rc == -ENOMSG)
> > +               return rc;
> > +
> 
> Ah, good point, however I think we should do this instead:
> 
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index 830b791339d2..2fa89162e05e 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -2710,7 +2710,7 @@ NDCTL_EXPORT int
> ndctl_cmd_xlat_firmware_status(struct ndctl_cmd *cmd)
>         struct ndctl_dimm_ops *ops = dimm ? dimm->ops : NULL;
> 
>         if (!dimm || !ops || !ops->xlat_firmware_status)
> -               return -ENOMSG;
> +               return 0;
>         return ops->xlat_firmware_status(cmd);
>  }
> 
> 
> A failure to translate just means proceed with the information that we
> *do* have, not a failure.

Ah good catch and I agree. Dan, will you send a proper patch for this
or shall I?


_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm