[PATCH v3 0/3] nvdimm: Add an IOCTL pass thru for DSM calls

[PATCH v3 0/3] nvdimm: Add an IOCTL pass thru for DSM calls

[Jerry Hoemann]

The NVDIMM code in the kernel supports an IOCTL interface to user
space based upon the Intel Example DSM:

	http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf

This interface cannot be used by other NVDIMM DSMs that support
incompatible functions.

This patch set adds a generic "passthru" IOCTL interface which
is not tied to a particular DSM.

A new _IOC_NR ND_CMD_CALL_DSM == "10" is added for the pass thru call.

The new data structure nd_cmd_dsmcall_pkg serves as a wrapper for
the passthru calls.  This wrapper supplies the data that the kernel
needs to make the _DSM call.

Unlike the definitions of the _DSM functions themselves, the nd_cmd_dsmcall_pkg
provides the calling information (input/output sizes) in an uniform
manner making the kernel marshaling of the arguments straight
forward.

This shifts the marshaling burden from the kernel to the user
space application while still permitting the kernel to internally
calling _DSM functions.

The kernel functions __nd_ioctl and acpi_nfit_ctl were modified
to accomodate ND_CMD_CALL_DSM.

These changes are based upon the 4.3 kernel.


Changes in version 3:
---------------------
1. Changed name ND_CMD_PASSTHRU to ND_CMD_CALL_DSM.

2. Value of ND_CMD_CALL_DSM is 10, not 100.

3. Changed name of nd_passthru_pkg to nd_cmd_dsmcall_pkg.

4. Removed separate functions for handling ND_CMD_CALL_DSM.
   Moved functionality to __nd_ioctl and acpi_nfit_ctl proper.
   The resultant code looks very different from prior versions.

5. BUGFIX: __nd_ioctl: Change the if read_only switch to use
	 _IOC_NR cmd (not ioctl_cmd) for better protection.

	Do we want to make a stand alone patch for this issue?


Changes in version 2:
---------------------
1. Cleanup access mode check in nd_ioctl and nvdimm_ioctl.
2. Change name of ndn_pkg to nd_passthru_pkg
3. Adjust sizes in nd_passthru_pkg. DSM intergers are 64 bit.
4. No new ioctl type, instead tunnel into the existing number space.
5. Push down one function level where determine ioctl cmd type.
6. re-work diagnostic print/dump message in pass-thru functions.



Jerry Hoemann (3):
  nvdimm: Clean-up access mode check.
  nvdimm: Add wrapper for IOCTL pass thru
  nvdimm: Add IOCTL pass thru functions

 drivers/acpi/nfit.c        | 109 ++++++++++++++++++++++++++++-----------------
 drivers/nvdimm/bus.c       |  73 +++++++++++++++++++++---------
 include/uapi/linux/ndctl.h |  18 ++++++++
 3 files changed, 139 insertions(+), 61 deletions(-)

-- 
1.7.11.3

[PATCH v3 0/3] nvdimm: Add an IOCTL pass thru for DSM calls

[Jerry Hoemann]

The NVDIMM code in the kernel supports an IOCTL interface to user
space based upon the Intel Example DSM:

	http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf

This interface cannot be used by other NVDIMM DSMs that support
incompatible functions.

This patch set adds a generic "passthru" IOCTL interface which
is not tied to a particular DSM.

A new _IOC_NR ND_CMD_CALL_DSM == "10" is added for the pass thru call.

The new data structure nd_cmd_dsmcall_pkg serves as a wrapper for
the passthru calls.  This wrapper supplies the data that the kernel
needs to make the _DSM call.

Unlike the definitions of the _DSM functions themselves, the nd_cmd_dsmcall_pkg
provides the calling information (input/output sizes) in an uniform
manner making the kernel marshaling of the arguments straight
forward.

This shifts the marshaling burden from the kernel to the user
space application while still permitting the kernel to internally
calling _DSM functions.

The kernel functions __nd_ioctl and acpi_nfit_ctl were modified
to accomodate ND_CMD_CALL_DSM.

These changes are based upon the 4.3 kernel.


Changes in version 3:
---------------------
1. Changed name ND_CMD_PASSTHRU to ND_CMD_CALL_DSM.

2. Value of ND_CMD_CALL_DSM is 10, not 100.

3. Changed name of nd_passthru_pkg to nd_cmd_dsmcall_pkg.

4. Removed separate functions for handling ND_CMD_CALL_DSM.
   Moved functionality to __nd_ioctl and acpi_nfit_ctl proper.
   The resultant code looks very different from prior versions.

5. BUGFIX: __nd_ioctl: Change the if read_only switch to use
	 _IOC_NR cmd (not ioctl_cmd) for better protection.

	Do we want to make a stand alone patch for this issue?


Changes in version 2:
---------------------
1. Cleanup access mode check in nd_ioctl and nvdimm_ioctl.
2. Change name of ndn_pkg to nd_passthru_pkg
3. Adjust sizes in nd_passthru_pkg. DSM intergers are 64 bit.
4. No new ioctl type, instead tunnel into the existing number space.
5. Push down one function level where determine ioctl cmd type.
6. re-work diagnostic print/dump message in pass-thru functions.



Jerry Hoemann (3):
  nvdimm: Clean-up access mode check.
  nvdimm: Add wrapper for IOCTL pass thru
  nvdimm: Add IOCTL pass thru functions

 drivers/acpi/nfit.c        | 109 ++++++++++++++++++++++++++++-----------------
 drivers/nvdimm/bus.c       |  73 +++++++++++++++++++++---------
 include/uapi/linux/ndctl.h |  18 ++++++++
 3 files changed, 139 insertions(+), 61 deletions(-)

