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

On Mon, Mar 6, 2017 at 5:33 PM, Linda Knippers <linda.knippers(a)hpe.com&gt; wrote: > On 03/06/2017 07:39 PM, Dan Williams wrote: >> On Mon, Mar 6, 2017 at 4:25 PM, Linda Knippers <linda.knippers(a)hpe.com&gt; 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(a)hpe.com&gt; >>> --- >>> 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)