Jerry Hoemann <jerry.hoemann@xxxxxxx> writes:
> On Tue, Nov 10, 2015 at 12:51:59PM -0500, Jeff Moyer wrote:
>> Jerry Hoemann <jerry.hoemann@xxxxxxx> 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-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
