x86, dmi: Uninline dmi_get_bios_year()

PATCH Superseded Andy Shevchenko 4 participants 11 messages Mar 20, 2018

Subsystems (4)

x86 ACPI PCI Dmi/smbios support

lore

Share & tools

lore

Andy Shevchenko

author

2018-03-20 13:54 UTC #0 lore raw

v3 cover x86, dmi: Uninline 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.

This is a fix up series on top what had been applied already.

Since v2:
- resend as fix up series
- address comments from Bjorn, Jean, Lukas, Rafael

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 (2):
x86/PCI: Fix a potential regression when use dmi_get_bios_year()
firmware: dmi_scan: Uninline dmi_get_bios_year() helper

 arch/x86/pci/acpi.c         |  4 +++-
 drivers/firmware/dmi_scan.c | 20 ++++++++++++++++++++
 include/linux/dmi.h         | 11 ++---------
 3 files changed, 25 insertions(+), 10 deletions(-)

Signature · 1 line

2.16.2

Andy Shevchenko

author

2018-03-20 13:54 UTC #1 lore raw

PATCH v3 1/2 x86/PCI: Fix a potential regression when use dmi_get_bios_year()

dmi_get_bios_year() may return 0 when it is incapable to parse the BIOS date string. Previously this has been checked in pci_acpi_crs_quirks().

Update the code to restore old behaviour.

Reported-by Jean Delvare <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

---
 arch/x86/pci/acpi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

arch/x86/pci/acpi.c +3 -1

index 00e60de30328..5559dcaddd5e 100644
@@ -140,7 +140,9 @@ static const struct dmi_system_id pci_crs_quirks[] __initconst = {
 
 void __init pci_acpi_crs_quirks(void)
 {
-	if ((dmi_get_bios_year() < 2008) && (iomem_resource.end <= 0xffffffff))
+	int year = dmi_get_bios_year();
+
+	if (year >= 0 && year < 2008 && iomem_resource.end <= 0xffffffff)
 		pci_use_crs = false;
 
 	dmi_check_system(pci_crs_quirks);

Signature · 1 line

2.16.2

Jean Delvare

Reviewed-by on v3 1/2

2018-03-20 15:33 UTC #2 lore raw

Re: [PATCH v3 1/2] x86/PCI: Fix a potential regression when use dmi_get_bios_year()

Hi Andy,

Quoted 5 lines · Andy Shevchenko

dmi_get_bios_year() may return 0 when it is incapable to parse the BIOS date string. Previously this has been checked in pci_acpi_crs_quirks().
Update the code to restore old behaviour.

When fixing a regression, a good practice is to tag the fix commit as such:

Fixes 69c42d493db4 ("x86/pci: Simplify code by using the new dmi_get_bios_year() helper")

Quoted 11 lines

Reported-by Jean Delvare <[email protected]>
Signed-off-by Andy Shevchenko <[email protected]>
---
 arch/x86/pci/acpi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
arch/x86/pci/acpi.c +0 -0
index 00e60de30328..5559dcaddd5e 100644

arch/x86/pci/acpi.c static const struct dmi_system_id pci_crs_quirks[] __initconst = { · line 148

Quoted 10 lines

arch/x86/pci/acpi.c +3 -1

@@ -140,7 +140,9 @@ static const struct dmi_system_id pci_crs_quirks[] __initconst = {
 
 void __init pci_acpi_crs_quirks(void)
 {
-	if ((dmi_get_bios_year() < 2008) && (iomem_resource.end <= 0xffffffff))
+	int year = dmi_get_bios_year();
+
+	if (year >= 0 && year < 2008 && iomem_resource.end <= 0xffffffff)
 		pci_use_crs = false;
 
 	dmi_check_system(pci_crs_quirks);

This works with the non-inline version of dmi_get_bios_year() introduced by patch 2/2 of this series. However the inline version of this function would return 0 on error, which makes the condition above always true.

The patches need to be applied in the opposite order, or the test above should be changed to "year > 0".

Reviewed-by Jean Delvare <[email protected]>

Signature · 2 lines

Jean Delvare
SUSE L3 Support

Andy Shevchenko

author

2018-03-20 15:49 UTC #3 lore raw

Re: [PATCH v3 1/2] x86/PCI: Fix a potential regression when use dmi_get_bios_year()

Quoted 7 lines · Jean Delvare

Hi Andy,
Quoted 4 lines · Andy Shevchenko
dmi_get_bios_year() may return 0 when it is incapable to parse the BIOS date string. Previously this has been checked in pci_acpi_crs_quirks().

Quoted 7 lines

Quoted 1 line
Update the code to restore old behaviour.
When fixing a regression, a good practice is to tag the fix commit as such:
Fixes 69c42d493db4 ("x86/pci: Simplify code by using the new
dmi_get_bios_year() helper")

Someone (perhaps Ingo) told me that Fixes doesn't make much sense if the fixing commit is not yet in vanilla.

Since it's supposed to go via tip tree I would like to hear Ingo's opinion before adding this line to the patch.

Quoted 30 lines

Quoted 21 lines
Reported-by Jean Delvare <[email protected]>
Signed-off-by Andy Shevchenko <[email protected]>
---
 arch/x86/pci/acpi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
arch/x86/pci/acpi.c +3 -1
index 00e60de30328..5559dcaddd5e 100644
@@ -140,7 +140,9 @@ static const struct dmi_system_id
 pci_crs_quirks[] __initconst = {
 
 void __init pci_acpi_crs_quirks(void)
 {
-	if ((dmi_get_bios_year() < 2008) && (iomem_resource.end <=
 0xffffffff))
+	int year = dmi_get_bios_year();
+
+	if (year >= 0 && year < 2008 && iomem_resource.end <=
 0xffffffff)
arch/x86/pci/acpi.c static const struct dmi_system_id pci_crs_quirks[] __initconst = { · line 148
Quoted 3 lines
arch/x86/pci/acpi.c +0 -0
 		pci_use_crs = false;
 
 	dmi_check_system(pci_crs_quirks);
This works with the non-inline version of dmi_get_bios_year() introduced by patch 2/2 of this series. However the inline version of this function would return 0 on error, which makes the condition above always true.

Quoted 3 lines

The patches need to be applied in the opposite order, or the test above
should be changed to "year > 0".

Err... Yes, I use to have them in opposite order, but at some point I decided to reverse and forgot about the condition.

So, you are right, it needs to be applied in reversed order.

Quoted 3 lines

Reviewed-by Jean Delvare <[email protected]>

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Thomas Gleixner

2018-03-20 16:47 UTC #4 lore raw

Re: [PATCH v3 1/2] x86/PCI: Fix a potential regression when use dmi_get_bios_year()

Quoted 10 lines · Andy Shevchenko

Quoted 9 lines · Jean Delvare
Hi Andy,
Quoted 5 lines · Andy Shevchenko
dmi_get_bios_year() may return 0 when it is incapable to parse the BIOS date string. Previously this has been checked in pci_acpi_crs_quirks().
Update the code to restore old behaviour.

Quoted 8 lines

Quoted 5 lines
When fixing a regression, a good practice is to tag the fix commit as such:
Fixes 69c42d493db4 ("x86/pci: Simplify code by using the new
dmi_get_bios_year() helper")
Someone (perhaps Ingo) told me that Fixes doesn't make much sense if the fixing commit is not yet in vanilla.

No. If the commit is already queued for the next merge window then a fixes tag is appropriate.

Thanks,

tglx

Andy Shevchenko

author

2018-03-20 17:18 UTC #5 lore raw

Re: [PATCH v3 1/2] x86/PCI: Fix a potential regression when use dmi_get_bios_year()

Quoted 10 lines · Thomas Gleixner

Quoted 9 lines · Andy Shevchenko
Quoted 6 lines · Jean Delvare
When fixing a regression, a good practice is to tag the fix commit as
such:
Fixes 69c42d493db4 ("x86/pci: Simplify code by using the new
dmi_get_bios_year() helper")

Quoted 7 lines

Quoted 3 lines
Someone (perhaps Ingo) told me that Fixes doesn't make much sense if the
fixing commit is not yet in vanilla.
No. If the commit is already queued for the next merge window then a fixes
tag is appropriate.

Thank you, Thomas, for clarification.
Will do this in v4.

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Jean Delvare

2018-03-20 18:34 UTC #6 lore raw

Re: [PATCH v3 1/2] x86/PCI: Fix a potential regression when use dmi_get_bios_year()

In reply to Thomas Gleixner

Quoted 5 lines · Thomas Gleixner

Quoted 4 lines · Andy Shevchenko
Quoted 3 lines · Jean Delvare
When fixing a regression, a good practice is to tag the fix commit as such:

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
Fixes 69c42d493db4 ("x86/pci: Simplify code by using the new
dmi_get_bios_year() helper")
Someone (perhaps Ingo) told me that Fixes doesn't make much sense if the fixing commit is not yet in vanilla.
No. If the commit is already queued for the next merge window then a fixes tag is appropriate.

Even if it won't be used for stable branches, it's still valuable for distribution kernel maintainers to find out the fixes to patches they backport.

Thanks,

Signature · 2 lines

Jean Delvare
SUSE L3 Support

Rafael J. Wysocki

Reviewed-by on v3 1/2

2018-03-20 16:53 UTC #7 lore raw

Re: [PATCH v3 1/2] x86/PCI: Fix a potential regression when use dmi_get_bios_year()

In reply to Andy Shevchenko

Quoted 18 lines · CET Andy Shevchenko

dmi_get_bios_year() may return 0 when it is incapable to parse the BIOS date string. Previously this has been checked in pci_acpi_crs_quirks().
Update the code to restore old behaviour.
Reported-by Jean Delvare <[email protected]>
Signed-off-by Andy Shevchenko <[email protected]>
---
 arch/x86/pci/acpi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
arch/x86/pci/acpi.c +0 -0
index 00e60de30328..5559dcaddd5e 100644

arch/x86/pci/acpi.c static const struct dmi_system_id pci_crs_quirks[] __initconst = { · line 148

Quoted 10 lines

arch/x86/pci/acpi.c +3 -1

@@ -140,7 +140,9 @@ static const struct dmi_system_id pci_crs_quirks[] __initconst = {
 
 void __init pci_acpi_crs_quirks(void)
 {
-	if ((dmi_get_bios_year() < 2008) && (iomem_resource.end <= 0xffffffff))
+	int year = dmi_get_bios_year();
+
+	if (year >= 0 && year < 2008 && iomem_resource.end <= 0xffffffff)
 		pci_use_crs = false;
 
 	dmi_check_system(pci_crs_quirks);

Reviewed-by Rafael J. Wysocki <[email protected]>

Andy Shevchenko

author

2018-03-20 13:54 UTC #8 lore raw

PATCH v3 2/2 firmware: dmi_scan: Uninline dmi_get_bios_year() helper

In reply to Andy Shevchenko

Uninline dmi_get_bios_year() which, in particular, allows us to optimize it in the future.

While doing this, convert the function to return an error code when BIOS date is not present, parsable or CONFIG_DMI=n.

Additionally, during the move, add a bit of documentation.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/firmware/dmi_scan.c | 20 ++++++++++++++++++++
 include/linux/dmi.h         | 11 ++---------
 2 files changed, 22 insertions(+), 9 deletions(-)

drivers/firmware/dmi_scan.c +20 -0

index ae4f1b181c23..bfb6ce6abc83 100644
@@ -1040,6 +1040,26 @@ bool dmi_get_date(int field, int *yearp, int *monthp, int *dayp)
 }
 EXPORT_SYMBOL(dmi_get_date);
 
+/**
+ *	dmi_get_bios_year - get a year out of DMI_BIOS_DATE field
+ *
+ *	Returns year on success, -ENXIO if DMI is not selected,
+ *	or a different negative error code if DMI field is not present
+ *	or not parsable.
+ */
+int dmi_get_bios_year(void)
+{
+	bool exists;
+	int year;
+
+	exists = dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL);
+	if (!exists)
+		return -ENODATA;
+
+	return year ? year : -ERANGE;
+}
+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 -9

index b5b8c26b04fb..c46fdb36700b 100644
@@ -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;

@@ -134,6 +135,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

@@ -149,13 +151,4 @@ static inline const struct dmi_system_id *
 
 #endif
 
-static inline int dmi_get_bios_year(void)
-{
-	int year;
-
-	dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL);
-
-	return year;
-}
-
 #endif	/* __DMI_H__ */

Signature · 1 line

2.16.2

Jean Delvare

Reviewed-by on v3 2/2

2018-03-20 15:44 UTC #9 lore raw

Re: [PATCH v3 2/2] firmware: dmi_scan: Uninline dmi_get_bios_year() helper

Hi Andy,

Quoted 5 lines · Andy Shevchenko

Uninline dmi_get_bios_year() which, in particular, allows us to optimize it in the future.
While doing this, convert the function to return an error code when BIOS date is not present, parsable or CONFIG_DMI=n.

I suggest "or not parsable,", else the sentence construct is confusing.

Quoted 66 lines

Additionally, during the move, add a bit of documentation.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/firmware/dmi_scan.c | 20 ++++++++++++++++++++
 include/linux/dmi.h         | 11 ++---------
 2 files changed, 22 insertions(+), 9 deletions(-)

drivers/firmware/dmi_scan.c +20 -0

index ae4f1b181c23..bfb6ce6abc83 100644
@@ -1040,6 +1040,26 @@ bool dmi_get_date(int field, int *yearp, int *monthp, int *dayp)
 }
 EXPORT_SYMBOL(dmi_get_date);
 
+/**
+ *	dmi_get_bios_year - get a year out of DMI_BIOS_DATE field
+ *
+ *	Returns year on success, -ENXIO if DMI is not selected,
+ *	or a different negative error code if DMI field is not present
+ *	or not parsable.
+ */
+int dmi_get_bios_year(void)
+{
+	bool exists;
+	int year;
+
+	exists = dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL);
+	if (!exists)
+		return -ENODATA;
+
+	return year ? year : -ERANGE;
+}
+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

index b5b8c26b04fb..c46fdb36700b 100644
@@ -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;

@@ -134,6 +135,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

@@ -149,13 +151,4 @@ static inline const struct dmi_system_id *
 
 #endif

include/linux/dmi.h static inline const struct dmi_system_id * · line 154

Quoted 10 lines

include/linux/dmi.h +0 -9

-static inline int dmi_get_bios_year(void)
-{
-	int year;
-
-	dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL);
-
-	return year;
-}
-
 #endif	/* __DMI_H__ */

Reviewed-by Jean Delvare <[email protected]>

Signature · 2 lines

Jean Delvare
SUSE L3 Support

Rafael J. Wysocki

Reviewed-by on v3 2/2

2018-03-20 16:54 UTC #10 lore raw

Re: [PATCH v3 2/2] firmware: dmi_scan: Uninline dmi_get_bios_year() helper

In reply to Andy Shevchenko

Quoted 72 lines · CET Andy Shevchenko

Uninline dmi_get_bios_year() which, in particular, allows us to optimize it in the future.

While doing this, convert the function to return an error code when BIOS date is not present, parsable or CONFIG_DMI=n.

Additionally, during the move, add a bit of documentation.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/firmware/dmi_scan.c | 20 ++++++++++++++++++++
 include/linux/dmi.h         | 11 ++---------
 2 files changed, 22 insertions(+), 9 deletions(-)

drivers/firmware/dmi_scan.c +20 -0

index ae4f1b181c23..bfb6ce6abc83 100644
@@ -1040,6 +1040,26 @@ bool dmi_get_date(int field, int *yearp, int *monthp, int *dayp)
 }
 EXPORT_SYMBOL(dmi_get_date);
 
+/**
+ *	dmi_get_bios_year - get a year out of DMI_BIOS_DATE field
+ *
+ *	Returns year on success, -ENXIO if DMI is not selected,
+ *	or a different negative error code if DMI field is not present
+ *	or not parsable.
+ */
+int dmi_get_bios_year(void)
+{
+	bool exists;
+	int year;
+
+	exists = dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL);
+	if (!exists)
+		return -ENODATA;
+
+	return year ? year : -ERANGE;
+}
+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

index b5b8c26b04fb..c46fdb36700b 100644
@@ -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;

@@ -134,6 +135,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

@@ -149,13 +151,4 @@ static inline const struct dmi_system_id *
 
 #endif

include/linux/dmi.h static inline const struct dmi_system_id * · line 154

Quoted 10 lines

include/linux/dmi.h +0 -9

-static inline int dmi_get_bios_year(void)
-{
-	int year;
-
-	dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL);
-
-	return year;
-}
-
 #endif	/* __DMI_H__ */

Reviewed-by Rafael J. Wysocki <[email protected]>