Hi Dan,
Apologies for responding late to this. My response below
Dan Williams <dan.j.williams(a)intel.com> writes:
On Tue, Feb 18, 2020 at 1:03 PM Dan Williams
<dan.j.williams(a)intel.com> wrote:
>
> On Tue, Feb 18, 2020 at 1:00 PM Jeff Moyer <jmoyer(a)redhat.com> wrote:
> >
> > Vaibhav Jain <vaibhav(a)linux.ibm.com> writes:
> >
> > > Presently the error code returned via out variable 'cmd_rc' from
the
> > > nvdimm-bus controller function is ignored when called from
> > > __nd_ioctl() and never communicated back to user-space code that called
> > > an ioctl on dimm/bus.
> > >
> > > This minor patch updates __nd_ioctl() to propagate the value of out
> > > variable 'cmd_rc' back to user-space in case it reports an error.
> > >
> > > Signed-off-by: Vaibhav Jain <vaibhav(a)linux.ibm.com>
> > > ---
> > > drivers/nvdimm/bus.c | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
> > > index a8b515968569..5b687a27fdf2 100644
> > > --- a/drivers/nvdimm/bus.c
> > > +++ b/drivers/nvdimm/bus.c
> > > @@ -1153,6 +1153,11 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus,
struct nvdimm *nvdimm,
> > > if (rc < 0)
> > > goto out_unlock;
> > >
> > > + if (cmd_rc < 0) {
> > > + rc = cmd_rc;
> > > + goto out_unlock;
> > > + }
> > > +
> > > if (!nvdimm && cmd == ND_CMD_CLEAR_ERROR && cmd_rc
>= 0) {
> > > struct nd_cmd_clear_error *clear_err = buf;
> >
> > Looks good to me.
> >
> > Reviewed-by: Jeff Moyer <jmoyer(a)redhat.com>
>
> Applied.
Unapplied. This breaks the NVDIMM unit test, and now that I look
closer you are likely overlooking the fact that cmd_rc is a
translation of the firmware status, while the ioctl rc is whether the
command was successfully submitted. If you want the equivalent of
cmd_rc in userspace you need to translate the firmware status. See
ndctl_cmd_submit_xlat() in libndctl as an example of how the
equivalent of cmd_rc is generated from the firmware status.
This seems to be departure from rest of the libndvdimm where a non zero
value of out-var 'cmd_rc' is treated as an error and communicated back to the
caller.
However I agree to the points you made that semantics for __nd_ioctl()
are different hence 'cmd_rc' need not be treated the same way as others.
I think it will be better if these points are documented as
code-comments in this function to make it more clearer as to why
negative value 'cmd_rd' will be ignored and how userspace can get hold
of it if needed.
--
Vaibhav Jain <vaibhav(a)linux.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.