[RFC v6 1/8] ACPI / util: Fix acpi_evaluate_dsm() argument type

[RFC v6 1/8] ACPI / util: Fix acpi_evaluate_dsm() argument type

[Jerry Hoemann]

The ACPI spec speicifies that arguments "Revision ID" and
"Function Index" to a _DSM are type "Integer."  Type Integers
are 64 bit quantities.

The function evaluate_dsm specifies these types as simple "int"
which are 32 bits.  Correct type passed to acpi_evaluate_dsm
and its callers and derived callers to pass correct type.

acpi_check_dsm and acpi_evaluate_dsm_typed had similar issue
and were corrected as well.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/acpi/utils.c    | 4 ++--
 include/acpi/acpi_bus.h | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index f2f9873..87bd54d 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -629,7 +629,7 @@ acpi_status acpi_evaluate_lck(acpi_handle handle, int lock)
  * some old BIOSes do expect a buffer or an integer etc.
  */
 union acpi_object *
-acpi_evaluate_dsm(acpi_handle handle, const u8 *uuid, int rev, int func,
+acpi_evaluate_dsm(acpi_handle handle, const u8 *uuid, u64 rev, u64 func,
 		  union acpi_object *argv4)
 {
 	acpi_status ret;
@@ -678,7 +678,7 @@ EXPORT_SYMBOL(acpi_evaluate_dsm);
  * functions. Currently only support 64 functions at maximum, should be
  * enough for now.
  */
-bool acpi_check_dsm(acpi_handle handle, const u8 *uuid, int rev, u64 funcs)
+bool acpi_check_dsm(acpi_handle handle, const u8 *uuid, u64 rev, u64 funcs)
 {
 	int i;
 	u64 mask = 0;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 14362a8..f092cc6 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -61,12 +61,12 @@ bool acpi_ata_match(acpi_handle handle);
 bool acpi_bay_match(acpi_handle handle);
 bool acpi_dock_match(acpi_handle handle);
 
-bool acpi_check_dsm(acpi_handle handle, const u8 *uuid, int rev, u64 funcs);
+bool acpi_check_dsm(acpi_handle handle, const u8 *uuid, u64 rev, u64 funcs);
 union acpi_object *acpi_evaluate_dsm(acpi_handle handle, const u8 *uuid,
-			int rev, int func, union acpi_object *argv4);
+			u64 rev, u64 func, union acpi_object *argv4);
 
 static inline union acpi_object *
-acpi_evaluate_dsm_typed(acpi_handle handle, const u8 *uuid, int rev, int func,
+acpi_evaluate_dsm_typed(acpi_handle handle, const u8 *uuid, u64 rev, u64 func,
 			union acpi_object *argv4, acpi_object_type type)
 {
 	union acpi_object *obj;
-- 
1.7.11.3

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

[RFC v6 2/8] nvdimm: Add wrapper for IOCTL pass thru

[Jerry Hoemann]

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

Add dsm_call command to nvdimm_bus_cmd_name and nvdimm_cmd_name.

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

diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index 7cc28ab..459c6ba 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -125,6 +125,7 @@ enum {
 	ND_CMD_VENDOR_EFFECT_LOG_SIZE = 7,
 	ND_CMD_VENDOR_EFFECT_LOG = 8,
 	ND_CMD_VENDOR = 9,
+	ND_CMD_CALL_DSM = 10,
 };
 
 enum {
@@ -139,6 +140,7 @@ static inline const char *nvdimm_bus_cmd_name(unsigned cmd)
 		[ND_CMD_ARS_START] = "ars_start",
 		[ND_CMD_ARS_STATUS] = "ars_status",
 		[ND_CMD_CLEAR_ERROR] = "clear_error",
+		[ND_CMD_CALL_DSM] = "dsm_call",
 	};
 
 	if (cmd < ARRAY_SIZE(names) && names[cmd])
@@ -158,6 +160,7 @@ static inline const char *nvdimm_cmd_name(unsigned cmd)
 		[ND_CMD_VENDOR_EFFECT_LOG_SIZE] = "effect_size",
 		[ND_CMD_VENDOR_EFFECT_LOG] = "effect_log",
 		[ND_CMD_VENDOR] = "vendor",
+		[ND_CMD_CALL_DSM] = "dsm_call",
 	};
 
 	if (cmd < ARRAY_SIZE(names) && names[cmd])
@@ -224,4 +227,23 @@ enum ars_masks {
 	ARS_STATUS_MASK = 0x0000FFFF,
 	ARS_EXT_STATUS_SHIFT = 16,
 };
+
+
+struct nd_cmd_pkg {
+	__u32   ncp_type;
+	__u32   ncp_rev;
+	__u64   ncp_command;
+	__u32   ncp_size_in;                    /* size of input payload */
+	__u32   ncp_size_out;                   /* size of user buffer   */
+	__u32   ncp_reserved2[9];               /* reserved must be zero */
+	__u32   ncp_pot_size;                   /* potential output size */
+	unsigned char ncp_payload[];            /* Contents of call      */
+};
+
+#define NCP_TYPE_BUS                   1
+#define NCP_TYPE_DIMM_INTEL1           2
+#define NCP_TYPE_DIMM_N_HPE1           3
+#define NCP_TYPE_DIMM_N_HPE2           4
+
+
 #endif /* __NDCTL_H__ */
-- 
1.7.11.3

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

[RFC v6 3/8] nvdimm: Increase max envelope size for IOCTL

[Jerry Hoemann]

In __nd_ioctl must first read in the fixed sized portion of an ioctl
so that it can then determine the size of the variable part.

Prepare for ND_CMD_CALL_DSM calls which have larger fixed portion
wrapper.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 include/linux/libnvdimm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 833867b..af31d1c 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -27,7 +27,7 @@ enum {
 	/* need to set a limit somewhere, but yes, this is likely overkill */
 	ND_IOCTL_MAX_BUFLEN = SZ_4M,
 	ND_CMD_MAX_ELEM = 5,
-	ND_CMD_MAX_ENVELOPE = 16,
+	ND_CMD_MAX_ENVELOPE = 256,
 	ND_MAX_MAPPINGS = 32,
 
 	/* region flag indicating to direct-map persistent memory by default */
-- 
1.7.11.3

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

[RFC v6 4/8] nvdimm: Add UUIDs

[Jerry Hoemann]

Add UUID associated with HPE Type N NVDIMM.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/acpi/nfit.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/acpi/nfit.h b/drivers/acpi/nfit.h
index c75576b..10dd0d3 100644
--- a/drivers/acpi/nfit.h
+++ b/drivers/acpi/nfit.h
@@ -23,6 +23,8 @@
 
 #define UUID_NFIT_BUS "2f10e7a4-9e91-11e4-89d3-123b93f75cba"
 #define UUID_NFIT_DIMM "4309ac30-0d11-11e4-9191-0800200c9a66"
+#define UUID_NFIT_DIMM_N_HPE1 "9002c334-acf3-4c0e-9642-a235f0d53bc6"
+#define UUID_NFIT_DIMM_N_HPE2 "5008664B-B758-41A0-A03C-27C2F2D04F7E"
 #define ACPI_NFIT_MEM_FAILED_MASK (ACPI_NFIT_MEM_SAVE_FAILED \
 		| ACPI_NFIT_MEM_RESTORE_FAILED | ACPI_NFIT_MEM_FLUSH_FAILED \
 		| ACPI_NFIT_MEM_NOT_ARMED)
@@ -38,6 +40,8 @@ enum nfit_uuids {
 	NFIT_SPA_PCD,
 	NFIT_DEV_BUS,
 	NFIT_DEV_DIMM,
+	NFIT_DEV_DIMM_N_HPE1,
+	NFIT_DEV_DIMM_N_HPE2,
 	NFIT_UUID_MAX,
 };
 
@@ -110,6 +114,7 @@ struct nfit_mem {
 	struct list_head list;
 	struct acpi_device *adev;
 	unsigned long dsm_mask;
+	const u8 *dsm_uuid;
 };
 
 struct acpi_nfit_desc {
-- 
1.7.11.3

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

[RFC v6 5/8] nvdimm: data structure changes for dsm calls

[Jerry Hoemann]

Indicate "dsm_call" capability.
Track uuid.

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

diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index af31d1c..83996b6 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -68,7 +68,9 @@ struct nd_mapping {
 
 struct nvdimm_bus_descriptor {
 	const struct attribute_group **attr_groups;
+	unsigned int call_dsm:1;
 	unsigned long dsm_mask;
+	const u8 *dsm_uuid;
 	char *provider_name;
 	ndctl_fn ndctl;
 	int (*flush_probe)(struct nvdimm_bus_descriptor *nd_desc);
-- 
1.7.11.3

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

[RFC v6 6/8] libnvdimm: advertise 'call_dsm' support

[Jerry Hoemann]

Indicate to userspace that the generic 'dsm_call' capability is
available for the given control device.  Over time we want to deprecate
the per-dsm function ioctl commands and direct everything through the
generic envelope.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/nvdimm/core.c      | 3 +++
 drivers/nvdimm/dimm_devs.c | 8 +++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c
index 79646d0..cc449e0 100644
--- a/drivers/nvdimm/core.c
+++ b/drivers/nvdimm/core.c
@@ -253,6 +253,9 @@ static ssize_t commands_show(struct device *dev,
 
 	for_each_set_bit(cmd, &nd_desc->dsm_mask, BITS_PER_LONG)
 		len += sprintf(buf + len, "%s ", nvdimm_bus_cmd_name(cmd));
+	if (nd_desc->call_dsm)
+		len += sprintf(buf + len, "%s ",
+				nvdimm_bus_cmd_name(ND_CMD_CALL_DSM));
 	len += sprintf(buf + len, "\n");
 	return len;
 }
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index c56f882..c2a9f58 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -274,14 +274,20 @@ EXPORT_SYMBOL_GPL(nvdimm_provider_data);
 static ssize_t commands_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
+	struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
 	struct nvdimm *nvdimm = to_nvdimm(dev);
+	struct nvdimm_bus_descriptor *nd_desc;
 	int cmd, len = 0;
 
-	if (!nvdimm->dsm_mask)
+	if (!nvdimm->dsm_mask || !nvdimm_bus)
 		return sprintf(buf, "\n");
 
 	for_each_set_bit(cmd, nvdimm->dsm_mask, BITS_PER_LONG)
 		len += sprintf(buf + len, "%s ", nvdimm_cmd_name(cmd));
+	nd_desc = nvdimm_bus->nd_desc;
+	if (nd_desc->call_dsm)
+		len += sprintf(buf + len, "%s ",
+				nvdimm_cmd_name(ND_CMD_CALL_DSM));
 	len += sprintf(buf + len, "\n");
 	return len;
 }
-- 
1.7.11.3

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

[RFC v6 7/8] nvdimm: Add IOCTL pass thru functions

[Jerry Hoemann]

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

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/acpi/nfit.c  | 149 +++++++++++++++++++++++++++++++++++++++++++--------
 drivers/nvdimm/bus.c |  45 +++++++++++++++-
 2 files changed, 170 insertions(+), 24 deletions(-)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index d0f35e6..b17e617 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -56,6 +56,21 @@ struct nfit_table_prev {
 	struct list_head flushes;
 };
 
+struct cmd_type_tbl {
+	enum nfit_uuids	key_uuid;	/* Internal handle		*/
+	int		key_type;	/* Exported handle		*/
+	int		rev;		/* _DSM rev			*/
+	u64		mask;		/* 0 bit excludes underlying func.*/
+};
+
+struct cmd_type_tbl cmd_type_tbl[] = {
+	{ NFIT_DEV_BUS,		NCP_TYPE_BUS,		1, ~0UL},
+	{ NFIT_DEV_DIMM,	NCP_TYPE_DIMM_INTEL1,	1, ~0UL},
+	{ NFIT_DEV_DIMM_N_HPE1, NCP_TYPE_DIMM_N_HPE1,	1, ~0UL},
+	{ NFIT_DEV_DIMM_N_HPE2, NCP_TYPE_DIMM_N_HPE2,	1, ~0UL},
+	{ -1, -1, -1, 0},
+};
+
 static u8 nfit_uuid[NFIT_UUID_MAX][16];
 
 const u8 *to_nfit_uuid(enum nfit_uuids id)
@@ -64,6 +79,18 @@ const u8 *to_nfit_uuid(enum nfit_uuids id)
 }
 EXPORT_SYMBOL(to_nfit_uuid);
 
+static int
+known_nvdimm_type(u32 type, struct cmd_type_tbl *tbl)
+{
+	int i;
+
+	for (i = 0;  tbl[i].key_type >= 0 ; i++) {
+		if (tbl[i].key_type == type)
+			return 1;
+	}
+	return 0;
+}
+
 static struct acpi_nfit_desc *to_acpi_nfit_desc(
 		struct nvdimm_bus_descriptor *nd_desc)
 {
@@ -171,8 +198,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 +208,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_DSM) {
+		call_dsm = buf;
+		func = call_dsm->ncp_command;
+	}
 
 	if (nvdimm) {
 		struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
@@ -191,7 +225,7 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
 		cmd_name = nvdimm_cmd_name(cmd);
 		dsm_mask = nfit_mem->dsm_mask;
 		desc = nd_cmd_dimm_desc(cmd);
-		uuid = to_nfit_uuid(NFIT_DEV_DIMM);
+		uuid = nfit_mem->dsm_uuid;
 		handle = adev->handle;
 	} else {
 		struct acpi_device *adev = to_acpi_dev(acpi_desc);
@@ -199,7 +233,7 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
 		cmd_name = nvdimm_bus_cmd_name(cmd);
 		dsm_mask = nd_desc->dsm_mask;
 		desc = nd_cmd_bus_desc(cmd);
-		uuid = to_nfit_uuid(NFIT_DEV_BUS);
+		uuid = nd_desc->dsm_uuid;
 		handle = adev->handle;
 		dimm_name = "bus";
 	}
@@ -207,7 +241,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;
@@ -218,25 +252,50 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
 	in_buf.buffer.length = 0;
 
 	/* libnvdimm has already validated the input envelope */
-	for (i = 0; i < desc->in_num; i++)
+	for (i = 0; i < desc->in_num; i++) {
 		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->ncp_payload;
+		in_buf.buffer.length = call_dsm->ncp_size_in;
+		if (!known_nvdimm_type(call_dsm->ncp_type, cmd_type_tbl)) {
+			dev_dbg(dev, "%s:%s unsupported uuid\n", dimm_name,
+					cmd_name);
+			return -EINVAL;
+		}
+		rev  = call_dsm->ncp_rev;
+	}
 
 	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);
+		dev_dbg(dev, "%s:%s unexpected output object type cmd: %s %llu, type: %d\n",
+			__func__, dimm_name, cmd_name, func, out_obj->type);
+
 		return -EINVAL;
 	}
 
+	if (call_dsm) {
+		call_dsm->ncp_pot_size = out_obj->buffer.length;
+		memcpy(call_dsm->ncp_payload + call_dsm->ncp_size_in,
+			out_obj->buffer.pointer,
+			min(call_dsm->ncp_pot_size, call_dsm->ncp_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,13 +977,60 @@ static struct nvdimm *acpi_nfit_dimm_by_handle(struct acpi_nfit_desc *acpi_desc,
 	return NULL;
 }
 
+
+/*
+ * determine if the _DSM specified by UUID is supported and return
+ * mask of supported functions in nd_cmd_mask.
+ */
+
+static int acpi_nfit_sup_func(acpi_handle handle, const u8 *uuid,
+		int rev, unsigned long *nd_cmd_mask)
+{
+	int i;
+	u64 mask = 0;
+	union acpi_object *obj;
+
+	obj = acpi_evaluate_dsm(handle, uuid, rev, 0, NULL);
+	if (!obj)
+		return 0;
+	/* For compatibility, old BIOSes may return an integer */
+	if (obj->type == ACPI_TYPE_INTEGER)
+		mask = obj->integer.value;
+	else if (obj->type == ACPI_TYPE_BUFFER)
+		for (i = 0; i < obj->buffer.length && i < 8; i++)
+			mask |= (((u8)obj->buffer.pointer[i]) << (i * 8));
+	ACPI_FREE(obj);
+
+	*nd_cmd_mask = mask;
+
+	return !!mask;
+}
+
+
+static inline void
+to_nfit_uuid_msk(acpi_handle handle, struct cmd_type_tbl *tbl,
+		u8 const **cmd_uuid, unsigned long *cmd_mask)
+{
+	unsigned long mask = 0;
+	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_nfit_sup_func(handle, uuid, rev, &mask)) {
+			*cmd_mask = mask & tbl[i].mask;
+			*cmd_uuid = uuid;
+			break;
+		}
+	}
+}
+
 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);
-	int i;
 
 	nfit_mem->dsm_mask = acpi_desc->dimm_dsm_force_en;
 	adev = to_acpi_dev(acpi_desc);
@@ -939,9 +1045,7 @@ 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 (acpi_check_dsm(adev_dimm->handle, uuid, 1, 1ULL << i))
-			set_bit(i, &nfit_mem->dsm_mask);
+	to_nfit_uuid_msk(adev_dimm->handle, cmd_type_tbl, &nfit_mem->dsm_uuid, &nfit_mem->dsm_mask);
 
 	return 0;
 }
@@ -1003,18 +1107,15 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *acpi_desc)
 static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
 {
 	struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc;
-	const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
 	struct acpi_device *adev;
-	int i;
 
 	nd_desc->dsm_mask = acpi_desc->bus_dsm_force_en;
 	adev = to_acpi_dev(acpi_desc);
 	if (!adev)
 		return;
 
-	for (i = ND_CMD_ARS_CAP; i <= ND_CMD_CLEAR_ERROR; i++)
-		if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
-			set_bit(i, &nd_desc->dsm_mask);
+	nd_desc->call_dsm = 1;
+	to_nfit_uuid_msk(adev->handle, cmd_type_tbl, &nd_desc->dsm_uuid, &nd_desc->dsm_mask);
 }
 
 static ssize_t range_index_show(struct device *dev,
@@ -2463,6 +2564,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 3355748..32d2c5a 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_DSM] = {
+		.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_DSM] = {
+		.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_DSM) {
+		struct nd_cmd_pkg *pkg = buf;
+
+		return pkg->ncp_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_DSM) {
+		struct nd_cmd_pkg *pkg =
+				(struct nd_cmd_pkg *) in_field;
+		return pkg->ncp_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,16 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		dimm_name = "bus";
 	}
 
