x86, dmi: introduce and use dmi_get_bios_year()
Subsystems (4)
v2 cover x86, dmi: introduce and use dmi_get_bios_year()
Introduce a new helper to extract a year from DMI BIOS date since there are most existing users do and newcomers would utilize as well.
Ingo, this is respin of the patches in x86/platform. I think, if it's possible to rebase, would be easier just to replace existing ones.
Otherwise I would reorganize them as a series of fixups.
Bjorn, Jean, Rafael, thank you for your reviews!
Since v1:
- move the helper to be regular function (Bjorn, Rafael)
- fix a potential regression introduced in pci_acpi_crs_quirks() (Jean)
- adjust subject lines and commit message bodies (Bjorn)
- add tags (Bjorn, Jean)
Andy Shevchenko (4):
firmware: dmi_scan: Introduce the dmi_get_bios_year() helper x86/PCI: Simplify code by using the new dmi_get_bios_year() helper ACPI / sleep: Simplify code by using the new dmi_get_bios_year()
helper
PCI: Simplify code by using the new dmi_get_bios_year() helper
arch/x86/pci/acpi.c | 10 +++++----- arch/x86/pci/direct.c | 5 ++--- arch/x86/pci/mmconfig-shared.c | 9 ++------- drivers/acpi/sleep.c | 4 +--- drivers/firmware/dmi_scan.c | 11 +++++++++++ drivers/pci/pci.c | 6 +----- include/linux/dmi.h | 2 ++ 7 files changed, 24 insertions(+), 23 deletions(-)
Signature · 1 line
2.16.1
PATCH v2 1/4 firmware: dmi_scan: Introduce the dmi_get_bios_year() helper
The pattern to only extract the year portion of date is used in several places and more users may come.
By using this helper they may create slightly cleaner code.
--- drivers/firmware/dmi_scan.c | 11 +++++++++++ include/linux/dmi.h | 2 ++ 2 files changed, 13 insertions(+)
drivers/firmware/dmi_scan.c +11 -0
@@ -1015,6 +1015,17 @@ bool dmi_get_date(int field, int *yearp, int *monthp, int *dayp) }EXPORT_SYMBOL (dmi_get_date); +int dmi_get_bios_year (void ) +{ +bool exists; +int year; + + exists =dmi_get_date (DMI_BIOS_DATE , &year,NULL ,NULL ); + +return exists ? year : -ENODATA ; +} +EXPORT_SYMBOL (dmi_get_bios_year); +/** * dmi_walk - Walk the DMI table and get called back for every record * @decode: Callback function
include/linux/dmi.h +2 -0
@@ -106,6 +106,7 @@ extern void dmi_scan_machine(void);extern void dmi_memdev_walk (void );extern void dmi_set_dump_stack_arch_desc (void );extern bool dmi_get_date (int field,int *yearp,int *monthp,int *dayp); +extern int dmi_get_bios_year (void );extern int dmi_name_in_vendors (const char *str);extern int dmi_name_in_serial (const char *str);extern int dmi_available;
@@ -133,6 +134,7 @@ static inline bool dmi_get_date(int field, int *yearp, int *monthp, int *dayp) *dayp =0 ;return false; } +static inline int dmi_get_bios_year (void ) {return -ENXIO ; }static inline int dmi_name_in_vendors (const char *s) {return 0 ; }static inline int dmi_name_in_serial (const char *s) {return 0 ; }#define dmi_available 0
Signature · 1 line
2.16.1
Re: [PATCH v2 1/4] firmware: dmi_scan: Introduce the dmi_get_bios_year() helper
Quoted 6 lines · Andy Shevchenko
drivers/firmware/dmi_scan.c +0
-0@@ -1015,6 +1015,17 @@ bool dmi_get_date(int field, int *yearp, int *monthp, int *dayp) }EXPORT_SYMBOL (dmi_get_date);
drivers/firmware/dmi_scan.c bool dmi_get_date(int field, int *yearp, int *monthp, int *dayp) · line 1027
Quoted 10 lines
drivers/firmware/dmi_scan.c +10
-0+int dmi_get_bios_year (void ) +{ +bool exists; +int year; + + exists =dmi_get_date (DMI_BIOS_DATE , &year,NULL ,NULL ); + +return exists ? year : -ENODATA ; +} +EXPORT_SYMBOL (dmi_get_bios_year);
It would be good if kerneldoc was added to this function. One thing to mention is that direct usage of the function in a conditional only works reliably when asserting an exact or minimum BIOS date. It doesn't work reliably when asserting a maximum BIOS date unless the return value is explicitly checked for -ENODATA. (Fortunately that use case seems to be rare, but still worth mentioning IMHO.)
drivers/firmware/dmi_scan.c bool dmi_get_date(int field, int *yearp, int *monthp, int *dayp) · line 1028
Quoted 1 line
drivers/firmware/dmi_scan.c +1
-0+static inline int dmi_get_bios_year (void ) {return -ENXIO ; }
Shouldn't this be -ENODATA as well for consistency? Otherwise one would have to check for -ENODATA *and* -ENXIO.
Thanks,
Lukas
Re: [PATCH v2 1/4] firmware: dmi_scan: Introduce the dmi_get_bios_year() helper
Quoted 8 lines · Lukas Wunner
Quoted 7 lines · Andy Shevchenko
drivers/firmware/dmi_scan.c +0
-0@@ -1015,6 +1015,17 @@ bool dmi_get_date(int field, int *yearp, int *monthp, int *dayp) }EXPORT_SYMBOL (dmi_get_date);
Quoted 12 lines
drivers/firmware/dmi_scan.c bool dmi_get_date(int field, int *yearp, int *monthp, int *dayp) · line 1027
Quoted 10 lines
drivers/firmware/dmi_scan.c +10
-0+int dmi_get_bios_year (void ) +{ +bool exists; +int year; + + exists =dmi_get_date (DMI_BIOS_DATE , &year,NULL ,NULL ); + +return exists ? year : -ENODATA ; +} +EXPORT_SYMBOL (dmi_get_bios_year);It would be good if kerneldoc was added to this function.
Perhaps.
Quoted 6 lines
One thing
to mention is that direct usage of the function in a conditional only works reliably when asserting an exact or minimum BIOS date. It doesn't
work reliably when asserting a maximum BIOS date unless the return value is explicitly checked for-ENODATA.
Just < 0.
Quoted 9 lines
(Fortunately that use case
seems to be rare, but still worth mentioning IMHO.)drivers/firmware/dmi_scan.c bool dmi_get_date(int field, int *yearp, int · line 1029
Quoted 1 line
drivers/firmware/dmi_scan.c +1
-0+static inline int dmi_get_bios_year (void ) {return -ENXIO ; }Shouldn't this be
-ENODATAas well for consistency? Otherwise one would
have to check for-ENODATA*and*-ENXIO.
I was thinking about this, though checking for negative errors is a pattern in kernel. If user would like to distinguish what really happened, then they may to check for explicit code.
ENXIO is chosen to be consistent with other calls when !DMI. ENODATA on the other hand is not related to access to the data, but to the contents of it. So, I would like to keep them different.
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
Re: [PATCH v2 1/4] firmware: dmi_scan: Introduce the dmi_get_bios_year() helper
In reply to Andy ShevchenkoHi Andy,
Quoted 17 lines · Andy Shevchenko
The pattern to only extract the year portion of date is used in several places and more users may come.
By using this helper they may create slightly cleaner code.
Signed-off-by Andy Shevchenko <[email protected]>--- drivers/firmware/dmi_scan.c | 11 +++++++++++ include/linux/dmi.h | 2 ++ 2 files changed, 13 insertions(+)drivers/firmware/dmi_scan.c +0
-0@@ -1015,6 +1015,17 @@ bool dmi_get_date(int field, int *yearp, int *monthp, int *dayp) }
drivers/firmware/dmi_scan.c bool dmi_get_date(int field, int *yearp, int *monthp, int *dayp) · line 1025
Quoted 10 lines
drivers/firmware/dmi_scan.c +8
-0EXPORT_SYMBOL (dmi_get_date); +int dmi_get_bios_year (void ) +{ +bool exists; +int year; + + exists =dmi_get_date (DMI_BIOS_DATE , &year,NULL ,NULL ); + +return exists ? year : -ENODATA ;
If the DMI field exists but year can't be parsed, "exists" is true but "year" is 0. From the caller's perspective, it is the same as if the field did not exist (year is not available in both cases), but the function returns 0 in one case and -ENODATA in the other. Don't you think that:
return year ? year : -ENODATA;
would serve the caller better?
Quoted 19 lines
drivers/firmware/dmi_scan.c +3
-0+} +EXPORT_SYMBOL (dmi_get_bios_year); +/** * dmi_walk - Walk the DMI table and get called back for every record * @decode: Callback function include/linux/dmi.h +1
-0@@ -106,6 +106,7 @@ extern void dmi_scan_machine(void);extern void dmi_memdev_walk (void );extern void dmi_set_dump_stack_arch_desc (void );extern bool dmi_get_date (int field,int *yearp,int *monthp,int *dayp); +extern int dmi_get_bios_year (void );extern int dmi_name_in_vendors (const char *str);extern int dmi_name_in_serial (const char *str);extern int dmi_available;
include/linux/dmi.h static inline bool dmi_get_date(int field, int *yearp, int *monthp, int *dayp) · line 140
Quoted 7 lines
include/linux/dmi.h +1
-0@@ -133,6 +134,7 @@ static inline bool dmi_get_date(int field, int *yearp, int *monthp, int *dayp) *dayp =0 ;return false; } +static inline int dmi_get_bios_year (void ) {return -ENXIO ; }static inline int dmi_name_in_vendors (const char *s) {return 0 ; }static inline int dmi_name_in_serial (const char *s) {return 0 ; }#define dmi_available 0
Looks good otherwise.
Signature · 2 lines
Jean Delvare SUSE L3 Support
PATCH v2 2/4 x86/PCI: Simplify code by using the new dmi_get_bios_year() helper
In reply to Andy ShevchenkoUse new dmi_get_bios_year() helper instead of open-coding its functionality.
No changes in functionality.
--- arch/x86/pci/acpi.c | 10 +++++----- arch/x86/pci/direct.c | 5 ++--- arch/x86/pci/mmconfig-shared.c | 9 ++------- 3 files changed, 9 insertions(+), 15 deletions(-)
arch/x86/pci/acpi.c +5 -5
@@ -138,14 +138,14 @@ static const struct dmi_system_id pci_crs_quirks[] __initconst = { {} }; +#define in_range (b, first, len) ((b) >= (first) && (b) <= (first) + (len) - 1) +void __init pci_acpi_crs_quirks (void ) { -int year; +int year =dmi_get_bios_year (); -if (dmi_get_date (DMI_BIOS_DATE , &year,NULL ,NULL ) && year <2008 ) { -if (iomem_resource.end <=0xffffffff ) - pci_use_crs = false; - } +if (in_range (year,0 ,2008 ) && iomem_resource.end <=0xffffffff ) + pci_use_crs = false;dmi_check_system (pci_crs_quirks);
arch/x86/pci/direct.c +2 -3
@@ -195,14 +195,13 @@ static const struct pci_raw_ops pci_direct_conf2 = {static int __init pci_sanity_check (const struct pci_raw_ops *o) {u32 x =0 ; -int year, devfn; +int devfn;if (pci_probe &PCI_NO_CHECKS )return 1 ;/* Assume Type 1 works for newer systems. This handles machines that don't have anything on PCI Bus 0. */ -dmi_get_date (DMI_BIOS_DATE , &year,NULL ,NULL ); -if (year >=2001 ) +if (dmi_get_bios_year () >=2001 )return 1 ;for (devfn =0 ; devfn <0x100 ; devfn++) {
arch/x86/pci/mmconfig-shared.c +2 -7
@@ -547,19 +547,14 @@ static void __init pci_mmcfg_reject_broken(int early)static int __init acpi_mcfg_check_entry (struct acpi_table_mcfg *mcfg,struct acpi_mcfg_allocation *cfg) { -int year; -if (cfg->address <0xFFFFFFFF )return 0 ;if (!strncmp (mcfg->header.oem_id,"SGI" ,3 ))return 0 ; -if (mcfg->header.revision >=1 ) { -if (dmi_get_date (DMI_BIOS_DATE , &year,NULL ,NULL ) && - year >=2010 ) -return 0 ; - } +if (mcfg->header.revision >=1 &&dmi_get_bios_year () >=2010 ) +return 0 ; pr_err(PREFIX "MCFG region for %04x [bus %02x-%02x] at %#llx " "is above 4GB, ignored\n", cfg->pci_segment,
Signature · 1 line
2.16.1
Re: [PATCH v2 2/4] x86/PCI: Simplify code by using the new dmi_get_bios_year() helper
Hi Andy,
Quoted 23 lines · Andy Shevchenko
Use new
dmi_get_bios_year()helper instead of open-coding its functionality.No changes in functionality.
Acked-by Bjorn Helgaas <[email protected]>Signed-off-by Andy Shevchenko <[email protected]>--- arch/x86/pci/acpi.c | 10 +++++----- arch/x86/pci/direct.c | 5 ++--- arch/x86/pci/mmconfig-shared.c | 9 ++------- 3 files changed, 9 insertions(+), 15 deletions(-)arch/x86/pci/acpi.c +2
-0@@ -138,14 +138,14 @@ static const struct dmi_system_id pci_crs_quirks[] __initconst = { {} }; +#define in_range (b, first, len) ((b) >= (first) && (b) <= (first) + (len) - 1) +
arch/x86/pci/acpi.c static const struct dmi_system_id pci_crs_quirks[] __initconst = { · line 147
Quoted 10 lines
arch/x86/pci/acpi.c +2
-5void __init pci_acpi_crs_quirks (void ) { -int year; +int year =dmi_get_bios_year (); -if (dmi_get_date (DMI_BIOS_DATE , &year,NULL ,NULL ) && year <2008 ) { -if (iomem_resource.end <=0xffffffff ) - pci_use_crs = false; - } +if (in_range (year,0 ,2008 ) && iomem_resource.end <=0xffffffff )
I don't like this, sorry. I find "in_range" confusing, there are many definitions of it throughout the kernel which differ in the details (some take a length as 3rd parameter, others take upper boundary, some include the boundaries, some do not...) The following:
if (year >= 0 && year < 2008 && iomem_resource.end <= 0xffffffff)
is a lot more readable in my opinion. No need to look-up a tricky macro definition to figure what happens if year is 2008.
Quoted 3 lines
arch/x86/pci/acpi.c +1
-0+ pci_use_crs = false;dmi_check_system (pci_crs_quirks);
Signature · 2 lines
Jean Delvare SUSE L3 Support
PATCH v2 3/4 ACPI / sleep: Simplify code by using the new dmi_get_bios_year() helper
In reply to Andy ShevchenkoUse new dmi_get_bios_year() helper instead of open-coding its functionality.
No changes in functionality.
--- drivers/acpi/sleep.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/acpi/sleep.c +1 -3
@@ -376,12 +376,10 @@ void __init acpi_sleep_no_blacklist(void)static void __init acpi_sleep_dmi_check (void ) { -int year; -if (ignore_blacklist)return ; -if (dmi_get_date (DMI_BIOS_DATE , &year,NULL ,NULL ) && year >=2012 ) +if (dmi_get_bios_year () >=2012 )acpi_nvs_nosave_s3 ();dmi_check_system (acpisleep_dmi_table);
Signature · 1 line
2.16.1
PATCH v2 4/4 PCI: Simplify code by using the new dmi_get_bios_year() helper
In reply to Andy ShevchenkoUse new dmi_get_bios_year() helper instead of open-coding its functionality.
No changes in functionality.
--- drivers/pci/pci.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
drivers/pci/pci.c +1 -5
@@ -2258,8 +2258,6 @@ void pci_config_pm_runtime_put(struct pci_dev *pdev)*/ bool pci_bridge_d3_possible (struct pci_dev *bridge) { -unsigned int year; -if (!pci_is_pcie (bridge))return false;
@@ -2287,10 +2285,8 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)* It should be safe to put PCIe ports from 2015 or newer * to D3. */ -if (dmi_get_date (DMI_BIOS_DATE , &year,NULL ,NULL ) && - year >=2015 ) { +if (dmi_get_bios_year () >=2015 )return true; - }break ; }
Signature · 1 line
2.16.1
Quoted 16 lines · Andy Shevchenko
Introduce a new helper to extract a year from DMI BIOS date since there are most existing users do and newcomers would utilize as well.
Ingo, this is respin of the patches in x86/platform. I think, if it's possible to rebase, would be easier just to replace existing ones.
Otherwise I would reorganize them as a series of fixups.
Bjorn, Jean, Rafael, thank you for your reviews!
Since v1:
- move the helper to be regular function (Bjorn, Rafael)
- fix a potential regression introduced inpci_acpi_crs_quirks()(Jean)
- adjust subject lines and commit message bodies (Bjorn)
- add tags (Bjorn, Jean)
Quoted 6 lines
Andy Shevchenko (4):
firmware:dmi_scan: Introduce thedmi_get_bios_year()helper x86/PCI: Simplify code by using the newdmi_get_bios_year()helper ACPI / sleep: Simplify code by using the newdmi_get_bios_year()
helper
PCI: Simplify code by using the newdmi_get_bios_year()helper
I can apply this or if you want to route it differently, please feel free to add
to all of the patches in the series.
Thanks!
Quoted 21 lines · Rafael J. Wysocki
Quoted 19 lines · Andy Shevchenko
Introduce a new helper to extract a year from DMI BIOS date since there
are most existing users do and newcomers would utilize as well.Ingo, this is respin of the patches in x86/platform. I think, if it's
possible to rebase, would be easier just to replace existing ones.Otherwise I would reorganize them as a series of fixups.
Bjorn, Jean, Rafael, thank you for your reviews!
Since v1:
- move the helper to be regular function (Bjorn, Rafael)
- fix a potential regression introduced inpci_acpi_crs_quirks()(Jean)
- adjust subject lines and commit message bodies (Bjorn)
- add tags (Bjorn, Jean)
Quoted 8 lines
Quoted 6 lines
Andy Shevchenko (4):
firmware:dmi_scan: Introduce thedmi_get_bios_year()helper x86/PCI: Simplify code by using the newdmi_get_bios_year()helper ACPI / sleep: Simplify code by using the newdmi_get_bios_year()
helper
PCI: Simplify code by using the newdmi_get_bios_year()helperI can apply this or if you want to route it differently,
I don't know how to proceed here, Ingo applied v1 and there might be options like remove old and reapply new, or do a series of fixups.
Ingo, can you suggest how to do this in the best way?
Quoted 7 lines
please feel free to add
Reviewed-by Rafael J. Wysocki <[email protected]>to all of the patches in the series.
Thanks!
Thank you!
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
Rafael J. Wysocki
maintainerQuoted 29 lines · Andy Shevchenko
Quoted 28 lines · Rafael J. Wysocki
Quoted 25 lines · Andy Shevchenko
Introduce a new helper to extract a year from DMI BIOS date since there
are most existing users do and newcomers would utilize as well.Ingo, this is respin of the patches in x86/platform. I think, if it's
possible to rebase, would be easier just to replace existing ones.Otherwise I would reorganize them as a series of fixups.
Bjorn, Jean, Rafael, thank you for your reviews!
Since v1:
- move the helper to be regular function (Bjorn, Rafael)
- fix a potential regression introduced inpci_acpi_crs_quirks()(Jean)
- adjust subject lines and commit message bodies (Bjorn)
- add tags (Bjorn, Jean)Andy Shevchenko (4):
firmware:dmi_scan: Introduce thedmi_get_bios_year()helper x86/PCI: Simplify code by using the newdmi_get_bios_year()helper ACPI / sleep: Simplify code by using the newdmi_get_bios_year()
helper
PCI: Simplify code by using the newdmi_get_bios_year()helper
Quoted 6 lines
Quoted 1 line
I can apply this or if you want to route it differently,
I don't know how to proceed here, Ingo applied v1 and there might be options like remove old and reapply new, or do a series of fixups.
Ingo, can you suggest how to do this in the best way?
AFAICS the tip workflow is that things are not rebased in there, only reverted if need be, and if the previous version was taken to tip, reverting it is not worth the hassle IMO.
I'd just wait for it to be integrated and post an update on top of it or even on top of linux-next.
Quoted 5 lines · Rafael J. Wysocki
Quoted 5 lines
Quoted 1 line
I can apply this or if you want to route it differently,
I don't know how to proceed here, Ingo applied v1 and there might be options like remove old and reapply new, or do a series of fixups.
Quoted 8 lines
Quoted 1 line
Ingo, can you suggest how to do this in the best way?
AFAICS the tip workflow is that things are not rebased in there, only reverted if need be, and if the previous version was taken to tip, reverting it is not worth the hassle IMO.
I'd just wait for it to be integrated and post an update on top of it or even on top of linux-next.
Yeah, indeed, we rebase in -tip only if absolutely necessary - and there were no real problems with the v1 version, right?
So it would be nice to have v2 as a delta against what is already in -tip.
Thanks,
Ingo