Re: [PATCH 3/3] libnvdimm: New ACPI 6.2 DSM functions

[Dan Williams]

On Fri, Jun 9, 2017 at 9:59 AM, Jerry Hoemann  wrote:
> On Thu, Jun 08, 2017 at 10:28:51PM -0700, Dan Williams wrote:
>> On Thu, Jun 8, 2017 at 1:02 AM, Johannes Thumshirn  
>> wrote:
>> > On 06/07/2017 07:04 PM, Jerry Hoemann wrote:
>> >> @@ -179,6 +217,10 @@ 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",
>> >> + [5] = "trans_spa",
>> >> + [7] = "ars_err_inj",
>> >> + [8] = "ars_err_inj_clr",
>> >> + [9] = "ars_err_inj_stat",
>> >>   [ND_CMD_CALL] = "cmd_call",
>> >>   };
>> >
>> > Can you please add the new values to the enum in uapi/ndctl.h? I don't
>> > really like the magic numbers here.
>>
>> I think the reason Jerry didn't add them is due to symmetry. All the
>> current ND_CMD_ definitions have corresponding ND_IOCTL_ definitions.
>> These new commands we're only adding function number support and using
>> ND_CMD_CALL for the ioctl transport.
>
>   Yes, this is correct.
>
>>   We can still add definitions for
>> them, but perhaps with an ACPI_NFIT_DSM_ prefix? The only consumer in
>> the kernel of this is debug output code. I think perhaps we should
>> just delete them here and either print raw numbers or enable
>> acpi_nfit_ctl() to the decode and not rely on nvdimm_bus_cmd_name()
>> which is meant to represent NVDIMM bus type generic command numbers.
>
> This also comes into play when doing:
>
> cat /sys/class/nd/ndctl0/device/nd/ndctl0/device/commands
>
> Without change here the new functions would show up as "unknown unknown 
> unknown unknown."
>
> Dan, are you suggesting I change the above to:
>
>[ND_CMD_CLEAR_ERROR] = "clear_error",
>  + [5] = "5",
>  + [7] = "7",
>  + [8] = "8",
>  + [9] = "9",
>[ND_CMD_CALL] = "cmd_call",
>
> Note, "6" is currently reserved by ACPI.  I could add that also.

I think it should behave the same as the dimm level 'cmd' vs 'func'
distinction. Where only the ND_CMD instances are enumerated in the
sysfs '/sys/bus/nd/devices/ndbus0/commands' attribute. The rest of the
supported functions are identified in the 'dsm_mask' attribute. We
don't currently have one of those for the bus level but we can add it
to "/sys/bus/nd/devices/ndbus0/nfit/dsm_mask". This would be similar
to "/sys/bus/nd/devices/nmem0/nfit/dsm_mask".

Re: [PATCH 3/3] libnvdimm: New ACPI 6.2 DSM functions

[Dan Williams]

On Fri, Jun 9, 2017 at 9:59 AM, Jerry Hoemann  wrote:
> On Thu, Jun 08, 2017 at 10:28:51PM -0700, Dan Williams wrote:
>> On Thu, Jun 8, 2017 at 1:02 AM, Johannes Thumshirn  
>> wrote:
>> > On 06/07/2017 07:04 PM, Jerry Hoemann wrote:
>> >> @@ -179,6 +217,10 @@ 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",
>> >> + [5] = "trans_spa",
>> >> + [7] = "ars_err_inj",
>> >> + [8] = "ars_err_inj_clr",
>> >> + [9] = "ars_err_inj_stat",
>> >>   [ND_CMD_CALL] = "cmd_call",
>> >>   };
>> >
>> > Can you please add the new values to the enum in uapi/ndctl.h? I don't
>> > really like the magic numbers here.
>>
>> I think the reason Jerry didn't add them is due to symmetry. All the
>> current ND_CMD_ definitions have corresponding ND_IOCTL_ definitions.
>> These new commands we're only adding function number support and using
>> ND_CMD_CALL for the ioctl transport.
>
>   Yes, this is correct.
>
>>   We can still add definitions for
>> them, but perhaps with an ACPI_NFIT_DSM_ prefix? The only consumer in
>> the kernel of this is debug output code. I think perhaps we should
>> just delete them here and either print raw numbers or enable
>> acpi_nfit_ctl() to the decode and not rely on nvdimm_bus_cmd_name()
>> which is meant to represent NVDIMM bus type generic command numbers.
>
> This also comes into play when doing:
>
> cat /sys/class/nd/ndctl0/device/nd/ndctl0/device/commands
>
> Without change here the new functions would show up as "unknown unknown 
> unknown unknown."
>
> Dan, are you suggesting I change the above to:
>
>[ND_CMD_CLEAR_ERROR] = "clear_error",
>  + [5] = "5",
>  + [7] = "7",
>  + [8] = "8",
>  + [9] = "9",
>[ND_CMD_CALL] = "cmd_call",
>
> Note, "6" is currently reserved by ACPI.  I could add that also.

