[PATCH v2 0/3] apci, nfit: DSM improvements

[PATCH v2 0/3] apci, nfit: DSM improvements

[Linda Knippers]

The first two patches in this series are motivated by feedback
from distribution developers, test engineers, and management tool
developers.  We need the ability to test and support Linux and
management tools on systems that support more than one DSM family
for NVDIMM-N.

We also need the ability to test and support new functions without
requiring users to update their kernel.  These changes will also
facilate development as we move toward DSM standardization.
The ability to restrict DSM functions to the currently documented
set is still in place.

The third patch cleans up a cosmetic/modinfo parsing issue with one 
of the existing module parameters.

Changes in v2:
- Switched from allowing all functions advertised by the
  firmware through function 0 to an override module parameter.

Linda Knippers (3):
  Allow override of built-in bitmasks for NVDIMM DSMs
  Allow specifying a default DSM family
  Remove unnecessary newline

 drivers/acpi/nfit/core.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

-- 
1.8.3.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 1/3] Allow override of built-in bitmasks for NVDIMM DSMs

[Linda Knippers]

As it is today, we can't enable or test new NVDIMM management functions
provided by new firmware and tools without changing the kernel.  We also
can't prevent documented DSM functions from being called in the
case of buggy firmware.  This patch provides a module parameter that
overrides the DSM function mask that is built into the kernel.

If the "disable_vendor_specific" module parameter is also used
we ignore the new parameter. 

Signed-off-by: Linda Knippers <linda.knippers@hpe.com>
---
 drivers/acpi/nfit/core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 662036b..97d42ff 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -51,6 +51,10 @@
 MODULE_PARM_DESC(disable_vendor_specific,
 		"Limit commands to the publicly specified set\n");
 
+static unsigned long override_dsm_mask;
+module_param(override_dsm_mask, ulong, S_IRUGO);
+MODULE_PARM_DESC(override_dsm_mask, "Bitmask of allowed NVDIMM DSM functions");
+
 LIST_HEAD(acpi_descs);
 DEFINE_MUTEX(acpi_desc_lock);
 
@@ -1402,7 +1406,9 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
 
 	/* limit the supported commands to those that are publicly documented */
 	nfit_mem->family = i;
-	if (nfit_mem->family == NVDIMM_FAMILY_INTEL) {
+	if (override_dsm_mask && !disable_vendor_specific)
+		dsm_mask = override_dsm_mask;
+	else if (nfit_mem->family == NVDIMM_FAMILY_INTEL) {
 		dsm_mask = 0x3fe;
 		if (disable_vendor_specific)
 			dsm_mask &= ~(1 << ND_CMD_VENDOR);
-- 
1.8.3.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 2/3] Allow specifying a default DSM family

[Linda Knippers]

Provide the ability to request a default DSM family. If it is not
supported, then fall back to the normal discovery order.

This is helpful for testing platforms that support multiple DSM families.
It will also allow administrators to request the DSM family that their
management tools support, which may not be the first one found using
the current discovery order.

Signed-off-by: Linda Knippers <linda.knippers@hpe.com>
---
 drivers/acpi/nfit/core.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 97d42ff..78c46a7 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -55,6 +55,11 @@
 module_param(override_dsm_mask, ulong, S_IRUGO);
 MODULE_PARM_DESC(override_dsm_mask, "Bitmask of allowed NVDIMM DSM functions");
 
+static int default_dsm_family = -1;
+module_param(default_dsm_family, int, S_IRUGO);
+MODULE_PARM_DESC(default_dsm_family,
+		"Try this DSM type first when identifying NVDIMM family");
+
 LIST_HEAD(acpi_descs);
 DEFINE_MUTEX(acpi_desc_lock);
 
@@ -1371,7 +1376,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
 	struct device *dev = acpi_desc->dev;
 	unsigned long dsm_mask;
 	const u8 *uuid;
-	int i;
+	int i = -1;
 
 	/* nfit test assumes 1:1 relationship between commands and dsms */
 	nfit_mem->dsm_mask = acpi_desc->dimm_cmd_force_en;
@@ -1399,10 +1404,23 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
 	 * Until standardization materializes we need to consider 4
 	 * different command sets.  Note, that checking for function0 (bit0)
 	 * tells us if any commands are reachable through this uuid.
+	 * First check for a requested default.
 	 */
-	for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
-		if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
-			break;
+	if (default_dsm_family >= NVDIMM_FAMILY_INTEL &&
+			default_dsm_family <= NVDIMM_FAMILY_MSFT) {
+		if (acpi_check_dsm(adev_dimm->handle,
+				to_nfit_uuid(default_dsm_family), 1, 1))
+			i = default_dsm_family;
+		else
+			dev_dbg(dev, "default_dsm_family %d not supported\n",
+				default_dsm_family);
+	}
+	if (i == -1) {
+		for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
+			if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i),
+					1, 1))
+				break;
+	}
 
 	/* limit the supported commands to those that are publicly documented */
 	nfit_mem->family = i;
-- 
1.8.3.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 3/3] Remove unnecessary newline

[Linda Knippers]

The newline in MODULE_PARM_DESC causes modinfo to print the parameter
data type on a separate line, which is different from all the other
module parameters and could potentially cause a problem for someone
parsing the output of modinfo.

Signed-off-by: Linda Knippers <linda.knippers@hpe.com>
---
 drivers/acpi/nfit/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 78c46a7..7a4c549 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -49,7 +49,7 @@
 static bool disable_vendor_specific;
 module_param(disable_vendor_specific, bool, S_IRUGO);
 MODULE_PARM_DESC(disable_vendor_specific,
-		"Limit commands to the publicly specified set\n");
+		"Limit commands to the publicly specified set");
 
 static unsigned long override_dsm_mask;
 module_param(override_dsm_mask, ulong, S_IRUGO);
-- 
1.8.3.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/3] Allow specifying a default DSM family

[Dan Williams]

On Mon, Mar 6, 2017 at 4:25 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
> Provide the ability to request a default DSM family. If it is not
> supported, then fall back to the normal discovery order.
>
> This is helpful for testing platforms that support multiple DSM families.
> It will also allow administrators to request the DSM family that their
> management tools support, which may not be the first one found using
> the current discovery order.
>
> Signed-off-by: Linda Knippers <linda.knippers@hpe.com>
> ---
>  drivers/acpi/nfit/core.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 97d42ff..78c46a7 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -55,6 +55,11 @@
>  module_param(override_dsm_mask, ulong, S_IRUGO);
>  MODULE_PARM_DESC(override_dsm_mask, "Bitmask of allowed NVDIMM DSM functions");
>
> +static int default_dsm_family = -1;
> +module_param(default_dsm_family, int, S_IRUGO);
> +MODULE_PARM_DESC(default_dsm_family,
> +               "Try this DSM type first when identifying NVDIMM family");
> +
>  LIST_HEAD(acpi_descs);
>  DEFINE_MUTEX(acpi_desc_lock);
>
> @@ -1371,7 +1376,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>         struct device *dev = acpi_desc->dev;
>         unsigned long dsm_mask;
>         const u8 *uuid;
> -       int i;
> +       int i = -1;
>
>         /* nfit test assumes 1:1 relationship between commands and dsms */
>         nfit_mem->dsm_mask = acpi_desc->dimm_cmd_force_en;
> @@ -1399,10 +1404,23 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>          * Until standardization materializes we need to consider 4
>          * different command sets.  Note, that checking for function0 (bit0)
>          * tells us if any commands are reachable through this uuid.
> +        * First check for a requested default.
>          */
> -       for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
> -               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
> -                       break;
> +       if (default_dsm_family >= NVDIMM_FAMILY_INTEL &&
> +                       default_dsm_family <= NVDIMM_FAMILY_MSFT) {
> +               if (acpi_check_dsm(adev_dimm->handle,
> +                               to_nfit_uuid(default_dsm_family), 1, 1))
> +                       i = default_dsm_family;
> +               else
> +                       dev_dbg(dev, "default_dsm_family %d not supported\n",
> +                               default_dsm_family);
> +       }
> +       if (i == -1) {
> +               for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
> +                       if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i),
> +                                       1, 1))
> +                               break;
> +       }

I think we can make this simpler and more deterministic with a "force"
option? Something like:

@@ -1397,8 +1397,12 @@ static int acpi_nfit_add_dimm(struct
acpi_nfit_desc *acpi_desc,
         * tells us if any commands are reachable through this uuid.
         */
        for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
-               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
-                       break;
+               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1)) {
+                       if (force_dsm_family < 0)
+                               break;
+                       else if (i == force_dsm_family)
+                               break;
+               }

        /* limit the supported commands to those that are publicly documented */
        nfit_mem->family = i;

...because the user specifying the override should know ahead of time
if that family is available, and we should fail the detection
otherwise.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/3] Allow specifying a default DSM family

[Linda Knippers]

On 03/06/2017 07:39 PM, Dan Williams wrote:
> On Mon, Mar 6, 2017 at 4:25 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
>> Provide the ability to request a default DSM family. If it is not
>> supported, then fall back to the normal discovery order.
>>
>> This is helpful for testing platforms that support multiple DSM families.
>> It will also allow administrators to request the DSM family that their
>> management tools support, which may not be the first one found using
>> the current discovery order.
>>
>> Signed-off-by: Linda Knippers <linda.knippers@hpe.com>
>> ---
>>  drivers/acpi/nfit/core.c | 26 ++++++++++++++++++++++----
>>  1 file changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>> index 97d42ff..78c46a7 100644
>> --- a/drivers/acpi/nfit/core.c
>> +++ b/drivers/acpi/nfit/core.c
>> @@ -55,6 +55,11 @@
>>  module_param(override_dsm_mask, ulong, S_IRUGO);
>>  MODULE_PARM_DESC(override_dsm_mask, "Bitmask of allowed NVDIMM DSM functions");
>>
>> +static int default_dsm_family = -1;
>> +module_param(default_dsm_family, int, S_IRUGO);
>> +MODULE_PARM_DESC(default_dsm_family,
>> +               "Try this DSM type first when identifying NVDIMM family");
>> +
>>  LIST_HEAD(acpi_descs);
>>  DEFINE_MUTEX(acpi_desc_lock);
>>
>> @@ -1371,7 +1376,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>>         struct device *dev = acpi_desc->dev;
>>         unsigned long dsm_mask;
>>         const u8 *uuid;
>> -       int i;
>> +       int i = -1;
>>
>>         /* nfit test assumes 1:1 relationship between commands and dsms */
>>         nfit_mem->dsm_mask = acpi_desc->dimm_cmd_force_en;
>> @@ -1399,10 +1404,23 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>>          * Until standardization materializes we need to consider 4
>>          * different command sets.  Note, that checking for function0 (bit0)
>>          * tells us if any commands are reachable through this uuid.
>> +        * First check for a requested default.
>>          */
>> -       for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
>> -               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
>> -                       break;
>> +       if (default_dsm_family >= NVDIMM_FAMILY_INTEL &&
>> +                       default_dsm_family <= NVDIMM_FAMILY_MSFT) {
>> +               if (acpi_check_dsm(adev_dimm->handle,
>> +                               to_nfit_uuid(default_dsm_family), 1, 1))
>> +                       i = default_dsm_family;
>> +               else
>> +                       dev_dbg(dev, "default_dsm_family %d not supported\n",
>> +                               default_dsm_family);
>> +       }
>> +       if (i == -1) {
>> +               for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
>> +                       if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i),
>> +                                       1, 1))
>> +                               break;
>> +       }
> 
> I think we can make this simpler and more deterministic with a "force"
> option? Something like:
> 
> @@ -1397,8 +1397,12 @@ static int acpi_nfit_add_dimm(struct
> acpi_nfit_desc *acpi_desc,
>          * tells us if any commands are reachable through this uuid.
>          */
>         for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
> -               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
> -                       break;
> +               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1)) {
> +                       if (force_dsm_family < 0)
> +                               break;
> +                       else if (i == force_dsm_family)
> +                               break;
> +               }
> 
>         /* limit the supported commands to those that are publicly documented */
>         nfit_mem->family = i;
> 
> ...because the user specifying the override should know ahead of time
> if that family is available, and we should fail the detection
> otherwise.