-- 
1.7.11.3

[PATCH v3 1/3] nvdimm: Clean-up access mode check.

[Jerry Hoemann]

Change nd_ioctl and nvdimm_ioctl access mode check to use O_RDONLY.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/nvdimm/bus.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 7e2c43f..1c81203 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -602,14 +602,14 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 static long nd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	long id = (long) file->private_data;
-	int rc = -ENXIO, read_only;
+	int rc = -ENXIO, ro;
 	struct nvdimm_bus *nvdimm_bus;
 
-	read_only = (O_RDWR != (file->f_flags & O_ACCMODE));
+	ro = ((file->f_flags & O_ACCMODE) == O_RDONLY);
 	mutex_lock(&nvdimm_bus_list_mutex);
 	list_for_each_entry(nvdimm_bus, &nvdimm_bus_list, list) {
 		if (nvdimm_bus->id == id) {
-			rc = __nd_ioctl(nvdimm_bus, NULL, read_only, cmd, arg);
+			rc = __nd_ioctl(nvdimm_bus, NULL, ro, cmd, arg);
 			break;
 		}
 	}
@@ -633,10 +633,10 @@ static int match_dimm(struct device *dev, void *data)
 
 static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-	int rc = -ENXIO, read_only;
+	int rc = -ENXIO, ro;
 	struct nvdimm_bus *nvdimm_bus;
 
-	read_only = (O_RDWR != (file->f_flags & O_ACCMODE));
+	ro = ((file->f_flags & O_ACCMODE) == O_RDONLY);
 	mutex_lock(&nvdimm_bus_list_mutex);
 	list_for_each_entry(nvdimm_bus, &nvdimm_bus_list, list) {
 		struct device *dev = device_find_child(&nvdimm_bus->dev,
@@ -647,7 +647,7 @@ static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 			continue;
 
 		nvdimm = to_nvdimm(dev);
-		rc = __nd_ioctl(nvdimm_bus, nvdimm, read_only, cmd, arg);
+		rc = __nd_ioctl(nvdimm_bus, nvdimm, ro, cmd, arg);
 		put_device(dev);
 		break;
 	}
-- 
1.7.11.3

[PATCH v3 1/3] nvdimm: Clean-up access mode check.

[Jerry Hoemann]

Change nd_ioctl and nvdimm_ioctl access mode check to use O_RDONLY.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/nvdimm/bus.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 7e2c43f..1c81203 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -602,14 +602,14 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 static long nd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	long id = (long) file->private_data;
-	int rc = -ENXIO, read_only;
+	int rc = -ENXIO, ro;
 	struct nvdimm_bus *nvdimm_bus;
 
-	read_only = (O_RDWR != (file->f_flags & O_ACCMODE));
+	ro = ((file->f_flags & O_ACCMODE) == O_RDONLY);
 	mutex_lock(&nvdimm_bus_list_mutex);
 	list_for_each_entry(nvdimm_bus, &nvdimm_bus_list, list) {
 		if (nvdimm_bus->id == id) {
-			rc = __nd_ioctl(nvdimm_bus, NULL, read_only, cmd, arg);
+			rc = __nd_ioctl(nvdimm_bus, NULL, ro, cmd, arg);
 			break;
 		}
 	}
@@ -633,10 +633,10 @@ static int match_dimm(struct device *dev, void *data)
 
 static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-	int rc = -ENXIO, read_only;
+	int rc = -ENXIO, ro;
 	struct nvdimm_bus *nvdimm_bus;
 
-	read_only = (O_RDWR != (file->f_flags & O_ACCMODE));
+	ro = ((file->f_flags & O_ACCMODE) == O_RDONLY);
 	mutex_lock(&nvdimm_bus_list_mutex);
 	list_for_each_entry(nvdimm_bus, &nvdimm_bus_list, list) {
 		struct device *dev = device_find_child(&nvdimm_bus->dev,
@@ -647,7 +647,7 @@ static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 			continue;
 
 		nvdimm = to_nvdimm(dev);
-		rc = __nd_ioctl(nvdimm_bus, nvdimm, read_only, cmd, arg);
+		rc = __nd_ioctl(nvdimm_bus, nvdimm, ro, cmd, arg);
 		put_device(dev);
 		break;
 	}
-- 
1.7.11.3

[PATCH v3 2/3] nvdimm: Add wrapper for IOCTL pass thru

[Jerry Hoemann]

Add struct nd_passthru_pkg which serves as a warapper for
the data being passed via a pass thru to a NVDIMM DSM.
This wrapper specifies the extra information in a uniform
manner allowing the kenrel to call a DSM without knowing
specifics of the DSM.

Add dsm_call command to nvdimm_bus_cmd_name and nvdimm_cmd_name.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 include/uapi/linux/ndctl.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index 5b4a4be..fdb30d2 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -109,6 +109,7 @@ enum {
 	ND_CMD_VENDOR_EFFECT_LOG_SIZE = 7,
 	ND_CMD_VENDOR_EFFECT_LOG = 8,
 	ND_CMD_VENDOR = 9,
+	ND_CMD_CALL_DSM = 10,
 };
 
 enum {
@@ -122,6 +123,7 @@ static inline const char *nvdimm_bus_cmd_name(unsigned cmd)
 		[ND_CMD_ARS_CAP] = "ars_cap",
 		[ND_CMD_ARS_START] = "ars_start",
 		[ND_CMD_ARS_STATUS] = "ars_status",
+		[ND_CMD_CALL_DSM] = "dsm_call",
 	};
 
 	if (cmd < ARRAY_SIZE(names) && names[cmd])
@@ -141,6 +143,7 @@ static inline const char *nvdimm_cmd_name(unsigned cmd)
 		[ND_CMD_VENDOR_EFFECT_LOG_SIZE] = "effect_size",
 		[ND_CMD_VENDOR_EFFECT_LOG] = "effect_log",
 		[ND_CMD_VENDOR] = "vendor",
+		[ND_CMD_CALL_DSM] = "dsm_call",
 	};
 
 	if (cmd < ARRAY_SIZE(names) && names[cmd])
@@ -204,4 +207,19 @@ enum ars_masks {
 	ARS_STATUS_MASK = 0x0000FFFF,
 	ARS_EXT_STATUS_SHIFT = 16,
 };
+
+
+struct nd_cmd_dsmcall_pkg {
+	struct {
+		__u8	dsm_uuid[16];
+		__u64	dsm_rev;		/* revision of dsm call  */
+		__u64	dsm_fun_idx;		/* DSM function id       */
+		__u32	dsm_in;			/* size of _DSM input    */
+		__u32	dsm_out;		/* size of user buffer   */
+		__u64	reserved[12];		/* reserved must be zero */
+		__u32	dsm_size;		/* size _DSM would write */
+	} h;
+	unsigned char dsm_buf[];		/* Contents of DSM call  */
+};
+
 #endif /* __NDCTL_H__ */
-- 
1.7.11.3

[PATCH v3 2/3] nvdimm: Add wrapper for IOCTL pass thru

[Jerry Hoemann]

Add struct nd_passthru_pkg which serves as a warapper for
the data being passed via a pass thru to a NVDIMM DSM.
This wrapper specifies the extra information in a uniform
manner allowing the kenrel to call a DSM without knowing
specifics of the DSM.

Add dsm_call command to nvdimm_bus_cmd_name and nvdimm_cmd_name.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 include/uapi/linux/ndctl.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index 5b4a4be..fdb30d2 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -109,6 +109,7 @@ enum {
 	ND_CMD_VENDOR_EFFECT_LOG_SIZE = 7,
 	ND_CMD_VENDOR_EFFECT_LOG = 8,
 	ND_CMD_VENDOR = 9,
+	ND_CMD_CALL_DSM = 10,
 };
 
 enum {
@@ -122,6 +123,7 @@ static inline const char *nvdimm_bus_cmd_name(unsigned cmd)
 		[ND_CMD_ARS_CAP] = "ars_cap",
 		[ND_CMD_ARS_START] = "ars_start",
 		[ND_CMD_ARS_STATUS] = "ars_status",
+		[ND_CMD_CALL_DSM] = "dsm_call",
 	};
 
 	if (cmd < ARRAY_SIZE(names) && names[cmd])
@@ -141,6 +143,7 @@ static inline const char *nvdimm_cmd_name(unsigned cmd)
 		[ND_CMD_VENDOR_EFFECT_LOG_SIZE] = "effect_size",
 		[ND_CMD_VENDOR_EFFECT_LOG] = "effect_log",
 		[ND_CMD_VENDOR] = "vendor",
+		[ND_CMD_CALL_DSM] = "dsm_call",
 	};
 
 	if (cmd < ARRAY_SIZE(names) && names[cmd])
