[RFC v9 0/5] nvdimm: Add an IOCTL pass thru for DSM calls

[RFC v9 0/5] 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.

An alternative DSM specification for Type N DSM being developed
by Hewlett Packard Enterprise can be found at:

	https://github.com/HewlettPackard/hpe-nvm/tree/master/Documentation

To accommodate multiple and conflicting DSM specifications, this patch
set adds a generic "pass-thru" IOCTL interface which is not tied to
a particular DSM.

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

The new data structure nd_cmd_pkg serves as a wrapper for the
pass-thru 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_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
call _DSM functions.

The kernel functions __nd_ioctl and acpi_nfit_ctl were modified
to accommodate ND_CMD_CALL.


Changes in version 9:
---------------------
0. Based on https://git.kernel.org/cgit/linux/kernel/git/djbw/nvdimm.git/log/?h=for-4.7/dsm

1. Fixed a broken printk statement in error path.

Addressed the following code review requests:

1. Separated determination of uuid and dsm_mask into separate functions.

2. Reverted to the repeatedly calling acpi_check_dsm for each bit in
   dsm_mask instead of just calling firmware once.  Function is
   called 22 times per nvdimm.

4. Removed bit mask from nfit_cmd_family_tbl.

5. Added separate cmd_mask to struct nfit_mem.

6. Changed *dsm_mask to cmd_mask in struct nvdimm.
   
7. changed __nd_ioctl to filter based upon cmd_mask not dsm_mask.




Changes in version 8:
---------------------
1. augmented family_to_uuid() to return uuid.  This to address bug
   in prior version where acpi_nfit_ctl wasn't updating uuid
   with value associated with command family.

2. patch 0006 changes name of nvdimm_bus_descriptor.dsm_mask to .cmd_mask

3. patch 0008 adds field cmd_ioctl if kernel supports full ioctl
   as with Intel example dsm.

4. patch 0009 make determination if kernel supports the full
   cmd_ioctl for that dsm.  Updates the commands_show function
   to invert the sense of display of commands.  All dsm support
   pass-thru, only Intel example support the full ioctl interface.

5. patch 0010 adds explicit ioctl interface to return command mask.
   This was done in part to avoid "unknown" command in sysfs.



Changes in version 7:
--------------------
0. change name ND_CMD_CALL_DSM to ND_CMD_CALL
    - part of abstracting out DSM missed in version 6.

1. change name in struct nd_call_dsm
    a) "ncp_" -> "nd_"
    b) ncp_pot_size -> nd_fw_size
    c) ncp_type -> nd_family
    o) cascade name changes to other patches

2. Expanded comment around data structure nd_cmd_pkg

3. At Dan's request, hard coding "root" UUID.
    a) retract extension of dsm_uuid to nvdimm_bus_descriptor.
    b) reverted nfit.c/acpi_nfit_init_dsms() with the exception of
	allowing function 0 in mask.

4. At Dan's request, removed "rev" from nd_cmd_pkg.  Hard-coding
    use of rev "1" in acpi_nfit_ctl.


Changes in version 6:
---------------------
Built against
git://git.kernel.org/pub/scm/linux/kernel/git/djbw/nvdimm.git
libnvdimm-pending

0. Patches "Clean-up access mode check" and "Fix security issue with DSM IOCTL"
   already in above libnvdimm-pending.  So omitted here.

1. Incorporated changes from Dan's RFC patch set
   https://lists.01.org/pipermail/linux-nvdimm/2016-January/004049.html

2. Dan asked me to abstract out the DSM aspects from the ndm_cmd_dsmcall_pkg.
   This became nd_cmd_pkg.  UUIDs are no longer passed in from
   user applications.

3. To accommodate multiple UUIDS, added table cmd_type_tbl which is used
   to determine UUID for the acpi object by calling function 0 for
   each UUID in table until success.

   This table also provides a MASK field that the kernel can use
   to exclude functions being called.

   This table can be thought of a list of "acceptable" DSMs.

4. The cmd_type_tbl is also used by acpi_nfit_ctl to map the
   external handle of calls to internal handle, UUID.

   Note, code only validates that the requested type of call is one in
   cmd_type_tbl, but it might not necessarily be the same found during
   acpi_nfit_add_dimm.  The ACPI SPEC appears to allow and firmware
   does implement multiple UUID per object.

   In the case where type is in table, but the UUID isn't supported
   by the underlying firmware, firmware shall return an error when
   called.

   This allows for use of a secondary DSM on an object.  This could
   be considered a feature or a defect.  This can be tightened
   up if needed.



Changes in version 5:
---------------------
0. Fixed submit comment for drivers/acpi/utils.c.


Changes in version 4:
---------------------
0. Added patch to correct parameter type passed to acpi_evaluate_dsm
   ACPI defines arguments rev and fun as 64 bit quantities and the ioctl
   exports to user face rev and func. We want those to match the ACPI spec.

   Also modified acpi_evaluate_dsm_typed and acpi_check dsm which had
   similar issue.

1. nd_cmd_dsmcall_pkg rearrange a reserve and rounded up total size
   to 16 byte boundary.

2. Created stand alone patch for the pre-existing security issue related
   to "read only" IOCTL calls.

3. Added patch for increasing envelope size of IOCTL.  Needed to
   be able to read in the wrapper to know remaining size to copy in.

   Note: in_env, out_env are statics sized based upon this change.