+	if (cmd == ND_CMD_CALL_DSM) {
+		if (!nd_desc->call_dsm)
+			return -ENOTTY;
+		if (copy_from_user(&call_dsm, p, sizeof(call_dsm)))
+			return -EFAULT;
+		func = call_dsm.ncp_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 +648,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_DSM:
 			dev_dbg(&nvdimm_bus->dev, "'%s' command while read-only.\n",
 					nvdimm ? nvdimm_cmd_name(cmd)
 					: nvdimm_bus_cmd_name(cmd));
@@ -643,6 +676,16 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		in_len += in_size;
 	}
 
+	if (cmd == ND_CMD_CALL_DSM) {
+		dev_dbg(dev, "%s:%s rev: %u, idx: %llu, in: %zu, out: %zu, len %zu\n",
+				__func__, dimm_name, call_dsm.ncp_rev,
+				call_dsm.ncp_command, in_len, out_len, buf_len);
+
+		for (i = 0; i < ARRAY_SIZE(call_dsm.ncp_reserved2); i++)
+			if (call_dsm.ncp_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

[RFC v6 8/8] 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 3187322..35512cd 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_DSM) {
+		struct nd_cmd_pkg *call_dsm = buf;
+
+		buf_len = call_dsm->ncp_size_in + call_dsm->ncp_size_out;
+		buf = (void *) call_dsm->ncp_payload;
+		func = call_dsm->ncp_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->dsm_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 v6 2/8] nvdimm: Add wrapper for IOCTL pass thru

[Dan Williams]

On Fri, Mar 11, 2016 at 1:16 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> Add struct nd_cmd_pkg which serves as a warapper for
> the data being passed via a pass thru to a NVDIMM DSM.
> This wrapper specifies the extra information in a uniform
> manner allowing the kenrel to call a DSM without knowing
> specifics of the DSM.
>
> Add dsm_call command to nvdimm_bus_cmd_name and nvdimm_cmd_name.
>
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  include/uapi/linux/ndctl.h | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
> index 7cc28ab..459c6ba 100644
> --- a/include/uapi/linux/ndctl.h
> +++ b/include/uapi/linux/ndctl.h
> @@ -125,6 +125,7 @@ enum {
>         ND_CMD_VENDOR_EFFECT_LOG_SIZE = 7,
>         ND_CMD_VENDOR_EFFECT_LOG = 8,
>         ND_CMD_VENDOR = 9,
> +       ND_CMD_CALL_DSM = 10,
>  };
>
>  enum {
> @@ -139,6 +140,7 @@ static inline const char *nvdimm_bus_cmd_name(unsigned cmd)
>                 [ND_CMD_ARS_START] = "ars_start",
>                 [ND_CMD_ARS_STATUS] = "ars_status",
>                 [ND_CMD_CLEAR_ERROR] = "clear_error",
> +               [ND_CMD_CALL_DSM] = "dsm_call",
>         };
>
>         if (cmd < ARRAY_SIZE(names) && names[cmd])
> @@ -158,6 +160,7 @@ static inline const char *nvdimm_cmd_name(unsigned cmd)
>                 [ND_CMD_VENDOR_EFFECT_LOG_SIZE] = "effect_size",
>                 [ND_CMD_VENDOR_EFFECT_LOG] = "effect_log",
>                 [ND_CMD_VENDOR] = "vendor",
> +               [ND_CMD_CALL_DSM] = "dsm_call",
>         };
>
>         if (cmd < ARRAY_SIZE(names) && names[cmd])
> @@ -224,4 +227,23 @@ enum ars_masks {
>         ARS_STATUS_MASK = 0x0000FFFF,
>         ARS_EXT_STATUS_SHIFT = 16,
>  };
> +
> +
> +struct nd_cmd_pkg {
> +       __u32   ncp_type;

I think 'family' is more clear, to signify the class of commands that
the 'command' parameter references.  Also let's drop the ncp_ prefix,
it's non-idiomatic with the rest of the definitions in this file.

> +       __u32   ncp_rev;

Why do we need a separate revision field?  A 'revision' change
effectively selects a different 'family' of commands, so I don't think
we need to reflect that ACPI-specific aspect of the commands at this
level.

> +       __u64   ncp_command;
> +       __u32   ncp_size_in;                    /* size of input payload */
> +       __u32   ncp_size_out;                   /* size of user buffer   */

What's "user" in this context?  How about "/* size of output payload */"

> +       __u32   ncp_reserved2[9];               /* reserved must be zero */
> +       __u32   ncp_pot_size;                   /* potential output size */

It's not 'potential'' it's the 'actual' output size written by the
kernel/firmware, right?

> +       unsigned char ncp_payload[];            /* Contents of call      */
> +};
> +
> +#define NCP_TYPE_BUS                   1
> +#define NCP_TYPE_DIMM_INTEL1           2
> +#define NCP_TYPE_DIMM_N_HPE1           3
> +#define NCP_TYPE_DIMM_N_HPE2           4