It's more complicated when you have a mixture of NVDIMM types, where not all
NVDIMMs support the same families.  For example, you could have an Intel
NVDIMM in the same system as an HPE NVDIMM-N.  The reason I called it
"default" is because it should be the default for all devices that support
that family but I didn't want other types of NVDIMMs to end up with
no family at all.

-- ljk

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/3] Allow specifying a default DSM family

[Dan Williams]

On Mon, Mar 6, 2017 at 5:33 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
> On 03/06/2017 07:39 PM, Dan Williams wrote:
>> On Mon, Mar 6, 2017 at 4:25 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>> Provide the ability to request a default DSM family. If it is not
>>> supported, then fall back to the normal discovery order.
>>>
>>> This is helpful for testing platforms that support multiple DSM families.
>>> It will also allow administrators to request the DSM family that their
>>> management tools support, which may not be the first one found using
>>> the current discovery order.
>>>
>>> Signed-off-by: Linda Knippers <linda.knippers@hpe.com>
>>> ---
>>>  drivers/acpi/nfit/core.c | 26 ++++++++++++++++++++++----
>>>  1 file changed, 22 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>>> index 97d42ff..78c46a7 100644
>>> --- a/drivers/acpi/nfit/core.c
>>> +++ b/drivers/acpi/nfit/core.c
>>> @@ -55,6 +55,11 @@
>>>  module_param(override_dsm_mask, ulong, S_IRUGO);
>>>  MODULE_PARM_DESC(override_dsm_mask, "Bitmask of allowed NVDIMM DSM functions");
>>>
>>> +static int default_dsm_family = -1;
>>> +module_param(default_dsm_family, int, S_IRUGO);
>>> +MODULE_PARM_DESC(default_dsm_family,
>>> +               "Try this DSM type first when identifying NVDIMM family");
>>> +
>>>  LIST_HEAD(acpi_descs);
>>>  DEFINE_MUTEX(acpi_desc_lock);
>>>
>>> @@ -1371,7 +1376,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>>>         struct device *dev = acpi_desc->dev;
>>>         unsigned long dsm_mask;
>>>         const u8 *uuid;
>>> -       int i;
>>> +       int i = -1;
>>>
>>>         /* nfit test assumes 1:1 relationship between commands and dsms */
>>>         nfit_mem->dsm_mask = acpi_desc->dimm_cmd_force_en;
>>> @@ -1399,10 +1404,23 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>>>          * Until standardization materializes we need to consider 4
>>>          * different command sets.  Note, that checking for function0 (bit0)
>>>          * tells us if any commands are reachable through this uuid.
>>> +        * First check for a requested default.
>>>          */
>>> -       for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
>>> -               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
>>> -                       break;
>>> +       if (default_dsm_family >= NVDIMM_FAMILY_INTEL &&
>>> +                       default_dsm_family <= NVDIMM_FAMILY_MSFT) {
>>> +               if (acpi_check_dsm(adev_dimm->handle,
>>> +                               to_nfit_uuid(default_dsm_family), 1, 1))
>>> +                       i = default_dsm_family;
>>> +               else
>>> +                       dev_dbg(dev, "default_dsm_family %d not supported\n",
>>> +                               default_dsm_family);
>>> +       }
>>> +       if (i == -1) {
>>> +               for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
>>> +                       if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i),
>>> +                                       1, 1))
>>> +                               break;
>>> +       }
>>
>> I think we can make this simpler and more deterministic with a "force"
>> option? Something like:
>>
>> @@ -1397,8 +1397,12 @@ static int acpi_nfit_add_dimm(struct
>> acpi_nfit_desc *acpi_desc,
>>          * tells us if any commands are reachable through this uuid.
>>          */
>>         for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
>> -               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
>> -                       break;
>> +               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1)) {
>> +                       if (force_dsm_family < 0)
>> +                               break;
>> +                       else if (i == force_dsm_family)
>> +                               break;
>> +               }
>>
>>         /* limit the supported commands to those that are publicly documented */
>>         nfit_mem->family = i;
>>
>> ...because the user specifying the override should know ahead of time
>> if that family is available, and we should fail the detection
>> otherwise.
>
> It's more complicated when you have a mixture of NVDIMM types, where not all
> NVDIMMs support the same families.  For example, you could have an Intel
> NVDIMM in the same system as an HPE NVDIMM-N.  The reason I called it
> "default" is because it should be the default for all devices that support
> that family but I didn't want other types of NVDIMMs to end up with
> no family at all.
>

Ok, but I still think we can make the logic a bit simpler. I'm
reacting to the new "if (i == -1)" branch and 2 locations where we
call "acpi_check_dsm()". How about this to centralize everything?

