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.

This supports DSM as defined in:

http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf
https://github.com/HewlettPackard/hpe-nvm/tree/master/Documentation

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

---

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.

This supports DSM as defined in:

http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf
https://github.com/HewlettPackard/hpe-nvm/tree/master/Documentation

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

---
 drivers/acpi/nfit.c  | 108 +++++++++++++++++++++++++++++++++++++++++++++------
 drivers/nvdimm/bus.c |  43 +++++++++++++++++++-
 2 files changed, 139 insertions(+), 12 deletions(-)

drivers/acpi/nfit.c +2 -0

index d0f35e6..7dffcb5 100644
@@ -56,6 +56,24 @@ struct nfit_table_prev {
        struct list_head flushes;
 };
 
+struct cmd_family_tbl {
+       enum nfit_uuids key_uuid;       /* Internal handle              */

drivers/acpi/nfit.c +47 -8

+       { NFIT_DEV_DIMM,        ND_TYPE_DIMM_INTEL1,    1},
+       { NFIT_DEV_DIMM_N_HPE1, ND_TYPE_DIMM_N_HPE1,    1},
+       { NFIT_DEV_DIMM_N_HPE2, ND_TYPE_DIMM_N_HPE2,    1},
+       { -1, -1, -1},
+};
+
 static u8 nfit_uuid[NFIT_UUID_MAX][16];
 
 const u8 *to_nfit_uuid(enum nfit_uuids id)

@@ -64,6 +82,19 @@ const u8 *to_nfit_uuid(enum nfit_uuids id)
 }
 EXPORT_SYMBOL(to_nfit_uuid);
 
+static const u8 *
+family_to_uuid(int type)
+{
+       struct cmd_family_tbl *tbl = nfit_cmd_family_tbl;
+       int i;
+
+       for (i = 0;  tbl[i].key_type >= 0 ; i++) {
+               if (tbl[i].key_type == type)
+                       return to_nfit_uuid(tbl[i].key_uuid);
+       }
+       return 0;
+}
+
 static struct acpi_nfit_desc *to_acpi_nfit_desc(
                struct nvdimm_bus_descriptor *nd_desc)
 {

@@ -171,8 +202,9 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
                unsigned int buf_len, int *cmd_rc)
 {
        struct acpi_nfit_desc *acpi_desc = to_acpi_nfit_desc(nd_desc);
-       const struct nd_cmd_desc *desc = NULL;
        union acpi_object in_obj, in_buf, *out_obj;
+       struct nd_cmd_pkg *call_dsm = NULL;
+       const struct nd_cmd_desc *desc = NULL;
        struct device *dev = acpi_desc->dev;
        const char *cmd_name, *dimm_name;
        unsigned long dsm_mask;

@@ -180,6 +212,12 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
        const u8 *uuid;
        u32 offset;
        int rc, i;
+       __u64 rev = 1, func = cmd;
+
+       if (cmd == ND_CMD_CALL) {
+               call_dsm = buf;
+               func = call_dsm->nd_command;
+       }
 
        if (nvdimm) {
                struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);

@@ -207,7 +245,7 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
        if (!desc || (cmd && (desc->out_num + desc->in_num == 0)))
                return -ENOTTY;
 
-       if (!test_bit(cmd, &dsm_mask))
+       if (!test_bit(func, &dsm_mask))
                return -ENOTTY;
 
        in_obj.type = ACPI_TYPE_PACKAGE;

@@ -222,21 +260,44 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
                in_buf.buffer.length += nd_cmd_in_size(nvdimm, cmd, desc,
                                i, buf);
 
+       if (call_dsm) {
+               /* must skip over package wrapper */
+               in_buf.buffer.pointer = (void *) &call_dsm->nd_payload;
+               in_buf.buffer.length = call_dsm->nd_size_in;
+               uuid = family_to_uuid(call_dsm->nd_family);
+               if (!uuid) {
+                       dev_dbg(dev, "%s:%s unsupported uuid\n", dimm_name,
+                                       cmd_name);
+                       return -EINVAL;
+               }
+       }
+
        if (IS_ENABLED(CONFIG_ACPI_NFIT_DEBUG)) {
-               dev_dbg(dev, "%s:%s cmd: %s input length: %d\n", __func__,
-                               dimm_name, cmd_name, in_buf.buffer.length);
-               print_hex_dump_debug(cmd_name, DUMP_PREFIX_OFFSET, 4,
-                               4, in_buf.buffer.pointer, min_t(u32, 128,
-                                       in_buf.buffer.length), true);
+               dev_dbg(dev, "%s:%s cmd: %d: %llu input length: %d\n", __func__,
+                               dimm_name, cmd, func, in_buf.buffer.length);
+               print_hex_dump_debug("nvdimm in  ", DUMP_PREFIX_OFFSET, 4, 4,
+                       in_buf.buffer.pointer,
+                       min_t(u32, 256, in_buf.buffer.length), true);
+
        }
 
-       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) {

drivers/acpi/nfit.c +33 -3

+
+               ACPI_FREE(out_obj);
+               return 0;
+       }
+
        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);

@@ -918,12 +979,30 @@ static struct nvdimm *acpi_nfit_dimm_by_handle(struct acpi_nfit_desc *acpi_desc,
        return NULL;
 }
 
+static int determine_uuid(acpi_handle handle, struct cmd_family_tbl *tbl,
+                                               u8 const **cmd_uuid)
+{
+       int i;
+
+       for (i = 0;  tbl[i].key_uuid >= 0 ; i++) {
+               const u8 *uuid = to_nfit_uuid(tbl[i].key_uuid);
+               int rev = tbl[i].rev;
+
+               if (acpi_check_dsm(handle, uuid, rev, 1ULL)) {
+                       *cmd_uuid = uuid;
+                       return 1;
+               }
+       }
+       return 0;
+}
+
+
 static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
                struct nfit_mem *nfit_mem, u32 device_handle)
 {
        struct acpi_device *adev, *adev_dimm;
        struct device *dev = acpi_desc->dev;
-       const u8 *uuid = to_nfit_uuid(NFIT_DEV_DIMM);
+       const u8 *uuid;
        int i;
 
        nfit_mem->dsm_mask = acpi_desc->dimm_dsm_force_en;

@@ -939,7 +1018,12 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
                return force_enable_dimms ? 0 : -ENODEV;
        }
 
