Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1264616 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] nvdimm: Add an IOCTL pass thru for DSM calls Jerry Hoemann <jerry.hoemann@hpe.com> - 2015-11-06 23:30 +0100
    [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru. Jerry Hoemann <jerry.hoemann@hpe.com> - 2015-11-06 23:30 +0100
      Re: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru. Jeff Moyer <jmoyer@redhat.com> - 2015-11-10 19:00 +0100
        Re: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru. Dan Williams <dan.j.williams@intel.com> - 2015-11-10 19:10 +0100
        Re: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru. Jerry Hoemann <jerry.hoemann@hpe.com> - 2015-11-10 20:50 +0100
          Re: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru. Jeff Moyer <jmoyer@redhat.com> - 2015-11-10 21:30 +0100
            Re: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru. Jerry Hoemann <jerry.hoemann@hpe.com> - 2015-11-11 01:50 +0100
              Re: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru. Dan Williams <dan.j.williams@intel.com> - 2015-11-11 01:50 +0100
              Re: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru. Jeff Moyer <jmoyer@redhat.com> - 2015-11-11 16:50 +0100
          Re: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru. Dan Williams <dan.j.williams@intel.com> - 2015-11-10 21:30 +0100
            Re: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru. Linda Knippers <linda.knippers@hpe.com> - 2015-11-10 22:00 +0100
              Re: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru. Dan Williams <dan.j.williams@intel.com> - 2015-11-10 23:30 +0100
      RE: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru. "Elliott, Robert (Persistent Memory)" <elliott@hpe.com> - 2015-11-10 20:10 +0100
        Re: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru. Jerry Hoemann <jerry.hoemann@hpe.com> - 2015-11-10 22:40 +0100
    Re: [PATCH 0/4] nvdimm: Add an IOCTL pass thru for DSM calls Jeff Moyer <jmoyer@redhat.com> - 2015-11-10 16:40 +0100
      Re: [PATCH 0/4] nvdimm: Add an IOCTL pass thru for DSM calls Jerry Hoemann <jerry.hoemann@hpe.com> - 2015-11-10 22:40 +0100

#1264616 — [PATCH 0/4] 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(-)

-- 
1.7.11.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1264617 — [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 <jerry.hoemann@hpe.com>
---
 include/uapi/linux/ndctl.h | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index 5b4a4be..1c81a99 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -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__ */
-- 
1.7.11.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

Jerry Hoemann <jerry.hoemann@hpe.com> writes:

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

> +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).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On Tue, Nov 10, 2015 at 9:51 AM, Jeff Moyer <jmoyer@redhat.com> wrote:
> Jerry Hoemann <jerry.hoemann@hpe.com> writes:
>
>> 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?

+1 for making this just a new command number without a new top-level
number space.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On Tue, Nov 10, 2015 at 12:51:59PM -0500, Jeff Moyer wrote:
> Jerry Hoemann <jerry.hoemann@hpe.com> writes:
> 
> > 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.

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


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

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.

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

-- 

-----------------------------------------------------------------------------
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:  jerry.hoemann@hpe.com
-----------------------------------------------------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

Jerry Hoemann <jerry.hoemann@hpe.com> writes:

> On Tue, Nov 10, 2015 at 12:51:59PM -0500, Jeff Moyer wrote:
>> Jerry Hoemann <jerry.hoemann@hpe.com> writes:
>> 
>> > 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.

OK, I see your point.

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

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

>> #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?
>> 
>> > +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 *.
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On Tue, Nov 10, 2015 at 03:26:38PM -0500, Jeff Moyer wrote:
> >
> > 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.



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


...

> >> > +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 *.
> >
> > 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];
> 

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.

-- 

-----------------------------------------------------------------------------
Jerry Hoemann            Software Engineer      Hewlett-Packard Enterprise
-----------------------------------------------------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On Tue, Nov 10, 2015 at 4:44 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Tue, Nov 10, 2015 at 03:26:38PM -0500, Jeff Moyer wrote:
[..]
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

Jerry Hoemann <jerry.hoemann@hpe.com> writes:

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On Tue, Nov 10, 2015 at 11:49 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Tue, Nov 10, 2015 at 12:51:59PM -0500, Jeff Moyer wrote:
>> Jerry Hoemann <jerry.hoemann@hpe.com> writes:
>>
>> > 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.

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On 11/10/2015 3:27 PM, Dan Williams wrote:
> On Tue, Nov 10, 2015 at 11:49 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> On Tue, Nov 10, 2015 at 12:51:59PM -0500, Jeff Moyer wrote:
>>> Jerry Hoemann <jerry.hoemann@hpe.com> writes:
>>>
>>>> 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.
>
> 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.

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On Tue, Nov 10, 2015 at 12:53 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
> On 11/10/2015 3:27 PM, Dan Williams wrote:
>>
>> On Tue, Nov 10, 2015 at 11:49 AM, Jerry Hoemann <jerry.hoemann@hpe.com>
>> wrote:
>>>
>>> On Tue, Nov 10, 2015 at 12:51:59PM -0500, Jeff Moyer wrote:
>>>>
>>>> Jerry Hoemann <jerry.hoemann@hpe.com> writes:
>>>>
>>>>> 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.
>>
>>
>> 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.

> 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.
>
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

> -----Original Message-----
> From: Linux-nvdimm [mailto:linux-nvdimm-bounces@lists.01.org] On Behalf Of
> Hoemann, Jerry
> Sent: Friday, November 6, 2015 4:27 PM
> Subject: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru.
...
> diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
...
> +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



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On Tue, Nov 10, 2015 at 12:04:22PM -0700, Elliott, Robert (Persistent Memory) wrote:
> > -----Original Message-----
> > From: Linux-nvdimm [mailto:linux-nvdimm-bounces@lists.01.org] On Behalf Of
> > Hoemann, Jerry
> > Sent: Friday, November 6, 2015 4:27 PM
> > Subject: [PATCH 1/4] nvdimm: Add wrapper for IOCTL pass thru.
> ...
> > diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
> ...
> > +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?
> 

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

  Jerry
-- 

-----------------------------------------------------------------------------
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:  jerry.hoemann@hpe.com
-----------------------------------------------------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1266583

Jerry Hoemann <jerry.hoemann@hpe.com> writes:

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

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.

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1266797

On Tue, Nov 10, 2015 at 10:33:29AM -0500, Jeff Moyer wrote:
> Jerry Hoemann <jerry.hoemann@hpe.com> writes:
> 
> > 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.
> 
> What does 'ndn' stand for?  If it stands for NVDIMM-N, then I think

  Yes, hold over from earlier, less generic version.

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



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


> 
> Cheers,
> Jeff

-- 

-----------------------------------------------------------------------------
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:  jerry.hoemann@hpe.com
-----------------------------------------------------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web