s/NCP_TYPE_/ND_FAMILY_/

I think it would be helpful to put comments in the code here about
where these families are defined.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v6 2/8] nvdimm: Add wrapper for IOCTL pass thru

[Jerry Hoemann]

On Fri, Mar 11, 2016 at 02:04:04PM -0800, Dan Williams wrote:
> On Fri, Mar 11, 2016 at 1:16 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> > +
> > +
> > +struct nd_cmd_pkg {
> > +       __u32   ncp_type;
> 
> I think 'family' is more clear, to signify the class of commands that
> the 'command' parameter references.


Oh, family of commands for a DSM.  I was thinking you wanted
family of DSM for a given nvdimm type which didn't seem quite
right.

I'll change.


>					 Also let's drop the ncp_ prefix,
> it's non-idiomatic with the rest of the definitions in this file.


I like field names with prefixes that refer back to the structure they're
defined in.  It makes reading code easier.  (e.g. how many times is
"size" defined in a struct?)

would you be okay with a prefix "nd_"?  it doesn't quite tie as closely
to the struct, but it should make them unique w/in the kernel.


> 
> > +       __u32   ncp_rev;
> 
> Why do we need a separate revision field?  A revision change
> effectively selects a different 'family' of commands, so I don't think
> we need to reflect that ACPI-specific aspect of the commands at this
> level.

 To be clear, if/when a DSM specification is extended in the future, with
 a new command added (all other commands remaining) and the revision to
 the DSM is incremented, you want to have a new family defined/used
 by the ioctl interface?

 I should be able to do this.



> 
> > +       __u64   ncp_command;
> > +       __u32   ncp_size_in;                    /* size of input payload */
> > +       __u32   ncp_size_out;                   /* size of user buffer   */
> 
> What's "user" in this context?  How about "/* size of output payload */"

Yes, a confusing name.

This is total space (including input portion) that the user
space caller has set aside for the output of the DSM call.

The kernel when copying out to user space will not exceed
the size given.

Need to convey that both of these are INPUT parameter the caller
is passing.


> 
> > +       __u32   ncp_reserved2[9];               /* reserved must be zero */
> > +       __u32   ncp_pot_size;                   /* potential output size */
> 
> It's not 'potential'' it's the 'actual' output size written by the
> kernel/firmware, right?


No, it is potential.

Going though multiple version of the spec, I would see dsm calls
where the amount of the data the dsm returned was not something that
the caller could know apriori.

So, the interface returns the amount of data that will fit in
the space provided and tell the user app in ncp_pot_size the
amount of data it wants to return.  If what the dsm call wants to
return fits in the space provided, it is what is written.  If what
the DSM would want to return is greater, it is the greater.

This allows for idempotent calls to be redone with appropriately
sized buffer.


This interface needs a man page.  :)