-       for (i = ND_CMD_SMART; i <= ND_CMD_VENDOR; i++)
+       if (!determine_uuid(adev_dimm->handle, nfit_cmd_family_tbl,
+                                                       &nfit_mem->dsm_uuid))
+               return force_enable_dimms ? 0 : -ENODEV;
+
+       uuid = nfit_mem->dsm_uuid;
+       for (i = 0; i <= ND_MAX_CMD; i++)
                if (acpi_check_dsm(adev_dimm->handle, uuid, 1, 1ULL << i))
                        set_bit(i, &nfit_mem->dsm_mask);
 

@@ -1012,7 +1096,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
        if (!adev)
                return;
 
-       for (i = ND_CMD_ARS_CAP; i <= ND_CMD_CLEAR_ERROR; i++)
+       for (i = 0; i <= ND_CMD_CLEAR_ERROR; i++)
                if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
                        set_bit(i, &nd_desc->dsm_mask);
 }

@@ -2463,6 +2547,8 @@ static __init int nfit_init(void)
        acpi_str_to_uuid(UUID_PERSISTENT_VIRTUAL_CD, nfit_uuid[NFIT_SPA_PCD]);
        acpi_str_to_uuid(UUID_NFIT_BUS, nfit_uuid[NFIT_DEV_BUS]);
        acpi_str_to_uuid(UUID_NFIT_DIMM, nfit_uuid[NFIT_DEV_DIMM]);
+       acpi_str_to_uuid(UUID_NFIT_DIMM_N_HPE1, nfit_uuid[NFIT_DEV_DIMM_N_HPE1]);
+       acpi_str_to_uuid(UUID_NFIT_DIMM_N_HPE2, nfit_uuid[NFIT_DEV_DIMM_N_HPE2]);
 
        nfit_wq = create_singlethread_workqueue("nfit");
        if (!nfit_wq)

drivers/nvdimm/bus.c +0 -0

index 19f822d..0c1c4ab 100644

drivers/nvdimm/bus.c +7 -0

+               .out_num = 1,
+               .out_sizes = { UINT_MAX, },
+       },
 };
 
 const struct nd_cmd_desc *nd_cmd_bus_desc(int cmd)

@@ -500,6 +512,10 @@ u32 nd_cmd_in_size(struct nvdimm *nvdimm, int cmd,
                struct nd_cmd_vendor_hdr *hdr = buf;
 
                return hdr->in_length;
+       } else if (cmd == ND_CMD_CALL) {
+               struct nd_cmd_pkg *pkg = buf;
+
+               return pkg->nd_size_in;
        }
 
        return UINT_MAX;

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

This supports DSM as defined in:

http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf
https://github.com/HewlettPackard/hpe-nvm/tree/master/Documentation

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

---
 drivers/acpi/nfit.c  | 108 +++++++++++++++++++++++++++++++++++++++++++++------
 drivers/nvdimm/bus.c |  43 +++++++++++++++++++-
 2 files changed, 139 insertions(+), 12 deletions(-)

drivers/acpi/nfit.c +0 -0

index d0f35e6..7dffcb5 100644
@@ -56,6 +56,24 @@ struct nfit_table_prev {
        struct list_head flushes;
 };
 