@@ -204,4 +207,19 @@ enum ars_masks {
 	ARS_STATUS_MASK = 0x0000FFFF,
 	ARS_EXT_STATUS_SHIFT = 16,
 };
+
+
+struct nd_cmd_dsmcall_pkg {
+	struct {
+		__u8	dsm_uuid[16];
+		__u64	dsm_rev;		/* revision of dsm call  */
+		__u64	dsm_fun_idx;		/* DSM function id       */
+		__u32	dsm_in;			/* size of _DSM input    */
+		__u32	dsm_out;		/* size of user buffer   */
+		__u64	reserved[12];		/* reserved must be zero */
+		__u32	dsm_size;		/* size _DSM would write */
+	} h;
+	unsigned char dsm_buf[];		/* Contents of DSM call  */
+};
+
 #endif /* __NDCTL_H__ */
-- 
1.7.11.3

[PATCH v3 3/3] nvdimm: Add IOCTL pass thru functions

[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 <jerry.hoemann@hpe.com>
---
 drivers/acpi/nfit.c  | 109 ++++++++++++++++++++++++++++++++-------------------
 drivers/nvdimm/bus.c |  61 +++++++++++++++++++++-------
 2 files changed, 115 insertions(+), 55 deletions(-)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index c1b8d03..e509145 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -75,7 +75,11 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
 	acpi_handle handle;
 	const u8 *uuid;
 	u32 offset;
-	int rc, i;
+	int rc = 0, i;
+	__u64 rev = 1, func = cmd;
+
+	struct nd_cmd_dsmcall_pkg *pkg = buf;
+	int dsm_call = (cmd == ND_CMD_CALL_DSM);
 
 	if (nvdimm) {
 		struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
@@ -85,48 +89,62 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
 			return -ENOTTY;
 		dimm_name = nvdimm_name(nvdimm);
 		cmd_name = nvdimm_cmd_name(cmd);
-		dsm_mask = nfit_mem->dsm_mask;
+		if (!dsm_call)
+			dsm_mask = nfit_mem->dsm_mask;
 		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)
+			uuid = to_nfit_uuid(NFIT_DEV_BUS);
 		handle = adev->handle;
 		dimm_name = "bus";
 	}
 
-	if (!desc || (cmd && (desc->out_num + desc->in_num == 0)))
-		return -ENOTTY;
+	if (!dsm_call) {
+		if (!desc || (cmd && (desc->out_num + desc->in_num == 0)))
+			return -ENOTTY;
 
-	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;
-	in_buf.buffer.pointer = buf;
 	in_buf.buffer.length = 0;
 
-	/* libnvdimm has already validated the input envelope */
-	for (i = 0; i < desc->in_num; i++)
-		in_buf.buffer.length += nd_cmd_in_size(nvdimm, cmd, desc,
+	if (dsm_call) {
+		in_buf.buffer.pointer = (void *) &pkg->dsm_buf;
+		in_buf.buffer.length = pkg->h.dsm_in;
+		uuid = pkg->h.dsm_uuid;
+		rev  = pkg->h.dsm_rev;
+		func = pkg->h.dsm_fun_idx;
+	} 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)) {
-		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) {
 		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,
+			out_obj->buffer.pointer,
+			min_t(u32, 256, out_obj->buffer.length), true);
 	}
 
-	for (i = 0, offset = 0; i < desc->out_num; i++) {
+	if (dsm_call) {
+		pkg->h.dsm_size = out_obj->buffer.length;
+		memcpy(pkg->dsm_buf + pkg->h.dsm_in,
+			out_obj->buffer.pointer,
+			min(pkg->h.dsm_size, pkg->h.dsm_out));
+
+	} 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[] = {
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 1c81203..83900d50 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -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;
 		dimm_name = "bus";
 	}
 
-	if (!desc || (desc->out_num + desc->in_num == 0) ||
-			!test_bit(cmd, &dsm_mask))
-		return -ENOTTY;
+	if (!dsm_call)
+		if (!desc || (desc->out_num + desc->in_num == 0) ||
+				!test_bit(cmd, &dsm_mask))
+			return -ENOTTY;
 
 	/* fail write commands (when read-only) */
 	if (read_only)
-		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:
 			dev_dbg(&nvdimm_bus->dev, "'%s' command while read-only.\n",
 					nvdimm ? nvdimm_cmd_name(cmd)
 					: nvdimm_bus_cmd_name(cmd));
@@ -526,7 +533,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		}
 
 	/* process an input envelope */
-	for (i = 0; i < desc->in_num; i++) {
+	for (i = 0; !dsm_call && i < desc->in_num; i++) {
 		u32 in_size, copy;
 
 		in_size = nd_cmd_in_size(nvdimm, cmd, desc, i, in_env);
@@ -544,8 +551,31 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		in_len += in_size;
 	}
 
+	if (dsm_call) {
+		/* Caller must tell us size of input to _DSM. */
+		/* This may be bigger that the fixed portion of the pakcage */
+
+		if (copy_from_user(&pkg, p, sizeof(pkg))) {
+			rc = -EFAULT;
+			goto out;
+		}
+
+		in_len  = pkg.h.dsm_in;
+		out_len = pkg.h.dsm_out;
+		buf_len = sizeof(pkg.h) + in_len + out_len;
+
+		dev_dbg(dev, "%s:%s rev: %llu, idx: %llu, in: %zu, out: %zu, len %zu\n",
+			__func__, dimm_name,
+			pkg.h.dsm_rev, pkg.h.dsm_fun_idx,
+			in_len, out_len, buf_len);
+
+		for (i = 0; i < ARRAY_SIZE(pkg.h.reserved); i++)
+			if (pkg.h.reserved[i])
+				return -EINVAL;
+	}
+
 	/* process an output envelope */
-	for (i = 0; i < desc->out_num; i++) {
+	for (i = 0; !dsm_call && i < desc->out_num; i++) {
 		u32 out_size = nd_cmd_out_size(nvdimm, cmd, desc, i,
 				(u32 *) in_env, (u32 *) out_env);
 		u32 copy;
@@ -565,7 +595,9 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		out_len += out_size;
 	}
 
-	buf_len = out_len + in_len;
+	if (!dsm_call)
+		buf_len = out_len + in_len;
+
 	if (buf_len > ND_IOCTL_MAX_BUFLEN) {
 		dev_dbg(dev, "%s:%s cmd: %s buf_len: %zu > %d\n", __func__,
 				dimm_name, cmd_name, buf_len,
@@ -595,7 +627,8 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
  out_unlock:
 	nvdimm_bus_unlock(&nvdimm_bus->dev);
  out:
-	vfree(buf);
+	if (buf)
+		vfree(buf);
 	return rc;
 }
 
-- 
1.7.11.3

[PATCH v3 3/3] nvdimm: Add IOCTL pass thru functions

[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 <jerry.hoemann@hpe.com>
---
 drivers/acpi/nfit.c  | 109 ++++++++++++++++++++++++++++++++-------------------
 drivers/nvdimm/bus.c |  61 +++++++++++++++++++++-------
 2 files changed, 115 insertions(+), 55 deletions(-)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index c1b8d03..e509145 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -75,7 +75,11 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
 	acpi_handle handle;
 	const u8 *uuid;
 	u32 offset;
-	int rc, i;
+	int rc = 0, i;
+	__u64 rev = 1, func = cmd;
+
+	struct nd_cmd_dsmcall_pkg *pkg = buf;
+	int dsm_call = (cmd == ND_CMD_CALL_DSM);
 
 	if (nvdimm) {
 		struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
@@ -85,48 +89,62 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
 			return -ENOTTY;
 		dimm_name = nvdimm_name(nvdimm);
 		cmd_name = nvdimm_cmd_name(cmd);
-		dsm_mask = nfit_mem->dsm_mask;
+		if (!dsm_call)
+			dsm_mask = nfit_mem->dsm_mask;
 		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)
+			uuid = to_nfit_uuid(NFIT_DEV_BUS);
 		handle = adev->handle;
 		dimm_name = "bus";
 	}
 
-	if (!desc || (cmd && (desc->out_num + desc->in_num == 0)))
-		return -ENOTTY;
+	if (!dsm_call) {
+		if (!desc || (cmd && (desc->out_num + desc->in_num == 0)))
+			return -ENOTTY;
 
-	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;
-	in_buf.buffer.pointer = buf;
 	in_buf.buffer.length = 0;
 
-	/* libnvdimm has already validated the input envelope */
-	for (i = 0; i < desc->in_num; i++)
-		in_buf.buffer.length += nd_cmd_in_size(nvdimm, cmd, desc,
+	if (dsm_call) {
+		in_buf.buffer.pointer = (void *) &pkg->dsm_buf;
+		in_buf.buffer.length = pkg->h.dsm_in;
+		uuid = pkg->h.dsm_uuid;
+		rev  = pkg->h.dsm_rev;
+		func = pkg->h.dsm_fun_idx;
+	} 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)) {
-		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) {
 		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,
+			out_obj->buffer.pointer,
+			min_t(u32, 256, out_obj->buffer.length), true);
 	}
 
-	for (i = 0, offset = 0; i < desc->out_num; i++) {
+	if (dsm_call) {
+		pkg->h.dsm_size = out_obj->buffer.length;
+		memcpy(pkg->dsm_buf + pkg->h.dsm_in,
+			out_obj->buffer.pointer,
+			min(pkg->h.dsm_size, pkg->h.dsm_out));
+
+	} 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[] = {
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 1c81203..83900d50 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -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;
 		dimm_name = "bus";
 	}
 
-	if (!desc || (desc->out_num + desc->in_num == 0) ||
-			!test_bit(cmd, &dsm_mask))
-		return -ENOTTY;
+	if (!dsm_call)
+		if (!desc || (desc->out_num + desc->in_num == 0) ||
+				!test_bit(cmd, &dsm_mask))
+			return -ENOTTY;
 
 	/* fail write commands (when read-only) */
 	if (read_only)
-		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:
 			dev_dbg(&nvdimm_bus->dev, "'%s' command while read-only.\n",
 					nvdimm ? nvdimm_cmd_name(cmd)
 					: nvdimm_bus_cmd_name(cmd));
@@ -526,7 +533,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		}
 
 	/* process an input envelope */
-	for (i = 0; i < desc->in_num; i++) {
+	for (i = 0; !dsm_call && i < desc->in_num; i++) {
 		u32 in_size, copy;
 
 		in_size = nd_cmd_in_size(nvdimm, cmd, desc, i, in_env);
@@ -544,8 +551,31 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		in_len += in_size;
 	}
 
+	if (dsm_call) {
+		/* Caller must tell us size of input to _DSM. */
+		/* This may be bigger that the fixed portion of the pakcage */
+
+		if (copy_from_user(&pkg, p, sizeof(pkg))) {
+			rc = -EFAULT;
+			goto out;
+		}
+
+		in_len  = pkg.h.dsm_in;
+		out_len = pkg.h.dsm_out;
+		buf_len = sizeof(pkg.h) + in_len + out_len;
+
+		dev_dbg(dev, "%s:%s rev: %llu, idx: %llu, in: %zu, out: %zu, len %zu\n",
+			__func__, dimm_name,
+			pkg.h.dsm_rev, pkg.h.dsm_fun_idx,
+			in_len, out_len, buf_len);
+
+		for (i = 0; i < ARRAY_SIZE(pkg.h.reserved); i++)
+			if (pkg.h.reserved[i])
+				return -EINVAL;
+	}
+
 	/* process an output envelope */
-	for (i = 0; i < desc->out_num; i++) {
+	for (i = 0; !dsm_call && i < desc->out_num; i++) {
 		u32 out_size = nd_cmd_out_size(nvdimm, cmd, desc, i,
 				(u32 *) in_env, (u32 *) out_env);
 		u32 copy;
@@ -565,7 +595,9 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		out_len += out_size;
 	}
 
-	buf_len = out_len + in_len;
+	if (!dsm_call)
+		buf_len = out_len + in_len;
+
 	if (buf_len > ND_IOCTL_MAX_BUFLEN) {
 		dev_dbg(dev, "%s:%s cmd: %s buf_len: %zu > %d\n", __func__,
 				dimm_name, cmd_name, buf_len,
@@ -595,7 +627,8 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
  out_unlock:
 	nvdimm_bus_unlock(&nvdimm_bus->dev);
  out:
-	vfree(buf);
+	if (buf)
+		vfree(buf);
 	return rc;
 }
 
-- 
1.7.11.3

Re: [PATCH v3 3/3] nvdimm: Add IOCTL pass thru functions

[Dan Williams]

On Wed, Dec 2, 2015 at 1:05 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> 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 <jerry.hoemann@hpe.com>
> ---
>  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:

> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> index c1b8d03..e509145 100644
> --- a/drivers/acpi/nfit.c
> +++ b/drivers/acpi/nfit.c
> @@ -75,7 +75,11 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
>         acpi_handle handle;
>         const u8 *uuid;
>         u32 offset;
> -       int rc, i;
> +       int rc = 0, i;
> +       __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.

> +
> +       struct nd_cmd_dsmcall_pkg *pkg = buf;
> +       int dsm_call = (cmd == ND_CMD_CALL_DSM);
>
>         if (nvdimm) {
>                 struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
> @@ -85,48 +89,62 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
>                         return -ENOTTY;
>                 dimm_name = nvdimm_name(nvdimm);
>                 cmd_name = nvdimm_cmd_name(cmd);
> -               dsm_mask = nfit_mem->dsm_mask;
> +               if (!dsm_call)
> +                       dsm_mask = nfit_mem->dsm_mask;

Unpack the function id from the dsm_call so we can do the standard
check against dsm_mask.  Something like introduce 'dsm_func_id' that
is equal to 'cmd' in the legacy case.  That change to be explicit
about when we are referring to the "ioctl command number" vs the "dsm
function number" can be done in a lead in patch

>                 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)
> +                       uuid = to_nfit_uuid(NFIT_DEV_BUS);
>                 handle = adev->handle;
>                 dimm_name = "bus";
>         }
>
> -       if (!desc || (cmd && (desc->out_num + desc->in_num == 0)))
> -               return -ENOTTY;
> +       if (!dsm_call) {
> +               if (!desc || (cmd && (desc->out_num + desc->in_num == 0)))
> +                       return -ENOTTY;

Any reason to not create an nd_cmd_{dimm|bus}_desc() entry for this
command and reuse this check?

>
> -       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;
> -       in_buf.buffer.pointer = buf;
>         in_buf.buffer.length = 0;
>
> -       /* libnvdimm has already validated the input envelope */
> -       for (i = 0; i < desc->in_num; i++)
> -               in_buf.buffer.length += nd_cmd_in_size(nvdimm, cmd, desc,
> +       if (dsm_call) {
> +               in_buf.buffer.pointer = (void *) &pkg->dsm_buf;
> +               in_buf.buffer.length = pkg->h.dsm_in;
> +               uuid = pkg->h.dsm_uuid;

'uuid' is determined above.

> +               rev  = pkg->h.dsm_rev;
> +               func = pkg->h.dsm_fun_idx;

This can be unified with the legacy case when func == cmd.

> +       } 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)) {
> -               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);

Would be nice to have both the cmd name and the dsm function name in
the debug output.  Something like:

dev_dbg("%s %s\n", cmd_name, cmd == func ? "" : func_name)

>         }
>
> -       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,
> +                       out_obj->buffer.pointer,
> +                       min_t(u32, 256, out_obj->buffer.length), true);
>         }
>
> -       for (i = 0, offset = 0; i < desc->out_num; i++) {
> +       if (dsm_call) {
> +               pkg->h.dsm_size = out_obj->buffer.length;
> +               memcpy(pkg->dsm_buf + pkg->h.dsm_in,
> +                       out_obj->buffer.pointer,
> +                       min(pkg->h.dsm_size, pkg->h.dsm_out));

Reuse nd_cmd_out_size()?  It already supports variable-size fields.

> +
> +       } 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[] = {
> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
> index 1c81203..83900d50 100644
> --- a/drivers/nvdimm/bus.c
> +++ b/drivers/nvdimm/bus.c
> @@ -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;
>                 dimm_name = "bus";
>         }
>
> -       if (!desc || (desc->out_num + desc->in_num == 0) ||
> -                       !test_bit(cmd, &dsm_mask))
> -               return -ENOTTY;
> +       if (!dsm_call)
> +               if (!desc || (desc->out_num + desc->in_num == 0) ||
> +                               !test_bit(cmd, &dsm_mask))
> +                       return -ENOTTY;

Same comments as the acpi_nfit_ctl path.

>
>         /* fail write commands (when read-only) */
>         if (read_only)
> -               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.

I think we'll need to delete the protection going forward because we
won't be teaching the kernel about new DSM function numbers.  For
security purposes I'm looking at adding a mechanism for userspace to
disable commands by function number until next reboot.

Re: [PATCH v3 3/3] nvdimm: Add IOCTL pass thru functions

[Dan Williams]

On Wed, Dec 2, 2015 at 1:05 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> 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 <jerry.hoemann@hpe.com>
> ---
>  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:

> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> index c1b8d03..e509145 100644
> --- a/drivers/acpi/nfit.c
> +++ b/drivers/acpi/nfit.c
> @@ -75,7 +75,11 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
>         acpi_handle handle;
>         const u8 *uuid;
>         u32 offset;
> -       int rc, i;
> +       int rc = 0, i;
> +       __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.

> +
> +       struct nd_cmd_dsmcall_pkg *pkg = buf;
> +       int dsm_call = (cmd == ND_CMD_CALL_DSM);
>
>         if (nvdimm) {
>                 struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
> @@ -85,48 +89,62 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
>                         return -ENOTTY;
>                 dimm_name = nvdimm_name(nvdimm);
>                 cmd_name = nvdimm_cmd_name(cmd);
> -               dsm_mask = nfit_mem->dsm_mask;
> +               if (!dsm_call)
> +                       dsm_mask = nfit_mem->dsm_mask;

Unpack the function id from the dsm_call so we can do the standard
check against dsm_mask.  Something like introduce 'dsm_func_id' that
is equal to 'cmd' in the legacy case.  That change to be explicit
about when we are referring to the "ioctl command number" vs the "dsm
function number" can be done in a lead in patch

>                 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)
> +                       uuid = to_nfit_uuid(NFIT_DEV_BUS);
>                 handle = adev->handle;
>                 dimm_name = "bus";
>         }
>
> -       if (!desc || (cmd && (desc->out_num + desc->in_num == 0)))
> -               return -ENOTTY;
> +       if (!dsm_call) {
> +               if (!desc || (cmd && (desc->out_num + desc->in_num == 0)))
> +                       return -ENOTTY;

Any reason to not create an nd_cmd_{dimm|bus}_desc() entry for this
command and reuse this check?

>
> -       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;
> -       in_buf.buffer.pointer = buf;
>         in_buf.buffer.length = 0;
>
> -       /* libnvdimm has already validated the input envelope */
> -       for (i = 0; i < desc->in_num; i++)
> -               in_buf.buffer.length += nd_cmd_in_size(nvdimm, cmd, desc,
> +       if (dsm_call) {
> +               in_buf.buffer.pointer = (void *) &pkg->dsm_buf;
> +               in_buf.buffer.length = pkg->h.dsm_in;
> +               uuid = pkg->h.dsm_uuid;

'uuid' is determined above.

> +               rev  = pkg->h.dsm_rev;
> +               func = pkg->h.dsm_fun_idx;

This can be unified with the legacy case when func == cmd.

> +       } 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)) {
> -               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);

Would be nice to have both the cmd name and the dsm function name in
the debug output.  Something like:

dev_dbg("%s %s\n", cmd_name, cmd == func ? "" : func_name)

>         }
>
> -       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,
> +                       out_obj->buffer.pointer,
> +                       min_t(u32, 256, out_obj->buffer.length), true);
>         }
>
> -       for (i = 0, offset = 0; i < desc->out_num; i++) {
> +       if (dsm_call) {
> +               pkg->h.dsm_size = out_obj->buffer.length;
> +               memcpy(pkg->dsm_buf + pkg->h.dsm_in,
> +                       out_obj->buffer.pointer,
> +                       min(pkg->h.dsm_size, pkg->h.dsm_out));

Reuse nd_cmd_out_size()?  It already supports variable-size fields.

> +
> +       } 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[] = {
> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
> index 1c81203..83900d50 100644
> --- a/drivers/nvdimm/bus.c
> +++ b/drivers/nvdimm/bus.c
> @@ -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;
>                 dimm_name = "bus";
>         }
>
> -       if (!desc || (desc->out_num + desc->in_num == 0) ||
> -                       !test_bit(cmd, &dsm_mask))
> -               return -ENOTTY;
> +       if (!dsm_call)
> +               if (!desc || (desc->out_num + desc->in_num == 0) ||
> +                               !test_bit(cmd, &dsm_mask))
> +                       return -ENOTTY;

Same comments as the acpi_nfit_ctl path.

>
>         /* fail write commands (when read-only) */
>         if (read_only)
> -               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.

I think we'll need to delete the protection going forward because we
won't be teaching the kernel about new DSM function numbers.  For
security purposes I'm looking at adding a mechanism for userspace to
disable commands by function number until next reboot.

Re: [PATCH v3 3/3] nvdimm: Add IOCTL pass thru functions

[Jerry Hoemann]

On Tue, Dec 08, 2015 at 06:10:20PM -0800, Dan Williams wrote:
> On Wed, Dec 2, 2015 at 1:05 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> > 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 <jerry.hoemann@hpe.com>
> > ---
> >  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:


 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.



> 
> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> > index c1b8d03..e509145 100644
> > --- a/drivers/acpi/nfit.c
> > +++ b/drivers/acpi/nfit.c
> > @@ -75,7 +75,11 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,

 ...

> > +       __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?


...



> >
> >         /* fail write commands (when read-only) */
> >         if (read_only)
> > -               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.

  Will do.

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


-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

-- 

Re: [PATCH v3 3/3] nvdimm: Add IOCTL pass thru functions

[Jerry Hoemann]

On Tue, Dec 08, 2015 at 06:10:20PM -0800, Dan Williams wrote:
> On Wed, Dec 2, 2015 at 1:05 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> > 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 <jerry.hoemann@hpe.com>
> > ---
> >  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:


 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.



> 
> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> > index c1b8d03..e509145 100644
> > --- a/drivers/acpi/nfit.c
> > +++ b/drivers/acpi/nfit.c
> > @@ -75,7 +75,11 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,

 ...

> > +       __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?


...



> >
> >         /* fail write commands (when read-only) */
> >         if (read_only)
> > -               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.

  Will do.

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


-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

-- 

Re: [PATCH v3 3/3] nvdimm: Add IOCTL pass thru functions

[Dan Williams]

On Wed, Dec 9, 2015 at 4:24 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>
> On Tue, Dec 08, 2015 at 06:10:20PM -0800, Dan Williams wrote:
>> On Wed, Dec 2, 2015 at 1:05 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> > 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 <jerry.hoemann@hpe.com>
>> > ---
>> >  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:
>
>
>  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. ]