I think it should behave the same as the dimm level 'cmd' vs 'func'
distinction. Where only the ND_CMD instances are enumerated in the
sysfs '/sys/bus/nd/devices/ndbus0/commands' attribute. The rest of the
supported functions are identified in the 'dsm_mask' attribute. We
don't currently have one of those for the bus level but we can add it
to "/sys/bus/nd/devices/ndbus0/nfit/dsm_mask". This would be similar
to "/sys/bus/nd/devices/nmem0/nfit/dsm_mask".

Re: [PATCH 3/3] libnvdimm: New ACPI 6.2 DSM functions

[Jerry Hoemann]

On Thu, Jun 08, 2017 at 10:28:51PM -0700, Dan Williams wrote:
> On Thu, Jun 8, 2017 at 1:02 AM, Johannes Thumshirn  wrote:
> > On 06/07/2017 07:04 PM, Jerry Hoemann wrote:
> >> @@ -179,6 +217,10 @@ 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",
> >> + [5] = "trans_spa",
> >> + [7] = "ars_err_inj",
> >> + [8] = "ars_err_inj_clr",
> >> + [9] = "ars_err_inj_stat",
> >>   [ND_CMD_CALL] = "cmd_call",
> >>   };
> >
> > Can you please add the new values to the enum in uapi/ndctl.h? I don't
> > really like the magic numbers here.
> 
> I think the reason Jerry didn't add them is due to symmetry. All the
> current ND_CMD_ definitions have corresponding ND_IOCTL_ definitions.
> These new commands we're only adding function number support and using
> ND_CMD_CALL for the ioctl transport.

  Yes, this is correct.

>   We can still add definitions for
> them, but perhaps with an ACPI_NFIT_DSM_ prefix? The only consumer in
> the kernel of this is debug output code. I think perhaps we should
> just delete them here and either print raw numbers or enable
> acpi_nfit_ctl() to the decode and not rely on nvdimm_bus_cmd_name()
> which is meant to represent NVDIMM bus type generic command numbers.

This also comes into play when doing:

cat /sys/class/nd/ndctl0/device/nd/ndctl0/device/commands

Without change here the new functions would show up as "unknown unknown unknown 
unknown."

Dan, are you suggesting I change the above to:

   [ND_CMD_CLEAR_ERROR] = "clear_error",
 + [5] = "5",
 + [7] = "7",
 + [8] = "8",
 + [9] = "9",
   [ND_CMD_CALL] = "cmd_call",

Note, "6" is currently reserved by ACPI.  I could add that also.



-- 

-
Jerry Hoemann  Software Engineer   Hewlett Packard Enterprise
-

Re: [PATCH 3/3] libnvdimm: New ACPI 6.2 DSM functions

[Jerry Hoemann]

On Thu, Jun 08, 2017 at 10:28:51PM -0700, Dan Williams wrote:
> On Thu, Jun 8, 2017 at 1:02 AM, Johannes Thumshirn  wrote:
> > On 06/07/2017 07:04 PM, Jerry Hoemann wrote:
> >> @@ -179,6 +217,10 @@ 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",
> >> + [5] = "trans_spa",
> >> + [7] = "ars_err_inj",
> >> + [8] = "ars_err_inj_clr",
> >> + [9] = "ars_err_inj_stat",
> >>   [ND_CMD_CALL] = "cmd_call",
> >>   };
> >
> > Can you please add the new values to the enum in uapi/ndctl.h? I don't
> > really like the magic numbers here.
> 
> I think the reason Jerry didn't add them is due to symmetry. All the
> current ND_CMD_ definitions have corresponding ND_IOCTL_ definitions.
> These new commands we're only adding function number support and using
> ND_CMD_CALL for the ioctl transport.

  Yes, this is correct.

>   We can still add definitions for
> them, but perhaps with an ACPI_NFIT_DSM_ prefix? The only consumer in
> the kernel of this is debug output code. I think perhaps we should
> just delete them here and either print raw numbers or enable
> acpi_nfit_ctl() to the decode and not rely on nvdimm_bus_cmd_name()
> which is meant to represent NVDIMM bus type generic command numbers.

This also comes into play when doing:

cat /sys/class/nd/ndctl0/device/nd/ndctl0/device/commands

