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 [off-list ref] writes:

On Fri, Jun 5, 2020 at 10:13 AM Ira Weiny [off-list ref] wrote:

quoted

On Fri, Jun 05, 2020 at 05:11:34AM +0530, Vaibhav Jain wrote:

quoted

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" <redacted>
Cc: Dan Williams <redacted>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Vaibhav Jain <redacted>
---
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.

quoted

---
 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...

quoted

      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...

quoted

      }

      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.

quoted

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.

-- 
Cheers
~ Vaibhav