I will put a larger comment block in the next version.



> 
> > +       unsigned char ncp_payload[];            /* Contents of call      */
> > +};
> > +
> > +#define NCP_TYPE_BUS                   1
> > +#define NCP_TYPE_DIMM_INTEL1           2
> > +#define NCP_TYPE_DIMM_N_HPE1           3
> > +#define NCP_TYPE_DIMM_N_HPE2           4
> 
> s/NCP_TYPE_/ND_FAMILY_/

  Will do.

> 
> I think it would be helpful to put comments in the code here about
> where these families are defined.

  Will do.

-- 

-----------------------------------------------------------------------------
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 v6 2/8] nvdimm: Add wrapper for IOCTL pass thru

[Dan Williams]

On Fri, Mar 11, 2016 at 3:29 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Fri, Mar 11, 2016 at 02:04:04PM -0800, Dan Williams wrote:
>> On Fri, Mar 11, 2016 at 1:16 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> > +
>> > +
>> > +struct nd_cmd_pkg {
>> > +       __u32   ncp_type;
>>
>> I think 'family' is more clear, to signify the class of commands that
>> the 'command' parameter references.
>
>
> Oh, family of commands for a DSM.  I was thinking you wanted
> family of DSM for a given nvdimm type which didn't seem quite
> right.
>
> I'll change.
>
>
>>                                        Also let's drop the ncp_ prefix,
>> it's non-idiomatic with the rest of the definitions in this file.
>
>
> I like field names with prefixes that refer back to the structure they're
> defined in.  It makes reading code easier.  (e.g. how many times is
> "size" defined in a struct?)

True, I do appreciate that aspect.

> would you be okay with a prefix "nd_"?  it doesn't quite tie as closely
> to the struct, but it should make them unique w/in the kernel.

That sounds like a reasonable compromise.

>>
>> > +       __u32   ncp_rev;
>>
>> Why do we need a separate revision field?  A revision change
>> effectively selects a different 'family' of commands, so I don't think
>> we need to reflect that ACPI-specific aspect of the commands at this
>> level.
>
>  To be clear, if/when a DSM specification is extended in the future, with
>  a new command added (all other commands remaining) and the revision to
>  the DSM is incremented, you want to have a new family defined/used
>  by the ioctl interface?
>
>  I should be able to do this.

Yes, to date we've handled extending and changing command sets within
the same revision.

>> > +       __u64   ncp_command;
>> > +       __u32   ncp_size_in;                    /* size of input payload */
>> > +       __u32   ncp_size_out;                   /* size of user buffer   */
>>
>> What's "user" in this context?  How about "/* size of output payload */"
>
> Yes, a confusing name.
>
> This is total space (including input portion) that the user
> space caller has set aside for the output of the DSM call.
>
> The kernel when copying out to user space will not exceed
> the size given.
>
> Need to convey that both of these are INPUT parameter the caller
> is passing.
>
>
>>
>> > +       __u32   ncp_reserved2[9];               /* reserved must be zero */
>> > +       __u32   ncp_pot_size;                   /* potential output size */
>>
>> It's not 'potential'' it's the 'actual' output size written by the
>> kernel/firmware, right?
>
>
> No, it is potential.
>
> Going though multiple version of the spec, I would see dsm calls
> where the amount of the data the dsm returned was not something that
> the caller could know apriori.
>
> So, the interface returns the amount of data that will fit in
> the space provided and tell the user app in ncp_pot_size the
> amount of data it wants to return.  If what the dsm call wants to
> return fits in the space provided, it is what is written.  If what
> the DSM would want to return is greater, it is the greater.

The case where firmware output overflows the provided buffer the
implementation considers that a fatal error.  All the variable output
commands indicate (explicitly or implicitly) a maximum output payload
size, so an overflow means the driver and the firmware are
incompatible.

> This allows for idempotent calls to be redone with appropriately
> sized buffer.

I'd prefer we keep the status quo of just failing or otherwise
refusing to support commands that exhibit this behavior.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v6 2/8] nvdimm: Add wrapper for IOCTL pass thru