The kernel only needs to know that this ioctl command has one
variable-size input and one variable-size output buffer that need to
be read out of the envelope.  That can be represented with the current
field buffer size helpers.

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

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.

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

>> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
>> > index c1b8d03..e509145 100644
>> > --- a/drivers/acpi/nfit.c
>> > +++ b/drivers/acpi/nfit.c
>> > @@ -75,7 +75,11 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
>
>  ...
>
>> > +       __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?

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

>
>
> ...
>
>
>
>> >
>> >         /* fail write commands (when read-only) */
>> >         if (read_only)
>> > -               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.
>
>   Will do.
>
>>
>> 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.

Ok let's keep it for now.

Re: [PATCH v3 3/3] nvdimm: Add IOCTL pass thru functions

[Dan Williams]

On Wed, Dec 9, 2015 at 4:24 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>
> On Tue, Dec 08, 2015 at 06:10:20PM -0800, Dan Williams wrote:
>> On Wed, Dec 2, 2015 at 1:05 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> > 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 <jerry.hoemann@hpe.com>
>> > ---
>> >  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:
>
>
>  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. ]

The kernel only needs to know that this ioctl command has one
variable-size input and one variable-size output buffer that need to
be read out of the envelope.  That can be represented with the current
field buffer size helpers.

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

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.

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

>> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
>> > index c1b8d03..e509145 100644
>> > --- a/drivers/acpi/nfit.c
>> > +++ b/drivers/acpi/nfit.c
>> > @@ -75,7 +75,11 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
>
>  ...
>
>> > +       __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?

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

>
>
> ...
>
>
>
>> >
>> >         /* fail write commands (when read-only) */
>> >         if (read_only)
>> > -               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.
>
>   Will do.
>
>>
>> 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.

Ok let's keep it for now.

Re: [PATCH v3 3/3] nvdimm: Add IOCTL pass thru functions

[Jerry Hoemann]

On Wed, Dec 09, 2015 at 04:48:55PM -0800, Dan Williams wrote:
> 
> 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.
> 
> >  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?


I have implemented this and will send as part of next version.


> 
> > 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?
> 
> 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?)

Or as separate stand alone to just those reviewers (and yourself.)
(and if this option, before or after this series.)



-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

Re: [PATCH v3 3/3] nvdimm: Add IOCTL pass thru functions

[Jerry Hoemann]

On Wed, Dec 09, 2015 at 04:48:55PM -0800, Dan Williams wrote:
> 
> 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.
> 
> >  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?


I have implemented this and will send as part of next version.


> 
> > 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?
> 
> 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?)

Or as separate stand alone to just those reviewers (and yourself.)
(and if this option, before or after this series.)



-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

Re: [PATCH v3 3/3] nvdimm: Add IOCTL pass thru functions

[Dan Williams]

On Fri, Dec 11, 2015 at 10:09 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Wed, Dec 09, 2015 at 04:48:55PM -0800, Dan Williams wrote:
>> 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 <robert.moore@intel.com>
Lv Zheng <lv.zheng@intel.com>
Rafael J. Wysocki <rafael.j.wysocki@intel.com>
<linux-acpi@vger.kernel.org>

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.

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

Re: [PATCH v3 3/3] nvdimm: Add IOCTL pass thru functions

[Dan Williams]

On Fri, Dec 11, 2015 at 10:09 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Wed, Dec 09, 2015 at 04:48:55PM -0800, Dan Williams wrote:
>> 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 <robert.moore@intel.com>
Lv Zheng <lv.zheng@intel.com>
Rafael J. Wysocki <rafael.j.wysocki@intel.com>
<linux-acpi@vger.kernel.org>

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.

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

Re: [PATCH v3 3/3] nvdimm: Add IOCTL pass thru functions

[Jerry Hoemann]

On Fri, Dec 11, 2015 at 10:18:59AM -0800, Dan Williams wrote:
> On Fri, Dec 11, 2015 at 10:09 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> > On Wed, Dec 09, 2015 at 04:48:55PM -0800, Dan Williams wrote:
> >> 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 <robert.moore@intel.com>
> Lv Zheng <lv.zheng@intel.com>
> Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> <linux-acpi@vger.kernel.org>
> 
> 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.


Wasn't sure the level of lint checking being done on the linux kernel.
The examples were all of the variety of a caller using and int that
would be passed to a function with a parameter that would now be
an u64.  I think Lint can detect such issues.

In addition to acpi_evaluate_dsm,  there are also:
acpi_check_dsm, acpi_evaluate_dsm_typed using int for these.

Then there were other functions outside of acpica that passed
value in ints that went to one of the three functions above.

But, I can certainly limit the change to the acpica function
declaration/definition.  From what I could see the other
code would still work, just have potential lint issues.

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

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

Re: [PATCH v3 3/3] nvdimm: Add IOCTL pass thru functions

[Jerry Hoemann]

On Fri, Dec 11, 2015 at 10:18:59AM -0800, Dan Williams wrote:
> On Fri, Dec 11, 2015 at 10:09 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> > On Wed, Dec 09, 2015 at 04:48:55PM -0800, Dan Williams wrote:
> >> 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 <robert.moore@intel.com>
> Lv Zheng <lv.zheng@intel.com>
> Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> <linux-acpi@vger.kernel.org>
> 
> 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.


Wasn't sure the level of lint checking being done on the linux kernel.
The examples were all of the variety of a caller using and int that
would be passed to a function with a parameter that would now be
an u64.  I think Lint can detect such issues.

In addition to acpi_evaluate_dsm,  there are also:
acpi_check_dsm, acpi_evaluate_dsm_typed using int for these.

Then there were other functions outside of acpica that passed
value in ints that went to one of the three functions above.

But, I can certainly limit the change to the acpica function
declaration/definition.  From what I could see the other
code would still work, just have potential lint issues.

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

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------