[PATCH] libnvdimm/bus: return the outvar 'cmd_rc' error code in __nd_ioctl()

[PATCH] libnvdimm/bus: return the outvar 'cmd_rc' error code in __nd_ioctl()

[Vaibhav Jain]

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@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;
 
-- 
2.24.1
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH] libnvdimm/bus: return the outvar 'cmd_rc' error code in __nd_ioctl()

[Jeff Moyer]

Vaibhav Jain <vaibhav@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@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@redhat.com>
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH] libnvdimm/bus: return the outvar 'cmd_rc' error code in __nd_ioctl()

[Dan Williams]

On Tue, Feb 18, 2020 at 1:00 PM Jeff Moyer <jmoyer@redhat.com> wrote:
>
> Vaibhav Jain <vaibhav@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@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@redhat.com>

Applied.
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH] libnvdimm/bus: return the outvar 'cmd_rc' error code in __nd_ioctl()

[Dan Williams]

On Tue, Feb 18, 2020 at 1:03 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Tue, Feb 18, 2020 at 1:00 PM Jeff Moyer <jmoyer@redhat.com> wrote:
> >
> > Vaibhav Jain <vaibhav@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@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@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.
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [PATCH] libnvdimm/bus: return the outvar 'cmd_rc' error code in __nd_ioctl()

[Vaibhav Jain]

Hi Dan,

Apologies for responding late to this. My response below

Dan Williams <dan.j.williams@intel.com> writes:

> On Tue, Feb 18, 2020 at 1:03 PM Dan Williams <dan.j.williams@intel.com> wrote:
>>
>> On Tue, Feb 18, 2020 at 1:00 PM Jeff Moyer <jmoyer@redhat.com> wrote:
>> >
>> > Vaibhav Jain <vaibhav@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@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@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@linux.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org