Hi Dan, Apologies for responding late to this. My response below
Dan Williams <[email protected]> writes: > On Tue, Feb 18, 2020 at 1:03 PM Dan Williams <[email protected]> wrote: >> >> On Tue, Feb 18, 2020 at 1:00 PM Jeff Moyer <[email protected]> wrote: >> > >> > Vaibhav Jain <[email protected]> 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 <[email protected]> >> > > --- >> > > 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 <[email protected]> >> >> 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 <[email protected]> Linux Technology Center, IBM India Pvt. Ltd. _______________________________________________ Linux-nvdimm mailing list -- [email protected] To unsubscribe send an email to [email protected]
