| Message ID | 20200604234136.253703-5-vaibhav@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc/papr_scm: Add support for reporting nvdimm health | expand |
On Fri, Jun 05, 2020 at 05:11:34AM +0530, Vaibhav Jain wrote: > Since papr_scm_ndctl() can be called from outside papr_scm, its > exposed to the possibility of receiving NULL as value of 'cmd_rc' > argument. This patch updates papr_scm_ndctl() to protect against such > possibility by assigning it pointer to a local variable in case cmd_rc > == NULL. > > Finally the patch also updates the 'default' clause of the switch-case > block removing a 'return' statement thereby ensuring that value of > 'cmd_rc' is always logged when papr_scm_ndctl() returns. > > Cc: "Aneesh Kumar K . V" <aneesh.kumar@linux.ibm.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Cc: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com> > --- > Changelog: > > v9..v10 > * New patch in the series Thanks for making this a separate patch it is easier to see what is going on here. > --- > arch/powerpc/platforms/pseries/papr_scm.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c > index 0c091622b15e..6512fe6a2874 100644 > --- a/arch/powerpc/platforms/pseries/papr_scm.c > +++ b/arch/powerpc/platforms/pseries/papr_scm.c > @@ -355,11 +355,16 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, > { > struct nd_cmd_get_config_size *get_size_hdr; > struct papr_scm_priv *p; > + int rc; > > /* Only dimm-specific calls are supported atm */ > if (!nvdimm) > return -EINVAL; > > + /* Use a local variable in case cmd_rc pointer is NULL */ > + if (!cmd_rc) > + cmd_rc = &rc; > + This protects you from the NULL. However... > p = nvdimm_provider_data(nvdimm); > > switch (cmd) { > @@ -381,12 +386,13 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, > break; > > default: > - return -EINVAL; > + dev_dbg(&p->pdev->dev, "Unknown command = %d\n", cmd); > + *cmd_rc = -EINVAL; ... I think you are conflating rc and cmd_rc... > } > > dev_dbg(&p->pdev->dev, "returned with cmd_rc = %d\n", *cmd_rc); > > - return 0; > + return *cmd_rc; ... this changes the behavior of the current commands. Now if the underlying papr_scm_meta_[get|set]() fails you return that failure as rc rather than 0. Is that ok? Also 'logging cmd_rc' in the invalid cmd case does not seem quite right unless you really want rc to be cmd_rc. The architecture is designed to separate errors which occur in the kernel vs errors in the firmware/dimm. Are they always the same? The current code differentiates them. Ira > } > > static ssize_t flags_show(struct device *dev, > -- > 2.26.2 >
On Fri, Jun 5, 2020 at 10:13 AM Ira Weiny <ira.weiny@intel.com> wrote: > > On Fri, Jun 05, 2020 at 05:11:34AM +0530, Vaibhav Jain wrote: > > Since papr_scm_ndctl() can be called from outside papr_scm, its > > exposed to the possibility of receiving NULL as value of 'cmd_rc' > > argument. This patch updates papr_scm_ndctl() to protect against such > > possibility by assigning it pointer to a local variable in case cmd_rc > > == NULL. > > > > Finally the patch also updates the 'default' clause of the switch-case > > block removing a 'return' statement thereby ensuring that value of > > 'cmd_rc' is always logged when papr_scm_ndctl() returns. > > > > Cc: "Aneesh Kumar K . V" <aneesh.kumar@linux.ibm.com> > > Cc: Dan Williams <dan.j.williams@intel.com> > > Cc: Michael Ellerman <mpe@ellerman.id.au> > > Cc: Ira Weiny <ira.weiny@intel.com> > > Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com> > > --- > > Changelog: > > > > v9..v10 > > * New patch in the series > > Thanks for making this a separate patch it is easier to see what is going on > here. > > > --- > > arch/powerpc/platforms/pseries/papr_scm.c | 10 ++++++++-- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c > > index 0c091622b15e..6512fe6a2874 100644 > > --- a/arch/powerpc/platforms/pseries/papr_scm.c > > +++ b/arch/powerpc/platforms/pseries/papr_scm.c > > @@ -355,11 +355,16 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, > > { > > struct nd_cmd_get_config_size *get_size_hdr; > > struct papr_scm_priv *p; > > + int rc; > > > > /* Only dimm-specific calls are supported atm */ > > if (!nvdimm) > > return -EINVAL; > > > > + /* Use a local variable in case cmd_rc pointer is NULL */ > > + if (!cmd_rc) > > + cmd_rc = &rc; > > + > > This protects you from the NULL. However... > > > p = nvdimm_provider_data(nvdimm); > > > > switch (cmd) { > > @@ -381,12 +386,13 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, > > break; > > > > default: > > - return -EINVAL; > > + dev_dbg(&p->pdev->dev, "Unknown command = %d\n", cmd); > > + *cmd_rc = -EINVAL; > > ... I think you are conflating rc and cmd_rc... > > > } > > > > dev_dbg(&p->pdev->dev, "returned with cmd_rc = %d\n", *cmd_rc); > > > > - return 0; > > + return *cmd_rc; > > ... this changes the behavior of the current commands. Now if the underlying > papr_scm_meta_[get|set]() fails you return that failure as rc rather than 0. > > Is that ok? The expectation is that rc is "did the command get sent to the device, or did it fail for 'transport' reasons". The role of cmd_rc is to translate the specific status response of the command into a common error code. The expectations are: rc < 0: Error code, Linux terminated the ioctl before talking to hardware rc == 0: Linux successfully submitted the command to hardware, cmd_rc is valid for command specific response rc > 0: Linux successfully submitted the command, but detected that only a subset of the data was accepted for "write"-style commands, or that only subset of data was returned for "read"-style commands. I.e. short-write / short-read semantics. cmd_rc is valid in this case and its up to userspace to determine if a short transfer is an error or not. > Also 'logging cmd_rc' in the invalid cmd case does not seem quite right unless > you really want rc to be cmd_rc. > > The architecture is designed to separate errors which occur in the kernel vs > errors in the firmware/dimm. Are they always the same? The current code > differentiates them. Yeah, they're distinct, transport vs end-point / command-specific status returns.
Hi Ira and Dan, Thanks for reviewing this patch. Have updated the patch based on your feedback to upadate cmd_rc only when the nd_cmd was handled and return '0' in that case. Other errors in case the nd_cmd was unrecognized or invalid result in error returned from this functions as you suggested. ~ Vaibhav Dan Williams <dan.j.williams@intel.com> writes: > On Fri, Jun 5, 2020 at 10:13 AM Ira Weiny <ira.weiny@intel.com> wrote: >> >> On Fri, Jun 05, 2020 at 05:11:34AM +0530, Vaibhav Jain wrote: >> > Since papr_scm_ndctl() can be called from outside papr_scm, its >> > exposed to the possibility of receiving NULL as value of 'cmd_rc' >> > argument. This patch updates papr_scm_ndctl() to protect against such >> > possibility by assigning it pointer to a local variable in case cmd_rc >> > == NULL. >> > >> > Finally the patch also updates the 'default' clause of the switch-case >> > block removing a 'return' statement thereby ensuring that value of >> > 'cmd_rc' is always logged when papr_scm_ndctl() returns. >> > >> > Cc: "Aneesh Kumar K . V" <aneesh.kumar@linux.ibm.com> >> > Cc: Dan Williams <dan.j.williams@intel.com> >> > Cc: Michael Ellerman <mpe@ellerman.id.au> >> > Cc: Ira Weiny <ira.weiny@intel.com> >> > Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com> >> > --- >> > Changelog: >> > >> > v9..v10 >> > * New patch in the series >> >> Thanks for making this a separate patch it is easier to see what is going on >> here. >> >> > --- >> > arch/powerpc/platforms/pseries/papr_scm.c | 10 ++++++++-- >> > 1 file changed, 8 insertions(+), 2 deletions(-) >> > >> > diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c >> > index 0c091622b15e..6512fe6a2874 100644 >> > --- a/arch/powerpc/platforms/pseries/papr_scm.c >> > +++ b/arch/powerpc/platforms/pseries/papr_scm.c >> > @@ -355,11 +355,16 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, >> > { >> > struct nd_cmd_get_config_size *get_size_hdr; >> > struct papr_scm_priv *p; >> > + int rc; >> > >> > /* Only dimm-specific calls are supported atm */ >> > if (!nvdimm) >> > return -EINVAL; >> > >> > + /* Use a local variable in case cmd_rc pointer is NULL */ >> > + if (!cmd_rc) >> > + cmd_rc = &rc; >> > + >> >> This protects you from the NULL. However... >> >> > p = nvdimm_provider_data(nvdimm); >> > >> > switch (cmd) { >> > @@ -381,12 +386,13 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, >> > break; >> > >> > default: >> > - return -EINVAL; >> > + dev_dbg(&p->pdev->dev, "Unknown command = %d\n", cmd); >> > + *cmd_rc = -EINVAL; >> >> ... I think you are conflating rc and cmd_rc... >> >> > } >> > >> > dev_dbg(&p->pdev->dev, "returned with cmd_rc = %d\n", *cmd_rc); >> > >> > - return 0; >> > + return *cmd_rc; >> >> ... this changes the behavior of the current commands. Now if the underlying >> papr_scm_meta_[get|set]() fails you return that failure as rc rather than 0. >> >> Is that ok? > > The expectation is that rc is "did the command get sent to the device, > or did it fail for 'transport' reasons". The role of cmd_rc is to > translate the specific status response of the command into a common > error code. The expectations are: > > rc < 0: Error code, Linux terminated the ioctl before talking to hardware > > rc == 0: Linux successfully submitted the command to hardware, cmd_rc > is valid for command specific response > > rc > 0: Linux successfully submitted the command, but detected that > only a subset of the data was accepted for "write"-style commands, or > that only subset of data was returned for "read"-style commands. I.e. > short-write / short-read semantics. cmd_rc is valid in this case and > its up to userspace to determine if a short transfer is an error or > not. > >> Also 'logging cmd_rc' in the invalid cmd case does not seem quite right unless >> you really want rc to be cmd_rc. >> >> The architecture is designed to separate errors which occur in the kernel vs >> errors in the firmware/dimm. Are they always the same? The current code >> differentiates them. > > Yeah, they're distinct, transport vs end-point / command-specific > status returns.
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c index 0c091622b15e..6512fe6a2874 100644 --- a/arch/powerpc/platforms/pseries/papr_scm.c +++ b/arch/powerpc/platforms/pseries/papr_scm.c @@ -355,11 +355,16 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, { struct nd_cmd_get_config_size *get_size_hdr; struct papr_scm_priv *p; + int rc; /* Only dimm-specific calls are supported atm */ if (!nvdimm) return -EINVAL; + /* Use a local variable in case cmd_rc pointer is NULL */ + if (!cmd_rc) + cmd_rc = &rc; + p = nvdimm_provider_data(nvdimm); switch (cmd) { @@ -381,12 +386,13 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, break; default: - return -EINVAL; + dev_dbg(&p->pdev->dev, "Unknown command = %d\n", cmd); + *cmd_rc = -EINVAL; } dev_dbg(&p->pdev->dev, "returned with cmd_rc = %d\n", *cmd_rc); - return 0; + return *cmd_rc; } static ssize_t flags_show(struct device *dev,
Since papr_scm_ndctl() can be called from outside papr_scm, its exposed to the possibility of receiving NULL as value of 'cmd_rc' argument. This patch updates papr_scm_ndctl() to protect against such possibility by assigning it pointer to a local variable in case cmd_rc == NULL. Finally the patch also updates the 'default' clause of the switch-case block removing a 'return' statement thereby ensuring that value of 'cmd_rc' is always logged when papr_scm_ndctl() returns. Cc: "Aneesh Kumar K . V" <aneesh.kumar@linux.ibm.com> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Ira Weiny <ira.weiny@intel.com> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com> --- Changelog: v9..v10 * New patch in the series --- arch/powerpc/platforms/pseries/papr_scm.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)