Without change here the new functions would show up as "unknown unknown unknown 
unknown."

Dan, are you suggesting I change the above to:

   [ND_CMD_CLEAR_ERROR] = "clear_error",
 + [5] = "5",
 + [7] = "7",
 + [8] = "8",
 + [9] = "9",
   [ND_CMD_CALL] = "cmd_call",

Note, "6" is currently reserved by ACPI.  I could add that also.



-- 

-
Jerry Hoemann  Software Engineer   Hewlett Packard Enterprise
-

Re: [PATCH 3/3] libnvdimm: New ACPI 6.2 DSM functions

[Dan Williams]

On Thu, Jun 8, 2017 at 1:02 AM, Johannes Thumshirn  wrote:
> On 06/07/2017 07:04 PM, Jerry Hoemann wrote:
>> @@ -179,6 +217,10 @@ 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",
>> + [5] = "trans_spa",
>> + [7] = "ars_err_inj",
>> + [8] = "ars_err_inj_clr",
>> + [9] = "ars_err_inj_stat",
>>   [ND_CMD_CALL] = "cmd_call",
>>   };
>
> Can you please add the new values to the enum in uapi/ndctl.h? I don't
> really like the magic numbers here.

I think the reason Jerry didn't add them is due to symmetry. All the
current ND_CMD_ definitions have corresponding ND_IOCTL_ definitions.
These new commands we're only adding function number support and using
ND_CMD_CALL for the ioctl transport. We can still add definitions for
them, but perhaps with an ACPI_NFIT_DSM_ prefix? The only consumer in
the kernel of this is debug output code. I think perhaps we should
just delete them here and either print raw numbers or enable
acpi_nfit_ctl() to the decode and not rely on nvdimm_bus_cmd_name()
which is meant to represent NVDIMM bus type generic command numbers.

Re: [PATCH 3/3] libnvdimm: New ACPI 6.2 DSM functions

[Dan Williams]

On Thu, Jun 8, 2017 at 1:02 AM, Johannes Thumshirn  wrote:
> On 06/07/2017 07:04 PM, Jerry Hoemann wrote:
>> @@ -179,6 +217,10 @@ 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",
>> + [5] = "trans_spa",
>> + [7] = "ars_err_inj",
>> + [8] = "ars_err_inj_clr",
>> + [9] = "ars_err_inj_stat",
>>   [ND_CMD_CALL] = "cmd_call",
>>   };
>
> Can you please add the new values to the enum in uapi/ndctl.h? I don't
> really like the magic numbers here.

I think the reason Jerry didn't add them is due to symmetry. All the
current ND_CMD_ definitions have corresponding ND_IOCTL_ definitions.
These new commands we're only adding function number support and using
ND_CMD_CALL for the ioctl transport. We can still add definitions for
them, but perhaps with an ACPI_NFIT_DSM_ prefix? The only consumer in
the kernel of this is debug output code. I think perhaps we should
just delete them here and either print raw numbers or enable
acpi_nfit_ctl() to the decode and not rely on nvdimm_bus_cmd_name()
which is meant to represent NVDIMM bus type generic command numbers.

Re: [PATCH 3/3] libnvdimm: New ACPI 6.2 DSM functions

[Johannes Thumshirn]

On 06/07/2017 07:04 PM, Jerry Hoemann wrote:
> @@ -179,6 +217,10 @@ 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",
> + [5] = "trans_spa",
> + [7] = "ars_err_inj",
> + [8] = "ars_err_inj_clr",
> + [9] = "ars_err_inj_stat",
>   [ND_CMD_CALL] = "cmd_call",
>   };

Can you please add the new values to the enum in uapi/ndctl.h? I don't
really like the magic numbers here.

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 3/3] libnvdimm: New ACPI 6.2 DSM functions

[Johannes Thumshirn]

On 06/07/2017 07:04 PM, Jerry Hoemann wrote:
> @@ -179,6 +217,10 @@ 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",
> + [5] = "trans_spa",
> + [7] = "ars_err_inj",
> + [8] = "ars_err_inj_clr",
> + [9] = "ars_err_inj_stat",
>   [ND_CMD_CALL] = "cmd_call",
>   };

Can you please add the new values to the enum in uapi/ndctl.h? I don't
really like the magic numbers here.

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 3/3] libnvdimm: New ACPI 6.2 DSM functions

[Jerry Hoemann]

ACPI 6.2 added new NVDIMM root DSM functions.  Define their
data structures.

Update the definition of nd_cmd_ars_cap to match the 6.2 spec.

Add the new 6.2 functions names to nvdimm_bus_cmd_name.