[Vishal Verma]

On Fri, Mar 11, 2016 at 4:29 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>>                                        Also let's drop the ncp_ prefix,
>> it's non-idiomatic with the rest of the definitions in this file.
>
>
> I like field names with prefixes that refer back to the structure they're
> defined in.  It makes reading code easier.  (e.g. how many times is
> "size" defined in a struct?)
>
> would you be okay with a prefix "nd_"?  it doesn't quite tie as closely
> to the struct, but it should make them unique w/in the kernel.

Isn't this leaning towards the "Hungarian notation", and isn't that
discouraged in the kernel coding style..

>From Documentation/CodingStyle

Encoding the type of a function into the name (so-called Hungarian
notation) is brain damaged - the compiler knows the types anyway and can
check those, and it only confuses the programmer.

Re: [RFC v6 2/8] nvdimm: Add wrapper for IOCTL pass thru

[Dan Williams]

On Sat, Mar 12, 2016 at 7:49 AM, Vishal Verma <vishal@kernel.org> wrote:
> On Fri, Mar 11, 2016 at 4:29 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>>>                                        Also let's drop the ncp_ prefix,
>>> it's non-idiomatic with the rest of the definitions in this file.
>>
>>
>> I like field names with prefixes that refer back to the structure they're
>> defined in.  It makes reading code easier.  (e.g. how many times is
>> "size" defined in a struct?)
>>
>> would you be okay with a prefix "nd_"?  it doesn't quite tie as closely
>> to the struct, but it should make them unique w/in the kernel.
>
> Isn't this leaning towards the "Hungarian notation", and isn't that
> discouraged in the kernel coding style..
>
> From Documentation/CodingStyle
>
> Encoding the type of a function into the name (so-called Hungarian
> notation) is brain damaged - the compiler knows the types anyway and can
> check those, and it only confuses the programmer.

I wouldn't go that far... look at "struct bio", "struct file", or
"struct nd_region" that have a common prefix for (some) member fields.
That's not Hungarian notation, that's making the code easier to grep.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v6 2/8] nvdimm: Add wrapper for IOCTL pass thru

[Jerry Hoemann]

On Fri, Mar 11, 2016 at 03:48:55PM -0800, Dan Williams wrote:
> On Fri, Mar 11, 2016 at 3:29 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> > On Fri, Mar 11, 2016 at 02:04:04PM -0800, Dan Williams wrote:
> >> On Fri, Mar 11, 2016 at 1:16 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> >> > +
> >> > +
> >> > +struct nd_cmd_pkg {
> >> > +       __u32   ncp_type;
> >>
> >> I think 'family' is more clear, to signify the class of commands that
> >> the 'command' parameter references.
> >
> >
> > Oh, family of commands for a DSM.  I was thinking you wanted
> > family of DSM for a given nvdimm type which didn't seem quite
> > right.
> >
> > I'll change.
> >
> >
> >>                                        Also let's drop the ncp_ prefix,
> >> it's non-idiomatic with the rest of the definitions in this file.
> >
> >
> > I like field names with prefixes that refer back to the structure they're
> > defined in.  It makes reading code easier.  (e.g. how many times is
> > "size" defined in a struct?)
> 
> True, I do appreciate that aspect.
> 
> > would you be okay with a prefix "nd_"?  it doesn't quite tie as closely
> > to the struct, but it should make them unique w/in the kernel.
> 
> That sounds like a reasonable compromise.

Will do.

> 
> >>
> >> > +       __u32   ncp_rev;
> >>
> >> Why do we need a separate revision field?  A revision change
> >> effectively selects a different 'family' of commands, so I don't think
> >> we need to reflect that ACPI-specific aspect of the commands at this
> >> level.
> >
> >  To be clear, if/when a DSM specification is extended in the future, with
> >  a new command added (all other commands remaining) and the revision to
> >  the DSM is incremented, you want to have a new family defined/used
> >  by the ioctl interface?
> >
> >  I should be able to do this.
> 
> Yes, to date we've handled extending and changing command sets within
> the same revision.
> 
> >> > +       __u64   ncp_command;
> >> > +       __u32   ncp_size_in;                    /* size of input payload */
> >> > +       __u32   ncp_size_out;                   /* size of user buffer   */
> >>
> >> What's "user" in this context?  How about "/* size of output payload */"
> >
> > Yes, a confusing name.
> >
> > This is total space (including input portion) that the user
> > space caller has set aside for the output of the DSM call.
> >
> > The kernel when copying out to user space will not exceed
> > the size given.
> >
> > Need to convey that both of these are INPUT parameter the caller
> > is passing.
> >
> >
> >>
> >> > +       __u32   ncp_reserved2[9];               /* reserved must be zero */
> >> > +       __u32   ncp_pot_size;                   /* potential output size */
> >>
> >> It's not 'potential'' it's the 'actual' output size written by the
> >> kernel/firmware, right?
> >
> >
> > No, it is potential.
> >
> > Going though multiple version of the spec, I would see dsm calls
> > where the amount of the data the dsm returned was not something that
> > the caller could know apriori.
> >
> > So, the interface returns the amount of data that will fit in
> > the space provided and tell the user app in ncp_pot_size the
> > amount of data it wants to return.  If what the dsm call wants to
> > return fits in the space provided, it is what is written.  If what
> > the DSM would want to return is greater, it is the greater.
> 
> The case where firmware output overflows the provided buffer the
> implementation considers that a fatal error.  All the variable output
> commands indicate (explicitly or implicitly) a maximum output payload
> size, so an overflow means the driver and the firmware are
> incompatible.

Not wrong, just a different calling paradigm.

In addition to allowing user applications to query/size buffers
for the call, I have also found the information on how much data
firmware wants to return a useful diagnostic in diagnosing
firmware problems.

Also note, The upper limit on the amount of data the interface returns is
still in place.  I am not allowing for arbitrarily large returns.



> 
> > This allows for idempotent calls to be redone with appropriately
> > sized buffer.
> 
> I'd prefer we keep the status quo of just failing or otherwise
> refusing to support commands that exhibit this behavior.

I don't know what the final spec will look like.  It might be
an issue, it might not.  I would just prefer to have a flexible
kernel interface in place that will handle such situations so
we don't have to revisit it in the future.

-- 

-----------------------------------------------------------------------------
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 v6 2/8] nvdimm: Add wrapper for IOCTL pass thru

[Dan Williams]

On Sun, Mar 13, 2016 at 12:15 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Fri, Mar 11, 2016 at 03:48:55PM -0800, Dan Williams wrote:
>> On Fri, Mar 11, 2016 at 3:29 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> > On Fri, Mar 11, 2016 at 02:04:04PM -0800, Dan Williams wrote:
>> >> On Fri, Mar 11, 2016 at 1:16 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> >> > +
>> >> > +
>> >> > +struct nd_cmd_pkg {
>> >> > +       __u32   ncp_type;
>> >>
>> >> I think 'family' is more clear, to signify the class of commands that
>> >> the 'command' parameter references.
>> >
>> >
>> > Oh, family of commands for a DSM.  I was thinking you wanted
>> > family of DSM for a given nvdimm type which didn't seem quite
>> > right.
>> >
>> > I'll change.
>> >
>> >
>> >>                                        Also let's drop the ncp_ prefix,
>> >> it's non-idiomatic with the rest of the definitions in this file.
>> >
>> >
>> > I like field names with prefixes that refer back to the structure they're
>> > defined in.  It makes reading code easier.  (e.g. how many times is
>> > "size" defined in a struct?)
>>
>> True, I do appreciate that aspect.
>>
>> > would you be okay with a prefix "nd_"?  it doesn't quite tie as closely
>> > to the struct, but it should make them unique w/in the kernel.
>>
>> That sounds like a reasonable compromise.
>
> Will do.
>
>>
>> >>
>> >> > +       __u32   ncp_rev;
>> >>
>> >> Why do we need a separate revision field?  A revision change
>> >> effectively selects a different 'family' of commands, so I don't think
>> >> we need to reflect that ACPI-specific aspect of the commands at this
>> >> level.
>> >
>> >  To be clear, if/when a DSM specification is extended in the future, with
>> >  a new command added (all other commands remaining) and the revision to
>> >  the DSM is incremented, you want to have a new family defined/used
>> >  by the ioctl interface?
>> >
>> >  I should be able to do this.
>>
>> Yes, to date we've handled extending and changing command sets within
>> the same revision.
>>
>> >> > +       __u64   ncp_command;
>> >> > +       __u32   ncp_size_in;                    /* size of input payload */
>> >> > +       __u32   ncp_size_out;                   /* size of user buffer   */
>> >>
>> >> What's "user" in this context?  How about "/* size of output payload */"
>> >
>> > Yes, a confusing name.
>> >
>> > This is total space (including input portion) that the user
>> > space caller has set aside for the output of the DSM call.
>> >
>> > The kernel when copying out to user space will not exceed
>> > the size given.
>> >
>> > Need to convey that both of these are INPUT parameter the caller
>> > is passing.
>> >
>> >
>> >>
>> >> > +       __u32   ncp_reserved2[9];               /* reserved must be zero */
>> >> > +       __u32   ncp_pot_size;                   /* potential output size */
>> >>
>> >> It's not 'potential'' it's the 'actual' output size written by the
>> >> kernel/firmware, right?
>> >
>> >
>> > No, it is potential.
>> >
>> > Going though multiple version of the spec, I would see dsm calls
>> > where the amount of the data the dsm returned was not something that
>> > the caller could know apriori.
>> >
>> > So, the interface returns the amount of data that will fit in
>> > the space provided and tell the user app in ncp_pot_size the
>> > amount of data it wants to return.  If what the dsm call wants to
>> > return fits in the space provided, it is what is written.  If what
>> > the DSM would want to return is greater, it is the greater.
>>
>> The case where firmware output overflows the provided buffer the
>> implementation considers that a fatal error.  All the variable output
>> commands indicate (explicitly or implicitly) a maximum output payload
>> size, so an overflow means the driver and the firmware are
>> incompatible.
>
> Not wrong, just a different calling paradigm.
>
> In addition to allowing user applications to query/size buffers
> for the call, I have also found the information on how much data
> firmware wants to return a useful diagnostic in diagnosing
> firmware problems.
>
> Also note, The upper limit on the amount of data the interface returns is
> still in place.  I am not allowing for arbitrarily large returns.
>
>
>
>>
>> > This allows for idempotent calls to be redone with appropriately
>> > sized buffer.
>>
>> I'd prefer we keep the status quo of just failing or otherwise
>> refusing to support commands that exhibit this behavior.
>
> I don't know what the final spec will look like.  It might be
> an issue, it might not.  I would just prefer to have a flexible
> kernel interface in place that will handle such situations so
> we don't have to revisit it in the future.

In Linux kernel development it has been my experience that we don't
code for potential future eventualities.  We code the most pragmatic
minimal interface and evolve it over time if need be.  The expectation
is that occasional revisiting is less overhead than maintaining unused
functionality indefinitely.

Take this call_dsm interface as an example.  We've been able to boil
down the interface to something manageable rather than the
free-for-all that was originally proposed.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [RFC v6 2/8] nvdimm: Add wrapper for IOCTL pass thru

[Vishal Verma]

On Sat, 2016-03-12 at 09:50 -0800, Dan Williams wrote:
> On Sat, Mar 12, 2016 at 7:49 AM, Vishal Verma <vishal@kernel.org>
> wrote:
> > 
> > On Fri, Mar 11, 2016 at 4:29 PM, Jerry Hoemann <jerry.hoemann@hpe.c
> > om> wrote:
> > > 
> > > > 
> > > >                                        Also let's drop the ncp_
> > > > prefix,
> > > > it's non-idiomatic with the rest of the definitions in this
> > > > file.
> > > 
> > > I like field names with prefixes that refer back to the structure
> > > they're
> > > defined in.  It makes reading code easier.  (e.g. how many times
> > > is
> > > "size" defined in a struct?)
> > > 
> > > would you be okay with a prefix "nd_"?  it doesn't quite tie as
> > > closely
> > > to the struct, but it should make them unique w/in the kernel.
> > Isn't this leaning towards the "Hungarian notation", and isn't that
> > discouraged in the kernel coding style..
> > 
> > From Documentation/CodingStyle
> > 
> > Encoding the type of a function into the name (so-called Hungarian
> > notation) is brain damaged - the compiler knows the types anyway
> > and can
> > check those, and it only confuses the programmer.
> I wouldn't go that far... look at "struct bio", "struct file", or
> "struct nd_region" that have a common prefix for (some) member
> fields.
> That's not Hungarian notation, that's making the code easier to grep.

Ah yes - agreed.

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

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