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

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

[Jerry Hoemann]

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

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

Changes v3
----------
1. Fix checkpatch warnings

2. modify bus_dsm_mask_show to display name "dsm_mask" in sysfs.

3. modified submittal comments.

Changes v2
----------
1. Add bus_dsm_mask to filter root pass thru calls.

2. Add bus_dsm_mask_show to display bus_dsm_mask in sysfs

3. Extend override_dsm_mask to be used for bus_dms_mask also.




Details v1
----------

__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.


Jerry Hoemann (7):
  libnvdimm: passthru functions clear to send
  acpi, nfit: Enable DSM pass thru for root functions.
  libnvdimm: Add bus level dsm mask.
  acpi, nfit: Use bus_dsm_mask for passthru
  acpi, nfit: Show bus_dsm_mask in sysfs
  libnvdimm: New ACPI 6.2 DSM functions
  acpi, nfit: override mask

 drivers/acpi/nfit/core.c   | 23 +++++++++++++++++++++++
 drivers/nvdimm/bus.c       |  4 +++-
 include/linux/libnvdimm.h  |  1 +
 include/uapi/linux/ndctl.h | 41 ++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 67 insertions(+), 2 deletions(-)

-- 
1.8.5.6

[PATCH v3 1/7] 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 v3 2/7] 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 thru mechanism.

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

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 656acb5..b46fca2 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1623,6 +1623,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
 	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->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 v3 3/7] libnvdimm: Add bus level dsm mask.

[Jerry Hoemann]

Add a bus level dsm_mask to nvdimm_bus_descriptor to allow the passthru
calling mechanism to specify a different mask from the cmd_mask.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 include/linux/libnvdimm.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 6c80701..f8b8f43 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -54,6 +54,7 @@ typedef int (*ndctl_fn)(struct nvdimm_bus_descriptor *nd_desc,
 
 struct nvdimm_bus_descriptor {
 	const struct attribute_group **attr_groups;
+	unsigned long bus_dsm_mask;
 	unsigned long cmd_mask;
 	struct module *module;
 	char *provider_name;
-- 
1.8.5.6

[PATCH v3 4/7] acpi, nfit: Use bus_dsm_mask for passthru

[Jerry Hoemann]

Populate bus_dsm_mask and use it to filter dsm calls that user can
make through the pass thru interface.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/acpi/nfit/core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index b46fca2..971002b 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -253,6 +253,8 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
 		cmd_name = nvdimm_bus_cmd_name(cmd);
 		cmd_mask = nd_desc->cmd_mask;
 		dsm_mask = cmd_mask;
+		if (cmd == ND_CMD_CALL)
+			dsm_mask = nd_desc->bus_dsm_mask;
 		desc = nd_cmd_bus_desc(cmd);
 		uuid = to_nfit_uuid(NFIT_DEV_BUS);
 		handle = adev->handle;
@@ -1624,6 +1626,9 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
 		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);
+	for (i = 0; i < ND_CMD_CALL; i++)
+		if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
+			set_bit(i, &nd_desc->bus_dsm_mask);
 }
 
 static ssize_t range_index_show(struct device *dev,
-- 
1.8.5.6

[PATCH v3 5/7] acpi, nfit: Show bus_dsm_mask in sysfs

[Jerry Hoemann]

Display bus_dsm_mask in sysfs as /sys/bus/nd/devices/ndbusX/nfit/dsm_mask.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/acpi/nfit/core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 971002b..7d2f1a0 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -929,6 +929,17 @@ static int nfit_mem_init(struct acpi_nfit_desc *acpi_desc)
 	return 0;
 }
 
+static ssize_t bus_dsm_mask_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct nvdimm_bus *nvdimm_bus = to_nvdimm_bus(dev);
+	struct nvdimm_bus_descriptor *nd_desc = to_nd_desc(nvdimm_bus);
+
+	return sprintf(buf, "%#lx\n", nd_desc->bus_dsm_mask);
+}
+static struct device_attribute dev_attr_bus_dsm_mask =
+		__ATTR(dsm_mask, 0444, bus_dsm_mask_show, NULL);
+
 static ssize_t revision_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
@@ -1065,6 +1076,7 @@ static umode_t nfit_visible(struct kobject *kobj, struct attribute *a, int n)
 	&dev_attr_revision.attr,
 	&dev_attr_scrub.attr,
 	&dev_attr_hw_error_scrub.attr,
+	&dev_attr_bus_dsm_mask.attr,
 	NULL,
 };
 
-- 
1.8.5.6

[PATCH v3 6/7] libnvdimm: New ACPI 6.2 DSM functions

[Jerry Hoemann]

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

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 include/uapi/linux/ndctl.h | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index e23c37f..e15768f 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,
 
-- 
1.8.5.6

[PATCH v3 7/7] acpi, nfit: override mask

[Jerry Hoemann]

Have module parameter override_dsm_mask override the dsm_mask for
root calls like it does for non-root dsm calls.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/acpi/nfit/core.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 7d2f1a0..87acaf2 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1627,6 +1627,7 @@ 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;
+	unsigned long dsm_mask;
 	int i;
 
 	nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
@@ -1638,7 +1639,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
 		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);
-	for (i = 0; i < ND_CMD_CALL; i++)
+
+	dsm_mask = 0x3bf;
+	if (override_dsm_mask)
+		dsm_mask = override_dsm_mask;
+	for_each_set_bit(i, &dsm_mask, BITS_PER_LONG)
 		if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
 			set_bit(i, &nd_desc->bus_dsm_mask);
 }
-- 
1.8.5.6

Re: [PATCH v3 7/7] acpi, nfit: override mask

[Dan Williams]

On Thu, Jun 29, 2017 at 9:56 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> Have module parameter override_dsm_mask override the dsm_mask for
> root calls like it does for non-root dsm calls.
>
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  drivers/acpi/nfit/core.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 7d2f1a0..87acaf2 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -1627,6 +1627,7 @@ 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;
> +       unsigned long dsm_mask;
>         int i;
>
>         nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
> @@ -1638,7 +1639,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
>                 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);
> -       for (i = 0; i < ND_CMD_CALL; i++)
> +
> +       dsm_mask = 0x3bf;
> +       if (override_dsm_mask)
> +               dsm_mask = override_dsm_mask;
> +       for_each_set_bit(i, &dsm_mask, BITS_PER_LONG)
>                 if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
>                         set_bit(i, &nd_desc->bus_dsm_mask);
>  }

I don't think we need this patch. 'override_dsm_mask' is there to make
it easier for vendor-specific DSM testing and debug for DIMM-level
DSMs. The root bus is not vendor specific and the command set is not
evolving at the same rate we are seeing change at DIMM-level DSMs.

Re: [PATCH v3 3/7] libnvdimm: Add bus level dsm mask.

[Dan Williams]

On Thu, Jun 29, 2017 at 9:56 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> Add a bus level dsm_mask to nvdimm_bus_descriptor to allow the passthru
> calling mechanism to specify a different mask from the cmd_mask.
>
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  include/linux/libnvdimm.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
> index 6c80701..f8b8f43 100644
> --- a/include/linux/libnvdimm.h
> +++ b/include/linux/libnvdimm.h
> @@ -54,6 +54,7 @@ typedef int (*ndctl_fn)(struct nvdimm_bus_descriptor *nd_desc,
>
>  struct nvdimm_bus_descriptor {
>         const struct attribute_group **attr_groups;
> +       unsigned long bus_dsm_mask;
>         unsigned long cmd_mask;
>         struct module *module;
>         char *provider_name;
> --
> 1.8.5.6
>

This patch can be squashed with the next one, it's ok to touch
drivers/acpi and drivers/nvdimm in the same patch.

Re: [PATCH v3 4/7] acpi, nfit: Use bus_dsm_mask for passthru

[Dan Williams]

On Thu, Jun 29, 2017 at 9:56 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> Populate bus_dsm_mask and use it to filter dsm calls that user can
> make through the pass thru interface.
>
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  drivers/acpi/nfit/core.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index b46fca2..971002b 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -253,6 +253,8 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>                 cmd_name = nvdimm_bus_cmd_name(cmd);
>                 cmd_mask = nd_desc->cmd_mask;
>                 dsm_mask = cmd_mask;
> +               if (cmd == ND_CMD_CALL)
> +                       dsm_mask = nd_desc->bus_dsm_mask;
>                 desc = nd_cmd_bus_desc(cmd);
>                 uuid = to_nfit_uuid(NFIT_DEV_BUS);
>                 handle = adev->handle;
> @@ -1624,6 +1626,9 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
>                 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);
> +       for (i = 0; i < ND_CMD_CALL; i++)
> +               if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
> +                       set_bit(i, &nd_desc->bus_dsm_mask);
>  }

