Skip to contentSkip to conversation

nvdimm: Add an IOCTL pass thru for DSM calls

PATCH Superseded Jerry Hoemann 5 participants 28 messages Nov 6, 2015 – Nov 11, 2015

Subsystems (2)
ACPI libnvdimm: non-volatile memory device

v1 of 10 Superseded Latest is v10 →

lore
Share & tools
lore

Jerry Hoemann

author
#0 lore raw

cover nvdimm: Add an IOCTL pass thru for DSM calls

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

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

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

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

A new IOCTL type "P" is added for the pass thru call.

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

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

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

To make the resultant kernel code easier to understand the existing functions acpi_nfit_ctl and __nd_ioctl were renamed to .*_intel to denote calling mechanism as in 4.2 tailored to the Intel Example DSM. New functions acpi_nfit_ctl_passthru and __nd_ioctl_passthru were created to supply the pass thru interface.

These changes are based upon the 4.3 kernel.

Jerry Hoemann (4):
nvdimm: Add wrapper for IOCTL pass thru.
nvdimm: Add IOCTL pass thru
nvdimm: Add IOCTL pass thru
nvdimm: rename functions that aren't IOCTL passthru

 drivers/acpi/nfit.c        |  91 ++++++++++++++++++++++++++++++++--
 drivers/nvdimm/bus.c       | 118 +++++++++++++++++++++++++++++++++++++++++----
 drivers/nvdimm/dimm_devs.c |   6 +--
 include/linux/libnvdimm.h  |   3 +-
 include/uapi/linux/ndctl.h |  20 +++++++-
 5 files changed, 220 insertions(+), 18 deletions(-)
Signature · 1 line
1.7.11.3
author
#1 lore raw

PATCH 1/4 nvdimm: Add wrapper for IOCTL pass thru.

Add IOCTL type 'P' to denote NVDIMM_TYPE_PASSTHRU.

Add struct ndn_pkg which the pass thru IOCTL interfaces uses. ndn_pkg serves as a wrapper for the data being passed to the underlying DSM and specifies siz data in a uniform manner allowing the kernel to call the DSM without knowing specifics of the DSM.

Signed-off-by Jerry Hoemann <[email protected]>
---
 include/uapi/linux/ndctl.h | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
include/uapi/linux/ndctl.h +19 -1
index 5b4a4be..1c81a99 100644
@@ -15,6 +15,9 @@
 
 #include <linux/types.h>
 
+#define NVDIMM_TYPE_INTEL		'N'
+#define NVDIMM_TYPE_PASSTHRU		'P'
+
 struct nd_cmd_smart {
 	__u32 status;
 	__u8 data[128];

@@ -148,7 +151,8 @@ static inline const char *nvdimm_cmd_name(unsigned cmd)
 	return "unknown";
 }
 
-#define ND_IOCTL 'N'
+#define ND_IOCTL			NVDIMM_TYPE_INTEL
+
 
 #define ND_IOCTL_SMART			_IOWR(ND_IOCTL, ND_CMD_SMART,\
 					struct nd_cmd_smart)

@@ -204,4 +208,18 @@ enum ars_masks {
 	ARS_STATUS_MASK = 0x0000FFFF,
 	ARS_EXT_STATUS_SHIFT = 16,
 };
+
+
+struct ndn_pkg {
+	struct {
+		__u8	dsm_uuid[16];
+		__u32	dsm_in;			/* size of _DSM input    */
+		__u32	dsm_out;		/* size of user buffer   */
+		__u32	dsm_rev;		/* revision of dsm call  */
+		__u32	res[8];			/* reserved must be zero */
+		__u32	dsm_size;		/* size _DSM would write */
+	} h;
+	unsigned char buf[];
+} __packed;
+
 #endif /* __NDCTL_H__ */
Signature · 1 line
1.7.11.3
#2 lore raw

Re: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru.

Quoted 1 line · Jerry Hoemann

Add IOCTL type 'P' to denote NVDIMM_TYPE_PASSTHRU.

Can't you just make passthrough a separate command? If you actually add the ioctl definition for passthrough (which you didn't do for some reason?), it looks odd:

#define ND_IOCTL_PASSTHRU _IOWR(NVDIMM_TYPE_PASSTHRU,, ND_CMD_PASSTHRU, \
struct ndn_package)

Care to comment on why you chose a different type instead of specifying a new command?

include/uapi/linux/ndctl.h enum ars_masks { · line 222

Quoted 10 lines
include/uapi/linux/ndctl.h +10 -0
+struct ndn_pkg {
+	struct {
+		__u8	dsm_uuid[16];
+		__u32	dsm_in;			/* size of _DSM input    */
+		__u32	dsm_out;		/* size of user buffer   */
+		__u32	dsm_rev;		/* revision of dsm call  */
+		__u32	res[8];			/* reserved must be zero */
+		__u32	dsm_size;		/* size _DSM would write */
+	} h;
+	unsigned char buf[];

Please change that to:

	__u8 *buf;

since acpi_object.buffer.pointer is a u8 *.

Note that the size of this structure will be different for 32 vs. 64 bit, but I don't think it matters since offsets won't change (the pointer is at the end of the structure).

#3 lore raw

Re: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru.

Quoted 8 lines · Jeff Moyer
Quoted 1 line · Jerry Hoemann

Add IOCTL type 'P' to denote NVDIMM_TYPE_PASSTHRU.

Can't you just make passthrough a separate command? If you actually add the ioctl definition for passthrough (which you didn't do for some reason?), it looks odd:

Quoted 5 lines

#define ND_IOCTL_PASSTHRU _IOWR(NVDIMM_TYPE_PASSTHRU,, ND_CMD_PASSTHRU, \
struct ndn_package)

Care to comment on why you chose a different type instead of specifying a new command?

+1 for making this just a new command number without a new top-level number space.

author
#4 lore raw

Re: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru.

In reply to Jeff Moyer
Quoted 5 lines · Jeff Moyer
Quoted 1 line · Jerry Hoemann

Add IOCTL type 'P' to denote NVDIMM_TYPE_PASSTHRU.

Can't you just make passthrough a separate command? If you actually add

There are multiple conflicting NVDIMM _DSM running around, they are "device specific". So, we should plan in general and not just for the example DSM that Intel added support for. These DSM have over lapping and incompatible function ids.

The Intel example is an example, not standard. They are free to change it at will. So, we can't be certain there won't be a conflict some time in the future if we try to use their number space.

I'm trying to create a generic pass thru that any vendors can use. Putting this in the Intel function number space doesn't make a lot of sense to me.

Quoted 2 lines

the ioctl definition for passthrough (which you didn't do for some reason?), it looks odd:

The definition for the IOCTLs are in a user space application. These aren't required in the kernel as the kernel is only a pass thru.

As the DSM I'm working with isn't yet finalized, I've been told that i can't share the user space portion yet.

Quoted 19 lines

#define ND_IOCTL_PASSTHRU _IOWR(NVDIMM_TYPE_PASSTHRU,, ND_CMD_PASSTHRU, \
struct ndn_package)

Care to comment on why you chose a different type instead of specifying a new command?

include/uapi/linux/ndctl.h enum ars_masks { · line 222

Quoted 10 lines
include/uapi/linux/ndctl.h +10 -0
+struct ndn_pkg {
+	struct {
+		__u8	dsm_uuid[16];
+		__u32	dsm_in;			/* size of _DSM input    */
+		__u32	dsm_out;		/* size of user buffer   */
+		__u32	dsm_rev;		/* revision of dsm call  */
+		__u32	res[8];			/* reserved must be zero */
+		__u32	dsm_size;		/* size _DSM would write */
+	} h;
+	unsigned char buf[];

Please change that to:

Quoted 2 lines
	__u8 *buf;

since acpi_object.buffer.pointer is a u8 *.

buf isn't being passed to acpi_evaluate_dsm. its just being used for pointer offset in acpi_nfit_ctl_passthru. The "payload" that will be passed to acpi_evaluate_dsm follows.

Quoted 4 lines

Note that the size of this structure will be different for 32 vs. 64 bit, but I don't think it matters since offsets won't change (the pointer is at the end of the structure).

I assume you mean size of struct changes if I use the pointer as substitute for the zero sized array? or are you saying that the packed attribute doesn't affect the layout of the anonymous struct?

Signature · 7 lines
-----------------------------------------------------------------------------
Jerry Hoemann            Software Engineer      Hewlett-Packard Enterprise

3404 E Harmony Rd. MS 36                        phone:  (970) 898-1022
Ft. Collins, CO 80528                           FAX:    (970) 898-0707
                                                email:  [email protected]
-----------------------------------------------------------------------------
#5 lore raw

Re: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru.

Quoted 12 lines · Jerry Hoemann
Quoted 5 lines · Jeff Moyer
Quoted 1 line · Jerry Hoemann

Add IOCTL type 'P' to denote NVDIMM_TYPE_PASSTHRU.

Can't you just make passthrough a separate command? If you actually add

There are multiple conflicting NVDIMM _DSM running around, they are "device specific". So, we should plan in general and not just for the example DSM that Intel added support for. These DSM have over lapping and incompatible function ids.

Quoted 6 lines

The Intel example is an example, not standard. They are free to change it at will. So, we can't be certain there won't be a conflict some time in the future if we try to use their number space.

I'm trying to create a generic pass thru that any vendors can use. Putting this in the Intel function number space doesn't make a lot of sense to me.

OK, I see your point.

Quoted 6 lines
Quoted 2 lines

the ioctl definition for passthrough (which you didn't do for some reason?), it looks odd:

The definition for the IOCTLs are in a user space application. These aren't required in the kernel as the kernel is only a pass thru.

OK, I don't see the harm in including it in the kernel headers, but I'm not going to insist on it.

Quoted 2 lines

As the DSM I'm working with isn't yet finalized, I've been told that i can't share the user space portion yet.

That's OK, I don't think providing the userspace code is necessary for this patch set to make progress. (I didn't actually ask for it, to be clear.)

Quoted 21 lines
Quoted 18 lines

#define ND_IOCTL_PASSTHRU _IOWR(NVDIMM_TYPE_PASSTHRU,, ND_CMD_PASSTHRU, \
struct ndn_package)

Care to comment on why you chose a different type instead of specifying a new command?

include/uapi/linux/ndctl.h enum ars_masks { · line 222

Quoted 10 lines
include/uapi/linux/ndctl.h +10 -0
+struct ndn_pkg {
+	struct {
+		__u8	dsm_uuid[16];
+		__u32	dsm_in;			/* size of _DSM input    */
+		__u32	dsm_out;		/* size of user buffer   */
+		__u32	dsm_rev;		/* revision of dsm call  */
+		__u32	res[8];			/* reserved must be zero */
+		__u32	dsm_size;		/* size _DSM would write */
+	} h;
+	unsigned char buf[];

Please change that to:

Quoted 2 lines
	__u8 *buf;

since acpi_object.buffer.pointer is a u8 *.

Quoted 3 lines

buf isn't being passed to acpi_evaluate_dsm. its just being used for pointer offset in acpi_nfit_ctl_passthru. The "payload" that will be passed to acpi_evaluate_dsm follows.

+ in_buf.buffer.pointer = (void *) &pkg->buf;

I see. I misread that, because you didn't actually make buf a zero length array (see the structure definition quoted above). I guess you meant to write this:

    unsigned char buf[0];

Cheers,
Jeff

author
#6 lore raw

Re: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru.

Quoted 8 lines · Jeff Moyer
Quoted 4 lines

The definition for the IOCTLs are in a user space application. These aren't required in the kernel as the kernel is only a pass thru.

OK, I don't see the harm in including it in the kernel headers, but I'm not going to insist on it.

The IOCTL are defined in terms of the data structures representing the dsm functions. Since I'm not supposed to share the definitions of the DSM at this point, I can't share the IOCTL definitions.

When this restriction is lifted, I would be interested in pushing these definitions to the appropriate header file.

Quoted 6 lines
Quoted 2 lines

As the DSM I'm working with isn't yet finalized, I've been told that i can't share the user space portion yet.

That's OK, I don't think providing the userspace code is necessary for this patch set to make progress. (I didn't actually ask for it, to be clear.)

Understood. But it is sometimes nice to have a concrete example(s) of an interfaces usage.

...

Quoted 21 lines
Quoted 18 lines
Quoted 12 lines

include/uapi/linux/ndctl.h enum ars_masks { · line 222

Quoted 10 lines
include/uapi/linux/ndctl.h +10 -0
+struct ndn_pkg {
+	struct {
+		__u8	dsm_uuid[16];
+		__u32	dsm_in;			/* size of _DSM input    */
+		__u32	dsm_out;		/* size of user buffer   */
+		__u32	dsm_rev;		/* revision of dsm call  */
+		__u32	res[8];			/* reserved must be zero */
+		__u32	dsm_size;		/* size _DSM would write */
+	} h;
+	unsigned char buf[];

Please change that to:

Quoted 2 lines
	__u8 *buf;

since acpi_object.buffer.pointer is a u8 *.

buf isn't being passed to acpi_evaluate_dsm. its just being used for pointer offset in acpi_nfit_ctl_passthru. The "payload" that will be passed to acpi_evaluate_dsm follows.

(quoted diff) +1 -0
+	in_buf.buffer.pointer = (void *) &pkg->buf;
Quoted 6 lines

I see. I misread that, because you didn't actually make buf a zero length array (see the structure definition quoted above). I guess you meant to write this:

    unsigned char buf[0];

The ndn_pkg.buf struct uses a flexible array definition. This is in C99. An explicit zero length array is a gcc extension that has been around much
longer. They behave in a similar fashion, but aren't identical. In my
limited use they behave the same.

Signature · 3 lines
-----------------------------------------------------------------------------
Jerry Hoemann            Software Engineer      Hewlett-Packard Enterprise
-----------------------------------------------------------------------------
#7 lore raw

Re: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru.

Quoted 1 line · Jerry Hoemann

On Tue, Nov 10, 2015 at 03:26:38PM -0500, Jeff Moyer wrote:

[..]

Quoted 11 lines
Quoted 6 lines

I see. I misread that, because you didn't actually make buf a zero length array (see the structure definition quoted above). I guess you meant to write this:

    unsigned char buf[0];

The ndn_pkg.buf struct uses a flexible array definition. This is in C99. An explicit zero length array is a gcc extension that has been around much
longer. They behave in a similar fashion, but aren't identical. In my
limited use they behave the same.

"buf[0]" is more idiomatic for Linux. I know I expressed concern about compiler compatibility for ACPICA, but this path does not have ACPICA interactions.

#8 lore raw

Re: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru.

In reply to Jerry Hoemann
Quoted 4 lines · Jerry Hoemann

The ndn_pkg.buf struct uses a flexible array definition. This is in C99. An explicit zero length array is a gcc extension that has been around much
longer. They behave in a similar fashion, but aren't identical. In my
limited use they behave the same.

I could swear I've been bitten by this before, but you are correct.

Thanks,
Jeff

#9 lore raw

Re: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru.

In reply to Jerry Hoemann
Quoted 12 lines · Jerry Hoemann
Quoted 5 lines · Jeff Moyer
Quoted 1 line · Jerry Hoemann

Add IOCTL type 'P' to denote NVDIMM_TYPE_PASSTHRU.

Can't you just make passthrough a separate command? If you actually add

There are multiple conflicting NVDIMM _DSM running around, they are "device specific". So, we should plan in general and not just for the example DSM that Intel added support for. These DSM have over lapping and incompatible function ids.

Quoted 6 lines

The Intel example is an example, not standard. They are free to change it at will. So, we can't be certain there won't be a conflict some time in the future if we try to use their number space.

I'm trying to create a generic pass thru that any vendors can use. Putting this in the Intel function number space doesn't make a lot of sense to me.

It isn't the "Intel" function number space. The fact that they currently align is just a happy accident. The kernel is free to break the 1:1 ioctl number to DSM function number relationship, and I think it would make the implementation cleaner in this case.

#10 lore raw

Re: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru.

Quoted 17 lines · Dan Williams
Quoted 16 lines · Jerry Hoemann
Quoted 5 lines · Jeff Moyer
Quoted 1 line · Jerry Hoemann

Add IOCTL type 'P' to denote NVDIMM_TYPE_PASSTHRU.

Can't you just make passthrough a separate command? If you actually add

There are multiple conflicting NVDIMM _DSM running around, they are "device specific". So, we should plan in general and not just for the example DSM that Intel added support for. These DSM have over lapping and incompatible function ids.

The Intel example is an example, not standard. They are free to change it at will. So, we can't be certain there won't be a conflict some time in the future if we try to use their number space.

Quoted 5 lines
Quoted 2 lines

I'm trying to create a generic pass thru that any vendors can use. Putting this in the Intel function number space doesn't make a lot of sense to me.

It isn't the "Intel" function number space. The fact that they currently align is just a happy accident.

It's not really a happy accident. Your commit message says it
was derived from the Intel spec 'for convenience', which I think is convenient for anything that implements that spec.

We've discussed ways of supporting different command sets with you and determined that this pass-through mechanism was a good approach because it allows multiple different command sets to be support in a generic way. Blending the two flavors (generic pass through and explicit function definitions) is confusing to me.

Quoted 3 lines

The kernel is free to break
the 1:1 ioctl number to DSM function number relationship, and I think it would make the implementation cleaner in this case.

To me it's less clean and even for your own example spec, less convenient if Intel ever updates that spec.

-- ljk

Quoted 5 lines

_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm

#11 lore raw

Re: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru.

Quoted 30 lines · Linda Knippers
Quoted 29 lines · Dan Williams
Quoted 24 lines · Jerry Hoemann
Quoted 7 lines · Jeff Moyer
Quoted 1 line · Jerry Hoemann

Add IOCTL type 'P' to denote NVDIMM_TYPE_PASSTHRU.

Can't you just make passthrough a separate command? If you actually add

There are multiple conflicting NVDIMM _DSM running around, they are "device specific". So, we should plan in general and not just for the example DSM that Intel added support for. These DSM have over lapping and incompatible function ids.

The Intel example is an example, not standard. They are free to change it at will. So, we can't be certain there won't be a conflict some time in the future if we try to use their number space.

I'm trying to create a generic pass thru that any vendors can use. Putting
this in the Intel function number space doesn't make a lot of sense to me.

Quoted 8 lines
Quoted 2 lines

It isn't the "Intel" function number space. The fact that they currently align is just a happy accident.

It's not really a happy accident. Your commit message says it was derived from the Intel spec 'for convenience', which I think is convenient
for anything that implements that spec.

Right, and now its no longer convenient to keep things one to one.

Quoted 6 lines

We've discussed ways of supporting different command sets with you and determined that this pass-through mechanism was a good approach because it allows multiple different command sets to be support in a generic way. Blending the two flavors (generic pass through and explicit function definitions) is confusing to me.

Quoted 7 lines
Quoted 3 lines

The kernel is free to break
the 1:1 ioctl number to DSM function number relationship, and I think it would make the implementation cleaner in this case.

To me it's less clean and even for your own example spec, less convenient if Intel ever updates that spec.

If that spec is ever updated any new commands will be implemented with this new generic envelope as the marshaling mechanism. I'd also look to convert the existing commands into this new envelope and deprecate the existing per-DSM-function number approach. Finally I don't look at this as purely "passthru" as the kernel will want to crack open the input payload for commands that it cares about with kernel relevant side effects, like namespace label updates.

#12 lore raw

RE: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru.

In reply to Jerry Hoemann
Quoted 5 lines

-----Original Message-----
From: Linux-nvdimm [mailto:[email protected]] On Behalf Of Hoemann, Jerry
Sent: Friday, November 6, 2015 4:27 PM
Subject: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru.

...

Quoted 1 line
include/uapi/linux/ndctl.h +0 -0

...

include/uapi/linux/ndctl.h enum ars_masks { · line 223

Quoted 11 lines
include/uapi/linux/ndctl.h +11 -0
+struct ndn_pkg {
+	struct {
+		__u8	dsm_uuid[16];
+		__u32	dsm_in;			/* size of _DSM input    */
+		__u32	dsm_out;		/* size of user buffer   */
+		__u32	dsm_rev;		/* revision of dsm call  */
+		__u32	res[8];			/* reserved must be zero */
+		__u32	dsm_size;		/* size _DSM would write */
+	} h;
+	unsigned char buf[];
+} __packed;

Given that the _DSM arguments are defined as:
* Arg0 UUID: Buffer of 16 bytes
* Arg1 Revision ID: Integer (8 bytes)
* Arg2 Function Index: Integer (8 bytes)
* Arg3 Package: function-specific

1. The __u32 for dsm_rev is not big enough to express all possible 8 byte Revision IDs.

2. The unsigned int cmd (carried outside this structure) is not big enough on all platforms (e.g., 32-bit) to express all possible Function Indexes.

3. The Revision ID and Function Index values passed to the _DSM are defined as little-endian. Are they intended to use native endianness or be little-endian in this structure?

---
Robert Elliott, HPE Persistent Memory

author
#13 lore raw

Re: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru.

Quoted 29 lines · Robert (Persistent Memory)
Quoted 5 lines

-----Original Message-----
From: Linux-nvdimm [mailto:[email protected]] On Behalf Of Hoemann, Jerry
Sent: Friday, November 6, 2015 4:27 PM
Subject: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru.

...

Quoted 1 line
include/uapi/linux/ndctl.h +0 -0

...

include/uapi/linux/ndctl.h enum ars_masks { · line 223

Quoted 11 lines
include/uapi/linux/ndctl.h +11 -0
+struct ndn_pkg {
+	struct {
+		__u8	dsm_uuid[16];
+		__u32	dsm_in;			/* size of _DSM input    */
+		__u32	dsm_out;		/* size of user buffer   */
+		__u32	dsm_rev;		/* revision of dsm call  */
+		__u32	res[8];			/* reserved must be zero */
+		__u32	dsm_size;		/* size _DSM would write */
+	} h;
+	unsigned char buf[];
+} __packed;

Given that the _DSM arguments are defined as:
* Arg0 UUID: Buffer of 16 bytes
* Arg1 Revision ID: Integer (8 bytes)
* Arg2 Function Index: Integer (8 bytes)
* Arg3 Package: function-specific

1. The __u32 for dsm_rev is not big enough to express all possible 8 byte Revision IDs.

Quoted 9 lines

2. The unsigned int cmd (carried outside this structure) is not big enough on all platforms (e.g., 32-bit) to express all possible Function Indexes.

3. The Revision ID and Function Index values passed to the _DSM are defined as little-endian. Are they intended to use native endianness or be little-endian in this structure?

Thanks, Robert. I will look at these for version 2.

Jerry

Signature · 7 lines
-----------------------------------------------------------------------------
Jerry Hoemann            Software Engineer      Hewlett-Packard Enterprise

3404 E Harmony Rd. MS 36                        phone:  (970) 898-1022
Ft. Collins, CO 80528                           FAX:    (970) 898-0707
                                                email:  [email protected]
-----------------------------------------------------------------------------
author
#14 lore raw

PATCH 2/4 nvdimm: Add IOCTL pass thru

In reply to Jerry Hoemann

Add internal data structure for ndctl_passthru call.

Signed-off-by Jerry Hoemann <[email protected]>
---
 include/linux/libnvdimm.h | 1 +
 1 file changed, 1 insertion(+)
include/linux/libnvdimm.h +1 -0
index 3f021dc..01117e1 100644
@@ -72,6 +72,7 @@ struct nvdimm_bus_descriptor {
 	unsigned long dsm_mask;
 	char *provider_name;
 	ndctl_fn ndctl;
+	ndctl_fn ndctl_passthru;
 };
 
 struct nd_cmd_desc {
Signature · 1 line
1.7.11.3
#15 lore raw

Re: [PATCH 2/4] nvdimm: Add IOCTL pass thru

Quoted 12 lines · Jerry Hoemann

Add internal data structure for ndctl_passthru call.

Signed-off-by Jerry Hoemann <[email protected]>
---
 include/linux/libnvdimm.h | 1 +
 1 file changed, 1 insertion(+)
include/linux/libnvdimm.h +0 -0
index 3f021dc..01117e1 100644

include/linux/libnvdimm.h struct nvdimm_bus_descriptor { · line 75

Quoted 4 lines
include/linux/libnvdimm.h +1 -0
@@ -72,6 +72,7 @@ struct nvdimm_bus_descriptor {
 	unsigned long dsm_mask;
 	char *provider_name;
 	ndctl_fn ndctl;
+	ndctl_fn ndctl_passthru;

I don't think this is necessary. Vector off inside of __nd_ioctl. That especially makes sense if you do switch to passthrough as a command instead of a type, but it can work either way.

-Jeff

author
#16 lore raw

Re: [PATCH 2/4] nvdimm: Add IOCTL pass thru

Quoted 9 lines · Jeff Moyer
Quoted 7 lines · Jerry Hoemann

Add internal data structure for ndctl_passthru call.

Signed-off-by Jerry Hoemann <[email protected]>
---
 include/linux/libnvdimm.h | 1 +
 1 file changed, 1 insertion(+)
Quoted 14 lines

include/linux/libnvdimm.h struct nvdimm_bus_descriptor { · line 75

Quoted 9 lines
include/linux/libnvdimm.h +1 -0
index 3f021dc..01117e1 100644
@@ -72,6 +72,7 @@ struct nvdimm_bus_descriptor {
 	unsigned long dsm_mask;
 	char *provider_name;
 	ndctl_fn ndctl;
+	ndctl_fn ndctl_passthru;

I don't think this is necessary. Vector off inside of __nd_ioctl. That especially makes sense if you do switch to passthrough as a command instead of a type, but it can work either way.

In an earlier version, I added a "type" argument to ndctl_fn and switched internally based upon that. I just came to the conclusion that I'd rather have two separate acpi_nfit_ctl functions than one trying to do both sets of argument marshaling. This is quite different both internally and to the caller.

So, I thought it would be less confusing to the next engineer, and that this was a good logical separation point.

Signature · 7 lines
-----------------------------------------------------------------------------
Jerry Hoemann            Software Engineer      Hewlett-Packard Enterprise

3404 E Harmony Rd. MS 36                        phone:  (970) 898-1022
Ft. Collins, CO 80528                           FAX:    (970) 898-0707
                                                email:  [email protected]
-----------------------------------------------------------------------------
#17 lore raw

Re: [PATCH 2/4] nvdimm: Add IOCTL pass thru

Quoted 25 lines · Jerry Hoemann
Quoted 9 lines · Jeff Moyer
Quoted 7 lines · Jerry Hoemann

Add internal data structure for ndctl_passthru call.

Signed-off-by Jerry Hoemann <[email protected]>
---
 include/linux/libnvdimm.h | 1 +
 1 file changed, 1 insertion(+)
Quoted 14 lines

include/linux/libnvdimm.h struct nvdimm_bus_descriptor { · line 75

Quoted 9 lines
include/linux/libnvdimm.h +1 -0
index 3f021dc..01117e1 100644
@@ -72,6 +72,7 @@ struct nvdimm_bus_descriptor {
 	unsigned long dsm_mask;
 	char *provider_name;
 	ndctl_fn ndctl;
+	ndctl_fn ndctl_passthru;

I don't think this is necessary. Vector off inside of __nd_ioctl. That especially makes sense if you do switch to passthrough as a command instead of a type, but it can work either way.

Quoted 8 lines

In an earlier version, I added a "type" argument to ndctl_fn and switched internally based upon that. I just came to the conclusion that I'd rather have two separate acpi_nfit_ctl functions than one trying to do both sets of argument marshaling. This is quite different both internally and to the caller.

So, I thought it would be less confusing to the next engineer, and that this was a good logical separation point.

I'll leave this up to Dan. To me, it doesn't make sense to add a new ioctl function for every new type of ioctl that get's added (assuming more types will follow).

Cheers,
Jeff

author
#18 lore raw

PATCH 3/4 nvdimm: Add IOCTL pass thru

In reply to Jerry Hoemann

Add functions acpi_nfit_ctl_passthru and __nd_ioctl_passthru which allow kernel to call a nvdimm's _DSM as a passthru without the marshaling code of the nd_cmd_desc.

Signed-off-by Jerry Hoemann <[email protected]>
---
 drivers/acpi/nfit.c  |  85 +++++++++++++++++++++++++++++++++++++++++
 drivers/nvdimm/bus.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 186 insertions(+), 4 deletions(-)
drivers/acpi/nfit.c +85 -0
index c1b8d03..a6b458a 100644
@@ -190,6 +190,90 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
 	return rc;
 }
 
+
+static int acpi_nfit_ctl_passthru(struct nvdimm_bus_descriptor *nd_desc,
+		struct nvdimm *nvdimm, unsigned int cmd, void *buf,
+		unsigned int buf_len)
+{
+	struct acpi_nfit_desc *acpi_desc = to_acpi_nfit_desc(nd_desc);
+	union acpi_object in_obj, in_buf, *out_obj;
+	struct device *dev = acpi_desc->dev;
+	const char *dimm_name;
+	acpi_handle handle;
+	const u8 *uuid;
+	int rc = 0;
+	int rev = 0;
+
+	struct ndn_pkg *pkg = buf;
+
+	if (nvdimm) {
+		struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
+		struct acpi_device *adev = nfit_mem->adev;
+
+		if (!adev)
+			return -ENOTTY;
+		dimm_name = nvdimm_name(nvdimm);
+		handle = adev->handle;
+	} else {
+		struct acpi_device *adev = to_acpi_dev(acpi_desc);
+
+		handle = adev->handle;
+		dimm_name = "bus";
+	}
+	uuid = pkg->h.dsm_uuid;
+	rev  = pkg->h.dsm_rev ? pkg->h.dsm_rev : 1;
+
+	in_obj.type = ACPI_TYPE_PACKAGE;
+	in_obj.package.count = 1;
+	in_obj.package.elements = &in_buf;
+	in_buf.type = ACPI_TYPE_BUFFER;
+	in_buf.buffer.pointer = (void *) &pkg->buf;
+
+	in_buf.buffer.length = pkg->h.dsm_in;
+
+	if (IS_ENABLED(CONFIG_ACPI_NFIT_DEBUG)) {
+		dev_dbg(dev, "%s:%s cmd: %d input length: %d\n", __func__,
+				dimm_name, cmd, in_buf.buffer.length);
+		print_hex_dump_debug("cmd: ", DUMP_PREFIX_OFFSET, 4,
+				4, in_buf.buffer.pointer, min_t(u32, 128,
+					in_buf.buffer.length), true);
+	}
+
+	out_obj = acpi_evaluate_dsm(handle, uuid, rev, cmd, &in_obj);
+	if (!out_obj) {
+		dev_dbg(dev, "%s:%s _DSM failed cmd: %d\n", __func__, dimm_name,
+				cmd);
+		return -EINVAL;
+	}
+
+	if (out_obj->package.type != ACPI_TYPE_BUFFER) {
+		dev_dbg(dev, "%s:%s unexpected output object type cmd: %d type: %d\n",
+				__func__, dimm_name, cmd, out_obj->type);
+		rc = -EINVAL;
+		goto out;
+	}
+
+	if (IS_ENABLED(CONFIG_ACPI_NFIT_DEBUG)) {
+		dev_dbg(dev, "%s:%s cmd: %d output length: %d\n", __func__,
+				dimm_name, cmd, out_obj->buffer.length);
+		print_hex_dump_debug("cmd: ", DUMP_PREFIX_OFFSET, 4,
+				4, out_obj->buffer.pointer, min_t(u32, 128,
+					out_obj->buffer.length), true);
+	}
+
+	pkg->h.dsm_size = out_obj->buffer.length;
+	memcpy(pkg->buf + pkg->h.dsm_in,
+			out_obj->buffer.pointer,
+			min(pkg->h.dsm_size, pkg->h.dsm_out));
+
+
+ out:
+	ACPI_FREE(out_obj);
+
+	return rc;
+}
+
+
 static const char *spa_type_name(u16 type)
 {
 	static const char *to_name[] = {

@@ -1614,6 +1698,7 @@ static int acpi_nfit_add(struct acpi_device *adev)
 	nd_desc = &acpi_desc->nd_desc;
 	nd_desc->provider_name = "ACPI.NFIT";
 	nd_desc->ndctl = acpi_nfit_ctl;
+	nd_desc->ndctl_passthru = acpi_nfit_ctl_passthru;
 	nd_desc->attr_groups = acpi_nfit_attribute_groups;
 
 	acpi_desc->nvdimm_bus = nvdimm_bus_register(dev, nd_desc);
drivers/nvdimm/bus.c +101 -4

136 lines not on this page · raw message

Signature · 1 line
1.7.11.3
#19 lore raw

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

drivers/nvdimm/bus.c static int match_dimm(struct device *dev, void *data) · line 725

Quoted 11 lines · Jerry Hoemann
drivers/nvdimm/bus.c +3 -2
@@ -633,10 +718,11 @@ static int match_dimm(struct device *dev, void *data)
 
 static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-	int rc = -ENXIO, read_only;
+	int rc = -ENXIO, ro;
 	struct nvdimm_bus *nvdimm_bus;
+	unsigned int type = _IOC_TYPE(cmd);
 
-	read_only = (O_RDWR != (file->f_flags & O_ACCMODE));
+	ro = (O_RDWR != (file->f_flags & O_ACCMODE));

I'm still reviewing the rest of this, but this is bugging me. The existing check for read_only looks pretty fishy to me. O_WRONLY is a thing (even though it's probably not a supportable mode for this ioctl). Why not just check for O_RDONLY?

Cheers,
Jeff

author
#20 lore raw

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

Quoted 11 lines · Jeff Moyer
Quoted 9 lines · Jerry Hoemann
drivers/nvdimm/bus.c +2 -1
@@ -633,10 +718,11 @@ static int match_dimm(struct device *dev, void *data)
 
 static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-	int rc = -ENXIO, read_only;
+	int rc = -ENXIO, ro;
 	struct nvdimm_bus *nvdimm_bus;
+	unsigned int type = _IOC_TYPE(cmd);
Quoted 7 lines

drivers/nvdimm/bus.c static int match_dimm(struct device *dev, void *data) · line 725

Quoted 2 lines
drivers/nvdimm/bus.c +1 -1
-	read_only = (O_RDWR != (file->f_flags & O_ACCMODE));
+	ro = (O_RDWR != (file->f_flags & O_ACCMODE));

I'm still reviewing the rest of this, but this is bugging me. The existing check for read_only looks pretty fishy to me. O_WRONLY is a thing (even though it's probably not a supportable mode for this ioctl). Why not just check for O_RDONLY?

Good question. I'll look into changing for version 2. I suspect you would like something more like:

	ro = ((file->f_flags & O_ACCMODE) == O_RDONLY);
Quoted 3 lines

Cheers,
Jeff

Signature · 7 lines
-----------------------------------------------------------------------------
Jerry Hoemann            Software Engineer      Hewlett-Packard Enterprise

3404 E Harmony Rd. MS 36                        phone:  (970) 898-1022
Ft. Collins, CO 80528                           FAX:    (970) 898-0707
                                                email:  [email protected]
-----------------------------------------------------------------------------
#21 lore raw

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

Quoted 21 lines · Jerry Hoemann
Quoted 11 lines · Jeff Moyer
Quoted 9 lines · Jerry Hoemann
drivers/nvdimm/bus.c +2 -1
@@ -633,10 +718,11 @@ static int match_dimm(struct device *dev, void *data)
 
 static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-	int rc = -ENXIO, read_only;
+	int rc = -ENXIO, ro;
 	struct nvdimm_bus *nvdimm_bus;
+	unsigned int type = _IOC_TYPE(cmd);
Quoted 7 lines

drivers/nvdimm/bus.c static int match_dimm(struct device *dev, void *data) · line 725

Quoted 2 lines
drivers/nvdimm/bus.c +1 -1
-	read_only = (O_RDWR != (file->f_flags & O_ACCMODE));
+	ro = (O_RDWR != (file->f_flags & O_ACCMODE));

I'm still reviewing the rest of this, but this is bugging me. The existing check for read_only looks pretty fishy to me. O_WRONLY is a thing (even though it's probably not a supportable mode for this ioctl). Why not just check for O_RDONLY?

Quoted 4 lines

Good question. I'll look into changing for version 2. I suspect you would like something more like:

	ro = ((file->f_flags & O_ACCMODE) == O_RDONLY);

Yeah. I'd make that a separate patch, and put it first in the series since it's a cleanup than can be applied to older kernels if necessary.

Thanks,
Jeff

author
#22 lore raw

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

Quoted 23 lines · Jeff Moyer
Quoted 21 lines · Jerry Hoemann
Quoted 18 lines · Jeff Moyer

drivers/nvdimm/bus.c static int match_dimm(struct device *dev, void *data) · line 725

Quoted 11 lines · Jerry Hoemann
drivers/nvdimm/bus.c +3 -2
@@ -633,10 +718,11 @@ static int match_dimm(struct device *dev, void *data)
 
 static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-	int rc = -ENXIO, read_only;
+	int rc = -ENXIO, ro;
 	struct nvdimm_bus *nvdimm_bus;
+	unsigned int type = _IOC_TYPE(cmd);
 
-	read_only = (O_RDWR != (file->f_flags & O_ACCMODE));
+	ro = (O_RDWR != (file->f_flags & O_ACCMODE));

I'm still reviewing the rest of this, but this is bugging me. The existing check for read_only looks pretty fishy to me. O_WRONLY is a thing (even though it's probably not a supportable mode for this ioctl). Why not just check for O_RDONLY?

Quoted 8 lines
Quoted 4 lines

Good question. I'll look into changing for version 2. I suspect you would like something more like:

	ro = ((file->f_flags & O_ACCMODE) == O_RDONLY);

Yeah. I'd make that a separate patch, and put it first in the series since it's a cleanup than can be applied to older kernels if necessary.

Will do.

Signature · 7 lines
-----------------------------------------------------------------------------
Jerry Hoemann            Software Engineer      Hewlett-Packard Enterprise

3404 E Harmony Rd. MS 36                        phone:  (970) 898-1022
Ft. Collins, CO 80528                           FAX:    (970) 898-0707
                                                email:  [email protected]
-----------------------------------------------------------------------------
#23 lore raw

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

In reply to Jerry Hoemann

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

Quoted 2 lines · Jerry Hoemann
drivers/acpi/nfit.c +2 -0
+	uuid = pkg->h.dsm_uuid;
+	rev  = pkg->h.dsm_rev ? pkg->h.dsm_rev : 1;

Shouldn't revision id be required?

Cheers,
Jeff

author
#24 lore raw

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

Quoted 7 lines · Jeff Moyer

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

Quoted 2 lines · Jerry Hoemann
drivers/acpi/nfit.c +2 -0
+	uuid = pkg->h.dsm_uuid;
+	rev  = pkg->h.dsm_rev ? pkg->h.dsm_rev : 1;

Shouldn't revision id be required?

Yes it should be. Good catch.

I was testing use of reclaiming previously unused members of the structure. I should have removed this line.

And this reminds me that I should also add a test that the reserved fields of ndn_pkg are actually zero.

I'll address both in version 2.

thanks

Signature · 3 lines
-----------------------------------------------------------------------------
Jerry Hoemann            Software Engineer      Hewlett-Packard Enterprise
-----------------------------------------------------------------------------
author
#25 lore raw

PATCH 4/4 nvdimm: rename functions that aren't IOCTL passthru

In reply to Jerry Hoemann

rename functions acpi_nfit_ctl, __nd_ioctl, ndctl, to *_intel to denote that the functions implement the Intel example _DSM.

Signed-off-by Jerry Hoemann <[email protected]>
---
 drivers/acpi/nfit.c        |  6 +++---
 drivers/nvdimm/bus.c       | 15 ++++++++-------
 drivers/nvdimm/dimm_devs.c |  6 +++---
 include/linux/libnvdimm.h  |  2 +-
 4 files changed, 15 insertions(+), 14 deletions(-)
drivers/acpi/nfit.c +3 -3
index a6b458a..f85200d 100644
@@ -62,7 +62,7 @@ static struct acpi_device *to_acpi_dev(struct acpi_nfit_desc *acpi_desc)
 	return to_acpi_device(acpi_desc->dev);
 }
 
-static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,
+static int acpi_nfit_ctl_intel(struct nvdimm_bus_descriptor *nd_desc,
 		struct nvdimm *nvdimm, unsigned int cmd, void *buf,
 		unsigned int buf_len)
 {

@@ -1352,7 +1352,7 @@ static int acpi_nfit_blk_get_flags(struct nvdimm_bus_descriptor *nd_desc,
 	int rc;
 
 	memset(&flags, 0, sizeof(flags));
-	rc = nd_desc->ndctl(nd_desc, nvdimm, ND_CMD_DIMM_FLAGS, &flags,
+	rc = nd_desc->ndctl_intel(nd_desc, nvdimm, ND_CMD_DIMM_FLAGS, &flags,
 			sizeof(flags));
 
 	if (rc >= 0 && flags.status == 0)

@@ -1697,7 +1697,7 @@ static int acpi_nfit_add(struct acpi_device *adev)
 	acpi_desc->blk_do_io = acpi_nfit_blk_region_do_io;
 	nd_desc = &acpi_desc->nd_desc;
 	nd_desc->provider_name = "ACPI.NFIT";
-	nd_desc->ndctl = acpi_nfit_ctl;
+	nd_desc->ndctl_intel = acpi_nfit_ctl_intel;
 	nd_desc->ndctl_passthru = acpi_nfit_ctl_passthru;
 	nd_desc->attr_groups = acpi_nfit_attribute_groups;
drivers/nvdimm/bus.c +8 -7
index cfb10eb..4587d30 100644
@@ -479,8 +479,9 @@ static int nd_cmd_clear_to_send(struct nvdimm *nvdimm, unsigned int cmd)
 	return 0;
 }
 
-static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
-		int read_only, unsigned int ioctl_cmd, unsigned long arg)
+static int __nd_ioctl_intel(struct nvdimm_bus *nvdimm_bus,
+		struct nvdimm *nvdimm, int read_only, unsigned int ioctl_cmd,
+		unsigned long arg)
 {
 	struct nvdimm_bus_descriptor *nd_desc = nvdimm_bus->nd_desc;
 	size_t buf_len = 0, in_len = 0, out_len = 0;

@@ -587,7 +588,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 	if (rc)
 		goto out_unlock;
 
-	rc = nd_desc->ndctl(nd_desc, nvdimm, cmd, buf, buf_len);
+	rc = nd_desc->ndctl_intel(nd_desc, nvdimm, cmd, buf, buf_len);
 	if (rc < 0)
 		goto out_unlock;
 	if (copy_to_user(p, buf, buf_len))

@@ -677,11 +678,11 @@ static int __nd_ioctl_passthru(struct nvdimm_bus *nvdimm_bus,
 static long nd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	long id = (long) file->private_data;
-	int rc = -ENXIO, read_only;
+	int rc = -ENXIO, ro;
 	struct nvdimm_bus *nvdimm_bus;
 	unsigned int type = _IOC_TYPE(cmd);
 
-	read_only = (O_RDWR != (file->f_flags & O_ACCMODE));
+	ro = (O_RDWR != (file->f_flags & O_ACCMODE));
 	mutex_lock(&nvdimm_bus_list_mutex);
 	list_for_each_entry(nvdimm_bus, &nvdimm_bus_list, list) {
 		if (nvdimm_bus->id != id)

@@ -689,7 +690,7 @@ static long nd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 		switch (type) {
 		case NVDIMM_TYPE_INTEL:
-			rc = __nd_ioctl(nvdimm_bus, NULL, read_only, cmd, arg);
+			rc = __nd_ioctl_intel(nvdimm_bus, NULL, ro, cmd, arg);
 			break;
 		case NVDIMM_TYPE_PASSTHRU:
 			rc = __nd_ioctl_passthru(nvdimm_bus, NULL, 0, cmd, arg);

@@ -736,7 +737,7 @@ static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 		switch (type) {
 		case NVDIMM_TYPE_INTEL:
-			rc = __nd_ioctl(nvdimm_bus, nvdimm, ro, cmd, arg);
+			rc = __nd_ioctl_intel(nvdimm_bus, nvdimm, ro, cmd, arg);
 			break;
 		case NVDIMM_TYPE_PASSTHRU:
 			rc = __nd_ioctl_passthru(nvdimm_bus, nvdimm, ro, cmd, arg);
drivers/nvdimm/dimm_devs.c +3 -3
index 651b8d1..9c0cc23 100644
@@ -74,7 +74,7 @@ int nvdimm_init_nsarea(struct nvdimm_drvdata *ndd)
 
 	memset(cmd, 0, sizeof(*cmd));
 	nd_desc = nvdimm_bus->nd_desc;
-	return nd_desc->ndctl(nd_desc, to_nvdimm(ndd->dev),
+	return nd_desc->ndctl_intel(nd_desc, to_nvdimm(ndd->dev),
 			ND_CMD_GET_CONFIG_SIZE, cmd, sizeof(*cmd));
 }
 

@@ -118,7 +118,7 @@ int nvdimm_init_config_data(struct nvdimm_drvdata *ndd)
 			offset += cmd->in_length) {
 		cmd->in_length = min(config_size, max_cmd_size);
 		cmd->in_offset = offset;
-		rc = nd_desc->ndctl(nd_desc, to_nvdimm(ndd->dev),
+		rc = nd_desc->ndctl_intel(nd_desc, to_nvdimm(ndd->dev),
 				ND_CMD_GET_CONFIG_DATA, cmd,
 				cmd->in_length + sizeof(*cmd));
 		if (rc || cmd->status) {

@@ -170,7 +170,7 @@ int nvdimm_set_config_data(struct nvdimm_drvdata *ndd, size_t offset,
 		cmd_size = sizeof(*cmd) + cmd->in_length + sizeof(u32);
 		status = ((void *) cmd) + cmd_size - sizeof(u32);
 
-		rc = nd_desc->ndctl(nd_desc, to_nvdimm(ndd->dev),
+		rc = nd_desc->ndctl_intel(nd_desc, to_nvdimm(ndd->dev),
 				ND_CMD_SET_CONFIG_DATA, cmd, cmd_size);
 		if (rc || *status) {
 			rc = rc ? rc : -ENXIO;
include/linux/libnvdimm.h +1 -1
index 01117e1..594a772 100644
@@ -71,7 +71,7 @@ struct nvdimm_bus_descriptor {
 	const struct attribute_group **attr_groups;
 	unsigned long dsm_mask;
 	char *provider_name;
-	ndctl_fn ndctl;
+	ndctl_fn ndctl_intel;
 	ndctl_fn ndctl_passthru;
 };
Signature · 1 line
1.7.11.3
#26 lore raw
In reply to Jerry Hoemann
Quoted 8 lines · 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.

Quoted 8 lines

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

A new IOCTL type "P" is added for the pass thru call.

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

What does 'ndn' stand for? If it stands for NVDIMM-N, then I think that's too narrow a scope. Anyway, it helps readability if you call out what abbreviations mean, especially when it's non-obvious.

Quoted 4 lines

Jerry Hoemann (4):
nvdimm: Add wrapper for IOCTL pass thru.
nvdimm: Add IOCTL pass thru
nvdimm: Add IOCTL pass thru

You should really give each patch a different subject.

Cheers,
Jeff

author
#27 lore raw
Quoted 13 lines · Jeff Moyer
Quoted 11 lines · Jerry Hoemann

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

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

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

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

Quoted 7 lines
Quoted 5 lines

A new IOCTL type "P" is added for the pass thru call.

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

What does 'ndn' stand for? If it stands for NVDIMM-N, then I think

Yes, hold over from earlier, less generic version.

Quoted 2 lines

that's too narrow a scope. Anyway, it helps readability if you call out what abbreviations mean, especially when it's non-obvious.

Will fix in version 2.

Quoted 7 lines
Quoted 5 lines

Jerry Hoemann (4):
nvdimm: Add wrapper for IOCTL pass thru.
nvdimm: Add IOCTL pass thru
nvdimm: Add IOCTL pass thru

You should really give each patch a different subject.

Will do.

Quoted 3 lines

Cheers,
Jeff

Signature · 7 lines
-----------------------------------------------------------------------------
Jerry Hoemann            Software Engineer      Hewlett-Packard Enterprise

3404 E Harmony Rd. MS 36                        phone:  (970) 898-1022
Ft. Collins, CO 80528                           FAX:    (970) 898-0707
                                                email:  [email protected]
-----------------------------------------------------------------------------