@@ -1397,11 +1397,15 @@ static int acpi_nfit_add_dimm(struct
acpi_nfit_desc *acpi_desc,
         * tells us if any commands are reachable through this uuid.
         */
        for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
-               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
-                       break;
+               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1)) {
+                       if (family < 0 || i == default_dsm_family) {
+                               family = i;
+                               break;
+                       }
+               }

        /* limit the supported commands to those that are publicly documented */
-       nfit_mem->family = i;
+       nfit_mem->family = family;
        if (nfit_mem->family == NVDIMM_FAMILY_INTEL) {
                dsm_mask = 0x3fe;
                if (disable_vendor_specific)
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/3] Allow specifying a default DSM family

[Linda Knippers]

On 03/06/2017 08:56 PM, Dan Williams wrote:
> On Mon, Mar 6, 2017 at 5:33 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
>> On 03/06/2017 07:39 PM, Dan Williams wrote:
>>> On Mon, Mar 6, 2017 at 4:25 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>>> Provide the ability to request a default DSM family. If it is not
>>>> supported, then fall back to the normal discovery order.
>>>>
>>>> This is helpful for testing platforms that support multiple DSM families.
>>>> It will also allow administrators to request the DSM family that their
>>>> management tools support, which may not be the first one found using
>>>> the current discovery order.
>>>>
>>>> Signed-off-by: Linda Knippers <linda.knippers@hpe.com>
>>>> ---
>>>>  drivers/acpi/nfit/core.c | 26 ++++++++++++++++++++++----
>>>>  1 file changed, 22 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>>>> index 97d42ff..78c46a7 100644
>>>> --- a/drivers/acpi/nfit/core.c
>>>> +++ b/drivers/acpi/nfit/core.c
>>>> @@ -55,6 +55,11 @@
>>>>  module_param(override_dsm_mask, ulong, S_IRUGO);
>>>>  MODULE_PARM_DESC(override_dsm_mask, "Bitmask of allowed NVDIMM DSM functions");
>>>>
>>>> +static int default_dsm_family = -1;
>>>> +module_param(default_dsm_family, int, S_IRUGO);
>>>> +MODULE_PARM_DESC(default_dsm_family,
>>>> +               "Try this DSM type first when identifying NVDIMM family");
>>>> +
>>>>  LIST_HEAD(acpi_descs);
>>>>  DEFINE_MUTEX(acpi_desc_lock);
>>>>
>>>> @@ -1371,7 +1376,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>>>>         struct device *dev = acpi_desc->dev;
>>>>         unsigned long dsm_mask;
>>>>         const u8 *uuid;
>>>> -       int i;
>>>> +       int i = -1;
>>>>
>>>>         /* nfit test assumes 1:1 relationship between commands and dsms */
>>>>         nfit_mem->dsm_mask = acpi_desc->dimm_cmd_force_en;
>>>> @@ -1399,10 +1404,23 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>>>>          * Until standardization materializes we need to consider 4
>>>>          * different command sets.  Note, that checking for function0 (bit0)
>>>>          * tells us if any commands are reachable through this uuid.
>>>> +        * First check for a requested default.
>>>>          */
>>>> -       for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
>>>> -               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
>>>> -                       break;
>>>> +       if (default_dsm_family >= NVDIMM_FAMILY_INTEL &&
>>>> +                       default_dsm_family <= NVDIMM_FAMILY_MSFT) {
>>>> +               if (acpi_check_dsm(adev_dimm->handle,
>>>> +                               to_nfit_uuid(default_dsm_family), 1, 1))
>>>> +                       i = default_dsm_family;
>>>> +               else
>>>> +                       dev_dbg(dev, "default_dsm_family %d not supported\n",
>>>> +                               default_dsm_family);
>>>> +       }
>>>> +       if (i == -1) {
>>>> +               for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
>>>> +                       if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i),
>>>> +                                       1, 1))
>>>> +                               break;
>>>> +       }
>>>
>>> I think we can make this simpler and more deterministic with a "force"
>>> option? Something like:
>>>
>>> @@ -1397,8 +1397,12 @@ static int acpi_nfit_add_dimm(struct
>>> acpi_nfit_desc *acpi_desc,
>>>          * tells us if any commands are reachable through this uuid.
>>>          */
>>>         for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
>>> -               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
>>> -                       break;
>>> +               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1)) {
>>> +                       if (force_dsm_family < 0)
>>> +                               break;
>>> +                       else if (i == force_dsm_family)
>>> +                               break;
>>> +               }
>>>
>>>         /* limit the supported commands to those that are publicly documented */
>>>         nfit_mem->family = i;
>>>
>>> ...because the user specifying the override should know ahead of time
>>> if that family is available, and we should fail the detection
>>> otherwise.
>>
>> It's more complicated when you have a mixture of NVDIMM types, where not all
>> NVDIMMs support the same families.  For example, you could have an Intel
>> NVDIMM in the same system as an HPE NVDIMM-N.  The reason I called it
>> "default" is because it should be the default for all devices that support
>> that family but I didn't want other types of NVDIMMs to end up with
>> no family at all.
>>
> 
> Ok, but I still think we can make the logic a bit simpler. I'm
> reacting to the new "if (i == -1)" branch and 2 locations where we
> call "acpi_check_dsm()". How about this to centralize everything?
> 
> @@ -1397,11 +1397,15 @@ static int acpi_nfit_add_dimm(struct
> acpi_nfit_desc *acpi_desc,
>          * tells us if any commands are reachable through this uuid.
>          */
>         for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
> -               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
> -                       break;
> +               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1)) {
> +                       if (family < 0 || i == default_dsm_family) {
> +                               family = i;
> +                               break;
> +                       }
> +               }
> 
>         /* limit the supported commands to those that are publicly documented */
> -       nfit_mem->family = i;
> +       nfit_mem->family = family;
>         if (nfit_mem->family == NVDIMM_FAMILY_INTEL) {
>                 dsm_mask = 0x3fe;
>                 if (disable_vendor_specific)
> 

I think that will work.  I'll test it in the morning.

Thanks for the feedback. I wasn't crazy about the code myself.

-- ljk
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/3] Allow override of built-in bitmasks for NVDIMM DSMs

[Johannes Thumshirn]

On 03/07/2017 01:25 AM, Linda Knippers wrote:
> As it is today, we can't enable or test new NVDIMM management functions
> provided by new firmware and tools without changing the kernel.  We also
> can't prevent documented DSM functions from being called in the
> case of buggy firmware.  This patch provides a module parameter that
> overrides the DSM function mask that is built into the kernel.
> 
> If the "disable_vendor_specific" module parameter is also used
> we ignore the new parameter. 
> 
> Signed-off-by: Linda Knippers <linda.knippers@hpe.com>
> ---

Looks good to me,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>


-- 
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
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 3/3] Remove unnecessary newline

[Johannes Thumshirn]

On 03/07/2017 01:25 AM, Linda Knippers wrote:
> The newline in MODULE_PARM_DESC causes modinfo to print the parameter
> data type on a separate line, which is different from all the other
> module parameters and could potentially cause a problem for someone
> parsing the output of modinfo.
> 
> Signed-off-by: Linda Knippers <linda.knippers@hpe.com>
> ---

Thanks,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
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
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/3] Allow specifying a default DSM family

[Linda Knippers]

On 03/06/2017 08:56 PM, Dan Williams wrote:
> On Mon, Mar 6, 2017 at 5:33 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
>> On 03/06/2017 07:39 PM, Dan Williams wrote:
>>> On Mon, Mar 6, 2017 at 4:25 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>>> Provide the ability to request a default DSM family. If it is not
>>>> supported, then fall back to the normal discovery order.
>>>>
>>>> This is helpful for testing platforms that support multiple DSM families.
>>>> It will also allow administrators to request the DSM family that their
>>>> management tools support, which may not be the first one found using
>>>> the current discovery order.
>>>>
>>>> Signed-off-by: Linda Knippers <linda.knippers@hpe.com>
>>>> ---
>>>>  drivers/acpi/nfit/core.c | 26 ++++++++++++++++++++++----
>>>>  1 file changed, 22 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>>>> index 97d42ff..78c46a7 100644
>>>> --- a/drivers/acpi/nfit/core.c
>>>> +++ b/drivers/acpi/nfit/core.c
>>>> @@ -55,6 +55,11 @@
>>>>  module_param(override_dsm_mask, ulong, S_IRUGO);
>>>>  MODULE_PARM_DESC(override_dsm_mask, "Bitmask of allowed NVDIMM DSM functions");
>>>>
>>>> +static int default_dsm_family = -1;
>>>> +module_param(default_dsm_family, int, S_IRUGO);
>>>> +MODULE_PARM_DESC(default_dsm_family,
>>>> +               "Try this DSM type first when identifying NVDIMM family");
>>>> +
>>>>  LIST_HEAD(acpi_descs);
>>>>  DEFINE_MUTEX(acpi_desc_lock);
>>>>
>>>> @@ -1371,7 +1376,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>>>>         struct device *dev = acpi_desc->dev;
>>>>         unsigned long dsm_mask;
>>>>         const u8 *uuid;
>>>> -       int i;
>>>> +       int i = -1;
>>>>
>>>>         /* nfit test assumes 1:1 relationship between commands and dsms */
>>>>         nfit_mem->dsm_mask = acpi_desc->dimm_cmd_force_en;
>>>> @@ -1399,10 +1404,23 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>>>>          * Until standardization materializes we need to consider 4
>>>>          * different command sets.  Note, that checking for function0 (bit0)
>>>>          * tells us if any commands are reachable through this uuid.
>>>> +        * First check for a requested default.
>>>>          */
>>>> -       for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
>>>> -               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
>>>> -                       break;
>>>> +       if (default_dsm_family >= NVDIMM_FAMILY_INTEL &&
>>>> +                       default_dsm_family <= NVDIMM_FAMILY_MSFT) {
>>>> +               if (acpi_check_dsm(adev_dimm->handle,
>>>> +                               to_nfit_uuid(default_dsm_family), 1, 1))
>>>> +                       i = default_dsm_family;
>>>> +               else
>>>> +                       dev_dbg(dev, "default_dsm_family %d not supported\n",
>>>> +                               default_dsm_family);
>>>> +       }
>>>> +       if (i == -1) {
>>>> +               for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
>>>> +                       if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i),
>>>> +                                       1, 1))
>>>> +                               break;
>>>> +       }
>>>
>>> I think we can make this simpler and more deterministic with a "force"
>>> option? Something like:
>>>
>>> @@ -1397,8 +1397,12 @@ static int acpi_nfit_add_dimm(struct
>>> acpi_nfit_desc *acpi_desc,
>>>          * tells us if any commands are reachable through this uuid.
>>>          */
>>>         for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
>>> -               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
>>> -                       break;
>>> +               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1)) {
>>> +                       if (force_dsm_family < 0)
>>> +                               break;
>>> +                       else if (i == force_dsm_family)
>>> +                               break;
>>> +               }
>>>
>>>         /* limit the supported commands to those that are publicly documented */
>>>         nfit_mem->family = i;
>>>
>>> ...because the user specifying the override should know ahead of time
>>> if that family is available, and we should fail the detection
>>> otherwise.
>>
>> It's more complicated when you have a mixture of NVDIMM types, where not all
>> NVDIMMs support the same families.  For example, you could have an Intel
>> NVDIMM in the same system as an HPE NVDIMM-N.  The reason I called it
>> "default" is because it should be the default for all devices that support
>> that family but I didn't want other types of NVDIMMs to end up with
>> no family at all.
>>
> 
> Ok, but I still think we can make the logic a bit simpler. I'm
> reacting to the new "if (i == -1)" branch and 2 locations where we
> call "acpi_check_dsm()". How about this to centralize everything?
> 
> @@ -1397,11 +1397,15 @@ static int acpi_nfit_add_dimm(struct
> acpi_nfit_desc *acpi_desc,
>          * tells us if any commands are reachable through this uuid.
>          */
>         for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
> -               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
> -                       break;
> +               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1)) {
> +                       if (family < 0 || i == default_dsm_family) {
> +                               family = i;
> +                               break;

This actually doesn't work with the break since the for() will break
on the first match, just like the current code.  It works if I remove
the break, but then we cycle through all the families even if the
default_dsm_family parameter isn't used.  Do you care about that?

I think it can be simple or efficient but not both.

-- ljk
> +                       }
> +               }
> 
>         /* limit the supported commands to those that are publicly documented */
> -       nfit_mem->family = i;
> +       nfit_mem->family = family;
>         if (nfit_mem->family == NVDIMM_FAMILY_INTEL) {
>                 dsm_mask = 0x3fe;
>                 if (disable_vendor_specific)
> 

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/3] Allow specifying a default DSM family

[Linda Knippers]

On 03/07/2017 02:00 PM, Linda Knippers wrote:
> On 03/06/2017 08:56 PM, Dan Williams wrote:
>> On Mon, Mar 6, 2017 at 5:33 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>> On 03/06/2017 07:39 PM, Dan Williams wrote:
>>>> On Mon, Mar 6, 2017 at 4:25 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>>>> Provide the ability to request a default DSM family. If it is not
>>>>> supported, then fall back to the normal discovery order.
>>>>>
>>>>> This is helpful for testing platforms that support multiple DSM families.
>>>>> It will also allow administrators to request the DSM family that their
>>>>> management tools support, which may not be the first one found using
>>>>> the current discovery order.
>>>>>
>>>>> Signed-off-by: Linda Knippers <linda.knippers@hpe.com>
>>>>> ---
>>>>>  drivers/acpi/nfit/core.c | 26 ++++++++++++++++++++++----
>>>>>  1 file changed, 22 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>>>>> index 97d42ff..78c46a7 100644
>>>>> --- a/drivers/acpi/nfit/core.c
>>>>> +++ b/drivers/acpi/nfit/core.c
>>>>> @@ -55,6 +55,11 @@
>>>>>  module_param(override_dsm_mask, ulong, S_IRUGO);
>>>>>  MODULE_PARM_DESC(override_dsm_mask, "Bitmask of allowed NVDIMM DSM functions");
>>>>>
>>>>> +static int default_dsm_family = -1;
>>>>> +module_param(default_dsm_family, int, S_IRUGO);
>>>>> +MODULE_PARM_DESC(default_dsm_family,
>>>>> +               "Try this DSM type first when identifying NVDIMM family");
>>>>> +
>>>>>  LIST_HEAD(acpi_descs);
>>>>>  DEFINE_MUTEX(acpi_desc_lock);
>>>>>
>>>>> @@ -1371,7 +1376,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>>>>>         struct device *dev = acpi_desc->dev;
>>>>>         unsigned long dsm_mask;
>>>>>         const u8 *uuid;
>>>>> -       int i;
>>>>> +       int i = -1;
>>>>>
>>>>>         /* nfit test assumes 1:1 relationship between commands and dsms */
>>>>>         nfit_mem->dsm_mask = acpi_desc->dimm_cmd_force_en;
>>>>> @@ -1399,10 +1404,23 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>>>>>          * Until standardization materializes we need to consider 4
>>>>>          * different command sets.  Note, that checking for function0 (bit0)
>>>>>          * tells us if any commands are reachable through this uuid.
>>>>> +        * First check for a requested default.
>>>>>          */
>>>>> -       for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
>>>>> -               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
>>>>> -                       break;
>>>>> +       if (default_dsm_family >= NVDIMM_FAMILY_INTEL &&
>>>>> +                       default_dsm_family <= NVDIMM_FAMILY_MSFT) {
>>>>> +               if (acpi_check_dsm(adev_dimm->handle,
>>>>> +                               to_nfit_uuid(default_dsm_family), 1, 1))
>>>>> +                       i = default_dsm_family;
>>>>> +               else
>>>>> +                       dev_dbg(dev, "default_dsm_family %d not supported\n",
>>>>> +                               default_dsm_family);
>>>>> +       }
>>>>> +       if (i == -1) {
>>>>> +               for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
>>>>> +                       if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i),
>>>>> +                                       1, 1))
>>>>> +                               break;
>>>>> +       }
>>>>
>>>> I think we can make this simpler and more deterministic with a "force"
>>>> option? Something like:
>>>>
>>>> @@ -1397,8 +1397,12 @@ static int acpi_nfit_add_dimm(struct
>>>> acpi_nfit_desc *acpi_desc,
>>>>          * tells us if any commands are reachable through this uuid.
>>>>          */
>>>>         for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
>>>> -               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
>>>> -                       break;
>>>> +               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1)) {
>>>> +                       if (force_dsm_family < 0)
>>>> +                               break;
>>>> +                       else if (i == force_dsm_family)
>>>> +                               break;
>>>> +               }
>>>>
>>>>         /* limit the supported commands to those that are publicly documented */
>>>>         nfit_mem->family = i;
>>>>
>>>> ...because the user specifying the override should know ahead of time
>>>> if that family is available, and we should fail the detection
>>>> otherwise.
>>>
>>> It's more complicated when you have a mixture of NVDIMM types, where not all
>>> NVDIMMs support the same families.  For example, you could have an Intel
>>> NVDIMM in the same system as an HPE NVDIMM-N.  The reason I called it
>>> "default" is because it should be the default for all devices that support
>>> that family but I didn't want other types of NVDIMMs to end up with
>>> no family at all.
>>>
>>
>> Ok, but I still think we can make the logic a bit simpler. I'm
>> reacting to the new "if (i == -1)" branch and 2 locations where we
>> call "acpi_check_dsm()". How about this to centralize everything?
>>
>> @@ -1397,11 +1397,15 @@ static int acpi_nfit_add_dimm(struct
>> acpi_nfit_desc *acpi_desc,
>>          * tells us if any commands are reachable through this uuid.
>>          */
>>         for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
>> -               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
>> -                       break;
>> +               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1)) {
>> +                       if (family < 0 || i == default_dsm_family) {
>> +                               family = i;
>> +                               break;
> 
> This actually doesn't work with the break since the for() will break
> on the first match, just like the current code.  It works if I remove
> the break, but then we cycle through all the families even if the
> default_dsm_family parameter isn't used.  Do you care about that?
> 
> I think it can be simple or efficient but not both.

Something like this does work.

        for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
                if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1)) {
                        if (default_dsm_family != -1) {
                                if (i == default_dsm_family) {
                                        family = i;
                                        break;
                                }
                                if (family < 0)
                                        family = i;
                        }
                        else {
                                family = i;
                                break;
                        }
                }

> 
> -- ljk
>> +                       }
>> +               }
>>
>>         /* limit the supported commands to those that are publicly documented */
>> -       nfit_mem->family = i;
>> +       nfit_mem->family = family;
>>         if (nfit_mem->family == NVDIMM_FAMILY_INTEL) {
>>                 dsm_mask = 0x3fe;
>>                 if (disable_vendor_specific)
>>
> 

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/3] Allow specifying a default DSM family

[Dan Williams]

On Tue, Mar 7, 2017 at 11:00 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
> On 03/06/2017 08:56 PM, Dan Williams wrote:
>> On Mon, Mar 6, 2017 at 5:33 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>> On 03/06/2017 07:39 PM, Dan Williams wrote:
>>>> On Mon, Mar 6, 2017 at 4:25 PM, Linda Knippers <linda.knippers@hpe.com> wrote:
>>>>> Provide the ability to request a default DSM family. If it is not
>>>>> supported, then fall back to the normal discovery order.
>>>>>
>>>>> This is helpful for testing platforms that support multiple DSM families.
>>>>> It will also allow administrators to request the DSM family that their
>>>>> management tools support, which may not be the first one found using
>>>>> the current discovery order.
>>>>>
>>>>> Signed-off-by: Linda Knippers <linda.knippers@hpe.com>
>>>>> ---
>>>>>  drivers/acpi/nfit/core.c | 26 ++++++++++++++++++++++----
>>>>>  1 file changed, 22 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>>>>> index 97d42ff..78c46a7 100644
>>>>> --- a/drivers/acpi/nfit/core.c
>>>>> +++ b/drivers/acpi/nfit/core.c
>>>>> @@ -55,6 +55,11 @@
>>>>>  module_param(override_dsm_mask, ulong, S_IRUGO);
>>>>>  MODULE_PARM_DESC(override_dsm_mask, "Bitmask of allowed NVDIMM DSM functions");
>>>>>
>>>>> +static int default_dsm_family = -1;
>>>>> +module_param(default_dsm_family, int, S_IRUGO);
>>>>> +MODULE_PARM_DESC(default_dsm_family,
>>>>> +               "Try this DSM type first when identifying NVDIMM family");
>>>>> +
>>>>>  LIST_HEAD(acpi_descs);
>>>>>  DEFINE_MUTEX(acpi_desc_lock);
>>>>>
>>>>> @@ -1371,7 +1376,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>>>>>         struct device *dev = acpi_desc->dev;
>>>>>         unsigned long dsm_mask;
>>>>>         const u8 *uuid;
>>>>> -       int i;
>>>>> +       int i = -1;
>>>>>
>>>>>         /* nfit test assumes 1:1 relationship between commands and dsms */
>>>>>         nfit_mem->dsm_mask = acpi_desc->dimm_cmd_force_en;
>>>>> @@ -1399,10 +1404,23 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
>>>>>          * Until standardization materializes we need to consider 4
>>>>>          * different command sets.  Note, that checking for function0 (bit0)
>>>>>          * tells us if any commands are reachable through this uuid.
>>>>> +        * First check for a requested default.
>>>>>          */
>>>>> -       for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
>>>>> -               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
>>>>> -                       break;
>>>>> +       if (default_dsm_family >= NVDIMM_FAMILY_INTEL &&
>>>>> +                       default_dsm_family <= NVDIMM_FAMILY_MSFT) {
>>>>> +               if (acpi_check_dsm(adev_dimm->handle,
>>>>> +                               to_nfit_uuid(default_dsm_family), 1, 1))
>>>>> +                       i = default_dsm_family;
>>>>> +               else
>>>>> +                       dev_dbg(dev, "default_dsm_family %d not supported\n",
>>>>> +                               default_dsm_family);
>>>>> +       }
>>>>> +       if (i == -1) {
>>>>> +               for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
>>>>> +                       if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i),
>>>>> +                                       1, 1))
>>>>> +                               break;
>>>>> +       }
>>>>
>>>> I think we can make this simpler and more deterministic with a "force"
>>>> option? Something like:
>>>>
>>>> @@ -1397,8 +1397,12 @@ static int acpi_nfit_add_dimm(struct
>>>> acpi_nfit_desc *acpi_desc,
>>>>          * tells us if any commands are reachable through this uuid.
>>>>          */
>>>>         for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
>>>> -               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
>>>> -                       break;
>>>> +               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1)) {
>>>> +                       if (force_dsm_family < 0)
>>>> +                               break;
>>>> +                       else if (i == force_dsm_family)
>>>> +                               break;
>>>> +               }
>>>>
>>>>         /* limit the supported commands to those that are publicly documented */
>>>>         nfit_mem->family = i;
>>>>
>>>> ...because the user specifying the override should know ahead of time
>>>> if that family is available, and we should fail the detection
>>>> otherwise.
>>>
>>> It's more complicated when you have a mixture of NVDIMM types, where not all
>>> NVDIMMs support the same families.  For example, you could have an Intel
>>> NVDIMM in the same system as an HPE NVDIMM-N.  The reason I called it
>>> "default" is because it should be the default for all devices that support
>>> that family but I didn't want other types of NVDIMMs to end up with
>>> no family at all.
>>>
>>
>> Ok, but I still think we can make the logic a bit simpler. I'm
>> reacting to the new "if (i == -1)" branch and 2 locations where we
>> call "acpi_check_dsm()". How about this to centralize everything?
>>
>> @@ -1397,11 +1397,15 @@ static int acpi_nfit_add_dimm(struct
>> acpi_nfit_desc *acpi_desc,
>>          * tells us if any commands are reachable through this uuid.
>>          */
>>         for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++)
>> -               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1))
>> -                       break;
>> +               if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1)) {
>> +                       if (family < 0 || i == default_dsm_family) {
>> +                               family = i;
>> +                               break;
>
> This actually doesn't work with the break since the for() will break
> on the first match, just like the current code.  It works if I remove
> the break, but then we cycle through all the families even if the
> default_dsm_family parameter isn't used.  Do you care about that?
>
> I think it can be simple or efficient but not both.

It doesn't need to be efficient, this is a slow path.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 3/3] Remove unnecessary newline

[Linda Knippers]

The newline in MODULE_PARM_DESC causes modinfo to print the parameter
data type on a separate line, which is different from all the other
module parameters and could potentially cause a problem for someone
parsing the output of modinfo.

Signed-off-by: Linda Knippers <linda.knippers@hpe.com>
---
 drivers/acpi/nfit/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 0564cd6..0a51b75 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -49,7 +49,7 @@
 static bool disable_vendor_specific;
 module_param(disable_vendor_specific, bool, S_IRUGO);
 MODULE_PARM_DESC(disable_vendor_specific,
-		"Limit commands to the publicly specified set\n");
+		"Limit commands to the publicly specified set");
 
 static unsigned long override_dsm_mask;
 module_param(override_dsm_mask, ulong, S_IRUGO);
-- 
1.8.3.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 3/3] Remove unnecessary newline

[Linda Knippers]

On 02/13/2017 12:32 PM, Dan Williams wrote:
> On Mon, Feb 13, 2017 at 8:27 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
>> Newline in MODULE_PARM_DESC messes up the modprobe output so remove it.
> 
> Yes, looks good. When this "messes up" does that mean the output less
> pretty, or do you know if it breaks scripts parsing the output? Just
> want to be clear on the effects of this change.

If you do a 'modinfo nfit', it puts "(bool)" on a separate line.
That's unlike all the other parameters.  If someone is parsing the output
of modinfo, they may not get the type.

Without my patch, you get this:

parm:           force_enable_dimms:Ignore _STA (ACPI DIMM device) status (bool)
parm:           scrub_timeout:Initial scrub timeout in seconds (uint)
parm:           scrub_overflow_abort:Number of times we overflow ARS results before abort (uint)
parm:           disable_vendor_specific:Limit commands to the publicly specified set
(bool)
parm:           default_dsm_family:Try this DSM type first when identifying NVDIMM family (int)

instead of this:

parm:           force_enable_dimms:Ignore _STA (ACPI DIMM device) status (bool)
parm:           scrub_timeout:Initial scrub timeout in seconds (uint)
parm:           scrub_overflow_abort:Number of times we overflow ARS results before abort (uint)
parm:           disable_vendor_specific:Limit commands to the publicly specified set (bool)
parm:           default_dsm_family:Try this DSM type first when identifying NVDIMM family (int)


-- ljk
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 3/3] Remove unnecessary newline

[Dan Williams]

On Mon, Feb 13, 2017 at 8:27 AM, Linda Knippers <linda.knippers@hpe.com> wrote:
> Newline in MODULE_PARM_DESC messes up the modprobe output so remove it.

Yes, looks good. When this "messes up" does that mean the output less
pretty, or do you know if it breaks scripts parsing the output? Just
want to be clear on the effects of this change.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 3/3] Remove unnecessary newline

[Linda Knippers]

Newline in MODULE_PARM_DESC messes up the modprobe output so remove it.

Signed-off-by: Linda Knippers <linda.knippers@hpe.com>
---
 drivers/acpi/nfit/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index e38773f..6c522c8 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -49,7 +49,7 @@
 static bool disable_vendor_specific;
 module_param(disable_vendor_specific, bool, S_IRUGO);
 MODULE_PARM_DESC(disable_vendor_specific,
-		"Limit commands to the publicly specified set\n");
+		"Limit commands to the publicly specified set");
 
 static int default_dsm_family = -1;
 module_param(default_dsm_family, int, S_IRUGO);
-- 
1.8.3.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm