* [PATCH v2 0/3] apci, nfit: DSM improvements
@ 2017-03-07 0:25 Linda Knippers
2017-03-07 0:25 ` [PATCH 1/3] Allow override of built-in bitmasks for NVDIMM DSMs Linda Knippers
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Linda Knippers @ 2017-03-07 0:25 UTC (permalink / raw)
To: linux-nvdimm
The first two patches in this series are motivated by feedback
from distribution developers, test engineers, and management tool
developers. We need the ability to test and support Linux and
management tools on systems that support more than one DSM family
for NVDIMM-N.
We also need the ability to test and support new functions without
requiring users to update their kernel. These changes will also
facilate development as we move toward DSM standardization.
The ability to restrict DSM functions to the currently documented
set is still in place.
The third patch cleans up a cosmetic/modinfo parsing issue with one
of the existing module parameters.
Changes in v2:
- Switched from allowing all functions advertised by the
firmware through function 0 to an override module parameter.
Linda Knippers (3):
Allow override of built-in bitmasks for NVDIMM DSMs
Allow specifying a default DSM family
Remove unnecessary newline
drivers/acpi/nfit/core.c | 36 ++++++++++++++++++++++++++++++------
1 file changed, 30 insertions(+), 6 deletions(-)
--
1.8.3.1
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 1/3] Allow override of built-in bitmasks for NVDIMM DSMs 2017-03-07 0:25 [PATCH v2 0/3] apci, nfit: DSM improvements Linda Knippers @ 2017-03-07 0:25 ` Linda Knippers 2017-03-07 8:53 ` Johannes Thumshirn 2017-03-07 0:25 ` [PATCH 2/3] Allow specifying a default DSM family Linda Knippers 2017-03-07 0:25 ` [PATCH 3/3] Remove unnecessary newline Linda Knippers 2 siblings, 1 reply; 15+ messages in thread From: Linda Knippers @ 2017-03-07 0:25 UTC (permalink / raw) To: linux-nvdimm As it is today, we can't enable or test new NVDIMM management functions provided by new firmware and tools without changing the kernel. We also can't prevent documented DSM functions from being called in the case of buggy firmware. This patch provides a module parameter that overrides the DSM function mask that is built into the kernel. If the "disable_vendor_specific" module parameter is also used we ignore the new parameter. Signed-off-by: Linda Knippers <linda.knippers@hpe.com> --- drivers/acpi/nfit/core.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c index 662036b..97d42ff 100644 --- a/drivers/acpi/nfit/core.c +++ b/drivers/acpi/nfit/core.c @@ -51,6 +51,10 @@ MODULE_PARM_DESC(disable_vendor_specific, "Limit commands to the publicly specified set\n"); +static unsigned long override_dsm_mask; +module_param(override_dsm_mask, ulong, S_IRUGO); +MODULE_PARM_DESC(override_dsm_mask, "Bitmask of allowed NVDIMM DSM functions"); + LIST_HEAD(acpi_descs); DEFINE_MUTEX(acpi_desc_lock); @@ -1402,7 +1406,9 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc, /* limit the supported commands to those that are publicly documented */ nfit_mem->family = i; - if (nfit_mem->family == NVDIMM_FAMILY_INTEL) { + if (override_dsm_mask && !disable_vendor_specific) + dsm_mask = override_dsm_mask; + else if (nfit_mem->family == NVDIMM_FAMILY_INTEL) { dsm_mask = 0x3fe; if (disable_vendor_specific) dsm_mask &= ~(1 << ND_CMD_VENDOR); -- 1.8.3.1 _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] Allow override of built-in bitmasks for NVDIMM DSMs 2017-03-07 0:25 ` [PATCH 1/3] Allow override of built-in bitmasks for NVDIMM DSMs Linda Knippers @ 2017-03-07 8:53 ` Johannes Thumshirn 0 siblings, 0 replies; 15+ messages in thread From: Johannes Thumshirn @ 2017-03-07 8:53 UTC (permalink / raw) To: Linda Knippers, linux-nvdimm On 03/07/2017 01:25 AM, Linda Knippers wrote: > As it is today, we can't enable or test new NVDIMM management functions > provided by new firmware and tools without changing the kernel. We also > can't prevent documented DSM functions from being called in the > case of buggy firmware. This patch provides a module parameter that > overrides the DSM function mask that is built into the kernel. > > If the "disable_vendor_specific" module parameter is also used > we ignore the new parameter. > > Signed-off-by: Linda Knippers <linda.knippers@hpe.com> > --- Looks good to me, Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de> -- Johannes Thumshirn Storage jthumshirn@suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/3] Allow specifying a default DSM family 2017-03-07 0:25 [PATCH v2 0/3] apci, nfit: DSM improvements Linda Knippers 2017-03-07 0:25 ` [PATCH 1/3] Allow override of built-in bitmasks for NVDIMM DSMs Linda Knippers @ 2017-03-07 0:25 ` Linda Knippers 2017-03-07 0:39 ` Dan Williams 2017-03-07 0:25 ` [PATCH 3/3] Remove unnecessary newline Linda Knippers 2 siblings, 1 reply; 15+ messages in thread From: Linda Knippers @ 2017-03-07 0:25 UTC (permalink / raw) To: linux-nvdimm 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; + } /* limit the supported commands to those that are publicly documented */ nfit_mem->family = i; -- 1.8.3.1 _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] Allow specifying a default DSM family 2017-03-07 0:25 ` [PATCH 2/3] Allow specifying a default DSM family Linda Knippers @ 2017-03-07 0:39 ` Dan Williams 2017-03-07 1:33 ` Linda Knippers 0 siblings, 1 reply; 15+ messages in thread From: Dan Williams @ 2017-03-07 0:39 UTC (permalink / raw) To: Linda Knippers; +Cc: linux-nvdimm 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. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] Allow specifying a default DSM family 2017-03-07 0:39 ` Dan Williams @ 2017-03-07 1:33 ` Linda Knippers 2017-03-07 1:56 ` Dan Williams 0 siblings, 1 reply; 15+ messages in thread From: Linda Knippers @ 2017-03-07 1:33 UTC (permalink / raw) To: Dan Williams; +Cc: linux-nvdimm 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 _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] Allow specifying a default DSM family 2017-03-07 1:33 ` Linda Knippers @ 2017-03-07 1:56 ` Dan Williams 2017-03-07 2:13 ` Linda Knippers 2017-03-07 19:00 ` Linda Knippers 0 siblings, 2 replies; 15+ messages in thread From: Dan Williams @ 2017-03-07 1:56 UTC (permalink / raw) To: Linda Knippers; +Cc: linux-nvdimm 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) _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] Allow specifying a default DSM family 2017-03-07 1:56 ` Dan Williams @ 2017-03-07 2:13 ` Linda Knippers 2017-03-07 19:00 ` Linda Knippers 1 sibling, 0 replies; 15+ messages in thread From: Linda Knippers @ 2017-03-07 2:13 UTC (permalink / raw) To: Dan Williams; +Cc: linux-nvdimm 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 _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] Allow specifying a default DSM family 2017-03-07 1:56 ` Dan Williams 2017-03-07 2:13 ` Linda Knippers @ 2017-03-07 19:00 ` Linda Knippers 2017-03-07 19:05 ` Linda Knippers 2017-03-07 19:26 ` Dan Williams 1 sibling, 2 replies; 15+ messages in thread From: Linda Knippers @ 2017-03-07 19:00 UTC (permalink / raw) To: Dan Williams; +Cc: linux-nvdimm 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) > _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] Allow specifying a default DSM family 2017-03-07 19:00 ` Linda Knippers @ 2017-03-07 19:05 ` Linda Knippers 2017-03-07 19:26 ` Dan Williams 1 sibling, 0 replies; 15+ messages in thread From: Linda Knippers @ 2017-03-07 19:05 UTC (permalink / raw) To: Dan Williams; +Cc: linux-nvdimm 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) >> > _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] Allow specifying a default DSM family 2017-03-07 19:00 ` Linda Knippers 2017-03-07 19:05 ` Linda Knippers @ 2017-03-07 19:26 ` Dan Williams 1 sibling, 0 replies; 15+ messages in thread From: Dan Williams @ 2017-03-07 19:26 UTC (permalink / raw) To: Linda Knippers; +Cc: linux-nvdimm 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. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/3] Remove unnecessary newline 2017-03-07 0:25 [PATCH v2 0/3] apci, nfit: DSM improvements Linda Knippers 2017-03-07 0:25 ` [PATCH 1/3] Allow override of built-in bitmasks for NVDIMM DSMs Linda Knippers 2017-03-07 0:25 ` [PATCH 2/3] Allow specifying a default DSM family Linda Knippers @ 2017-03-07 0:25 ` Linda Knippers 2017-03-07 8:53 ` Johannes Thumshirn 2 siblings, 1 reply; 15+ messages in thread From: Linda Knippers @ 2017-03-07 0:25 UTC (permalink / raw) To: linux-nvdimm The newline in MODULE_PARM_DESC causes modinfo to print the parameter data type on a separate line, which is different from all the other module parameters and could potentially cause a problem for someone parsing the output of modinfo. Signed-off-by: Linda Knippers <linda.knippers@hpe.com> --- drivers/acpi/nfit/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c index 78c46a7..7a4c549 100644 --- a/drivers/acpi/nfit/core.c +++ b/drivers/acpi/nfit/core.c @@ -49,7 +49,7 @@ static bool disable_vendor_specific; module_param(disable_vendor_specific, bool, S_IRUGO); MODULE_PARM_DESC(disable_vendor_specific, - "Limit commands to the publicly specified set\n"); + "Limit commands to the publicly specified set"); static unsigned long override_dsm_mask; module_param(override_dsm_mask, ulong, S_IRUGO); -- 1.8.3.1 _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] Remove unnecessary newline 2017-03-07 0:25 ` [PATCH 3/3] Remove unnecessary newline Linda Knippers @ 2017-03-07 8:53 ` Johannes Thumshirn 0 siblings, 0 replies; 15+ messages in thread From: Johannes Thumshirn @ 2017-03-07 8:53 UTC (permalink / raw) To: Linda Knippers, linux-nvdimm On 03/07/2017 01:25 AM, Linda Knippers wrote: > The newline in MODULE_PARM_DESC causes modinfo to print the parameter > data type on a separate line, which is different from all the other > module parameters and could potentially cause a problem for someone > parsing the output of modinfo. > > Signed-off-by: Linda Knippers <linda.knippers@hpe.com> > --- Thanks, Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de> -- Johannes Thumshirn Storage jthumshirn@suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 0/3] apci, nfit: DSM improvements @ 2017-03-07 21:35 Linda Knippers 2017-03-07 21:35 ` [PATCH 2/3] Allow specifying a default DSM family Linda Knippers 0 siblings, 1 reply; 15+ messages in thread From: Linda Knippers @ 2017-03-07 21:35 UTC (permalink / raw) To: linux-nvdimm The first two patches in this series are motivated by feedback from distribution developers, test engineers, and management tool developers. We need the ability to test and support Linux and management tools on systems that support more than one DSM family for NVDIMM-N. We also need the ability to test and support new functions without requiring users to update their kernel. These changes will also facilate development as we move toward DSM standardization. The ability to restrict DSM functions to the currently documented set is still in place. The third patch cleans up a cosmetic/modinfo parsing issue with one of the existing module parameters. Changes in v3: - Simplified dsm address family determination based on Dan's feedback. Changes in v2: - Switched from allowing all functions advertised by the firmware through function 0 to an override module parameter. Linda Knippers (3): Allow override of built-in bitmasks for NVDIMM DSMs Allow specifying a default DSM family Remove unnecessary newline drivers/acpi/nfit/core.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) -- 1.8.3.1 _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/3] Allow specifying a default DSM family 2017-03-07 21:35 [PATCH v3 0/3] apci, nfit: DSM improvements Linda Knippers @ 2017-03-07 21:35 ` Linda Knippers 0 siblings, 0 replies; 15+ messages in thread From: Linda Knippers @ 2017-03-07 21:35 UTC (permalink / raw) To: linux-nvdimm 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 | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c index 97d42ff..0564cd6 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); @@ -1372,6 +1377,7 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc, unsigned long dsm_mask; const u8 *uuid; int i; + int family = -1; /* nfit test assumes 1:1 relationship between commands and dsms */ nfit_mem->dsm_mask = acpi_desc->dimm_cmd_force_en; @@ -1402,10 +1408,11 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc, */ 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 (family < 0 || i == default_dsm_family) + family = i; /* limit the supported commands to those that are publicly documented */ - nfit_mem->family = i; + nfit_mem->family = family; if (override_dsm_mask && !disable_vendor_specific) dsm_mask = override_dsm_mask; else if (nfit_mem->family == NVDIMM_FAMILY_INTEL) { -- 1.8.3.1 _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 0/3] apci, nfit: DSM improvements @ 2017-02-13 16:27 Linda Knippers 2017-02-13 16:27 ` [PATCH 2/3] Allow specifying a default DSM family Linda Knippers 0 siblings, 1 reply; 15+ messages in thread From: Linda Knippers @ 2017-02-13 16:27 UTC (permalink / raw) To: linux-nvdimm, dan.j.williams The first two patches in this series are motivated by feedback from distribution developers, test engineers, and management tool developers. We need the ability to test and support Linux and management tools on systems that support more than one DSM family for NVDIMM-N. We also need the ability to test and support new functions without requiring users to update their kernel. These changes will also facilate development as we move toward DSM standardization. The ability to restrict DSM functions to the currently documented set is still in place. The third patch cleans up a nit. Linda Knippers (3): Allow all supported HPE DSM functions to be called Allow specifying a default DSM family Remove unnecessary newline drivers/acpi/nfit/core.c | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) -- 1.8.3.1 _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/3] Allow specifying a default DSM family 2017-02-13 16:27 [PATCH 0/3] apci, nfit: DSM improvements Linda Knippers @ 2017-02-13 16:27 ` Linda Knippers 0 siblings, 0 replies; 15+ messages in thread From: Linda Knippers @ 2017-02-13 16:27 UTC (permalink / raw) To: linux-nvdimm, dan.j.williams 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 | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c index b80d4d9..e38773f 100644 --- a/drivers/acpi/nfit/core.c +++ b/drivers/acpi/nfit/core.c @@ -51,6 +51,12 @@ MODULE_PARM_DESC(disable_vendor_specific, "Limit commands to the publicly specified set\n"); +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); @@ -1367,7 +1373,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; @@ -1395,10 +1401,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; -- 1.8.3.1 _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-03-07 21:35 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-07 0:25 [PATCH v2 0/3] apci, nfit: DSM improvements Linda Knippers 2017-03-07 0:25 ` [PATCH 1/3] Allow override of built-in bitmasks for NVDIMM DSMs Linda Knippers 2017-03-07 8:53 ` Johannes Thumshirn 2017-03-07 0:25 ` [PATCH 2/3] Allow specifying a default DSM family Linda Knippers 2017-03-07 0:39 ` Dan Williams 2017-03-07 1:33 ` Linda Knippers 2017-03-07 1:56 ` Dan Williams 2017-03-07 2:13 ` Linda Knippers 2017-03-07 19:00 ` Linda Knippers 2017-03-07 19:05 ` Linda Knippers 2017-03-07 19:26 ` Dan Williams 2017-03-07 0:25 ` [PATCH 3/3] Remove unnecessary newline Linda Knippers 2017-03-07 8:53 ` Johannes Thumshirn -- strict thread matches above, loose matches on Subject: below -- 2017-03-07 21:35 [PATCH v3 0/3] apci, nfit: DSM improvements Linda Knippers 2017-03-07 21:35 ` [PATCH 2/3] Allow specifying a default DSM family Linda Knippers 2017-02-13 16:27 [PATCH 0/3] apci, nfit: DSM improvements Linda Knippers 2017-02-13 16:27 ` [PATCH 2/3] Allow specifying a default DSM family Linda Knippers
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.