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

[Linda Knippers <linda.knippers@hpe.com>]

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