drivers/acpi/nfit.c struct nfit_table_prev { · line 70

Quoted 12 lines

drivers/acpi/nfit.c +12 -0

+struct cmd_family_tbl {
+       enum nfit_uuids key_uuid;       /* Internal handle              */
+       int             key_type;       /* Exported handle              */
+       int             rev;            /* _DSM rev                     */
+};
+
+/*
+ * If adding new cmd family, determine maximum function index
+ */
+#define ND_MAX_CMD     20
+
+struct cmd_family_tbl nfit_cmd_family_tbl[] = {

Should be 'static', probably 'const' as well.

Quoted 7 lines

drivers/acpi/nfit.c +6 -0

+       { NFIT_DEV_DIMM,        ND_TYPE_DIMM_INTEL1,    1},
+       { NFIT_DEV_DIMM_N_HPE1, ND_TYPE_DIMM_N_HPE1,    1},
+       { NFIT_DEV_DIMM_N_HPE2, ND_TYPE_DIMM_N_HPE2,    1},
+       { -1, -1, -1},
+};
+

drivers/acpi/nfit.c static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, · line 295

Quoted 6 lines

drivers/acpi/nfit.c +5 -0

+       if (call_dsm) {
+               call_dsm->nd_fw_size = out_obj->buffer.length;
+               memcpy(call_dsm->nd_payload + call_dsm->nd_size_in,
+                       out_obj->buffer.pointer,
+                       min(call_dsm->nd_fw_size, call_dsm->nd_size_out));

Hmm, further down in this routine we return -ENXIO if the output payload is too small, 0 if the output payload exactly matches the size of the BIOS output, and a positive number of remaining bytes if the output payload is larger than the BIOS output. Yes, we can calculate this same result ourselves from the contents of the nd_cmd_pkg structure, but lets make the return value common for all the ioctls. Especially for the error case where we shouldn't successfully complete the ioctl if userspace failed to provide enough buffer space.

Quoted 7 lines

drivers/nvdimm/bus.c +0 -0

index 19f822d..0c1c4ab 100644
@@ -439,6 +439,12 @@ static const struct nd_cmd_desc __nd_cmd_dimm_descs[] = {
                .out_num = 3,
                .out_sizes = { 4, 4, UINT_MAX, },

drivers/nvdimm/bus.c static const struct nd_cmd_desc __nd_cmd_dimm_descs[] = { · line 444

Quoted 4 lines

drivers/nvdimm/bus.c +3 -0

        },
+       [ND_CMD_CALL] = {
+               .in_num = 2,
+               .in_sizes = {sizeof(struct nd_cmd_pkg), UINT_MAX, },

This caught my eye, and I'm not sure if checkpatch catches it, but lets have a space between the '{' and 'sizeof' to match the style of the other initializations in this array.

Quoted 6 lines

drivers/nvdimm/bus.c +3 -0

+               .out_num = 1,
+               .out_sizes = { UINT_MAX, },
+       },
 };
 

drivers/nvdimm/bus.c static const struct nd_cmd_desc __nd_cmd_bus_descs[] = { · line 484

Quoted 8 lines

drivers/nvdimm/bus.c +3 -0

 const struct nd_cmd_desc *nd_cmd_dimm_desc(int cmd)

@@ -473,6 +479,12 @@ static const struct nd_cmd_desc __nd_cmd_bus_descs[] = {
                .out_num = 3,
                .out_sizes = { 4, 4, 8, },
        },
+       [ND_CMD_CALL] = {
+               .in_num = 2,
+               .in_sizes = {sizeof(struct nd_cmd_pkg), UINT_MAX, },

Ditto.

drivers/nvdimm/bus.c u32 nd_cmd_out_size(struct nvdimm *nvdimm, int cmd, · line 543

Quoted 7 lines

drivers/nvdimm/bus.c +3 -0

@@ -522,6 +538,12 @@ u32 nd_cmd_out_size(struct nvdimm *nvdimm, int cmd,
                return out_field[1];
        else if (!nvdimm && cmd == ND_CMD_ARS_STATUS && idx == 2)
                return out_field[1] - 8;
+       else if (cmd == ND_CMD_CALL) {
+               struct nd_cmd_pkg *pkg =
+                               (struct nd_cmd_pkg *) in_field;

checkpatch should warn about a missing newline between declarations and code here.

Quoted 5 lines

drivers/nvdimm/bus.c +3 -0

+               return pkg->nd_size_out;
+       }
+
 

drivers/nvdimm/bus.c static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm, · line 613

Quoted 7 lines

drivers/nvdimm/bus.c +1 -0

        return UINT_MAX;
 }

@@ -588,7 +610,9 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
        unsigned int cmd = _IOC_NR(ioctl_cmd);
        void __user *p = (void __user *) arg;
        struct device *dev = &nvdimm_bus->dev;
+       struct nd_cmd_pkg call_dsm;

Sometimes this patch uses 'call_dsm' as the local variable name of a struct nd_cmd_pkg, and sometimes 'pkg'. Lets just use 'pkg' everywhere.

Quoted 24 lines · Dan Williams

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

This supports DSM as defined in:

http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf
https://github.com/HewlettPackard/hpe-nvm/tree/master/Documentation

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

---
 drivers/acpi/nfit.c  | 108 +++++++++++++++++++++++++++++++++++++++++++++------
 drivers/nvdimm/bus.c |  43 +++++++++++++++++++-
 2 files changed, 139 insertions(+), 12 deletions(-)

drivers/acpi/nfit.c +0 -0

index d0f35e6..7dffcb5 100644
@@ -56,6 +56,24 @@ struct nfit_table_prev {
        struct list_head flushes;
 };
 

Quoted 14 lines

drivers/acpi/nfit.c struct nfit_table_prev { · line 70

Quoted 12 lines

drivers/acpi/nfit.c +12 -0

+struct cmd_family_tbl {
+       enum nfit_uuids key_uuid;       /* Internal handle              */
+       int             key_type;       /* Exported handle              */
+       int             rev;            /* _DSM rev                     */
+};
+
+/*
+ * If adding new cmd family, determine maximum function index
+ */
+#define ND_MAX_CMD     20
+
+struct cmd_family_tbl nfit_cmd_family_tbl[] = {

Should be 'static', probably 'const' as well.

Yes.

Quoted 7 lines

Quoted 7 lines

drivers/acpi/nfit.c +6 -0

+       { NFIT_DEV_DIMM,        ND_TYPE_DIMM_INTEL1,    1},
+       { NFIT_DEV_DIMM_N_HPE1, ND_TYPE_DIMM_N_HPE1,    1},
+       { NFIT_DEV_DIMM_N_HPE2, ND_TYPE_DIMM_N_HPE2,    1},
+       { -1, -1, -1},
+};
+

...

Quoted 15 lines

drivers/acpi/nfit.c static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, · line 295

Quoted 6 lines

drivers/acpi/nfit.c +5 -0

+       if (call_dsm) {
+               call_dsm->nd_fw_size = out_obj->buffer.length;
+               memcpy(call_dsm->nd_payload + call_dsm->nd_size_in,
+                       out_obj->buffer.pointer,
+                       min(call_dsm->nd_fw_size, call_dsm->nd_size_out));

Hmm, further down in this routine we return -ENXIO if the output payload is too small, 0 if the output payload exactly matches the size of the BIOS output, and a positive number of remaining bytes if the output payload is larger than the BIOS output. Yes, we can calculate this same result ourselves from the contents of the nd_cmd_pkg structure, but lets make the return value common for all the ioctls. Especially for the error case where we shouldn't successfully complete the ioctl if userspace failed to provide enough buffer space.

drivers/acpi/nfit.c static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, · line 299

Quoted 4 lines

drivers/acpi/nfit.c +4 -0

+
+               ACPI_FREE(out_obj);
+               return 0;
+       }

Disagree. We've discussed this previously. The passthru interface needs to handle the case where the caller doesn't know in advance the size of the return. These cases are not errors, just a different semantic.

Quoted 75 lines · Jerry Hoemann

Quoted 24 lines · Dan Williams

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

This supports DSM as defined in:

http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf
https://github.com/HewlettPackard/hpe-nvm/tree/master/Documentation

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

---
 drivers/acpi/nfit.c  | 108 +++++++++++++++++++++++++++++++++++++++++++++------
 drivers/nvdimm/bus.c |  43 +++++++++++++++++++-
 2 files changed, 139 insertions(+), 12 deletions(-)

drivers/acpi/nfit.c +0 -0

index d0f35e6..7dffcb5 100644
@@ -56,6 +56,24 @@ struct nfit_table_prev {
        struct list_head flushes;
 };
 

Quoted 14 lines

drivers/acpi/nfit.c struct nfit_table_prev { · line 70

Quoted 12 lines

drivers/acpi/nfit.c +12 -0

+struct cmd_family_tbl {
+       enum nfit_uuids key_uuid;       /* Internal handle              */
+       int             key_type;       /* Exported handle              */
+       int             rev;            /* _DSM rev                     */
+};
+
+/*
+ * If adding new cmd family, determine maximum function index
+ */
+#define ND_MAX_CMD     20
+
+struct cmd_family_tbl nfit_cmd_family_tbl[] = {

Should be 'static', probably 'const' as well.

Yes.

Quoted 7 lines

Quoted 7 lines

drivers/acpi/nfit.c +6 -0

+       { NFIT_DEV_DIMM,        ND_TYPE_DIMM_INTEL1,    1},
+       { NFIT_DEV_DIMM_N_HPE1, ND_TYPE_DIMM_N_HPE1,    1},
+       { NFIT_DEV_DIMM_N_HPE2, ND_TYPE_DIMM_N_HPE2,    1},
+       { -1, -1, -1},
+};
+

...

Quoted 19 lines

drivers/acpi/nfit.c static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, · line 295

Quoted 6 lines

drivers/acpi/nfit.c +5 -0

+       if (call_dsm) {
+               call_dsm->nd_fw_size = out_obj->buffer.length;
+               memcpy(call_dsm->nd_payload + call_dsm->nd_size_in,
+                       out_obj->buffer.pointer,
+                       min(call_dsm->nd_fw_size, call_dsm->nd_size_out));

Hmm, further down in this routine we return -ENXIO if the output payload is too small, 0 if the output payload exactly matches the size of the BIOS output, and a positive number of remaining bytes if the output payload is larger than the BIOS output. Yes, we can calculate this same result ourselves from the contents of the nd_cmd_pkg structure, but lets make the return value common for all the ioctls. Especially for the error case where we shouldn't successfully complete the ioctl if userspace failed to provide enough buffer space.

drivers/acpi/nfit.c +4 -0

+
+               ACPI_FREE(out_obj);
+               return 0;
+       }

Quoted 3 lines

Disagree. We've discussed this previously. The passthru interface needs to handle the case where the caller doesn't know in advance the size of the return. These cases are not errors, just a different semantic.

I don't think the passthru interface deserves a different semantic. It does handle the case that the caller does not know the size, it returns -errno on too small, 0 on just right, and postive number on too big.

Quoted 76 lines · Dan Williams

Quoted 75 lines · Jerry Hoemann

Quoted 24 lines · Dan Williams

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

This supports DSM as defined in:

http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf
https://github.com/HewlettPackard/hpe-nvm/tree/master/Documentation

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

---
 drivers/acpi/nfit.c  | 108 +++++++++++++++++++++++++++++++++++++++++++++------
 drivers/nvdimm/bus.c |  43 +++++++++++++++++++-
 2 files changed, 139 insertions(+), 12 deletions(-)

drivers/acpi/nfit.c +0 -0

index d0f35e6..7dffcb5 100644
@@ -56,6 +56,24 @@ struct nfit_table_prev {
        struct list_head flushes;
 };
 

Quoted 14 lines

drivers/acpi/nfit.c struct nfit_table_prev { · line 70

Quoted 12 lines

drivers/acpi/nfit.c +12 -0

+struct cmd_family_tbl {
+       enum nfit_uuids key_uuid;       /* Internal handle              */
+       int             key_type;       /* Exported handle              */
+       int             rev;            /* _DSM rev                     */
+};
+
+/*
+ * If adding new cmd family, determine maximum function index
+ */
+#define ND_MAX_CMD     20
+
+struct cmd_family_tbl nfit_cmd_family_tbl[] = {

Should be 'static', probably 'const' as well.

Yes.

Quoted 7 lines

Quoted 7 lines

drivers/acpi/nfit.c +6 -0

+       { NFIT_DEV_DIMM,        ND_TYPE_DIMM_INTEL1,    1},
+       { NFIT_DEV_DIMM_N_HPE1, ND_TYPE_DIMM_N_HPE1,    1},
+       { NFIT_DEV_DIMM_N_HPE2, ND_TYPE_DIMM_N_HPE2,    1},
+       { -1, -1, -1},
+};
+

...

Quoted 19 lines

drivers/acpi/nfit.c static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, · line 295

Quoted 6 lines

drivers/acpi/nfit.c +5 -0

+       if (call_dsm) {
+               call_dsm->nd_fw_size = out_obj->buffer.length;
+               memcpy(call_dsm->nd_payload + call_dsm->nd_size_in,
+                       out_obj->buffer.pointer,
+                       min(call_dsm->nd_fw_size, call_dsm->nd_size_out));

Hmm, further down in this routine we return -ENXIO if the output payload is too small, 0 if the output payload exactly matches the size of the BIOS output, and a positive number of remaining bytes if the output payload is larger than the BIOS output. Yes, we can calculate this same result ourselves from the contents of the nd_cmd_pkg structure, but lets make the return value common for all the ioctls. Especially for the error case where we shouldn't successfully complete the ioctl if userspace failed to provide enough buffer space.

drivers/acpi/nfit.c +4 -0

+
+               ACPI_FREE(out_obj);
+               return 0;
+       }

Quoted 8 lines

Quoted 3 lines

Disagree. We've discussed this previously. The passthru interface needs to handle the case where the caller doesn't know in advance the size of the return. These cases are not errors, just a different semantic.

I don't think the passthru interface deserves a different semantic. It does handle the case that the caller does not know the size, it returns -errno on too small, 0 on just right, and postive number on too big.

I was using semantic in context of the underlying FW call. HPE fw has different semantics from Intel's fw.

-ENXIO is overloaded. More than one type of calling "error" could produce that return code. As such, user can't rely upon the contents of of payload to determine what correct size should be as user doesn't know why -ENXIO was returned.

On Wed, Apr 20, 2016 at 3:55 PM, Jerry Hoemann <[email protected]> wrote:

Quoted 12 lines

Quoted 8 lines

Quoted 3 lines

Disagree. We've discussed this previously. The passthru interface needs to handle the case where the caller doesn't know in advance the size of the return. These cases are not errors, just a different semantic.

I don't think the passthru interface deserves a different semantic. It does handle the case that the caller does not know the size, it returns -errno on too small, 0 on just right, and postive number on too big.

I was using semantic in context of the underlying FW call. HPE fw has different semantics from Intel's fw.

Quoted 4 lines

-ENXIO is overloaded. More than one type of calling "error" could produce that return code. As such, user can't rely upon the contents of of payload to determine what correct size should be as user doesn't know why -ENXIO was returned.

Ah, yes, this is indeed convincing. Ok, keep it where it is at. Apologies for making you argue that that again I had re-overlooked that aspect since the last time we discussed this.

Change the the name of:

nvdimm_bus_descriptor.dsm_mask
to
nvdimm_bus_descriptor.cmd_mask

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

---

Enable nfit_test to use the generic 'call_dsm' envelope.

Signed-off-by Dan Williams <[email protected]>

---

Enable nfit_test to use the generic 'call_dsm' envelope.

Signed-off-by Dan Williams <[email protected]>

---
 tools/testing/nvdimm/test/nfit.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

tools/testing/nvdimm/test/nfit.c +1 -0

index 1d3b8ce..5afc3d3 100644
@@ -336,12 +336,21 @@ static int nfit_test_ctl(struct nvdimm_bus_descriptor *nd_desc,
 {
        struct acpi_nfit_desc *acpi_desc = to_acpi_desc(nd_desc);
        struct nfit_test *t = container_of(acpi_desc, typeof(*t), acpi_desc);
+       unsigned int func = cmd;
        int i, rc = 0, __cmd_rc;
 

tools/testing/nvdimm/test/nfit.c +3 -1

+       }
+
        if (nvdimm) {
                struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
 

@@ -356,7 +365,7 @@ static int nfit_test_ctl(struct nvdimm_bus_descriptor *nd_desc,
                if (i >= ARRAY_SIZE(handle))
                        return -ENXIO;
 
-               switch (cmd) {
+               switch (func) {
                case ND_CMD_GET_CONFIG_SIZE:
                        rc = nfit_test_cmd_get_config_size(buf, buf_len);
                        break;

Quoted 22 lines · Jerry Hoemann

Enable nfit_test to use the generic 'call_dsm' envelope.

Signed-off-by Dan Williams <[email protected]>

---
 tools/testing/nvdimm/test/nfit.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

tools/testing/nvdimm/test/nfit.c +1 -0

index 1d3b8ce..5afc3d3 100644
@@ -336,12 +336,21 @@ static int nfit_test_ctl(struct nvdimm_bus_descriptor *nd_desc,
 {
        struct acpi_nfit_desc *acpi_desc = to_acpi_desc(nd_desc);
        struct nfit_test *t = container_of(acpi_desc, typeof(*t), acpi_desc);
+       unsigned int func = cmd;
        int i, rc = 0, __cmd_rc;
 
        if (!cmd_rc)
                cmd_rc = &__cmd_rc;
        *cmd_rc = 0;
 

tools/testing/nvdimm/test/nfit.c static int nfit_test_ctl(struct nvdimm_bus_descriptor *nd_desc, · line 351

Quoted 6 lines

tools/testing/nvdimm/test/nfit.c +6 -0

+       if (cmd == ND_CMD_CALL) {
+               struct nd_cmd_pkg *call_dsm = buf;
+
+               buf_len = call_dsm->nd_size_in + call_dsm->nd_size_out;
+               buf = (void *) call_dsm->nd_payload;
+               func = call_dsm->nd_command;

Same comment as patch1, now that we've switched from call_dsm to nd_cmd_pkg, let's use 'pkg' as the local variable name.

Quoted 19 lines

tools/testing/nvdimm/test/nfit.c +3 -1

+       }
+
        if (nvdimm) {
                struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
 

@@ -356,7 +365,7 @@ static int nfit_test_ctl(struct nvdimm_bus_descriptor *nd_desc,
                if (i >= ARRAY_SIZE(handle))
                        return -ENXIO;
 
-               switch (cmd) {
+               switch (func) {
                case ND_CMD_GET_CONFIG_SIZE:
                        rc = nfit_test_cmd_get_config_size(buf, buf_len);
                        break;

@@ -377,7 +386,7 @@ static int nfit_test_ctl(struct nvdimm_bus_descriptor *nd_desc,
                if (!nd_desc || !test_bit(cmd, &nd_desc->cmd_mask))
                        return -ENOTTY;
 

tools/testing/nvdimm/test/nfit.c static int nfit_test_ctl(struct nvdimm_bus_descriptor *nd_desc, · line 392

Quoted 5 lines

tools/testing/nvdimm/test/nfit.c +1 -1

-               switch (cmd) {
+               switch (func) {
                case ND_CMD_ARS_CAP:
                        rc = nfit_test_cmd_ars_cap(buf, buf_len);
                        break;

This patch is missing setting the ND_CMD_CALL bit in 'dimm_dsm_force_en' at the bottom of nfit_test0_setup(). That field should also be renamed to 'dimm_cmd_force_en' similar to the dsm_mask => cmd_mask rename..

DSMs have a mask of commands implemented on the DSM.

The current IOCTL interface exported for the NVDIMM DSM maps directly onto the underlying DSM.

With the addition of the pass thru mechanism, the IOCTL doesn't map directly onto the underlying DSM functions as the Pass thru isn't itself a DSM function and some DSM can use only the pass thru.

These changes separate out the concept of a command mask that applies to function that are supported by the IOCTL and a dsm mask that apply to the underlying firmware.

There is one noticiable exception, the cmd_mask == the dsm_mask for root functions.

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

---

DSMs have a mask of commands implemented on the DSM.

The current IOCTL interface exported for the NVDIMM DSM maps directly onto the underlying DSM.

With the addition of the pass thru mechanism, the IOCTL doesn't map directly onto the underlying DSM functions as the Pass thru isn't itself a DSM function and some DSM can use only the pass thru.

These changes separate out the concept of a command mask that applies to function that are supported by the IOCTL and a dsm mask that apply to the underlying firmware.

There is one noticiable exception, the cmd_mask == the dsm_mask for root functions.

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

---
 drivers/acpi/nfit.c        | 26 ++++++++++++++++++++++----
 drivers/acpi/nfit.h        |  1 +
 drivers/nvdimm/bus.c       | 10 ++++------
 drivers/nvdimm/dimm_devs.c | 12 ++++++------
 drivers/nvdimm/nd-core.h   |  2 +-
 include/linux/libnvdimm.h  |  2 +-
 6 files changed, 35 insertions(+), 18 deletions(-)

drivers/acpi/nfit.c +1 -0

index e3ff10b..fc3e7d9 100644
@@ -60,6 +60,7 @@ struct cmd_family_tbl {
        enum nfit_uuids key_uuid;       /* Internal handle              */
        int             key_type;       /* Exported handle              */
        int             rev;            /* _DSM rev                     */
+       int             does_ioctls;    /* Family supports all commands? */
 };
 
 /*

drivers/acpi/nfit.c +13 -0

        { -1, -1, -1},
 };
 

@@ -996,6 +997,19 @@ static int determine_uuid(acpi_handle handle, struct cmd_family_tbl *tbl,
        return 0;
 }
 
+static int determine_cmd_mask(struct cmd_family_tbl *tbl, u8 const *cmd_uuid)
+{
+       int i;
+
+       for (i = 0;  tbl[i].key_uuid >= 0 ; i++) {
+               const u8 *uuid = to_nfit_uuid(tbl[i].key_uuid);
+
+               if (memcmp(uuid, cmd_uuid, 16) == 0)
+                       return tbl[i].does_ioctls;
+       }
+       return 0;
+}
+
 
 static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
                struct nfit_mem *nfit_mem, u32 device_handle)

On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <[email protected]> wrote:

drivers/acpi/nfit.c struct cmd_family_tbl { · line 74

Quoted 7 lines

struct cmd_family_tbl nfit_cmd_family_tbl[] = {

drivers/acpi/nfit.c +3 -3

-       { NFIT_DEV_DIMM,        ND_TYPE_DIMM_INTEL1,    1},
-       { NFIT_DEV_DIMM_N_HPE1, ND_TYPE_DIMM_N_HPE1,    1},
-       { NFIT_DEV_DIMM_N_HPE2, ND_TYPE_DIMM_N_HPE2,    1},
+       { NFIT_DEV_DIMM,        ND_TYPE_DIMM_INTEL1,    1, 1},
+       { NFIT_DEV_DIMM_N_HPE1, ND_TYPE_DIMM_N_HPE1,    1, 0},
+       { NFIT_DEV_DIMM_N_HPE2, ND_TYPE_DIMM_N_HPE2,    1, 0},

Lets replace does_ioctl with a mask of the valid function numbers see below...

Quoted 14 lines

drivers/acpi/nfit.c +13 -0

+static int determine_cmd_mask(struct cmd_family_tbl *tbl, u8 const *cmd_uuid)
+{
+       int i;
+
+       for (i = 0;  tbl[i].key_uuid >= 0 ; i++) {
+               const u8 *uuid = to_nfit_uuid(tbl[i].key_uuid);
+
+               if (memcmp(uuid, cmd_uuid, 16) == 0)
+                       return tbl[i].does_ioctls;
+       }
+       return 0;
+}
+

drivers/acpi/nfit.c static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc, · line 1042

Quoted 5 lines

static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
struct nfit_mem *nfit_mem, u32 device_handle)

drivers/acpi/nfit.c +0 -0

@@ -1027,6 +1041,10 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
                if (acpi_check_dsm(adev_dimm->handle, uuid, 1, 1ULL << i))
                        set_bit(i, &nfit_mem->dsm_mask);

Now that there is no longer a 1:1 relationship with the commands the kernel knows and the DSM functions reported via acpi_check_dsm(), I want the kernel to mask off unknown function numbers in dsm_mask. At

a minimum we should be doing input validation on the publicly documented commands.

drivers/acpi/nfit.c static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc, · line 1047

Quoted 5 lines

drivers/acpi/nfit.c +4 -0

+       nfit_mem->cmd_mask = (1 << ND_CMD_CALL);
+       if (determine_cmd_mask(nfit_cmd_family_tbl, uuid))
+               nfit_mem->cmd_mask |= nfit_mem->dsm_mask;
+

For now just have an open coded if ND_TYPE_DIMM_INTEL1 cmd_mask == dsm_mask. Later I do want to have a translation routine to convert something like ND_TYPE_DIMM_N_HPE1 Function 2 (Get SMART) output into the ND_CMD_SMART format. Its ridiculous that we have two commands that do slightly different things.

The "ndctl list --dimms --health" utility doesn't want to care about vendor specific differences. However given that ND_TYPE_DIMM_N_HPE1 Function 2 output appears to be a super set of ND_CMD_SMART, I might deprecate ND_CMD_SMART and develop a new common SMART output format and have translation of the Intel data into that common superset format.

Quoted 1 line · Dan Williams

On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <[email protected]> wrote:

...

Quoted 8 lines

Quoted 7 lines

struct cmd_family_tbl nfit_cmd_family_tbl[] = {

drivers/acpi/nfit.c +3 -3

-       { NFIT_DEV_DIMM,        ND_TYPE_DIMM_INTEL1,    1},
-       { NFIT_DEV_DIMM_N_HPE1, ND_TYPE_DIMM_N_HPE1,    1},
-       { NFIT_DEV_DIMM_N_HPE2, ND_TYPE_DIMM_N_HPE2,    1},
+       { NFIT_DEV_DIMM,        ND_TYPE_DIMM_INTEL1,    1, 1},
+       { NFIT_DEV_DIMM_N_HPE1, ND_TYPE_DIMM_N_HPE1,    1, 0},
+       { NFIT_DEV_DIMM_N_HPE2, ND_TYPE_DIMM_N_HPE2,    1, 0},

Quoted 2 lines

Lets replace does_ioctl with a mask of the valid function numbers see below...

Is this new mask to be used in the construction of cmd_mask or in a bit and with dsm_mask?

....

Quoted 14 lines

Quoted 14 lines

drivers/acpi/nfit.c +13 -0

+static int determine_cmd_mask(struct cmd_family_tbl *tbl, u8 const *cmd_uuid)
+{
+       int i;
+
+       for (i = 0;  tbl[i].key_uuid >= 0 ; i++) {
+               const u8 *uuid = to_nfit_uuid(tbl[i].key_uuid);
+
+               if (memcmp(uuid, cmd_uuid, 16) == 0)
+                       return tbl[i].does_ioctls;
+       }
+       return 0;
+}
+

Quoted 9 lines

drivers/acpi/nfit.c static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc, · line 1042

Quoted 5 lines

static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
struct nfit_mem *nfit_mem, u32 device_handle)

drivers/acpi/nfit.c +0 -0

@@ -1027,6 +1041,10 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
                if (acpi_check_dsm(adev_dimm->handle, uuid, 1, 1ULL << i))
                        set_bit(i, &nfit_mem->dsm_mask);

Now that there is no longer a 1:1 relationship with the commands the kernel knows and the DSM functions reported via acpi_check_dsm(), I want the kernel to mask off unknown function numbers in dsm_mask. At

The dsm_mask is generated by what firmware reports it supports.

Are you concerned with FW returning back bogus data?

Quoted 2 lines

a minimum we should be doing input validation on the publicly documented commands.

cmd_msk is used by __nd_ioctl to verify calls function numbers are are valid (but not the arguments to the function.)

Quoted 11 lines

drivers/acpi/nfit.c static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc, · line 1047

Quoted 5 lines

drivers/acpi/nfit.c +4 -0

+       nfit_mem->cmd_mask = (1 << ND_CMD_CALL);
+       if (determine_cmd_mask(nfit_cmd_family_tbl, uuid))
+               nfit_mem->cmd_mask |= nfit_mem->dsm_mask;
+

For now just have an open coded if ND_TYPE_DIMM_INTEL1 cmd_mask == dsm_mask. Later I do want to have a translation routine to convert something like ND_TYPE_DIMM_N_HPE1 Function 2 (Get SMART) output into the ND_CMD_SMART format. Its ridiculous that we have two commands that do slightly different things.

I am not familiar with the phrase "open coded." Googling variations of the phrase suggests in-lining, that doesn't help me.

For ND_TYPE_DIMM_INTEL1, cmd_mask should never equal dsm_mask as cmd_mask should include (1 << ND_CMD_CALL) where dsm_mask shouldn't. Or, are you planning to add functions to the Intel DSM that would collide with the value of (1 << ND_CMD_CALL)? Do we need to change value of ND_CMD_CALL to avoid a collision?

Now if what you're wanting is determine_cmd_mask returning back a statically defined cmd mask such that we can do:

        nfit_mem->cmd_mask = determine_cmd_mask( ... );

And the values of these per UUID statically defined mask is what I am dynamically determining in the current version of my patch (e.g. a 7(?) line change) that is doable.

Quoted 14 lines · Jerry Hoemann

Quoted 1 line · Dan Williams

On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <[email protected]> wrote:

...

Quoted 8 lines

Quoted 7 lines

struct cmd_family_tbl nfit_cmd_family_tbl[] = {

drivers/acpi/nfit.c +3 -3

-       { NFIT_DEV_DIMM,        ND_TYPE_DIMM_INTEL1,    1},
-       { NFIT_DEV_DIMM_N_HPE1, ND_TYPE_DIMM_N_HPE1,    1},
-       { NFIT_DEV_DIMM_N_HPE2, ND_TYPE_DIMM_N_HPE2,    1},
+       { NFIT_DEV_DIMM,        ND_TYPE_DIMM_INTEL1,    1, 1},
+       { NFIT_DEV_DIMM_N_HPE1, ND_TYPE_DIMM_N_HPE1,    1, 0},
+       { NFIT_DEV_DIMM_N_HPE2, ND_TYPE_DIMM_N_HPE2,    1, 0},

Quoted 2 lines

Quoted 2 lines

Lets replace does_ioctl with a mask of the valid function numbers see below...

Quoted 4 lines

Is this new mask to be used in the construction of cmd_mask or in a bit and with dsm_mask?

The latter, "and with dsm_mask".

Quoted 15 lines · Dan Williams

Quoted 14 lines · Jerry Hoemann

Quoted 1 line · Dan Williams

On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <[email protected]> wrote:

...

Quoted 8 lines

Quoted 7 lines

struct cmd_family_tbl nfit_cmd_family_tbl[] = {

drivers/acpi/nfit.c +3 -3

-       { NFIT_DEV_DIMM,        ND_TYPE_DIMM_INTEL1,    1},
-       { NFIT_DEV_DIMM_N_HPE1, ND_TYPE_DIMM_N_HPE1,    1},
-       { NFIT_DEV_DIMM_N_HPE2, ND_TYPE_DIMM_N_HPE2,    1},
+       { NFIT_DEV_DIMM,        ND_TYPE_DIMM_INTEL1,    1, 1},
+       { NFIT_DEV_DIMM_N_HPE1, ND_TYPE_DIMM_N_HPE1,    1, 0},
+       { NFIT_DEV_DIMM_N_HPE2, ND_TYPE_DIMM_N_HPE2,    1, 0},

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Lets replace does_ioctl with a mask of the valid function numbers see below...

I think that the masking of dsm_mask is in addition to, not in replacement of what does_ioctl is doing in creating the cmd_mask. cmd_mask for *_N_HPE1 should only be (1 << ND_CMD_CALL) at least for now.

The pass thru calls return command mask. Previously, bit zero wasn't part of command mask, but as it is now, this left commands_show displaying "unknown" for function zero. Add an ioctl interface to return command mask.

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

---

The pass thru calls return command mask. Previously, bit zero wasn't part of command mask, but as it is now, this left commands_show displaying "unknown" for function zero. Add an ioctl interface to return command mask.

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

---
 drivers/nvdimm/bus.c       | 10 ++++++++--
 include/uapi/linux/ndctl.h |  9 +++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

Quoted 5 lines · Jerry Hoemann

The pass thru calls return command mask. Previously, bit zero wasn't part of command mask, but as it is now, this left commands_show displaying "unknown" for function zero. Add an ioctl interface to return command mask.

Quoted 5 lines

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

---
 drivers/nvdimm/bus.c       | 10 ++++++++--
 include/uapi/linux/ndctl.h |  9 +++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

Let's not add yet another ioctl for this... just add a 'dsm_mask' attribute to the nfit_mem device in acpi_nfit_dimm_attributes.

Quoted 12 lines · Dan Williams

Quoted 10 lines · Jerry Hoemann

The pass thru calls return command mask. Previously, bit zero wasn't part of command mask, but as it is now, this left commands_show displaying "unknown" for function zero. Add an ioctl interface to return command mask.

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

---
 drivers/nvdimm/bus.c       | 10 ++++++++--
 include/uapi/linux/ndctl.h |  9 +++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

Quoted 2 lines

Let's not add yet another ioctl for this... just add a 'dsm_mask' attribute to the nfit_mem device in acpi_nfit_dimm_attributes.

I don't understand this comment.

I can change just

        static const char * const names[] = {

include/uapi/linux/ndctl.h +2 -0

+               [ND_CMD_IMPLEMENTED] = "cmd_mask",
                [ND_CMD_ARS_CAP] = "ars_cap",
                [ND_CMD_ARS_START] = "ars_start",
                [ND_CMD_ARS_STATUS] = "ars_status",

@@ -150,6 +155,7 @@ static inline const char *nvdimm_bus_cmd_name(unsigned cmd)
 static inline const char *nvdimm_cmd_name(unsigned cmd)
 {
        static const char * const names[] = {
+               [ND_CMD_IMPLEMENTED] = "cmd_mask",
 

And have same effect w/o adding the full ioctl support.

But I don't see how modifying acpi_nfit_dimm_attributes affects what commands show will display.

Are you asking me to also add a new attribute to sysfs to display the command mask?

Quoted 13 lines · Jerry Hoemann

Quoted 12 lines · Dan Williams

Quoted 10 lines · Jerry Hoemann

The pass thru calls return command mask. Previously, bit zero wasn't part of command mask, but as it is now, this left commands_show displaying "unknown" for function zero. Add an ioctl interface to return command mask.

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

---
 drivers/nvdimm/bus.c       | 10 ++++++++--
 include/uapi/linux/ndctl.h |  9 +++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

Quoted 4 lines

Quoted 2 lines

Let's not add yet another ioctl for this... just add a 'dsm_mask' attribute to the nfit_mem device in acpi_nfit_dimm_attributes.

I don't understand this comment.

So, I'm trying to find the change that made bit zero part of the command mask, I think that is the source of the confusion. Why do we need to ever set bit zero in the cmd_mask?

Quoted 12 lines

I can change just

        static const char * const names[] = {

include/uapi/linux/ndctl.h +2 -0

+               [ND_CMD_IMPLEMENTED] = "cmd_mask",
                [ND_CMD_ARS_CAP] = "ars_cap",
                [ND_CMD_ARS_START] = "ars_start",
                [ND_CMD_ARS_STATUS] = "ars_status",

@@ -150,6 +155,7 @@ static inline const char *nvdimm_bus_cmd_name(unsigned cmd)
 static inline const char *nvdimm_cmd_name(unsigned cmd)
 {
        static const char * const names[] = {
+               [ND_CMD_IMPLEMENTED] = "cmd_mask",
 

Quoted 1 line

And have same effect w/o adding the full ioctl support.

...but there's no ND_IOCTL_IMPLEMENTED, so why would userspace ever care to know the name of a command it can't issue?

Quoted 3 lines

But I don't see how modifying acpi_nfit_dimm_attributes affects what commands show will display.

It doesn't, but I was wondering if you were trying to add support for discovering the dsm_mask via an ioctl.

Quoted 2 lines

Are you asking me to also add a new attribute to sysfs to display the command mask?

Yes, I think it would be useful to communicate the *dsm_mask* in sysfs, but the cmd_mask can stay as is without adding an ND_CMD_IMPLEMENTED entry.

Quoted 19 lines · Dan Williams

Quoted 17 lines · Jerry Hoemann

Quoted 6 lines · Dan Williams

Quoted 5 lines · Jerry Hoemann

The pass thru calls return command mask. Previously, bit zero wasn't part of command mask, but as it is now, this left commands_show displaying "unknown" for function zero. Add an ioctl interface to return command mask.

Quoted 8 lines

Quoted 5 lines

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

---
 drivers/nvdimm/bus.c       | 10 ++++++++--
 include/uapi/linux/ndctl.h |  9 +++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

Let's not add yet another ioctl for this... just add a 'dsm_mask' attribute to the nfit_mem device in acpi_nfit_dimm_attributes.

I don't understand this comment.

Quoted 4 lines

So, I'm trying to find the change that made bit zero part of the command mask, I think that is the source of the confusion. Why do we need to ever set bit zero in the cmd_mask?

In patch set
[RFC v9 1/5] nvdimm: Add IOCTL pass thru functions

static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
struct nfit_mem *nfit_mem, u32 device_handle)

 {
        struct acpi_device *adev, *adev_dimm;
        struct device *dev = acpi_desc->dev;

(quoted diff) +1 -1

-       const u8 *uuid = to_nfit_uuid(NFIT_DEV_DIMM);
+       const u8 *uuid;
        int i;
 
        nfit_mem->dsm_mask = acpi_desc->dimm_dsm_force_en;

@@ -939,7 +1018,12 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
                return force_enable_dimms ? 0 : -ENODEV;
        }
 

Quoted 12 lines

Quoted 12 lines

I can change just

        static const char * const names[] = {

include/uapi/linux/ndctl.h +2 -0

+               [ND_CMD_IMPLEMENTED] = "cmd_mask",
                [ND_CMD_ARS_CAP] = "ars_cap",
                [ND_CMD_ARS_START] = "ars_start",
                [ND_CMD_ARS_STATUS] = "ars_status",

@@ -150,6 +155,7 @@ static inline const char *nvdimm_bus_cmd_name(unsigned cmd)
 static inline const char *nvdimm_cmd_name(unsigned cmd)
 {
        static const char * const names[] = {
+               [ND_CMD_IMPLEMENTED] = "cmd_mask",
 

Quoted 4 lines

Quoted 1 line

And have same effect w/o adding the full ioctl support.

...but there's no ND_IOCTL_IMPLEMENTED, so why would userspace ever care to know the name of a command it can't issue?

I added 0 to dsm mask so a user applications could know which dsm function are actually implemented.

Quoted 3 lines

Yes, I think it would be useful to communicate the *dsm_mask* in sysfs, but the cmd_mask can stay as is without adding an ND_CMD_IMPLEMENTED entry.

I think this is a good idea and am completely happy to do it, but want to defer this until after we get basic functionality completed.

Quoted 7 lines · Jerry Hoemann

Quoted 3 lines · Dan Williams

Quoted 2 lines · Jerry Hoemann

Quoted 1 line · Dan Williams

On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <[email protected]> wrote:

        }

Review

Quoted 9 lines

(quoted diff) +6 -1

-       for (i = ND_CMD_SMART; i <= ND_CMD_VENDOR; i++)
+       if (!determine_uuid(adev_dimm->handle, nfit_cmd_family_tbl,
+                                                       &nfit_mem->dsm_uuid))
+               return force_enable_dimms ? 0 : -ENODEV;
+
+       uuid = nfit_mem->dsm_uuid;
+       for (i = 0; i <= ND_MAX_CMD; i++)
                if (acpi_check_dsm(adev_dimm->handle, uuid, 1, 1ULL << i))
                        set_bit(i, &nfit_mem->dsm_mask);

This is confusing the ND_CMD function number space with DSMs. We

already know the possible commands that may be supported because they are specified per-family. This goes back to my other request to limit the kernel to only handling known valid function numbers, i.e. don't probe for known invalid functions.

For_example:

unsigned long family_mask = nfit_cmd_family_tbl[family_id].family_mask;

for_each_set_bit(i, &family_mask, BITS_PER_LONG)

    if (acpi_check_dsm(adev_dimm->handle, uuid, 1, 1ULL << i))
        set_bit(i, &nfit_mem->dsm_mask);

Where, for example, family_mask is statically initialized to 0x1ff in the Intel case.

Quoted 18 lines

Quoted 16 lines

Quoted 12 lines

I can change just

        static const char * const names[] = {

include/uapi/linux/ndctl.h +2 -0

+               [ND_CMD_IMPLEMENTED] = "cmd_mask",
                [ND_CMD_ARS_CAP] = "ars_cap",
                [ND_CMD_ARS_START] = "ars_start",
                [ND_CMD_ARS_STATUS] = "ars_status",

@@ -150,6 +155,7 @@ static inline const char *nvdimm_bus_cmd_name(unsigned cmd)
 static inline const char *nvdimm_cmd_name(unsigned cmd)
 {
        static const char * const names[] = {
+               [ND_CMD_IMPLEMENTED] = "cmd_mask",
 

Quoted 1 line

And have same effect w/o adding the full ioctl support.

...but there's no ND_IOCTL_IMPLEMENTED, so why would userspace ever care to know the name of a command it can't issue?

Quoted 2 lines

I added 0 to dsm mask so a user applications could know which dsm function are actually implemented.

Right, I don't want to add an ioctl for discovering dsm_mask.

On Thu, Apr 21, 2016 at 11:18:40AM -0700, Dan Williams wrote:

Quoted 1 line

Right, I don't want to add an ioctl for discovering dsm_mask.

I can interpret your statement in multiple ways.

We need to be able to call the query command (function 0) in pass thru to support our firmware.

Now, if you're just saying you don't want to add

(quoted diff) +2 -0

+#define ND_IOCTL_CMD_MASK              _IOWR(ND_IOCTL, ND_CMD_IMPLEMENTED,\
+                                       struct nd_cmd_mask)

and other associated changes in ndctl.h, that is an entirely different and I can rework that patch.