nvdimm: Add an IOCTL pass thru for DSM calls

Add ioctl command ND_CMD_CALL_DSM to acpi_nfit_ctl and __nd_ioctl which allow kernel to call a nvdimm's _DSM as a passthru without using the marshaling code of the nd_cmd_desc.

Signed-off-by Jerry Hoemann <[email protected]>

---
 drivers/acpi/nfit.c  | 109 ++++++++++++++++++++++++++++++++-------------------
 drivers/nvdimm/bus.c |  61 +++++++++++++++++++++-------
 2 files changed, 115 insertions(+), 55 deletions(-)

drivers/acpi/nfit.c +5 -3

                desc = nd_cmd_dimm_desc(cmd);
-               uuid = to_nfit_uuid(NFIT_DEV_DIMM);
+               if (!dsm_call)
+                       uuid = to_nfit_uuid(NFIT_DEV_DIMM);
                handle = adev->handle;
        } else {
                struct acpi_device *adev = to_acpi_dev(acpi_desc);
 
                cmd_name = nvdimm_bus_cmd_name(cmd);
-               dsm_mask = nd_desc->dsm_mask;
+               if (!dsm_call)
+                       dsm_mask = nd_desc->dsm_mask;
                desc = nd_cmd_bus_desc(cmd);
-               uuid = to_nfit_uuid(NFIT_DEV_BUS);
+               if (!dsm_call)

drivers/acpi/nfit.c +3 -2

-       if (!test_bit(cmd, &dsm_mask))
-               return -ENOTTY;
+               if (!test_bit(cmd, &dsm_mask))
+                       return -ENOTTY;
+       }
 
        in_obj.type = ACPI_TYPE_PACKAGE;
        in_obj.package.count = 1;
        in_obj.package.elements = &in_buf;
        in_buf.type = ACPI_TYPE_BUFFER;

drivers/acpi/nfit.c +6 -0

+       } else {
+               /* libnvdimm has already validated the input envelope */
+               in_buf.buffer.pointer = buf;
+               for (i = 0; i < desc->in_num; i++)
+                       in_buf.buffer.length += nd_cmd_in_size(nvdimm, cmd, desc,
                                i, buf);
+       }
 
        if (IS_ENABLED(CONFIG_ACPI_NFIT_DEBUG)) {

drivers/acpi/nfit.c +6 -8

        }
 
-       out_obj = acpi_evaluate_dsm(handle, uuid, 1, cmd, &in_obj);
+       out_obj = acpi_evaluate_dsm(handle, uuid, rev, func, &in_obj);
        if (!out_obj) {
                dev_dbg(dev, "%s:%s _DSM failed cmd: %s\n", __func__, dimm_name,
                                cmd_name);

@@ -134,21 +152,28 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
        }
 
        if (out_obj->package.type != ACPI_TYPE_BUFFER) {
-               dev_dbg(dev, "%s:%s unexpected output object type cmd: %s type: %d\n",
-                               __func__, dimm_name, cmd_name, out_obj->type);
+               dev_dbg(dev, "%s:%s unexpected output object type cmd: %s %llu, type: %d\n",
+                               __func__, dimm_name, cmd_name, func, out_obj->type);
                rc = -EINVAL;
                goto out;
        }
 
        if (IS_ENABLED(CONFIG_ACPI_NFIT_DEBUG)) {
-               dev_dbg(dev, "%s:%s cmd: %s output length: %d\n", __func__,
-                               dimm_name, cmd_name, out_obj->buffer.length);
-               print_hex_dump_debug(cmd_name, DUMP_PREFIX_OFFSET, 4,
-                               4, out_obj->buffer.pointer, min_t(u32, 128,
-                                       out_obj->buffer.length), true);
+               dev_dbg(dev, "%s:%s cmd %d: %llu output length %d\n", __func__,
+                               dimm_name, cmd, func, out_obj->buffer.length);
+               print_hex_dump_debug("nvdimm out ", DUMP_PREFIX_OFFSET, 4, 4,

drivers/acpi/nfit.c +19 -14

+
+       } else {
+               for (i = 0, offset = 0; i < desc->out_num; i++) {
                u32 out_size = nd_cmd_out_size(nvdimm, cmd, desc, i, buf,
                                (u32 *) out_obj->buffer.pointer);
 

@@ -167,22 +192,23 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
                memcpy(buf + in_buf.buffer.length + offset,
                                out_obj->buffer.pointer + offset, out_size);
                offset += out_size;
-       }
-       if (offset + in_buf.buffer.length < buf_len) {
-               if (i >= 1) {
-                       /*
-                        * status valid, return the number of bytes left
-                        * unfilled in the output buffer
-                        */
-                       rc = buf_len - offset - in_buf.buffer.length;
-               } else {
-                       dev_err(dev, "%s:%s underrun cmd: %s buf_len: %d out_len: %d\n",
+               }
+               if (offset + in_buf.buffer.length < buf_len) {
+                       if (i >= 1) {
+                               /*
+                                * status valid, return the number of bytes left
+                                * unfilled in the output buffer
+                                */
+                               rc = buf_len - offset - in_buf.buffer.length;
+                       } else {
+                               dev_err(dev, "%s:%s underrun cmd: %s buf_len: %d out_len: %d\n",
                                        __func__, dimm_name, cmd_name, buf_len,
                                        offset);
-                       rc = -ENXIO;
-               }
-       } else
-               rc = 0;
+                               rc = -ENXIO;
+                       }
+               } else
+                       rc = 0;
+       }
 
  out:
        ACPI_FREE(out_obj);

@@ -190,6 +216,7 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
        return rc;
 }
 
