| Message ID | 1773e8607ae49fefbb288509603a0db353a72d91.1488844291.git.linda.knippers@hpe.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 6, 2017 at 4:25 PM, Linda Knippers <linda.knippers@hpe.com> 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@hpe.com> > --- > 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.
On 03/06/2017 07:39 PM, Dan Williams wrote: > On Mon, Mar 6, 2017 at 4:25 PM, Linda Knippers <linda.knippers@hpe.com> 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@hpe.com> >> --- >> 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. -- ljk
On Mon, Mar 6, 2017 at 5:33 PM, Linda Knippers <linda.knippers@hpe.com> wrote: > On 03/06/2017 07:39 PM, Dan Williams wrote: >> On Mon, Mar 6, 2017 at 4:25 PM, Linda Knippers <linda.knippers@hpe.com> 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@hpe.com> >>> --- >>> 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)
On 03/06/2017 08:56 PM, Dan Williams wrote: > On Mon, Mar 6, 2017 at 5:33 PM, Linda Knippers <linda.knippers@hpe.com> wrote: >> On 03/06/2017 07:39 PM, Dan Williams wrote: >>> On Mon, Mar 6, 2017 at 4:25 PM, Linda Knippers <linda.knippers@hpe.com> 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@hpe.com> >>>> --- >>>> 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
On 03/06/2017 08:56 PM, Dan Williams wrote: > On Mon, Mar 6, 2017 at 5:33 PM, Linda Knippers <linda.knippers@hpe.com> wrote: >> On 03/06/2017 07:39 PM, Dan Williams wrote: >>> On Mon, Mar 6, 2017 at 4:25 PM, Linda Knippers <linda.knippers@hpe.com> 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@hpe.com> >>>> --- >>>> 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) >
On 03/07/2017 02:00 PM, Linda Knippers wrote: > On 03/06/2017 08:56 PM, Dan Williams wrote: >> On Mon, Mar 6, 2017 at 5:33 PM, Linda Knippers <linda.knippers@hpe.com> wrote: >>> On 03/06/2017 07:39 PM, Dan Williams wrote: >>>> On Mon, Mar 6, 2017 at 4:25 PM, Linda Knippers <linda.knippers@hpe.com> 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@hpe.com> >>>>> --- >>>>> 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. Something like this does work. for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_MSFT; i++) if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1)) { if (default_dsm_family != -1) { if (i == default_dsm_family) { family = i; break; } if (family < 0) family = i; } else { family = i; break; } } > > -- 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) >> >
On Tue, Mar 7, 2017 at 11:00 AM, Linda Knippers <linda.knippers@hpe.com> wrote: > On 03/06/2017 08:56 PM, Dan Williams wrote: >> On Mon, Mar 6, 2017 at 5:33 PM, Linda Knippers <linda.knippers@hpe.com> wrote: >>> On 03/06/2017 07:39 PM, Dan Williams wrote: >>>> On Mon, Mar 6, 2017 at 4:25 PM, Linda Knippers <linda.knippers@hpe.com> 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@hpe.com> >>>>> --- >>>>> 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.
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; + } /* limit the supported commands to those that are publicly documented */ nfit_mem->family = i;
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@hpe.com> --- drivers/acpi/nfit/core.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)