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;
+ }
+ }
/* 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