Signed-off-by: Jerry Hoemann 
---
 include/uapi/linux/ndctl.h | 44 +++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index e23c37f..bcaf79e 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -105,7 +105,8 @@ struct nd_cmd_ars_cap {
__u32 status;
__u32 max_ars_out;
__u32 clear_err_unit;
-   __u32 reserved;
+   __u16 flags;
+   __u16 reserved;
 } __packed;
 
 struct nd_cmd_ars_start {
@@ -144,6 +145,43 @@ struct nd_cmd_clear_error {
__u64 cleared;
 } __packed;
 
+struct nd_cmd_trans_spa {
+   __u64 spa;
+   __u32 status;
+   __u8  flags;
+   __u8  _reserved[3];
+   __u64 trans_length;
+   __u32 num_nvdimms;
+   struct nd_nvdimm_device {
+   __u32 nfit_device_handle;
+   __u32 _reserved;
+   __u64 dpa;
+   } __packed devices[0];
+
+} __packed;
+
+struct nd_cmd_ars_err_inj {
+   __u64 err_inj_spa_range_base;
+   __u64 err_inj_spa_range_length;
+   __u8  err_inj_options;
+   __u32 status;
+} __packed;
+
+struct nd_cmd_ars_err_inj_clr {
+   __u64 err_inj_clr_spa_range_base;
+   __u64 err_inj_clr_spa_range_length;
+   __u32 status;
+} __packed;
+
+struct nd_cmd_ars_err_inj_stat {
+   __u32 status;
+   __u32 inj_err_rec_count;
+   struct nd_error_stat_query_record {
+   __u64 err_inj_stat_spa_range_base;
+   __u64 err_inj_stat_spa_range_length;
+   } __packed record[0];
+} __packed;
+
 enum {
ND_CMD_IMPLEMENTED = 0,
 
@@ -179,6 +217,10 @@ 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",
+   [5] = "trans_spa",
+   [7] = "ars_err_inj",
+   [8] = "ars_err_inj_clr",
+   [9] = "ars_err_inj_stat",
[ND_CMD_CALL] = "cmd_call",
};
 
-- 
1.8.5.6

[PATCH 3/3] libnvdimm: New ACPI 6.2 DSM functions

[Jerry Hoemann]

ACPI 6.2 added new NVDIMM root DSM functions.  Define their
data structures.

Update the definition of nd_cmd_ars_cap to match the 6.2 spec.

Add the new 6.2 functions names to nvdimm_bus_cmd_name.

Signed-off-by: Jerry Hoemann 
---
 include/uapi/linux/ndctl.h | 44 +++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index e23c37f..bcaf79e 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -105,7 +105,8 @@ struct nd_cmd_ars_cap {
__u32 status;
__u32 max_ars_out;
__u32 clear_err_unit;
-   __u32 reserved;
+   __u16 flags;
+   __u16 reserved;
 } __packed;
 
 struct nd_cmd_ars_start {
@@ -144,6 +145,43 @@ struct nd_cmd_clear_error {
__u64 cleared;
 } __packed;
 
+struct nd_cmd_trans_spa {
+   __u64 spa;
+   __u32 status;
+   __u8  flags;
+   __u8  _reserved[3];
+   __u64 trans_length;
+   __u32 num_nvdimms;
+   struct nd_nvdimm_device {
+   __u32 nfit_device_handle;
+   __u32 _reserved;
+   __u64 dpa;
+   } __packed devices[0];
+
+} __packed;
+
+struct nd_cmd_ars_err_inj {
+   __u64 err_inj_spa_range_base;
+   __u64 err_inj_spa_range_length;
+   __u8  err_inj_options;
+   __u32 status;
+} __packed;
+
+struct nd_cmd_ars_err_inj_clr {
+   __u64 err_inj_clr_spa_range_base;
+   __u64 err_inj_clr_spa_range_length;
+   __u32 status;
+} __packed;
+
+struct nd_cmd_ars_err_inj_stat {
+   __u32 status;
+   __u32 inj_err_rec_count;
+   struct nd_error_stat_query_record {
+   __u64 err_inj_stat_spa_range_base;
+   __u64 err_inj_stat_spa_range_length;
+   } __packed record[0];
+} __packed;
+
 enum {
ND_CMD_IMPLEMENTED = 0,
 
@@ -179,6 +217,10 @@ 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",
+   [5] = "trans_spa",
+   [7] = "ars_err_inj",
+   [8] = "ars_err_inj_clr",
+   [9] = "ars_err_inj_stat",
[ND_CMD_CALL] = "cmd_call",
};
 
-- 
1.8.5.6