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; > + } > + } > > /* 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) >
I think that will work. I'll test it in the morning. Thanks for the feedback. I wasn't crazy about the code myself. -- ljk _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
