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

[Jeff Moyer <jmoyer@redhat.com>]

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