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

On Fri, Mar 11, 2016 at 3:29 PM, Jerry Hoemann <jerry.hoemann(a)hpe.com&gt; 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(a)hpe.com&gt; 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.