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 <[email protected]>
>> wrote:
>>> On 03/06/2017 07:39 PM, Dan Williams wrote:
>>>> On Mon, Mar 6, 2017 at 4:25 PM, Linda Knippers <[email protected]>
>>>> 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 <[email protected]>
>>>>> ---
>>>>> 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
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
