[PATCH 0/3] Enable DSM pass thru for root functions

[PATCH 0/3] Enable DSM pass thru for root functions

[Jerry Hoemann]

The new ACPI 6.2 spec has added new NVDIMM root DSM functions
that managibility and test software needs to call.

This patch set enables the calling of root functions DSMs via the
pass thru mechanism.

Details

__nd_ioctl:
Check pass thru functions against nd_cmd_clear_to_send.

acpi_nfit_init_dsms:
Set additional bits in cmd_mask for new functions.

ndctl.h:
Define data structure for the new 6.2 functions.
Add new function names to nvdimm_bus_cmd_name.



Jerry Hoemann (3):
  libnvdimm: passthru functions clear to send
  acpi, nfit: Enable DSM pass thru for root functions.
  libnvdimm: New ACPI 6.2 DSM functions

 drivers/acpi/nfit/core.c   |  3 ++-
 drivers/nvdimm/bus.c       |  4 +++-
 include/uapi/linux/ndctl.h | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 49 insertions(+), 3 deletions(-)

-- 
1.8.5.6

[PATCH 1/3] libnvdimm: passthru functions clear to send

[Jerry Hoemann]

Have dsm functions called via the pass thru mechanism also
be checked against clear to send.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/nvdimm/bus.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index e9361bf..e16427d 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -907,6 +907,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 	static char in_env[ND_CMD_MAX_ENVELOPE];
 	const struct nd_cmd_desc *desc = NULL;
 	unsigned int cmd = _IOC_NR(ioctl_cmd);
+	unsigned int func = cmd;
 	void __user *p = (void __user *) arg;
 	struct device *dev = &nvdimm_bus->dev;
 	struct nd_cmd_pkg pkg;
@@ -972,6 +973,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 	}
 
 	if (cmd == ND_CMD_CALL) {
+		func = pkg.nd_command;
 		dev_dbg(dev, "%s:%s, idx: %llu, in: %zu, out: %zu, len %zu\n",
 				__func__, dimm_name, pkg.nd_command,
 				in_len, out_len, buf_len);
@@ -1020,7 +1022,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 	}
 
 	nvdimm_bus_lock(&nvdimm_bus->dev);
-	rc = nd_cmd_clear_to_send(nvdimm_bus, nvdimm, cmd, buf);
+	rc = nd_cmd_clear_to_send(nvdimm_bus, nvdimm, func, buf);
 	if (rc)
 		goto out_unlock;
 
-- 
1.8.5.6

[PATCH 2/3] acpi, nfit: Enable DSM pass thru for root functions.

[Jerry Hoemann]

Set ND_CMD_CALL in the cmd_mask to enable calling root
functions via the pass trhu mechanism.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/acpi/nfit/core.c   | 3 ++-
 include/uapi/linux/ndctl.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 656acb5..a848b17 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1620,9 +1620,10 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
 	if (!adev)
 		return;
 
-	for (i = ND_CMD_ARS_CAP; i <= ND_CMD_CLEAR_ERROR; i++)
+	for (i = ND_CMD_ARS_CAP; i <  ND_CMD_CALL; i++)
 		if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
 			set_bit(i, &nd_desc->cmd_mask);
+	set_bit(ND_CMD_CALL, &nd_desc->cmd_mask);
 }
 
 static ssize_t range_index_show(struct device *dev,
diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index 7ad3863..e23c37f 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -179,6 +179,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] = "cmd_call",
 	};
 
 	if (cmd < ARRAY_SIZE(names) && names[cmd])
-- 
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 <jerry.hoemann@hpe.com>
---
 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

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
jthumshirn@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

[Dan Williams]

On Thu, Jun 8, 2017 at 1:02 AM, Johannes Thumshirn <jthumshirn@suse.de> 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

[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 <jthumshirn@suse.de> 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 Fri, Jun 9, 2017 at 9:59 AM, Jerry Hoemann <jerry.hoemann@hpe.com> 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 <jthumshirn@suse.de> 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".