4. Moved copyin code to table driven nd_cmd_desc 

  Note, the last 40 lines or so of acpi_nfit_ctl will not return _DSM
  data unless the size allocated in user space buffer equals
  out_obj->buffer.length.

  The semantic we want in the pass thru case is to return as much
  of the _DSM data as the user space buffer would accommodate.

  Hence, in acpi_nfit_ctl I have retained the line:

		memcpy(pkg->dsm_buf + pkg->h.dsm_in,
			out_obj->buffer.pointer,
			min(pkg->h.dsm_size, pkg->h.dsm_out));

  and the early return from the function.




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 integers 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 (5):
  nvdimm: Add IOCTL pass thru functions
  libnvdimm: nvdimm_bus_descriptor field name change
  Subject: [PATCH v8 07/10] tools/testing/nvdimm: 'call_dsm' support
  nvdimm: Add concept of cmd mask
  nvdimm: Add ioctl to return command mask.

 drivers/acpi/nfit.c              | 134 ++++++++++++++++++++++++++++++++++-----
 drivers/acpi/nfit.h              |   1 +
 drivers/nvdimm/bus.c             |  57 +++++++++++++++--
 drivers/nvdimm/core.c            |   2 +-
 drivers/nvdimm/dimm_devs.c       |  12 ++--
 drivers/nvdimm/nd-core.h         |   2 +-
 include/linux/libnvdimm.h        |   4 +-
 include/uapi/linux/ndctl.h       |   9 +++
 tools/testing/nvdimm/test/nfit.c |  15 ++++-
 9 files changed, 202 insertions(+), 34 deletions(-)

-- 
1.7.11.3

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[RFC v9 1/5] 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.

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 <jerry.hoemann@hpe.com>
---
 drivers/acpi/nfit.c  | 108 +++++++++++++++++++++++++++++++++++++++++++++------
 drivers/nvdimm/bus.c |  43 +++++++++++++++++++-
 2 files changed, 139 insertions(+), 12 deletions(-)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index d0f35e6..7dffcb5 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -56,6 +56,24 @@ struct nfit_table_prev {
 	struct list_head flushes;
 };
 
+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[] = {
+	{ 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) {
 		dev_dbg(dev, "%s:%s _DSM failed cmd: %s\n", __func__, dimm_name,
 				cmd_name);
 		return -EINVAL;
 	}
 
+	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));
+
+		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)
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 19f822d..0c1c4ab 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -439,6 +439,12 @@ static const struct nd_cmd_desc __nd_cmd_dimm_descs[] = {
 		.out_num = 3,
 		.out_sizes = { 4, 4, UINT_MAX, },
 	},
+	[ND_CMD_CALL] = {
+		.in_num = 2,
+		.in_sizes = {sizeof(struct nd_cmd_pkg), UINT_MAX, },
+		.out_num = 1,
+		.out_sizes = { UINT_MAX, },
+	},
 };
 
 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, },
+		.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;
@@ -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;
+		return pkg->nd_size_out;
+	}
+
 
 	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;
 	const char *cmd_name, *dimm_name;
+	unsigned int func = cmd;
 	unsigned long dsm_mask;
 	void *buf;
 	int rc, i;
@@ -605,8 +629,14 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		dimm_name = "bus";
 	}
 
+	if (cmd == ND_CMD_CALL) {
+		if (copy_from_user(&call_dsm, p, sizeof(call_dsm)))
+			return -EFAULT;
+		func = call_dsm.nd_command;
+	}
+
 	if (!desc || (desc->out_num + desc->in_num == 0) ||
-			!test_bit(cmd, &dsm_mask))
+			!test_bit(func, &dsm_mask))
 		return -ENOTTY;
 
 	/* fail write commands (when read-only) */
@@ -616,6 +646,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		case ND_CMD_SET_CONFIG_DATA:
 		case ND_CMD_ARS_START:
 		case ND_CMD_CLEAR_ERROR:
+		case ND_CMD_CALL:
 			dev_dbg(&nvdimm_bus->dev, "'%s' command while read-only.\n",
 					nvdimm ? nvdimm_cmd_name(cmd)
 					: nvdimm_bus_cmd_name(cmd));
@@ -643,6 +674,16 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		in_len += in_size;
 	}
 
+	if (cmd == ND_CMD_CALL) {
+		dev_dbg(dev, "%s:%s, idx: %llu, in: %zu, out: %zu, len %zu\n",
+				__func__, dimm_name, call_dsm.nd_command,
+				in_len, out_len, buf_len);
+
+		for (i = 0; i < ARRAY_SIZE(call_dsm.nd_reserved2); i++)
+			if (call_dsm.nd_reserved2[i])
+				return -EINVAL;
+	}
+
 	/* process an output envelope */
 	for (i = 0; i < desc->out_num; i++) {
 		u32 out_size = nd_cmd_out_size(nvdimm, cmd, desc, i,
-- 
1.7.11.3

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 1/5] nvdimm: Add IOCTL pass thru functions

[Johannes Thumshirn]

On Sonntag, 17. April 2016 17:38:43 CEST Jerry Hoemann 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.
> 
> 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 <jerry.hoemann@hpe.com>
> ---

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                                                                                Storage
jthumshirn@suse.de                                                             +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 1/5] nvdimm: Add IOCTL pass thru functions

[Dan Williams]

On Sun, Apr 17, 2016 at 4:38 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.
>
> 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 <jerry.hoemann@hpe.com>
> ---
>  drivers/acpi/nfit.c  | 108 +++++++++++++++++++++++++++++++++++++++++++++------
>  drivers/nvdimm/bus.c |  43 +++++++++++++++++++-
>  2 files changed, 139 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> index d0f35e6..7dffcb5 100644
> --- a/drivers/acpi/nfit.c
> +++ b/drivers/acpi/nfit.c
> @@ -56,6 +56,24 @@ struct nfit_table_prev {
>         struct list_head flushes;
>  };
>
> +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.

