On Tue, Mar 7, 2017 at 11:00 AM, Linda Knippers <[email protected]> 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.
It doesn't need to be efficient, this is a slow path. _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
