| Message ID | 833a627a8cff27a72f946fa9432cacf2a855e485.1457730736.git.jerry.hoemann@hpe.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Mar 11, 2016 at 1:16 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > Add struct nd_cmd_pkg which serves as a warapper for > the data being passed via a pass thru to a NVDIMM DSM. > This wrapper specifies the extra information in a uniform > manner allowing the kenrel to call a DSM without knowing > specifics of the DSM. > > Add dsm_call command to nvdimm_bus_cmd_name and nvdimm_cmd_name. > > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> > --- > include/uapi/linux/ndctl.h | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h > index 7cc28ab..459c6ba 100644 > --- a/include/uapi/linux/ndctl.h > +++ b/include/uapi/linux/ndctl.h > @@ -125,6 +125,7 @@ enum { > ND_CMD_VENDOR_EFFECT_LOG_SIZE = 7, > ND_CMD_VENDOR_EFFECT_LOG = 8, > ND_CMD_VENDOR = 9, > + ND_CMD_CALL_DSM = 10, > }; > > enum { > @@ -139,6 +140,7 @@ static inline const char *nvdimm_bus_cmd_name(unsigned cmd) > [ND_CMD_ARS_START] = "ars_start", > [ND_CMD_ARS_STATUS] = "ars_status", > [ND_CMD_CLEAR_ERROR] = "clear_error", > + [ND_CMD_CALL_DSM] = "dsm_call", > }; > > if (cmd < ARRAY_SIZE(names) && names[cmd]) > @@ -158,6 +160,7 @@ static inline const char *nvdimm_cmd_name(unsigned cmd) > [ND_CMD_VENDOR_EFFECT_LOG_SIZE] = "effect_size", > [ND_CMD_VENDOR_EFFECT_LOG] = "effect_log", > [ND_CMD_VENDOR] = "vendor", > + [ND_CMD_CALL_DSM] = "dsm_call", > }; > > if (cmd < ARRAY_SIZE(names) && names[cmd]) > @@ -224,4 +227,23 @@ enum ars_masks { > ARS_STATUS_MASK = 0x0000FFFF, > ARS_EXT_STATUS_SHIFT = 16, > }; > + > + > +struct nd_cmd_pkg { > + __u32 ncp_type; I think 'family' is more clear, to signify the class of commands that the 'command' parameter references. Also let's drop the ncp_ prefix, it's non-idiomatic with the rest of the definitions in this file. > + __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. > + __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 */" > + __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? > + unsigned char ncp_payload[]; /* Contents of call */ > +}; > + > +#define NCP_TYPE_BUS 1 > +#define NCP_TYPE_DIMM_INTEL1 2 > +#define NCP_TYPE_DIMM_N_HPE1 3 > +#define NCP_TYPE_DIMM_N_HPE2 4 s/NCP_TYPE_/ND_FAMILY_/ I think it would be helpful to put comments in the code here about where these families are defined.
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@hpe.com> 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?) 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. > > > + __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. > > > + __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. This allows for idempotent calls to be redone with appropriately sized buffer. This interface needs a man page. :) I will put a larger comment block in the next version. > > > + unsigned char ncp_payload[]; /* Contents of call */ > > +}; > > + > > +#define NCP_TYPE_BUS 1 > > +#define NCP_TYPE_DIMM_INTEL1 2 > > +#define NCP_TYPE_DIMM_N_HPE1 3 > > +#define NCP_TYPE_DIMM_N_HPE2 4 > > s/NCP_TYPE_/ND_FAMILY_/ Will do. > > I think it would be helpful to put comments in the code here about > where these families are defined. Will do.
On Fri, Mar 11, 2016 at 3:29 PM, Jerry Hoemann <jerry.hoemann@hpe.com> 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@hpe.com> 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.
On Fri, Mar 11, 2016 at 4:29 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: >> 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?) > > 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. Isn't this leaning towards the "Hungarian notation", and isn't that discouraged in the kernel coding style.. From Documentation/CodingStyle Encoding the type of a function into the name (so-called Hungarian notation) is brain damaged - the compiler knows the types anyway and can check those, and it only confuses the programmer.
On Sat, Mar 12, 2016 at 7:49 AM, Vishal Verma <vishal@kernel.org> wrote: > On Fri, Mar 11, 2016 at 4:29 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: >>> 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?) >> >> 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. > > Isn't this leaning towards the "Hungarian notation", and isn't that > discouraged in the kernel coding style.. > > From Documentation/CodingStyle > > Encoding the type of a function into the name (so-called Hungarian > notation) is brain damaged - the compiler knows the types anyway and can > check those, and it only confuses the programmer. I wouldn't go that far... look at "struct bio", "struct file", or "struct nd_region" that have a common prefix for (some) member fields. That's not Hungarian notation, that's making the code easier to grep.
On Fri, Mar 11, 2016 at 03:48:55PM -0800, Dan Williams wrote: > On Fri, Mar 11, 2016 at 3:29 PM, Jerry Hoemann <jerry.hoemann@hpe.com> 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@hpe.com> 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. Will do. > > >> > >> > + __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. Not wrong, just a different calling paradigm. In addition to allowing user applications to query/size buffers for the call, I have also found the information on how much data firmware wants to return a useful diagnostic in diagnosing firmware problems. Also note, The upper limit on the amount of data the interface returns is still in place. I am not allowing for arbitrarily large returns. > > > 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. I don't know what the final spec will look like. It might be an issue, it might not. I would just prefer to have a flexible kernel interface in place that will handle such situations so we don't have to revisit it in the future.
On Sun, Mar 13, 2016 at 12:15 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > On Fri, Mar 11, 2016 at 03:48:55PM -0800, Dan Williams wrote: >> On Fri, Mar 11, 2016 at 3:29 PM, Jerry Hoemann <jerry.hoemann@hpe.com> 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@hpe.com> 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. > > Will do. > >> >> >> >> >> > + __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. > > Not wrong, just a different calling paradigm. > > In addition to allowing user applications to query/size buffers > for the call, I have also found the information on how much data > firmware wants to return a useful diagnostic in diagnosing > firmware problems. > > Also note, The upper limit on the amount of data the interface returns is > still in place. I am not allowing for arbitrarily large returns. > > > >> >> > 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. > > I don't know what the final spec will look like. It might be > an issue, it might not. I would just prefer to have a flexible > kernel interface in place that will handle such situations so > we don't have to revisit it in the future. In Linux kernel development it has been my experience that we don't code for potential future eventualities. We code the most pragmatic minimal interface and evolve it over time if need be. The expectation is that occasional revisiting is less overhead than maintaining unused functionality indefinitely. Take this call_dsm interface as an example. We've been able to boil down the interface to something manageable rather than the free-for-all that was originally proposed.
On Sat, 2016-03-12 at 09:50 -0800, Dan Williams wrote: > On Sat, Mar 12, 2016 at 7:49 AM, Vishal Verma <vishal@kernel.org> > wrote: > > > > On Fri, Mar 11, 2016 at 4:29 PM, Jerry Hoemann <jerry.hoemann@hpe.c > > om> wrote: > > > > > > > > > > > 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?) > > > > > > 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. > > Isn't this leaning towards the "Hungarian notation", and isn't that > > discouraged in the kernel coding style.. > > > > From Documentation/CodingStyle > > > > Encoding the type of a function into the name (so-called Hungarian > > notation) is brain damaged - the compiler knows the types anyway > > and can > > check those, and it only confuses the programmer. > I wouldn't go that far... look at "struct bio", "struct file", or > "struct nd_region" that have a common prefix for (some) member > fields. > That's not Hungarian notation, that's making the code easier to grep. Ah yes - agreed. > _______________________________________________ > Linux-nvdimm mailing list > Linux-nvdimm@lists.01.org > https://lists.01.org/mailman/listinfo/linux-nvdimm
diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h index 7cc28ab..459c6ba 100644 --- a/include/uapi/linux/ndctl.h +++ b/include/uapi/linux/ndctl.h @@ -125,6 +125,7 @@ enum { ND_CMD_VENDOR_EFFECT_LOG_SIZE = 7, ND_CMD_VENDOR_EFFECT_LOG = 8, ND_CMD_VENDOR = 9, + ND_CMD_CALL_DSM = 10, }; enum { @@ -139,6 +140,7 @@ static inline const char *nvdimm_bus_cmd_name(unsigned cmd) [ND_CMD_ARS_START] = "ars_start", [ND_CMD_ARS_STATUS] = "ars_status", [ND_CMD_CLEAR_ERROR] = "clear_error", + [ND_CMD_CALL_DSM] = "dsm_call", }; if (cmd < ARRAY_SIZE(names) && names[cmd]) @@ -158,6 +160,7 @@ static inline const char *nvdimm_cmd_name(unsigned cmd) [ND_CMD_VENDOR_EFFECT_LOG_SIZE] = "effect_size", [ND_CMD_VENDOR_EFFECT_LOG] = "effect_log", [ND_CMD_VENDOR] = "vendor", + [ND_CMD_CALL_DSM] = "dsm_call", }; if (cmd < ARRAY_SIZE(names) && names[cmd]) @@ -224,4 +227,23 @@ enum ars_masks { ARS_STATUS_MASK = 0x0000FFFF, ARS_EXT_STATUS_SHIFT = 16, }; + + +struct nd_cmd_pkg { + __u32 ncp_type; + __u32 ncp_rev; + __u64 ncp_command; + __u32 ncp_size_in; /* size of input payload */ + __u32 ncp_size_out; /* size of user buffer */ + __u32 ncp_reserved2[9]; /* reserved must be zero */ + __u32 ncp_pot_size; /* potential output size */ + unsigned char ncp_payload[]; /* Contents of call */ +}; + +#define NCP_TYPE_BUS 1 +#define NCP_TYPE_DIMM_INTEL1 2 +#define NCP_TYPE_DIMM_N_HPE1 3 +#define NCP_TYPE_DIMM_N_HPE2 4 + + #endif /* __NDCTL_H__ */
Add struct nd_cmd_pkg which serves as a warapper for the data being passed via a pass thru to a NVDIMM DSM. This wrapper specifies the extra information in a uniform manner allowing the kenrel to call a DSM without knowing specifics of the DSM. Add dsm_call command to nvdimm_bus_cmd_name and nvdimm_cmd_name. Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> --- include/uapi/linux/ndctl.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)