This loop checks for function 6 which is specified as reserved. Lets
explicitly test for the known good function numbers with something
like this:

/* this should be private in drivers/acpi/nfit/nfit.h */
enum nfit_aux_cmds {
        NFIT_CMD_TRANSLATE_SPA = 5,
        NFIT_CMD_ARS_INJECT_SET = 7,
        NFIT_CMD_ARS_INJECT_CLEAR = 8,
        NFIT_CMD_ARS_INJECT_GET = 9,
};

bus_dsm_mask =
        (1 << ND_CMD_ARS_CAP) |
        (1 << ND_CMD_ARS_START) |
        (1 << ND_CMD_ARS_STATUS) |
        (1 << ND_CMD_CLEAR_ERROR) |
        (1 << NFIT_CMD_TRANSLATE_SPA) |
        (1 << NFIT_CMD_ARS_INJECT_SET) |
        (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
        (1 << NFIT_CMD_ARS_INJECT_GET);

for_each_set_bit(i, &bus_dsm_mask...

Re: [PATCH v3 4/7] acpi, nfit: Use bus_dsm_mask for passthru

[Jerry Hoemann]

On Thu, Jun 29, 2017 at 02:35:14PM -0700, Dan Williams wrote:
> On Thu, Jun 29, 2017 at 9:56 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> > Populate bus_dsm_mask and use it to filter dsm calls that user can
> > make through the pass thru interface.
> >
> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> > ---
> >  drivers/acpi/nfit/core.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> > index b46fca2..971002b 100644
> > --- a/drivers/acpi/nfit/core.c
> > +++ b/drivers/acpi/nfit/core.c
> > @@ -253,6 +253,8 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
> >                 cmd_name = nvdimm_bus_cmd_name(cmd);
> >                 cmd_mask = nd_desc->cmd_mask;
> >                 dsm_mask = cmd_mask;
> > +               if (cmd == ND_CMD_CALL)
> > +                       dsm_mask = nd_desc->bus_dsm_mask;
> >                 desc = nd_cmd_bus_desc(cmd);
> >                 uuid = to_nfit_uuid(NFIT_DEV_BUS);
> >                 handle = adev->handle;
> > @@ -1624,6 +1626,9 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
> >                 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);
> > +       for (i = 0; i < ND_CMD_CALL; i++)
> > +               if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
> > +                       set_bit(i, &nd_desc->bus_dsm_mask);
> >  }
> 
> This loop checks for function 6 which is specified as reserved. Lets
> explicitly test for the known good function numbers with something
> like this:
> 
> /* this should be private in drivers/acpi/nfit/nfit.h */
> enum nfit_aux_cmds {
>         NFIT_CMD_TRANSLATE_SPA = 5,
>         NFIT_CMD_ARS_INJECT_SET = 7,
>         NFIT_CMD_ARS_INJECT_CLEAR = 8,
>         NFIT_CMD_ARS_INJECT_GET = 9,
> };
> 
> bus_dsm_mask =
>         (1 << ND_CMD_ARS_CAP) |
>         (1 << ND_CMD_ARS_START) |
>         (1 << ND_CMD_ARS_STATUS) |
>         (1 << ND_CMD_CLEAR_ERROR) |
>         (1 << NFIT_CMD_TRANSLATE_SPA) |
>         (1 << NFIT_CMD_ARS_INJECT_SET) |
>         (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
>         (1 << NFIT_CMD_ARS_INJECT_GET);
> 
> for_each_set_bit(i, &bus_dsm_mask...


  I added the for_each_set_bit check in patch 7 of the series.


-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

Re: [PATCH v3 4/7] acpi, nfit: Use bus_dsm_mask for passthru

[Dan Williams]

On Thu, Jun 29, 2017 at 2:47 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Thu, Jun 29, 2017 at 02:35:14PM -0700, Dan Williams wrote:
>> On Thu, Jun 29, 2017 at 9:56 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> > Populate bus_dsm_mask and use it to filter dsm calls that user can
>> > make through the pass thru interface.
>> >
>> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
>> > ---
>> >  drivers/acpi/nfit/core.c | 5 +++++
>> >  1 file changed, 5 insertions(+)
>> >
>> > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>> > index b46fca2..971002b 100644
>> > --- a/drivers/acpi/nfit/core.c
>> > +++ b/drivers/acpi/nfit/core.c
>> > @@ -253,6 +253,8 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>> >                 cmd_name = nvdimm_bus_cmd_name(cmd);
>> >                 cmd_mask = nd_desc->cmd_mask;
>> >                 dsm_mask = cmd_mask;
>> > +               if (cmd == ND_CMD_CALL)
>> > +                       dsm_mask = nd_desc->bus_dsm_mask;
>> >                 desc = nd_cmd_bus_desc(cmd);
>> >                 uuid = to_nfit_uuid(NFIT_DEV_BUS);
>> >                 handle = adev->handle;
>> > @@ -1624,6 +1626,9 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
>> >                 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);
>> > +       for (i = 0; i < ND_CMD_CALL; i++)
>> > +               if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
>> > +                       set_bit(i, &nd_desc->bus_dsm_mask);
>> >  }
>>
>> This loop checks for function 6 which is specified as reserved. Lets
>> explicitly test for the known good function numbers with something
>> like this:
>>
>> /* this should be private in drivers/acpi/nfit/nfit.h */
>> enum nfit_aux_cmds {
>>         NFIT_CMD_TRANSLATE_SPA = 5,
>>         NFIT_CMD_ARS_INJECT_SET = 7,
>>         NFIT_CMD_ARS_INJECT_CLEAR = 8,
>>         NFIT_CMD_ARS_INJECT_GET = 9,
>> };
>>
>> bus_dsm_mask =
>>         (1 << ND_CMD_ARS_CAP) |
>>         (1 << ND_CMD_ARS_START) |
>>         (1 << ND_CMD_ARS_STATUS) |
>>         (1 << ND_CMD_CLEAR_ERROR) |
>>         (1 << NFIT_CMD_TRANSLATE_SPA) |
>>         (1 << NFIT_CMD_ARS_INJECT_SET) |
>>         (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
>>         (1 << NFIT_CMD_ARS_INJECT_GET);
>>
>> for_each_set_bit(i, &bus_dsm_mask...
>
>
>   I added the for_each_set_bit check in patch 7 of the series.

True, but in a patch series we shouldn't introduce a bug in one patch
and fix it later in the same series. Also, if patch7 goes away we
would need to fold that enabling in here.

Part of trying to parse what 0x3bf meant lead me to this, and I'm
wondering if we should do the same for the other magic vendor dsm_mask
constants, but that's a patch for another time.

Re: [PATCH v3 7/7] acpi, nfit: override mask

[Jerry Hoemann]

On Thu, Jun 29, 2017 at 02:16:17PM -0700, Dan Williams wrote:
> On Thu, Jun 29, 2017 at 9:56 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> > Have module parameter override_dsm_mask override the dsm_mask for
> > root calls like it does for non-root dsm calls.
> >
> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> > ---
> >  drivers/acpi/nfit/core.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> > index 7d2f1a0..87acaf2 100644
> > --- a/drivers/acpi/nfit/core.c
> > +++ b/drivers/acpi/nfit/core.c
> > @@ -1627,6 +1627,7 @@ 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;
> > +       unsigned long dsm_mask;
> >         int i;
> >
> >         nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
> > @@ -1638,7 +1639,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
> >                 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);
> > -       for (i = 0; i < ND_CMD_CALL; i++)
> > +
> > +       dsm_mask = 0x3bf;
> > +       if (override_dsm_mask)
> > +               dsm_mask = override_dsm_mask;
> > +       for_each_set_bit(i, &dsm_mask, BITS_PER_LONG)
> >                 if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
> >                         set_bit(i, &nd_desc->bus_dsm_mask);
> >  }
> 
> I don't think we need this patch. 'override_dsm_mask' is there to make
> it easier for vendor-specific DSM testing and debug for DIMM-level
> DSMs. The root bus is not vendor specific and the command set is not
> evolving at the same rate we are seeing change at DIMM-level DSMs.

Override_dsm_mask is there to allow using old kernels with new
firmware/hardware.

It takes a long time to get even simple changes upstreamed, backported
to distros, released, distributed to customers, installed.

In testing, for months we have had to work around the inability to call
these functions from linux.  A waste of effort.

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

Re: [PATCH v3 7/7] acpi, nfit: override mask

[Dan Williams]

On Thu, Jun 29, 2017 at 3:18 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Thu, Jun 29, 2017 at 02:16:17PM -0700, Dan Williams wrote:
>> On Thu, Jun 29, 2017 at 9:56 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> > Have module parameter override_dsm_mask override the dsm_mask for
>> > root calls like it does for non-root dsm calls.
>> >
>> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
>> > ---
>> >  drivers/acpi/nfit/core.c | 7 ++++++-
>> >  1 file changed, 6 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>> > index 7d2f1a0..87acaf2 100644
>> > --- a/drivers/acpi/nfit/core.c
>> > +++ b/drivers/acpi/nfit/core.c
>> > @@ -1627,6 +1627,7 @@ 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;
>> > +       unsigned long dsm_mask;
>> >         int i;
>> >
>> >         nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
>> > @@ -1638,7 +1639,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
>> >                 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);
>> > -       for (i = 0; i < ND_CMD_CALL; i++)
>> > +
>> > +       dsm_mask = 0x3bf;
>> > +       if (override_dsm_mask)
>> > +               dsm_mask = override_dsm_mask;
>> > +       for_each_set_bit(i, &dsm_mask, BITS_PER_LONG)
>> >                 if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
>> >                         set_bit(i, &nd_desc->bus_dsm_mask);
>> >  }
>>
>> I don't think we need this patch. 'override_dsm_mask' is there to make
>> it easier for vendor-specific DSM testing and debug for DIMM-level
>> DSMs. The root bus is not vendor specific and the command set is not
>> evolving at the same rate we are seeing change at DIMM-level DSMs.
>
> Override_dsm_mask is there to allow using old kernels with new
> firmware/hardware.
>
> It takes a long time to get even simple changes upstreamed, backported
> to distros, released, distributed to customers, installed.
>
> In testing, for months we have had to work around the inability to call
> these functions from linux.  A waste of effort.

I bought this argument for the DIMM level DSMs, but not the bus. Those
move much more slowly precisely because they always need to go through
the ACPI Speficication Working Group. DIMM level DSMs can change at
will and I saw the need for the kernel to be able to keep up with that
thrash especially at this early stage. We don't otherwise pre-enable
unknown / future-possible ACPI standard interfaces.

Re: [PATCH v3 4/7] acpi, nfit: Use bus_dsm_mask for passthru

[Jerry Hoemann]

On Thu, Jun 29, 2017 at 02:55:55PM -0700, Dan Williams wrote:
> On Thu, Jun 29, 2017 at 2:47 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> > On Thu, Jun 29, 2017 at 02:35:14PM -0700, Dan Williams wrote:
> >> On Thu, Jun 29, 2017 at 9:56 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> >> > Populate bus_dsm_mask and use it to filter dsm calls that user can
> >> > make through the pass thru interface.
> >> >
> >> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> >> > ---
> >> >  drivers/acpi/nfit/core.c | 5 +++++
> >> >  1 file changed, 5 insertions(+)
> >> >
> >> > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> >> > index b46fca2..971002b 100644
> >> > --- a/drivers/acpi/nfit/core.c
> >> > +++ b/drivers/acpi/nfit/core.c
> >> > @@ -253,6 +253,8 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
> >> >                 cmd_name = nvdimm_bus_cmd_name(cmd);
> >> >                 cmd_mask = nd_desc->cmd_mask;
> >> >                 dsm_mask = cmd_mask;
> >> > +               if (cmd == ND_CMD_CALL)
> >> > +                       dsm_mask = nd_desc->bus_dsm_mask;
> >> >                 desc = nd_cmd_bus_desc(cmd);
> >> >                 uuid = to_nfit_uuid(NFIT_DEV_BUS);
> >> >                 handle = adev->handle;
> >> > @@ -1624,6 +1626,9 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
> >> >                 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);
> >> > +       for (i = 0; i < ND_CMD_CALL; i++)
> >> > +               if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
> >> > +                       set_bit(i, &nd_desc->bus_dsm_mask);
> >> >  }
> >>
> >> This loop checks for function 6 which is specified as reserved. Lets
> >> explicitly test for the known good function numbers with something
> >> like this:
> >>
> >> /* this should be private in drivers/acpi/nfit/nfit.h */
> >> enum nfit_aux_cmds {
> >>         NFIT_CMD_TRANSLATE_SPA = 5,
> >>         NFIT_CMD_ARS_INJECT_SET = 7,
> >>         NFIT_CMD_ARS_INJECT_CLEAR = 8,
> >>         NFIT_CMD_ARS_INJECT_GET = 9,
> >> };
> >>
> >> bus_dsm_mask =
> >>         (1 << ND_CMD_ARS_CAP) |
> >>         (1 << ND_CMD_ARS_START) |
> >>         (1 << ND_CMD_ARS_STATUS) |
> >>         (1 << ND_CMD_CLEAR_ERROR) |
> >>         (1 << NFIT_CMD_TRANSLATE_SPA) |
> >>         (1 << NFIT_CMD_ARS_INJECT_SET) |
> >>         (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
> >>         (1 << NFIT_CMD_ARS_INJECT_GET);
> >>
> >> for_each_set_bit(i, &bus_dsm_mask...
> >
> >
> >   I added the for_each_set_bit check in patch 7 of the series.
> 
> True, but in a patch series we shouldn't introduce a bug in one patch
> and fix it later in the same series. Also, if patch7 goes away we
> would need to fold that enabling in here.
> 

A bug?   What bad thing happens?


-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

Re: [PATCH v3 4/7] acpi, nfit: Use bus_dsm_mask for passthru

[Dan Williams]

On Thu, Jun 29, 2017 at 4:26 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Thu, Jun 29, 2017 at 02:55:55PM -0700, Dan Williams wrote:
>> On Thu, Jun 29, 2017 at 2:47 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> > On Thu, Jun 29, 2017 at 02:35:14PM -0700, Dan Williams wrote:
>> >> On Thu, Jun 29, 2017 at 9:56 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> >> > Populate bus_dsm_mask and use it to filter dsm calls that user can
>> >> > make through the pass thru interface.
>> >> >
>> >> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
>> >> > ---
>> >> >  drivers/acpi/nfit/core.c | 5 +++++
>> >> >  1 file changed, 5 insertions(+)
>> >> >
>> >> > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>> >> > index b46fca2..971002b 100644
>> >> > --- a/drivers/acpi/nfit/core.c
>> >> > +++ b/drivers/acpi/nfit/core.c
>> >> > @@ -253,6 +253,8 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>> >> >                 cmd_name = nvdimm_bus_cmd_name(cmd);
>> >> >                 cmd_mask = nd_desc->cmd_mask;
>> >> >                 dsm_mask = cmd_mask;
>> >> > +               if (cmd == ND_CMD_CALL)
>> >> > +                       dsm_mask = nd_desc->bus_dsm_mask;
>> >> >                 desc = nd_cmd_bus_desc(cmd);
>> >> >                 uuid = to_nfit_uuid(NFIT_DEV_BUS);
>> >> >                 handle = adev->handle;
>> >> > @@ -1624,6 +1626,9 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc)
>> >> >                 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);
>> >> > +       for (i = 0; i < ND_CMD_CALL; i++)
>> >> > +               if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
>> >> > +                       set_bit(i, &nd_desc->bus_dsm_mask);
>> >> >  }
>> >>
>> >> This loop checks for function 6 which is specified as reserved. Lets
>> >> explicitly test for the known good function numbers with something
>> >> like this:
>> >>
>> >> /* this should be private in drivers/acpi/nfit/nfit.h */
>> >> enum nfit_aux_cmds {
>> >>         NFIT_CMD_TRANSLATE_SPA = 5,
>> >>         NFIT_CMD_ARS_INJECT_SET = 7,
>> >>         NFIT_CMD_ARS_INJECT_CLEAR = 8,
>> >>         NFIT_CMD_ARS_INJECT_GET = 9,
>> >> };
>> >>
>> >> bus_dsm_mask =
>> >>         (1 << ND_CMD_ARS_CAP) |
>> >>         (1 << ND_CMD_ARS_START) |
>> >>         (1 << ND_CMD_ARS_STATUS) |
>> >>         (1 << ND_CMD_CLEAR_ERROR) |
>> >>         (1 << NFIT_CMD_TRANSLATE_SPA) |
>> >>         (1 << NFIT_CMD_ARS_INJECT_SET) |
>> >>         (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
>> >>         (1 << NFIT_CMD_ARS_INJECT_GET);
>> >>
>> >> for_each_set_bit(i, &bus_dsm_mask...
>> >
>> >
>> >   I added the for_each_set_bit check in patch 7 of the series.
>>
>> True, but in a patch series we shouldn't introduce a bug in one patch
>> and fix it later in the same series. Also, if patch7 goes away we
>> would need to fold that enabling in here.
>>
>
> A bug?   What bad thing happens?
>

Ok, yes, not a bug, but in general terms if the review feedback is
"patch4 has an issue" the answer (usually) can't be "but I I fix it in
patch7". That instead means the fix in patch7 needs to move to patch4.
So this is purely a Linux patch-review / process comment.