> +       { 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) {
>                 dev_dbg(dev, "%s:%s _DSM failed cmd: %s\n", __func__, dimm_name,
>                                 cmd_name);
>                 return -EINVAL;
>         }
>
> +       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.


> +
> +               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)
> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
> index 19f822d..0c1c4ab 100644
> --- a/drivers/nvdimm/bus.c
> +++ b/drivers/nvdimm/bus.c
> @@ -439,6 +439,12 @@ static const struct nd_cmd_desc __nd_cmd_dimm_descs[] = {
>                 .out_num = 3,
>                 .out_sizes = { 4, 4, UINT_MAX, },
>         },
> +       [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.

> +               .out_num = 1,
> +               .out_sizes = { UINT_MAX, },
> +       },
>  };
>
>  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.
.
> +               .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;
> @@ -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.

> +               return pkg->nd_size_out;
> +       }
> +
>
>         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.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 1/5] nvdimm: Add IOCTL pass thru functions

[Jerry Hoemann]

On Mon, Apr 18, 2016 at 07:15:24PM -0700, Dan Williams wrote:
> On Sun, Apr 17, 2016 at 4:38 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.
> >
> > 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 <jerry.hoemann@hpe.com>
> > ---
> >  drivers/acpi/nfit.c  | 108 +++++++++++++++++++++++++++++++++++++++++++++------
> >  drivers/nvdimm/bus.c |  43 +++++++++++++++++++-
> >  2 files changed, 139 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> > index d0f35e6..7dffcb5 100644
> > --- a/drivers/acpi/nfit.c
> > +++ b/drivers/acpi/nfit.c
> > @@ -56,6 +56,24 @@ struct nfit_table_prev {
> >         struct list_head flushes;
> >  };
> >
> > +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.


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

  ...


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


 ....


> > diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
> > index 19f822d..0c1c4ab 100644
> > --- a/drivers/nvdimm/bus.c
> > +++ b/drivers/nvdimm/bus.c
> > @@ -439,6 +439,12 @@ static const struct nd_cmd_desc __nd_cmd_dimm_descs[] = {
> >                 .out_num = 3,
> >                 .out_sizes = { 4, 4, UINT_MAX, },
> >         },
> > +       [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.


  Fine.


> 
> > +               .out_num = 1,
> > +               .out_sizes = { UINT_MAX, },
> > +       },
> >  };
> >
> >  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.

  Fine.

  ..

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

  struct nd_cmd_pkg had a longer named in past, hence breaking line not to
  exceed 80 characters.

  Will make one line and add \n.

> 
> > +               return pkg->nd_size_out;
> > +       }
> > +
> >
> >         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.

  Fine.

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 1/5] nvdimm: Add IOCTL pass thru functions

[Dan Williams]

On Wed, Apr 20, 2016 at 9:46 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Mon, Apr 18, 2016 at 07:15:24PM -0700, Dan Williams wrote:
>> On Sun, Apr 17, 2016 at 4:38 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.
>> >
>> > 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 <jerry.hoemann@hpe.com>
>> > ---
>> >  drivers/acpi/nfit.c  | 108 +++++++++++++++++++++++++++++++++++++++++++++------
>> >  drivers/nvdimm/bus.c |  43 +++++++++++++++++++-
>> >  2 files changed, 139 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
>> > index d0f35e6..7dffcb5 100644
>> > --- a/drivers/acpi/nfit.c
>> > +++ b/drivers/acpi/nfit.c
>> > @@ -56,6 +56,24 @@ struct nfit_table_prev {
>> >         struct list_head flushes;
>> >  };
>> >
>> > +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.
>
>
>>
>> > +       { 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},
>> > +};
>> > +
>
>   ...
>
>
>> >
>> > +       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.
>> +
>> +               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.

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.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 1/5] nvdimm: Add IOCTL pass thru functions

[Jerry Hoemann]

On Wed, Apr 20, 2016 at 01:08:05PM -0700, Dan Williams wrote:
> On Wed, Apr 20, 2016 at 9:46 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> > On Mon, Apr 18, 2016 at 07:15:24PM -0700, Dan Williams wrote:
> >> On Sun, Apr 17, 2016 at 4:38 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.
> >> >
> >> > 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 <jerry.hoemann@hpe.com>
> >> > ---
> >> >  drivers/acpi/nfit.c  | 108 +++++++++++++++++++++++++++++++++++++++++++++------
> >> >  drivers/nvdimm/bus.c |  43 +++++++++++++++++++-
> >> >  2 files changed, 139 insertions(+), 12 deletions(-)
> >> >
> >> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> >> > index d0f35e6..7dffcb5 100644
> >> > --- a/drivers/acpi/nfit.c
> >> > +++ b/drivers/acpi/nfit.c
> >> > @@ -56,6 +56,24 @@ struct nfit_table_prev {
> >> >         struct list_head flushes;
> >> >  };
> >> >
> >> > +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.
> >
> >
> >>
> >> > +       { 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},
> >> > +};
> >> > +
> >
> >   ...
> >
> >
> >> >
> >> > +       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.
> >> +
> >> +               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.
> 
> 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.

Even if ENXIO wasn't overloaded, it doesn't seem like the correct
error status.  POSIX defines ENXIO as:

	No such device or address.

	Input or output on a special file referred to a device that did not exist,
	or made a request beyond the limits of the device.
	This error may also occur when, for example, a tape drive is not online
	or a disk pack is not loaded on a device.