+
 static const char *spa_type_name(u16 type)
 {
        static const char *to_name[] = {

drivers/nvdimm/bus.c +8 -3

index 1c81203..83900d50 100644
@@ -492,31 +492,38 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
        struct device *dev = &nvdimm_bus->dev;
        const char *cmd_name, *dimm_name;
        unsigned long dsm_mask;
-       void *buf;
+       void *buf = NULL;
        int rc, i;
 
+       struct nd_cmd_dsmcall_pkg pkg;
+       int dsm_call = (cmd == ND_CMD_CALL_DSM);
+
        if (nvdimm) {
                desc = nd_cmd_dimm_desc(cmd);
                cmd_name = nvdimm_cmd_name(cmd);
-               dsm_mask = nvdimm->dsm_mask ? *(nvdimm->dsm_mask) : 0;
+               if (!dsm_call)
+                       dsm_mask = nvdimm->dsm_mask ? *(nvdimm->dsm_mask) : 0;
                dimm_name = dev_name(&nvdimm->dev);
        } else {
                desc = nd_cmd_bus_desc(cmd);
                cmd_name = nvdimm_bus_cmd_name(cmd);
-               dsm_mask = nd_desc->dsm_mask;
+               if (!dsm_call)
+                       dsm_mask = nd_desc->dsm_mask;

Quoted 10 lines · Jerry Hoemann

Add ioctl command ND_CMD_CALL_DSM to acpi_nfit_ctl and __nd_ioctl which allow kernel to call a nvdimm's _DSM as a passthru without using the marshaling code of the nd_cmd_desc.

Signed-off-by Jerry Hoemann <[email protected]>

---
 drivers/acpi/nfit.c  | 109 ++++++++++++++++++++++++++++++++-------------------
 drivers/nvdimm/bus.c |  61 +++++++++++++++++++++-------
 2 files changed, 115 insertions(+), 55 deletions(-)

In general I'd like to see this patch remove the need to sprinkle "if (dsm_call)" throughout the implementation ... specific examples below:

Quoted 6 lines

drivers/acpi/nfit.c +0 -0

index c1b8d03..e509145 100644
@@ -75,7 +75,11 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,

Quoted 1 line

+ __u64 rev = 1, func = cmd;

Why __u64 and not int for these? acpi_evaluate_dsm() takes an int for both so a bigger type here will be truncated down.

drivers/acpi/nfit.c

Quoted 12 lines

        /* fail write commands (when read-only) */
        if (read_only)

drivers/acpi/nfit.c +5 -4

-               switch (ioctl_cmd) {
-               case ND_IOCTL_VENDOR:
-               case ND_IOCTL_SET_CONFIG_DATA:
-               case ND_IOCTL_ARS_START:
+               switch (cmd) {
+               case ND_CMD_VENDOR:
+               case ND_CMD_SET_CONFIG_DATA:
+               case ND_CMD_ARS_START:
+               case ND_CMD_CALL_DSM:

I agree with your comment in the cover letter that this change should be a separate patch.

It bothers me that we'll block all ND_CMD_CALL_DSM in the read_only case. Let's leave ND_CMD_CALL_DSM out of this selection for now.

Quoted 12 lines · Dan Williams

Quoted 10 lines · Jerry Hoemann

Add ioctl command ND_CMD_CALL_DSM to acpi_nfit_ctl and __nd_ioctl which allow kernel to call a nvdimm's _DSM as a passthru without using the marshaling code of the nd_cmd_desc.

Signed-off-by Jerry Hoemann <[email protected]>

---
 drivers/acpi/nfit.c  | 109 ++++++++++++++++++++++++++++++++-------------------
 drivers/nvdimm/bus.c |  61 +++++++++++++++++++++-------
 2 files changed, 115 insertions(+), 55 deletions(-)

Quoted 2 lines

In general I'd like to see this patch remove the need to sprinkle "if (dsm_call)" throughout the implementation ... specific examples below:

The current code is exporting a very different interface for calling _DSM from the pass thru I'm proposing.

The current code explicitly knows the calling structure of each _DSM function. It knows the number and size of each input and output field. For variable size output functions the current kernel code knows which field describes the size of the output. The current code knows the dsm_mask for the _DSM.

For the pass thru that I'm proposing the kernel wouldn't need to know any of this. The information needed to make the _DSM call would be passed in by the caller. [ Yes, there are cases where some calls are made from within the kernel. But it would be those callers who use the data that needs to know about the data. ]

So the use of dsm_call marks where there is a fundamental difference in the two approaches. This is one reason why I didn't try to integrate these functions in my original submittal.

You previously expressed an interest in converting the user application to use a pass thru mode and deprecate the current ioctl. Is this something you're still interested in doing?

If yes, the work we need to do to integrate these two different approaches will just need to be undone. Further if we deprecate the current IOCTLs, then the nd_cmd_desc tables and related nd_cmd_in_size and nd_cmd_out_size could then be removed.

Quoted 5 lines

Quoted 5 lines

drivers/acpi/nfit.c +0 -0

index c1b8d03..e509145 100644
@@ -75,7 +75,11 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,

...

Quoted 4 lines

Quoted 1 line

+ __u64 rev = 1, func = cmd;

Why __u64 and not int for these? acpi_evaluate_dsm() takes an int for both so a bigger type here will be truncated down.

ACPI defines arguments to a _DSM as 64 bit quantities. We want the interface exported to the user to follow the ACPI spec. The variables above collect the value of rev or func from the two different sources (wrapper or legacy) and then passes to acpi_evaluate_dsm which defines the parameters as simple ints.

So, going from user interface to call of acpi_evaluate_dsm there will be a truncation somewhere.

Looking at acpi_evaluate_dsm(), it uses union acpi_object and fills in .integer.value for both rev and func. These are defined as u64.

So patching acpi_evaluate_dsm to make the rev and func parameters u64 might be do'able, but we'd still have potential sign issues with other callers to acpi_evaluate_dsm which look to be using simple ints in the call.

Do you want me to look at patching acpi_evaluate_dsm (and possibly its callers) as part of this patch set?

...

Quoted 13 lines

drivers/acpi/nfit.c

Quoted 12 lines

        /* fail write commands (when read-only) */
        if (read_only)

drivers/acpi/nfit.c +5 -4

-               switch (ioctl_cmd) {
-               case ND_IOCTL_VENDOR:
-               case ND_IOCTL_SET_CONFIG_DATA:
-               case ND_IOCTL_ARS_START:
+               switch (cmd) {
+               case ND_CMD_VENDOR:
+               case ND_CMD_SET_CONFIG_DATA:
+               case ND_CMD_ARS_START:
+               case ND_CMD_CALL_DSM:

Quoted 2 lines

I agree with your comment in the cover letter that this change should be a separate patch.

Will do.

Quoted 3 lines

It bothers me that we'll block all ND_CMD_CALL_DSM in the read_only case. Let's leave ND_CMD_CALL_DSM out of this selection for now.

Not thrilled here either, but it is the conservative approach for the kernel.

Since ND_CMD_CALL_DSM is a pass thru, the kernel doesn't have the knowledge whether the call being made is "read only" or not. Having ND_CMD_CALL_DSM in the switch doesn't prevent the user from making such calls, it only requires s/he opens the device for write.

Yes, still very much interested in this path. But the code will need to be around for quite awhile now that it has appeared in a released kernel and a released version of ndctl. So anything that makes the code more maintainable in the interim is beneficial.

Quoted 4 lines

If yes, the work we need to do to integrate these two different approaches will just need to be undone. Further if we deprecate the current IOCTLs, then the nd_cmd_desc tables and related nd_cmd_in_size and nd_cmd_out_size could then be removed.

I'm not seeing this as a large amount of work. Do you want to hand off this task to me?

Quoted 13 lines

ACPI defines arguments to a _DSM as 64 bit quantities. We want the interface exported to the user to follow the ACPI spec. The variables above collect the value of rev or func from the two different sources (wrapper or legacy) and then passes to acpi_evaluate_dsm which defines the parameters as simple ints.

So, going from user interface to call of acpi_evaluate_dsm there will be a truncation somewhere.

Looking at acpi_evaluate_dsm(), it uses union acpi_object and fills in .integer.value for both rev and func. These are defined as u64.

Quoted 6 lines

So patching acpi_evaluate_dsm to make the rev and func parameters u64 might be do'able, but we'd still have potential sign issues with other callers to acpi_evaluate_dsm which look to be using simple ints in the call.

Do you want me to look at patching acpi_evaluate_dsm (and possibly its callers) as part of this patch set?

Yes, updating the acpi_evaluate_dsm() definition seems the best choice.

Quoted 2 lines · Dan Williams

Yes, updating the acpi_evaluate_dsm() definition seems the best choice.

I have a patch for this. While not big (6 files), these files are outside of nvdimm and will have a two line of over a dozen reviewers/lists not previously reviewing this series.

Do you want me to send this patch as one of this series (w/ the extra reviewers?)

Quoted 7 lines · Jerry Hoemann

Quoted 2 lines · Dan Williams

Yes, updating the acpi_evaluate_dsm() definition seems the best choice.

I have a patch for this. While not big (6 files), these files are outside of nvdimm and will have a two line of over a dozen reviewers/lists not previously reviewing this series.

For an acpi change no need to cc all those folks and lists. Just cc the following for that change:

Bob Moore <[email protected]>
Lv Zheng <[email protected]>
Rafael J. Wysocki <[email protected]>
<[email protected]>

Why 6 files and not 2 for a prototype update? I wouldn't go touch existing callers of acpi_evaluate_dsm() if they have been living with the potential truncation all this time there's no need to change.

Quoted 3 lines

Do you want me to send this patch as one of this series (w/ the extra reviewers?)

Yes, send that patch with the series so the acpi developers have the context for what motivated the change.