ACPI / util: Fix acpi_evaluate_dsm() argument type
A message that is not in the archive (sent off-list or not yet received): [email protected]
RFC v6 1/8 ACPI / util: Fix acpi_evaluate_dsm() argument type
The ACPI spec speicifies that arguments "Revision ID" and "Function Index" to a _DSM are type "Integer." Type Integers are 64 bit quantities.
The function evaluate_dsm specifies these types as simple "int" which are 32 bits. Correct type passed to acpi_evaluate_dsm and its callers and derived callers to pass correct type.
acpi_check_dsm and acpi_evaluate_dsm_typed had similar issue and were corrected as well.
--- drivers/acpi/utils.c | 4 ++-- include/acpi/acpi_bus.h | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-)
drivers/acpi/utils.c +2 -2
@@ -629,7 +629,7 @@ acpi_status acpi_evaluate_lck(acpi_handle handle, int lock)* some old BIOSes do expect a buffer or an integer etc. */ union acpi_object * -acpi_evaluate_dsm (acpi_handle handle,const u8 *uuid,int rev,int func, +acpi_evaluate_dsm (acpi_handle handle,const u8 *uuid,u64 rev,u64 func,union acpi_object *argv4) {acpi_status ret;
@@ -678,7 +678,7 @@ EXPORT_SYMBOL(acpi_evaluate_dsm);* functions. Currently only support 64 functions at maximum, should be * enough for now. */ -bool acpi_check_dsm (acpi_handle handle,const u8 *uuid,int rev,u64 funcs) +bool acpi_check_dsm (acpi_handle handle,const u8 *uuid,u64 rev,u64 funcs) {int i;u64 mask =0 ;
include/acpi/acpi_bus.h +3 -3
@@ -61,12 +61,12 @@ bool acpi_ata_match(acpi_handle handle);bool acpi_bay_match (acpi_handle handle);bool acpi_dock_match (acpi_handle handle); -bool acpi_check_dsm (acpi_handle handle,const u8 *uuid,int rev,u64 funcs); +bool acpi_check_dsm (acpi_handle handle,const u8 *uuid,u64 rev,u64 funcs);union acpi_object *acpi_evaluate_dsm (acpi_handle handle,const u8 *uuid, -int rev,int func,union acpi_object *argv4); +u64 rev,u64 func,union acpi_object *argv4);static inline union acpi_object * -acpi_evaluate_dsm_typed (acpi_handle handle,const u8 *uuid,int rev,int func, +acpi_evaluate_dsm_typed (acpi_handle handle,const u8 *uuid,u64 rev,u64 func,union acpi_object *argv4,acpi_object_type type) {union acpi_object *obj;
Signature · 6 lines
1.7.11.3 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
RFC v6 2/8 nvdimm: Add wrapper for IOCTL pass thru
In reply to a message not in the archiveAdd 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.
--- include/uapi/linux/ndctl.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
include/uapi/linux/ndctl.h +22 -0
@@ -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__ */
Signature · 6 lines
1.7.11.3 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [RFC v6 2/8] nvdimm: Add wrapper for IOCTL pass thru
Quoted 42 lines · Jerry Hoemann
Add struct
nd_cmd_pkgwhich 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_callcommand tonvdimm_bus_cmd_nameandnvdimm_cmd_name.Signed-off-by Jerry Hoemann <[email protected]>--- include/uapi/linux/ndctl.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)include/uapi/linux/ndctl.h +3
-0@@ -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])
include/uapi/linux/ndctl.h enum ars_masks { · line 233
Quoted 7 lines
include/uapi/linux/ndctl.h +4
-0@@ -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.
include/uapi/linux/ndctl.h enum ars_masks { · line 234
Quoted 1 line
include/uapi/linux/ndctl.h +1
-0+__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.
include/uapi/linux/ndctl.h enum ars_masks { · line 237
Quoted 3 lines
include/uapi/linux/ndctl.h +3
-0+__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 */"
include/uapi/linux/ndctl.h enum ars_masks { · line 239
Quoted 2 lines
include/uapi/linux/ndctl.h +2
-0+__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?
include/uapi/linux/ndctl.h enum ars_masks { · line 246
Quoted 7 lines
include/uapi/linux/ndctl.h +7
-0+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.
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [RFC v6 2/8] nvdimm: Add wrapper for IOCTL pass thru
Quoted 8 lines · Dan Williams
include/uapi/linux/ndctl.h enum ars_masks { · line 233
Quoted 4 lines · Jerry Hoemann
include/uapi/linux/ndctl.h +4
-0+ + +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.
Quoted 2 lines
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.
Quoted 7 lines
Quoted 2 lines
+
__u32ncp_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.
Quoted 6 lines
include/uapi/linux/ndctl.h enum ars_masks { · line 237
Quoted 4 lines
include/uapi/linux/ndctl.h +3
-0+__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.
Quoted 6 lines
include/uapi/linux/ndctl.h enum ars_masks { · line 239
Quoted 3 lines
include/uapi/linux/ndctl.h +2
-0+__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.
Quoted 10 lines
include/uapi/linux/ndctl.h enum ars_masks { · line 246
Quoted 8 lines
include/uapi/linux/ndctl.h +7
-0+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 4s/NCP_TYPE_/ND_FAMILY_/
Will do.
Quoted 3 lines
I think it would be helpful to put comments in the code here about where these families are defined.
Will do.
Signature · 7 lines
----------------------------------------------------------------------------- Jerry Hoemann Software Engineer Hewlett Packard Enterprise ----------------------------------------------------------------------------- _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [RFC v6 2/8] nvdimm: Add wrapper for IOCTL pass thru
Quoted 18 lines · Jerry Hoemann
Quoted 8 lines · Dan Williams
include/uapi/linux/ndctl.h enum ars_masks { · line 233
Quoted 4 lines · Jerry Hoemann
include/uapi/linux/ndctl.h +4
-0+ + +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.
Quoted 7 lines
Quoted 2 lines
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.
Quoted 2 lines
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.
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
+
__u32ncp_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.
Quoted 6 lines
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.
Quoted 32 lines
Quoted 5 lines
include/uapi/linux/ndctl.h enum ars_masks { · line 237
Quoted 3 lines
include/uapi/linux/ndctl.h +3
-0+__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.
Quoted 6 lines
include/uapi/linux/ndctl.h enum ars_masks { · line 239
Quoted 3 lines
include/uapi/linux/ndctl.h +2
-0+__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.
Quoted 5 lines
So, the interface returns the amount of data that will fit in the space provided and tell the user app in
ncp_pot_sizethe 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.
Quoted 2 lines
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.
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [RFC v6 2/8] nvdimm: Add wrapper for IOCTL pass thru
Quoted 27 lines · Dan Williams
Quoted 18 lines · Jerry Hoemann
Quoted 8 lines · Dan Williams
include/uapi/linux/ndctl.h enum ars_masks { · line 233
Quoted 4 lines · Jerry Hoemann
include/uapi/linux/ndctl.h +4
-0+ + +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.
Quoted 7 lines
Quoted 2 lines
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?)
Quoted 6 lines
True, I do appreciate that aspect.
Quoted 2 lines
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.
Quoted 57 lines
Quoted 9 lines
Quoted 8 lines
Quoted 3 lines
+
__u32ncp_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.
Quoted 6 lines
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.
Quoted 32 lines
Quoted 5 lines
include/uapi/linux/ndctl.h enum ars_masks { · line 237
Quoted 3 lines
include/uapi/linux/ndctl.h +3
-0+__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.
Quoted 6 lines
include/uapi/linux/ndctl.h enum ars_masks { · line 239
Quoted 3 lines
include/uapi/linux/ndctl.h +2
-0+__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.
Quoted 5 lines
So, the interface returns the amount of data that will fit in the space provided and tell the user app in
ncp_pot_sizethe 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.
Quoted 5 lines
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.
Quoted 6 lines
Quoted 3 lines
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.
Signature · 7 lines
----------------------------------------------------------------------------- Jerry Hoemann Software Engineer Hewlett Packard Enterprise ----------------------------------------------------------------------------- _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [RFC v6 2/8] nvdimm: Add wrapper for IOCTL pass thru
Quoted 116 lines · Jerry Hoemann
Quoted 27 lines · Dan Williams
Quoted 18 lines · Jerry Hoemann
Quoted 8 lines · Dan Williams
include/uapi/linux/ndctl.h enum ars_masks { · line 233
Quoted 4 lines · Jerry Hoemann
include/uapi/linux/ndctl.h +4
-0+ + +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.
Quoted 7 lines
Quoted 2 lines
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?)
Quoted 6 lines
True, I do appreciate that aspect.
Quoted 2 lines
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.
Quoted 57 lines
Quoted 9 lines
Quoted 8 lines
Quoted 3 lines
+
__u32ncp_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.
Quoted 6 lines
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.
Quoted 32 lines
Quoted 5 lines
include/uapi/linux/ndctl.h enum ars_masks { · line 237
Quoted 3 lines
include/uapi/linux/ndctl.h +3
-0+__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.
Quoted 6 lines
include/uapi/linux/ndctl.h enum ars_masks { · line 239
Quoted 3 lines
include/uapi/linux/ndctl.h +2
-0+__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.
Quoted 5 lines
So, the interface returns the amount of data that will fit in the space provided and tell the user app in
ncp_pot_sizethe 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.Quoted 5 lines
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.
Quoted 4 lines
Quoted 3 lines
This allows for idempotent calls to be redone with appropriately sized buffer.
Quoted 7 lines
Quoted 2 lines
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.
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [RFC v6 2/8] nvdimm: Add wrapper for IOCTL pass thru
In reply to Jerry HoemannQuoted 10 lines · Jerry Hoemann
Quoted 2 lines
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..
Quoted 1 line
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.
Re: [RFC v6 2/8] nvdimm: Add wrapper for IOCTL pass thru
Quoted 12 lines · Vishal Verma
Quoted 10 lines · Jerry Hoemann
Quoted 2 lines
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.
Quoted 8 lines
Isn't this leaning towards the "Hungarian notation", and isn't that discouraged in the kernel coding style..
From
Documentation/CodingStyleEncoding 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.
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [RFC v6 2/8] nvdimm: Add wrapper for IOCTL pass thru
Quoted 26 lines · Dan Williams
Quoted 24 lines · Vishal Verma
Quoted 13 lines · Jerry Hoemann
Quoted 6 lines
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?)Quoted 3 lines
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
Quoted 8 lines
Quoted 4 lines
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.
Quoted 4 lines
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
RFC v6 3/8 nvdimm: Increase max envelope size for IOCTL
In reply to a message not in the archiveIn __nd_ioctl must first read in the fixed sized portion of an ioctl so that it can then determine the size of the variable part.
Prepare for ND_CMD_CALL_DSM calls which have larger fixed portion wrapper.
--- include/linux/libnvdimm.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/libnvdimm.h +1 -1
@@ -27,7 +27,7 @@ enum {/* need to set a limit somewhere, but yes, this is likely overkill */ ND_IOCTL_MAX_BUFLEN =SZ_4M ,ND_CMD_MAX_ELEM =5 , -ND_CMD_MAX_ENVELOPE =16 , +ND_CMD_MAX_ENVELOPE =256 , ND_MAX_MAPPINGS = 32,/* region flag indicating to direct-map persistent memory by default */
Signature · 6 lines
1.7.11.3 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
RFC v6 4/8 nvdimm: Add UUIDs
In reply to a message not in the archiveAdd UUID associated with HPE Type N NVDIMM.
--- drivers/acpi/nfit.h | 5 +++++ 1 file changed, 5 insertions(+)
drivers/acpi/nfit.h +5 -0
@@ -23,6 +23,8 @@#define UUID_NFIT_BUS "2f10e7a4-9e91-11e4-89d3-123b93f75cba"#define UUID_NFIT_DIMM "4309ac30-0d11-11e4-9191-0800200c9a66" +#define UUID_NFIT_DIMM_N_HPE1 "9002c334-acf3-4c0e-9642-a235f0d53bc6" +#define UUID_NFIT_DIMM_N_HPE2 "5008664B-B758-41A0-A03C-27C2F2D04F7E"#define ACPI_NFIT_MEM_FAILED_MASK (ACPI_NFIT_MEM_SAVE_FAILED \ | ACPI_NFIT_MEM_RESTORE_FAILED | ACPI_NFIT_MEM_FLUSH_FAILED \ | ACPI_NFIT_MEM_NOT_ARMED)
@@ -38,6 +40,8 @@ enum nfit_uuids {NFIT_SPA_PCD ,NFIT_DEV_BUS ,NFIT_DEV_DIMM , +NFIT_DEV_DIMM_N_HPE1 , +NFIT_DEV_DIMM_N_HPE2 , NFIT_UUID_MAX, };
@@ -110,6 +114,7 @@ struct nfit_mem {struct list_head list;struct acpi_device *adev;unsigned long dsm_mask; +const u8 *dsm_uuid; };struct acpi_nfit_desc {
Signature · 6 lines
1.7.11.3 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
RFC v6 5/8 nvdimm: data structure changes for dsm calls
In reply to a message not in the archiveIndicate "dsm_call" capability.
Track uuid.
--- include/linux/libnvdimm.h | 2 ++ 1 file changed, 2 insertions(+)
include/linux/libnvdimm.h +2 -0
@@ -68,7 +68,9 @@ struct nd_mapping {struct nvdimm_bus_descriptor {const struct attribute_group **attr_groups; +unsigned int call_dsm:1 ;unsigned long dsm_mask; +const u8 *dsm_uuid;char *provider_name;ndctl_fn ndctl;int (*flush_probe)(struct nvdimm_bus_descriptor *nd_desc);
Signature · 6 lines
1.7.11.3 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
RFC v6 6/8 libnvdimm: advertise 'call_dsm' support
In reply to a message not in the archiveIndicate to userspace that the generic 'dsm_call' capability is available for the given control device. Over time we want to deprecate the per-dsm function ioctl commands and direct everything through the generic envelope.
--- drivers/nvdimm/core.c | 3 +++ drivers/nvdimm/dimm_devs.c | 8 +++++++- 2 files changed, 10 insertions(+), 1 deletion(-)
drivers/nvdimm/core.c +3 -0
@@ -253,6 +253,9 @@ static ssize_t commands_show(struct device *dev,for_each_set_bit (cmd, &nd_desc->dsm_mask,BITS_PER_LONG ) len +=sprintf (buf + len,"%s " ,nvdimm_bus_cmd_name (cmd)); +if (nd_desc->call_dsm) + len +=sprintf (buf + len,"%s " , +nvdimm_bus_cmd_name (ND_CMD_CALL_DSM )); len +=sprintf (buf + len,"\n" );return len; }
drivers/nvdimm/dimm_devs.c +7 -1
@@ -274,14 +274,20 @@ EXPORT_SYMBOL_GPL(nvdimm_provider_data);static ssize_t commands_show (struct device *dev,struct device_attribute *attr,char *buf) { +struct nvdimm_bus *nvdimm_bus =walk_to_nvdimm_bus (dev);struct nvdimm *nvdimm =to_nvdimm (dev); +struct nvdimm_bus_descriptor *nd_desc;int cmd, len =0 ; -if (!nvdimm->dsm_mask) +if (!nvdimm->dsm_mask || !nvdimm_bus)return sprintf (buf,"\n" );for_each_set_bit (cmd, nvdimm->dsm_mask,BITS_PER_LONG ) len +=sprintf (buf + len,"%s " ,nvdimm_cmd_name (cmd)); + nd_desc = nvdimm_bus->nd_desc; +if (nd_desc->call_dsm) + len +=sprintf (buf + len,"%s " , +nvdimm_cmd_name (ND_CMD_CALL_DSM )); len +=sprintf (buf + len,"\n" );return len; }
Signature · 6 lines
1.7.11.3 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
RFC v6 7/8 nvdimm: Add IOCTL pass thru functions
In reply to a message not in the archiveAdd ioctl command ND_CMD_CALL_DSM to acpi_nfit_ctl and __nd_ioctl which allow kernel to call a nvdimm's _DSM as a passthru without using the marshaling code of the nd_cmd_desc.
--- drivers/acpi/nfit.c | 149 +++++++++++++++++++++++++++++++++++++++++++-------- drivers/nvdimm/bus.c | 45 +++++++++++++++- 2 files changed, 170 insertions(+), 24 deletions(-)
drivers/acpi/nfit.c +126 -23
@@ -56,6 +56,21 @@ struct nfit_table_prev {struct list_head flushes; }; +struct cmd_type_tbl { +enum nfit_uuids key_uuid;/* Internal handle */ +int key_type;/* Exported handle */ +int rev;/* _DSM rev */ +u64 mask;/* 0 bit excludes underlying func.*/ +}; + +struct cmd_type_tbl cmd_type_tbl[] = { + {NFIT_DEV_BUS ,NCP_TYPE_BUS ,1 , ~0UL }, + {NFIT_DEV_DIMM ,NCP_TYPE_DIMM_INTEL1 ,1 , ~0UL }, + {NFIT_DEV_DIMM_N_HPE1 ,NCP_TYPE_DIMM_N_HPE1 ,1 , ~0UL }, + {NFIT_DEV_DIMM_N_HPE2 ,NCP_TYPE_DIMM_N_HPE2 ,1 , ~0UL }, + {-1 ,-1 ,-1 ,0 }, +}; +static u8 nfit_uuid[NFIT_UUID_MAX ][16 ];const u8 *to_nfit_uuid (enum nfit_uuids id)
@@ -64,6 +79,18 @@ const u8 *to_nfit_uuid(enum nfit_uuids id) }EXPORT_SYMBOL (to_nfit_uuid); +static int +known_nvdimm_type (u32 type,struct cmd_type_tbl *tbl) +{ +int i; + +for (i =0 ; tbl[i].key_type >=0 ; i++) { +if (tbl[i].key_type == type) +return 1 ; + } +return 0 ; +} +static struct acpi_nfit_desc *to_acpi_nfit_desc (struct nvdimm_bus_descriptor *nd_desc) {
@@ -171,8 +198,9 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, unsigned int buf_len, int *cmd_rc) { struct acpi_nfit_desc *acpi_desc = to_acpi_nfit_desc(nd_desc); -const struct nd_cmd_desc *desc =NULL ;union acpi_object in_obj, in_buf, *out_obj; +struct nd_cmd_pkg *call_dsm =NULL ; +const struct nd_cmd_desc *desc =NULL ;struct device *dev = acpi_desc->dev;const char *cmd_name, *dimm_name;unsigned long dsm_mask;
@@ -180,6 +208,12 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,const u8 *uuid;u32 offset;int rc, i; +__u64 rev =1 , func = cmd; + +if (cmd ==ND_CMD_CALL_DSM ) { + call_dsm = buf; + func = call_dsm->ncp_command; + }if (nvdimm) {struct nfit_mem *nfit_mem =nvdimm_provider_data (nvdimm);
@@ -191,7 +225,7 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, cmd_name =nvdimm_cmd_name (cmd); dsm_mask = nfit_mem->dsm_mask; desc =nd_cmd_dimm_desc (cmd); - uuid =to_nfit_uuid (NFIT_DEV_DIMM ); + uuid = nfit_mem->dsm_uuid; handle = adev->handle; }else {struct acpi_device *adev =to_acpi_dev (acpi_desc);
@@ -199,7 +233,7 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, cmd_name =nvdimm_bus_cmd_name (cmd); dsm_mask = nd_desc->dsm_mask; desc =nd_cmd_bus_desc (cmd); - uuid =to_nfit_uuid (NFIT_DEV_BUS ); + uuid = nd_desc->dsm_uuid; handle = adev->handle; dimm_name ="bus" ; }
@@ -207,7 +241,7 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,if (!desc || (cmd && (desc->out_num + desc->in_num ==0 )))return -ENOTTY ; -if (!test_bit (cmd, &dsm_mask)) +if (!test_bit (func, &dsm_mask))return -ENOTTY ; in_obj.type =ACPI_TYPE_PACKAGE ;
@@ -218,25 +252,50 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, in_buf.buffer.length =0 ;/* libnvdimm has already validated the input envelope */ -for (i =0 ; i < desc->in_num; i++) +for (i =0 ; i < desc->in_num; i++) { in_buf.buffer.length +=nd_cmd_in_size (nvdimm, cmd, desc, i, buf); + } + +if (call_dsm) { +/* must skip over package wrapper */ + in_buf.buffer.pointer = (void *) &call_dsm->ncp_payload; + in_buf.buffer.length = call_dsm->ncp_size_in; +if (!known_nvdimm_type (call_dsm->ncp_type, cmd_type_tbl)) { +dev_dbg (dev,"%s:%s unsupported uuid\n" , dimm_name, + cmd_name); +return -EINVAL ; + } + rev = call_dsm->ncp_rev; + }if (IS_ENABLED (CONFIG_ACPI_NFIT_DEBUG )) { -dev_dbg (dev,"%s:%s cmd: %s input length: %d\n" , __func__, - dimm_name, cmd_name, in_buf.buffer.length); -print_hex_dump_debug (cmd_name,DUMP_PREFIX_OFFSET ,4 , -4 , in_buf.buffer.pointer,min_t (u32,128 , - in_buf.buffer.length), true); +dev_dbg (dev,"%s:%s cmd: %d: %llu input length: %d\n" , __func__, + dimm_name, cmd, func, in_buf.buffer.length); +print_hex_dump_debug ("nvdimm in " ,DUMP_PREFIX_OFFSET ,4 ,4 , + in_buf.buffer.pointer, +min_t (u32,256 , in_buf.buffer.length), true); + } - out_obj =acpi_evaluate_dsm (handle, uuid,1 , cmd, &in_obj); + out_obj =acpi_evaluate_dsm (handle, uuid, rev, func, &in_obj);if (!out_obj) { -dev_dbg (dev,"%s:%s _DSM failed cmd: %s\n" , __func__, dimm_name, - cmd_name); +dev_dbg (dev,"%s:%s unexpected output object type cmd: %s %llu, type: %d\n" , + __func__, dimm_name, cmd_name, func, out_obj->type); +return -EINVAL ; } +if (call_dsm) { + call_dsm->ncp_pot_size = out_obj->buffer.length; +memcpy (call_dsm->ncp_payload + call_dsm->ncp_size_in, + out_obj->buffer.pointer, +min (call_dsm->ncp_pot_size, call_dsm->ncp_size_out)); + +ACPI_FREE (out_obj); +return 0 ; + } +if (out_obj->package.type !=ACPI_TYPE_BUFFER ) {dev_dbg (dev,"%s:%s unexpected output object type cmd: %s type: %d\n" , __func__, dimm_name, cmd_name, out_obj->type);
@@ -918,13 +977,60 @@ static struct nvdimm *acpi_nfit_dimm_by_handle(struct acpi_nfit_desc *acpi_desc,return NULL ; } + +/* +* determine if the _DSM specified by UUID is supported and return +* mask of supported functions in nd_cmd_mask. +*/ + +static int acpi_nfit_sup_func (acpi_handle handle,const u8 *uuid, +int rev,unsigned long *nd_cmd_mask) +{ +int i; +u64 mask =0 ; +union acpi_object *obj; + + obj =acpi_evaluate_dsm (handle, uuid, rev,0 ,NULL ); +if (!obj) +return 0 ; +/* For compatibility, old BIOSes may return an integer */ +if (obj->type ==ACPI_TYPE_INTEGER ) + mask = obj->integer.value; +else if (obj->type ==ACPI_TYPE_BUFFER ) +for (i =0 ; i < obj->buffer.length && i <8 ; i++) + mask |= (((u8 )obj->buffer.pointer[i]) << (i *8 )); +ACPI_FREE (obj); + + *nd_cmd_mask = mask; + +return !!mask; +} + + +static inline void +to_nfit_uuid_msk (acpi_handle handle,struct cmd_type_tbl *tbl, +u8 const **cmd_uuid,unsigned long *cmd_mask) +{ +unsigned long mask =0 ; +int i; + +for (i =0 ; tbl[i].key_uuid >=0 ; i++) { +const u8 *uuid =to_nfit_uuid (tbl[i].key_uuid); +int rev = tbl[i].rev; + +if (acpi_nfit_sup_func (handle, uuid, rev, &mask)) { + *cmd_mask = mask & tbl[i].mask; + *cmd_uuid = uuid; +break ; + } + } +} +static int acpi_nfit_add_dimm (struct acpi_nfit_desc *acpi_desc,struct nfit_mem *nfit_mem,u32 device_handle) {struct acpi_device *adev, *adev_dimm;struct device *dev = acpi_desc->dev; -const u8 *uuid =to_nfit_uuid (NFIT_DEV_DIMM ); -int i; nfit_mem->dsm_mask = acpi_desc->dimm_dsm_force_en; adev =to_acpi_dev (acpi_desc);
@@ -939,9 +1045,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,return force_enable_dimms ?0 : -ENODEV ; } -for (i =ND_CMD_SMART ; i <=ND_CMD_VENDOR ; i++) -if (acpi_check_dsm (adev_dimm->handle, uuid,1 ,1ULL << i)) -set_bit (i, &nfit_mem->dsm_mask); +to_nfit_uuid_msk (adev_dimm->handle, cmd_type_tbl, &nfit_mem->dsm_uuid, &nfit_mem->dsm_mask);return 0 ; }
@@ -1003,18 +1107,15 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *acpi_desc)static void acpi_nfit_init_dsms (struct acpi_nfit_desc *acpi_desc) {struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc; -const u8 *uuid =to_nfit_uuid (NFIT_DEV_BUS );struct acpi_device *adev; -int i; nd_desc->dsm_mask = acpi_desc->bus_dsm_force_en; adev =to_acpi_dev (acpi_desc);if (!adev)return ; -for (i =ND_CMD_ARS_CAP ; i <=ND_CMD_CLEAR_ERROR ; i++) -if (acpi_check_dsm (adev->handle, uuid,1 ,1ULL << i)) -set_bit (i, &nd_desc->dsm_mask); + nd_desc->call_dsm =1 ; +to_nfit_uuid_msk (adev->handle, cmd_type_tbl, &nd_desc->dsm_uuid, &nd_desc->dsm_mask); } static ssize_t range_index_show(struct device *dev,
@@ -2463,6 +2564,8 @@ static __init int nfit_init(void)acpi_str_to_uuid (UUID_PERSISTENT_VIRTUAL_CD , nfit_uuid[NFIT_SPA_PCD ]);acpi_str_to_uuid (UUID_NFIT_BUS , nfit_uuid[NFIT_DEV_BUS ]);acpi_str_to_uuid (UUID_NFIT_DIMM , nfit_uuid[NFIT_DEV_DIMM ]); +acpi_str_to_uuid (UUID_NFIT_DIMM_N_HPE1 , nfit_uuid[NFIT_DEV_DIMM_N_HPE1 ]); +acpi_str_to_uuid (UUID_NFIT_DIMM_N_HPE2 , nfit_uuid[NFIT_DEV_DIMM_N_HPE2 ]); nfit_wq =create_singlethread_workqueue ("nfit" );if (!nfit_wq)
drivers/nvdimm/bus.c +44 -1
95 lines not on this page · raw message
Signature · 6 lines
1.7.11.3 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
RFC v6 8/8 tools/testing/nvdimm: 'call_dsm' support
In reply to a message not in the archiveEnable nfit_test to use the generic 'call_dsm' envelope.
--- tools/testing/nvdimm/test/nfit.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
tools/testing/nvdimm/test/nfit.c +11 -2
@@ -336,12 +336,21 @@ static int nfit_test_ctl(struct nvdimm_bus_descriptor *nd_desc, {struct acpi_nfit_desc *acpi_desc =to_acpi_desc (nd_desc);struct nfit_test *t =container_of (acpi_desc,typeof (*t), acpi_desc); +unsigned int func = cmd;int i, rc =0 , __cmd_rc;if (!cmd_rc) cmd_rc = &__cmd_rc; *cmd_rc =0 ; +if (cmd ==ND_CMD_CALL_DSM ) { +struct nd_cmd_pkg *call_dsm = buf; + + buf_len = call_dsm->ncp_size_in + call_dsm->ncp_size_out; + buf = (void *) call_dsm->ncp_payload; + func = call_dsm->ncp_command; + } +if (nvdimm) {struct nfit_mem *nfit_mem =nvdimm_provider_data (nvdimm);
@@ -356,7 +365,7 @@ static int nfit_test_ctl(struct nvdimm_bus_descriptor *nd_desc,if (i >=ARRAY_SIZE (handle))return -ENXIO ; -switch (cmd) { +switch (func) {case ND_CMD_GET_CONFIG_SIZE : rc =nfit_test_cmd_get_config_size (buf, buf_len);break ;
@@ -377,7 +386,7 @@ static int nfit_test_ctl(struct nvdimm_bus_descriptor *nd_desc,if (!nd_desc || !test_bit (cmd, &nd_desc->dsm_mask))return -ENOTTY ; -switch (cmd) { +switch (func) {case ND_CMD_ARS_CAP : rc =nfit_test_cmd_ars_cap (buf, buf_len);break ;
Signature · 6 lines
1.7.11.3 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm