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