This isn't our case.

Did you pattern the return for nd_ioctl from somewhere?
It might be useful to have some context.


-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 1/5] nvdimm: Add IOCTL pass thru functions

[Dan Williams]

On Wed, Apr 20, 2016 at 3:55 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Wed, Apr 20, 2016 at 01:08:05PM -0700, Dan Williams wrote:
>> On Wed, Apr 20, 2016 at 9:46 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> > On Mon, Apr 18, 2016 at 07:15:24PM -0700, Dan Williams wrote:
>> >> On Sun, Apr 17, 2016 at 4:38 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.
>> >> >
>> >> > 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 <jerry.hoemann@hpe.com>
>> >> > ---
>> >> >  drivers/acpi/nfit.c  | 108 +++++++++++++++++++++++++++++++++++++++++++++------
>> >> >  drivers/nvdimm/bus.c |  43 +++++++++++++++++++-
>> >> >  2 files changed, 139 insertions(+), 12 deletions(-)
>> >> >
>> >> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
>> >> > index d0f35e6..7dffcb5 100644
>> >> > --- a/drivers/acpi/nfit.c
>> >> > +++ b/drivers/acpi/nfit.c
>> >> > @@ -56,6 +56,24 @@ struct nfit_table_prev {
>> >> >         struct list_head flushes;
>> >> >  };
>> >> >
>> >> > +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.
>> >
>> >
>> >>
>> >> > +       { 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},
>> >> > +};
>> >> > +
>> >
>> >   ...
>> >
>> >
>> >> >
>> >> > +       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.
>> >> +
>> >> +               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.
>>
>> 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.

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.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 1/5] nvdimm: Add IOCTL pass thru functions

[Dan Williams]

On Wed, Apr 20, 2016 at 6:29 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Wed, Apr 20, 2016 at 3:55 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
[..]
>>> > 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.
>
> 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.

Btw, please include this explanation as a comment in the code so
future readers don't try to reconcile the difference and come to the
wrong conclusion like I happened to do... twice.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[RFC v9 2/5] libnvdimm: nvdimm_bus_descriptor field name change

[Jerry Hoemann]

Change the the name of:

        nvdimm_bus_descriptor.dsm_mask
to
        nvdimm_bus_descriptor.cmd_mask

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/acpi/nfit.c              | 6 +++---
 drivers/nvdimm/bus.c             | 2 +-
 drivers/nvdimm/core.c            | 2 +-
 include/linux/libnvdimm.h        | 2 +-
 tools/testing/nvdimm/test/nfit.c | 2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index 7dffcb5..e3ff10b 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -235,7 +235,7 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
 		struct acpi_device *adev = to_acpi_dev(acpi_desc);
 
 		cmd_name = nvdimm_bus_cmd_name(cmd);
-		dsm_mask = nd_desc->dsm_mask;
+		dsm_mask = nd_desc->cmd_mask;
 		desc = nd_cmd_bus_desc(cmd);
 		uuid = to_nfit_uuid(NFIT_DEV_BUS);
 		handle = adev->handle;
@@ -1091,14 +1091,14 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
 	struct acpi_device *adev;
 	int i;
 
-	nd_desc->dsm_mask = acpi_desc->bus_dsm_force_en;
+	nd_desc->cmd_mask = acpi_desc->bus_dsm_force_en;
 	adev = to_acpi_dev(acpi_desc);
 	if (!adev)
 		return;
 
 	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);
+			set_bit(i, &nd_desc->cmd_mask);
 }
 
 static ssize_t range_index_show(struct device *dev,
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 0c1c4ab..5d63ad5 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -625,7 +625,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 	} else {
 		desc = nd_cmd_bus_desc(cmd);
 		cmd_name = nvdimm_bus_cmd_name(cmd);
-		dsm_mask = nd_desc->dsm_mask;
+		dsm_mask = nd_desc->cmd_mask;
 		dimm_name = "bus";
 	}
 
diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c
index 182a93f..e8688a1 100644
--- a/drivers/nvdimm/core.c
+++ b/drivers/nvdimm/core.c
@@ -251,7 +251,7 @@ static ssize_t commands_show(struct device *dev,
 	struct nvdimm_bus *nvdimm_bus = to_nvdimm_bus(dev);
 	struct nvdimm_bus_descriptor *nd_desc = nvdimm_bus->nd_desc;
 
-	for_each_set_bit(cmd, &nd_desc->dsm_mask, BITS_PER_LONG)
+	for_each_set_bit(cmd, &nd_desc->cmd_mask, BITS_PER_LONG)
 		len += sprintf(buf + len, "%s ", nvdimm_bus_cmd_name(cmd));
 	len += sprintf(buf + len, "\n");
 	return len;
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index af31d1c..67a1ba0 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -68,7 +68,7 @@ struct nd_mapping {
 
 struct nvdimm_bus_descriptor {
 	const struct attribute_group **attr_groups;
-	unsigned long dsm_mask;
+	unsigned long cmd_mask;
 	char *provider_name;
 	ndctl_fn ndctl;
 	int (*flush_probe)(struct nvdimm_bus_descriptor *nd_desc);
diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
index 3187322..1d3b8ce 100644
--- a/tools/testing/nvdimm/test/nfit.c
+++ b/tools/testing/nvdimm/test/nfit.c
@@ -374,7 +374,7 @@ static int nfit_test_ctl(struct nvdimm_bus_descriptor *nd_desc,
 	} else {
 		struct ars_state *ars_state = &t->ars_state;
 
-		if (!nd_desc || !test_bit(cmd, &nd_desc->dsm_mask))
+		if (!nd_desc || !test_bit(cmd, &nd_desc->cmd_mask))
 			return -ENOTTY;
 
 		switch (cmd) {
-- 
1.7.11.3

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 2/5] libnvdimm: nvdimm_bus_descriptor field name change

[Johannes Thumshirn]

On Sonntag, 17. April 2016 17:38:44 CEST Jerry Hoemann wrote:
> Change the the name of:
> 
>         nvdimm_bus_descriptor.dsm_mask
> to
>         nvdimm_bus_descriptor.cmd_mask
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                                                                                Storage
jthumshirn@suse.de                                                             +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[RFC v9 3/5] Subject: [PATCH v8 07/10] tools/testing/nvdimm: 'call_dsm' support

[Jerry Hoemann]

Enable nfit_test to use the generic 'call_dsm' envelope.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 tools/testing/nvdimm/test/nfit.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
index 1d3b8ce..5afc3d3 100644
--- a/tools/testing/nvdimm/test/nfit.c
+++ b/tools/testing/nvdimm/test/nfit.c
@@ -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;
 
+	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;
+	}
+
 	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;
 
-		switch (cmd) {
+		switch (func) {
 		case ND_CMD_ARS_CAP:
 			rc = nfit_test_cmd_ars_cap(buf, buf_len);
 			break;
-- 
1.7.11.3

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 3/5] Subject: [PATCH v8 07/10] tools/testing/nvdimm: 'call_dsm' support

[Johannes Thumshirn]

On Sonntag, 17. April 2016 17:38:45 CEST Jerry Hoemann wrote:
> Enable nfit_test to use the generic 'call_dsm' envelope.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                                                                                Storage
jthumshirn@suse.de                                                             +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 3/5] Subject: [PATCH v8 07/10] tools/testing/nvdimm: 'call_dsm' support

[Dan Williams]

On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> Enable nfit_test to use the generic 'call_dsm' envelope.
>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  tools/testing/nvdimm/test/nfit.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
> index 1d3b8ce..5afc3d3 100644
> --- a/tools/testing/nvdimm/test/nfit.c
> +++ b/tools/testing/nvdimm/test/nfit.c
> @@ -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;
>
> +       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.

> +       }
> +
>         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;
>
> -               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..
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 3/5] Subject: [PATCH v8 07/10] tools/testing/nvdimm: 'call_dsm' support

[Jerry Hoemann]

On Mon, Apr 18, 2016 at 07:22:00PM -0700, Dan Williams wrote:
> On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> > Enable nfit_test to use the generic 'call_dsm' envelope.
> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  tools/testing/nvdimm/test/nfit.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
> > index 1d3b8ce..5afc3d3 100644
> > --- a/tools/testing/nvdimm/test/nfit.c
> > +++ b/tools/testing/nvdimm/test/nfit.c
> > @@ -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;
> >
> > +       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.

  Fine.


> 
> > +       }
> > +
> >         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;
> >
> > -               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..


Additionally in acpi_nfit_add_dimm I see:

        nfit_mem->dsm_mask = acpi_desc->dimm_dsm_force_en;


I assume you'll want that changed to:

        nfit_mem->cmd_mask = acpi_desc->dimm_cmd_force_en;



-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[RFC v9 4/5] nvdimm: Add concept of cmd mask

[Jerry Hoemann]

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 <jerry.hoemann@hpe.com>
---
 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(-)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index e3ff10b..fc3e7d9 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -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? */
 };
 
 /*
@@ -68,9 +69,9 @@ struct cmd_family_tbl {
 #define ND_MAX_CMD	20
 
 struct cmd_family_tbl nfit_cmd_family_tbl[] = {
-	{ 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},
 	{ -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)
@@ -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);
 
+	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;
+
 	return 0;
 }
 
@@ -1062,7 +1080,7 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *acpi_desc)
 
 		nvdimm = nvdimm_create(acpi_desc->nvdimm_bus, nfit_mem,
 				acpi_nfit_dimm_attribute_groups,
-				flags, &nfit_mem->dsm_mask);
+				flags, nfit_mem->cmd_mask);
 		if (!nvdimm)
 			return -ENOMEM;
 
diff --git a/drivers/acpi/nfit.h b/drivers/acpi/nfit.h
index 7179ea6..e2fcab3 100644
--- a/drivers/acpi/nfit.h
+++ b/drivers/acpi/nfit.h
@@ -114,6 +114,7 @@ struct nfit_mem {
 	struct list_head list;
 	struct acpi_device *adev;
 	unsigned long dsm_mask;
+	unsigned long cmd_mask;
 	const u8 *dsm_uuid;
 };
 
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 5d63ad5..b133301 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -612,31 +612,29 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 	struct device *dev = &nvdimm_bus->dev;
 	struct nd_cmd_pkg call_dsm;
 	const char *cmd_name, *dimm_name;
-	unsigned int func = cmd;
-	unsigned long dsm_mask;
+	unsigned long cmd_mask;
 	void *buf;
 	int rc, i;
 
 	if (nvdimm) {
 		desc = nd_cmd_dimm_desc(cmd);
 		cmd_name = nvdimm_cmd_name(cmd);
-		dsm_mask = nvdimm->dsm_mask ? *(nvdimm->dsm_mask) : 0;
+		cmd_mask = nvdimm->cmd_mask;
 		dimm_name = dev_name(&nvdimm->dev);
 	} else {
 		desc = nd_cmd_bus_desc(cmd);
 		cmd_name = nvdimm_bus_cmd_name(cmd);
-		dsm_mask = nd_desc->cmd_mask;
+		cmd_mask = nd_desc->cmd_mask;
 		dimm_name = "bus";
 	}
 
 	if (cmd == ND_CMD_CALL) {
 		if (copy_from_user(&call_dsm, p, sizeof(call_dsm)))
 			return -EFAULT;
-		func = call_dsm.nd_command;
 	}
 
 	if (!desc || (desc->out_num + desc->in_num == 0) ||
-			!test_bit(func, &dsm_mask))
+			!test_bit(cmd, &cmd_mask))
 		return -ENOTTY;
 
 	/* fail write commands (when read-only) */
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index c56f882..b86b45a 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -37,9 +37,9 @@ static int __validate_dimm(struct nvdimm_drvdata *ndd)
 
 	nvdimm = to_nvdimm(ndd->dev);
 
-	if (!nvdimm->dsm_mask)
+	if (!nvdimm->cmd_mask)
 		return -ENXIO;
-	if (!test_bit(ND_CMD_GET_CONFIG_DATA, nvdimm->dsm_mask))
+	if (!test_bit(ND_CMD_GET_CONFIG_DATA, &nvdimm->cmd_mask))
 		return -ENXIO;
 
 	return 0;
@@ -277,10 +277,10 @@ static ssize_t commands_show(struct device *dev,
 	struct nvdimm *nvdimm = to_nvdimm(dev);
 	int cmd, len = 0;
 
-	if (!nvdimm->dsm_mask)
+	if (!nvdimm->cmd_mask)
 		return sprintf(buf, "\n");
 
-	for_each_set_bit(cmd, nvdimm->dsm_mask, BITS_PER_LONG)
+	for_each_set_bit(cmd, &nvdimm->cmd_mask, BITS_PER_LONG)
 		len += sprintf(buf + len, "%s ", nvdimm_cmd_name(cmd));
 	len += sprintf(buf + len, "\n");
 	return len;
@@ -340,7 +340,7 @@ EXPORT_SYMBOL_GPL(nvdimm_attribute_group);
 
 struct nvdimm *nvdimm_create(struct nvdimm_bus *nvdimm_bus, void *provider_data,
 		const struct attribute_group **groups, unsigned long flags,
-		unsigned long *dsm_mask)
+		unsigned long cmd_mask)
 {
 	struct nvdimm *nvdimm = kzalloc(sizeof(*nvdimm), GFP_KERNEL);
 	struct device *dev;
@@ -355,7 +355,7 @@ struct nvdimm *nvdimm_create(struct nvdimm_bus *nvdimm_bus, void *provider_data,
 	}
 	nvdimm->provider_data = provider_data;
 	nvdimm->flags = flags;
-	nvdimm->dsm_mask = dsm_mask;
+	nvdimm->cmd_mask = cmd_mask;
 	atomic_set(&nvdimm->busy, 0);
 	dev = &nvdimm->dev;
 	dev_set_name(dev, "nmem%d", nvdimm->id);
diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h
index 1d1500f..da0d322 100644
--- a/drivers/nvdimm/nd-core.h
+++ b/drivers/nvdimm/nd-core.h
@@ -37,7 +37,7 @@ struct nvdimm_bus {
 struct nvdimm {
 	unsigned long flags;
 	void *provider_data;
-	unsigned long *dsm_mask;
+	unsigned long cmd_mask;
 	struct device dev;
 	atomic_t busy;
 	int id;
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 67a1ba0..41c975a 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -133,7 +133,7 @@ const char *nvdimm_name(struct nvdimm *nvdimm);
 void *nvdimm_provider_data(struct nvdimm *nvdimm);
 struct nvdimm *nvdimm_create(struct nvdimm_bus *nvdimm_bus, void *provider_data,
 		const struct attribute_group **groups, unsigned long flags,
-		unsigned long *dsm_mask);
+		unsigned long cmd_mask);
 const struct nd_cmd_desc *nd_cmd_dimm_desc(int cmd);
 const struct nd_cmd_desc *nd_cmd_bus_desc(int cmd);
 u32 nd_cmd_in_size(struct nvdimm *nvdimm, int cmd,
-- 
1.7.11.3

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 4/5] nvdimm: Add concept of cmd mask

[Johannes Thumshirn]

On Sonntag, 17. April 2016 17:38:46 CEST Jerry Hoemann wrote:
> 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 <jerry.hoemann@hpe.com>
> ---

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                                                                                Storage
jthumshirn@suse.de                                                             +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 4/5] nvdimm: Add concept of cmd mask

[Dan Williams]

On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> 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 <jerry.hoemann@hpe.com>
> ---
>  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(-)
>
> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> index e3ff10b..fc3e7d9 100644
> --- a/drivers/acpi/nfit.c
> +++ b/drivers/acpi/nfit.c
> @@ -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? */
>  };
>
>  /*
> @@ -68,9 +69,9 @@ struct cmd_family_tbl {
>  #define ND_MAX_CMD     20
>
>  struct cmd_family_tbl nfit_cmd_family_tbl[] = {
> -       { 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...

>         { -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)
> @@ -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.

> +       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.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 4/5] nvdimm: Add concept of cmd mask

[Jerry Hoemann]

On Mon, Apr 18, 2016 at 08:03:14PM -0700, Dan Williams wrote:
> On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:

...


> >  struct cmd_family_tbl nfit_cmd_family_tbl[] = {
> > -       { 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...
> 

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

 ....

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

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

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

But, I really don't understand what you're asking.


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

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 4/5] nvdimm: Add concept of cmd mask

[Dan Williams]

On Thu, Apr 21, 2016 at 10:28 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Mon, Apr 18, 2016 at 08:03:14PM -0700, Dan Williams wrote:
>> On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>
> ...
>
>
>> >  struct cmd_family_tbl nfit_cmd_family_tbl[] = {
>> > -       { 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...
>>
>
> 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".

>  ....
>
>> > +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)
>> > @@ -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?

I'm primarily concerned with vendors developing undocumented commands
without updating the kernel, and probing for 64 commands per-dimm when
we know only 9 commands are valid, for example.

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

We still need the dynamic step to check which of the possible commands
in the 'family' are implemented by the given dimm.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 4/5] nvdimm: Add concept of cmd mask

[Jerry Hoemann]

On Thu, Apr 21, 2016 at 11:25:37AM -0700, Dan Williams wrote:
> On Thu, Apr 21, 2016 at 10:28 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> > On Mon, Apr 18, 2016 at 08:03:14PM -0700, Dan Williams wrote:
> >> On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> >
> > ...
> >
> >
> >> >  struct cmd_family_tbl nfit_cmd_family_tbl[] = {
> >> > -       { 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...
> >>


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.


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


-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 4/5] nvdimm: Add concept of cmd mask

[Dan Williams]

On Fri, Apr 22, 2016 at 10:55 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Thu, Apr 21, 2016 at 11:25:37AM -0700, Dan Williams wrote:
>> On Thu, Apr 21, 2016 at 10:28 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> > On Mon, Apr 18, 2016 at 08:03:14PM -0700, Dan Williams wrote:
>> >> On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> >
>> > ...
>> >
>> >
>> >> >  struct cmd_family_tbl nfit_cmd_family_tbl[] = {
>> >> > -       { 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...
>> >>
>
>
> 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.
>

Sorry, I'm still not seeing the need for 'does_ioctl'.  If anything
down the road we might end up with a translation *routine*, per-family
or otherwise, to translate from ND_CMD to DSM.  For now just set the
cmd_mask to (1 << ND_CMD_CALL) for all families and 'or' in the other
commands explicitly for the Intel family in acpi_nfit_add_dimm().  In
other words, just code this into the acpi_nfit_add_dimm() directly
rather than having a flag in a table, I'm not seeing it's utility.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[RFC v9 5/5] nvdimm: Add ioctl to return command mask.

[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 <jerry.hoemann@hpe.com>
---
 drivers/nvdimm/bus.c       | 10 ++++++++--
 include/uapi/linux/ndctl.h |  9 +++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index b133301..047c364 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -404,7 +404,10 @@ void nvdimm_bus_destroy_ndctl(struct nvdimm_bus *nvdimm_bus)
 }
 
 static const struct nd_cmd_desc __nd_cmd_dimm_descs[] = {
-	[ND_CMD_IMPLEMENTED] = { },
+	[ND_CMD_IMPLEMENTED] = {
+		.out_num = 1,
+		.out_sizes = { 8, },
+	},
 	[ND_CMD_SMART] = {
 		.out_num = 2,
 		.out_sizes = { 4, 128, },
@@ -456,7 +459,10 @@ const struct nd_cmd_desc *nd_cmd_dimm_desc(int cmd)
 EXPORT_SYMBOL_GPL(nd_cmd_dimm_desc);
 
 static const struct nd_cmd_desc __nd_cmd_bus_descs[] = {
-	[ND_CMD_IMPLEMENTED] = { },
+	[ND_CMD_IMPLEMENTED] = {
+		.out_num = 1,
+		.out_sizes = { 1, },
+	},
 	[ND_CMD_ARS_CAP] = {
 		.in_num = 2,
 		.in_sizes = { 8, 8, },
diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index 9214af7..8c086b9 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -20,6 +20,10 @@ struct nd_cmd_smart {
 	__u8 data[128];
 } __packed;
 
+struct nd_cmd_mask {
+	__u8 data[8];
+} __packed;
+
 struct nd_cmd_smart_threshold {
 	__u32 status;
 	__u8 data[8];
@@ -136,6 +140,7 @@ enum {
 static inline const char *nvdimm_bus_cmd_name(unsigned cmd)
 {
 	static const char * const names[] = {
+		[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",
 		[ND_CMD_SMART] = "smart",
 		[ND_CMD_SMART_THRESHOLD] = "smart_thresh",
 		[ND_CMD_DIMM_FLAGS] = "flags",
@@ -169,6 +175,9 @@ static inline const char *nvdimm_cmd_name(unsigned cmd)
 
 #define ND_IOCTL 'N'
 
+#define ND_IOCTL_CMD_MASK		_IOWR(ND_IOCTL, ND_CMD_IMPLEMENTED,\
+					struct nd_cmd_mask)
+
 #define ND_IOCTL_SMART			_IOWR(ND_IOCTL, ND_CMD_SMART,\
 					struct nd_cmd_smart)
 
-- 
1.7.11.3

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 5/5] nvdimm: Add ioctl to return command mask.

[Johannes Thumshirn]

On Sonntag, 17. April 2016 17:38:47 CEST Jerry Hoemann wrote:
> 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 <jerry.hoemann@hpe.com>
> ---

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                                                                                Storage
jthumshirn@suse.de                                                             +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 5/5] nvdimm: Add ioctl to return command mask.

[Dan Williams]

On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> 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 <jerry.hoemann@hpe.com>
> ---
>  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.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 5/5] nvdimm: Add ioctl to return command mask.

[Jerry Hoemann]

On Tue, Apr 19, 2016 at 10:45:48AM -0700, Dan Williams wrote:
> On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> > 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 <jerry.hoemann@hpe.com>
> > ---
> >  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.

I can change just 
        static const char * const names[] = {
+               [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?


-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 5/5] nvdimm: Add ioctl to return command mask.

[Dan Williams]

On Wed, Apr 20, 2016 at 10:41 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Tue, Apr 19, 2016 at 10:45:48AM -0700, Dan Williams wrote:
>> On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> > 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 <jerry.hoemann@hpe.com>
>> > ---
>> >  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.

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?

> I can change just
>         static const char * const names[] = {
> +               [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 there's no ND_IOCTL_IMPLEMENTED, so why would userspace ever
care to know the name of a command it can't issue?

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

> 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.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 5/5] nvdimm: Add ioctl to return command mask.

[Jerry Hoemann]

On Thu, Apr 21, 2016 at 04:52:17AM -0700, Dan Williams wrote:
> On Wed, Apr 20, 2016 at 10:41 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> > On Tue, Apr 19, 2016 at 10:45:48AM -0700, Dan Williams wrote:
> >> On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> >> > 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 <jerry.hoemann@hpe.com>
> >> > ---
> >> >  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.
> 
> 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;
-       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);
 


> > I can change just
> >         static const char * const names[] = {
> > +               [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 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.

  The architecture defines the function in the DSM,  but a particular
  implementation may or may not implement all functions of a dsm.
  The dsm mask tells which function the FW implements.  And yes, we
  have different SKUs and they don't all implement all/same functions.

  A side effect of having "0" in the dsm_mask is that when
  commands_show executes:


        for_each_set_bit(cmd, &nvdimm->cmd_mask, BITS_PER_LONG)
                len += sprintf(buf + len, "%s ", nvdimm_cmd_name(cmd));
        len += sprintf(buf + len, "\n");

  The value of names[ND_CMD_IMPLEMENTED] is printed.   Currently that
  value is "unknown" which I didn't think looked correct.  Hence, the
  change.

  I my other patch, I am constructing cmd_mask as either
	(1<<ND_CMD)|dsm_mask  -or-
	(1<<ND_CMD)
  depending upon UUID.

  Now construction of cmd_mask is subject of an earlier patch set.


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


  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.



-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 5/5] nvdimm: Add ioctl to return command mask.

[Dan Williams]

On Thu, Apr 21, 2016 at 9:52 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Thu, Apr 21, 2016 at 04:52:17AM -0700, Dan Williams wrote:
>> On Wed, Apr 20, 2016 at 10:41 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> > On Tue, Apr 19, 2016 at 10:45:48AM -0700, Dan Williams wrote:
>> >> On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> >> > 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 <jerry.hoemann@hpe.com>
>> >> > ---
>> >> >  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.
>>
>> 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

Thanks for copying this here, I had overlooked this aspect of the patch.

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

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

>> > I can change just
>> >         static const char * const names[] = {
>> > +               [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 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.

Right, I don't want to add an ioctl for discovering dsm_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.
>
>
>   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.
>

Why wait, it's a trivial amount of code.  Replace the above changes
with something like the below (untested), or just let me know and I'll
append a patch like this to the end of your series.

@@ -802,6 +802,16 @@ static ssize_t handle_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(handle);

+static ssize_t dsm_mask_show(struct device *dev,
+               struct device_attribute *attr, char *buf)
+{
+       struct nvdimm *nvdimm = to_nvdimm(dev);
+       struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
+
+       return sprintf(buf, "%#lx\n", nfit_mem->dsm_mask);
+}
+static DEVICE_ATTR_RO(dsm_mask);
+
 static ssize_t phys_id_show(struct device *dev,
                struct device_attribute *attr, char *buf)
 {
@@ -879,6 +889,7 @@ static struct attribute *acpi_nfit_dimm_attributes[] = {
        &dev_attr_serial.attr,
        &dev_attr_rev_id.attr,
        &dev_attr_flags.attr,
+       &dev_attr_dsm_mask.attr,
        NULL,
 };
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 5/5] nvdimm: Add ioctl to return command mask.

[Jerry Hoemann]

On Thu, Apr 21, 2016 at 11:18:40AM -0700, Dan Williams wrote:
> On Thu, Apr 21, 2016 at 9:52 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> > On Thu, Apr 21, 2016 at 04:52:17AM -0700, Dan Williams wrote:
> >> On Wed, Apr 20, 2016 at 10:41 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> >> > On Tue, Apr 19, 2016 at 10:45:48AM -0700, Dan Williams wrote:
> >> >> On Sun, Apr 17, 2016 at 4:38 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> >         }
> >
> > -       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

  Yes, should have called it something like ND_MAX_DSM_FUN_IDX.

  I am looking at basing it off of bits in specified bits like below.

> 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.
> 
> >> > I can change just
> >> >         static const char * const names[] = {
> >> > +               [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 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.
> 
> 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


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

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v9 5/5] nvdimm: Add ioctl to return command mask.

[Dan Williams]

On Fri, Apr 22, 2016 at 4:15 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Thu, Apr 21, 2016 at 11:18:40AM -0700, Dan Williams wrote:
[..]
>> 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
>
>
> +#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.

So, I'm saying I don't want ND_CMD_CALL to be used to issue the "is
dsm function implemented?" check.  Especially because the kernel may
want to limit the mask for one reason or another, the raw data
returned from the DSM may not be accurate.  It also just comes across
as inefficient to me when the kernel has already probed this
information and is maintaining it internally.

Lastly ioctls are terrible and having one less piece of information
that is conveyed via an ioctl in favor of a sysfs attribute is a win.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm