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

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

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