dmi: Introduce dmi_get_bios_year() helper

...instead of open coding its functionality.

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

---
 arch/x86/pci/acpi.c            | 8 ++------
 arch/x86/pci/direct.c          | 5 ++---
 arch/x86/pci/mmconfig-shared.c | 9 ++-------
 3 files changed, 6 insertions(+), 16 deletions(-)

arch/x86/pci/acpi.c +2 -6

index 7df49c40665e..00e60de30328 100644
@@ -140,12 +140,8 @@ static const struct dmi_system_id pci_crs_quirks[] __initconst = {
 
 void __init pci_acpi_crs_quirks(void)
 {
-	int year;
-
-	if (dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL) && year < 2008) {
-		if (iomem_resource.end <= 0xffffffff)
-			pci_use_crs = false;
-	}
+	if ((dmi_get_bios_year() < 2008) && (iomem_resource.end <= 0xffffffff))
+		pci_use_crs = false;
 
 	dmi_check_system(pci_crs_quirks);
 

arch/x86/pci/direct.c +2 -3

index 2d9503323d10..a51074c55982 100644
@@ -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

index 96684d0adcf9..0b40482578b8 100644
@@ -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

...instead of open coding its functionality.

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

---
 arch/x86/pci/acpi.c            | 8 ++------
 arch/x86/pci/direct.c          | 5 ++---
 arch/x86/pci/mmconfig-shared.c | 9 ++-------
 3 files changed, 6 insertions(+), 16 deletions(-)

arch/x86/pci/acpi.c +0 -0

index 7df49c40665e..00e60de30328 100644
@@ -140,12 +140,8 @@ static const struct dmi_system_id pci_crs_quirks[] __initconst = {
 

arch/x86/pci/acpi.c +0 -0

 	dmi_check_system(pci_crs_quirks);
 

arch/x86/pci/direct.c +2 -3

index 2d9503323d10..a51074c55982 100644
@@ -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 +0 -2

index 96684d0adcf9..0b40482578b8 100644
@@ -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;
 

arch/x86/pci/mmconfig-shared.c +0 -0

 	pr_err(PREFIX "MCFG region for %04x [bus %02x-%02x] at %#llx "
 	       "is above 4GB, ignored\n", cfg->pci_segment,

Quoted 5 lines

(quoted diff) +0 -1

-	if (dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL) && year

< 2008) {

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

-		if (iomem_resource.end <= 0xffffffff)
-			pci_use_crs = false;
-	}

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

Quoted 3 lines

(quoted diff) +1 -0

+	if ((dmi_get_bios_year() < 2008) && (iomem_resource.end <=

0xffffffff))

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

+		pci_use_crs = false;

You are changing the behavior here, when DMI does not provide a BIOS date. Beforehand, the test would fail and pci_use_crs would be left alone. After your change, dmi_get_bios_year() will return 0, and "0 < 2008" is true, so pci_use_crs is set to false.

I have no opinion on what this driver should do in such case,

but I
certainly wouldn't expect a change of behavior from a "use a helper instead of open-coding" kind of patch.

I don't think you can safely assume that the code calling dmi_get_bios_year() will always do the right thing when 0 is being returned. It's up to the calling code to decide what the default behavior should be.

Quoted 9 lines · Jean Delvare

Quoted 8 lines

- if (dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL) && year
< 2008) {

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

-           if (iomem_resource.end <= 0xffffffff)
-                   pci_use_crs = false;
-   }
+   if ((dmi_get_bios_year() < 2008) && (iomem_resource.end <=

0xffffffff))
+ pci_use_crs = false;

Quoted 4 lines

You are changing the behavior here, when DMI does not provide a BIOS date. Beforehand, the test would fail and pci_use_crs would be left alone. After your change, dmi_get_bios_year() will return 0, and "0 < 2008" is true, so pci_use_crs is set to false.

Hmm... Indeed.

Quoted 1 line

I have no opinion on what this driver should do in such case,

I would assume that no BIOS date is related to prehistoric firmwares and
using _CRS would sound weird on them.

Quoted 2 lines · Andy Shevchenko

I would assume that no BIOS date is related to prehistoric firmwares and
using _CRS would sound weird on them.

Careful here.

You seem to be assuming that the DMI information is always valid and/or complete which is know to not be the case sometimes.

Quoted 10 lines · Rafael J. Wysocki

Quoted 3 lines · Andy Shevchenko

I would assume that no BIOS date is related to prehistoric firmwares and
using _CRS would sound weird on them.

Careful here.

You seem to be assuming that the DMI information is always valid and/or complete which is know to not be the case sometimes.

True. While the BIOS date is not the worst offender when it comes to broken DMI data, you must remember that the date comes as a string, and
older SMBIOS specifications did not even recommend a specific format for that string. As a matter of fact, my collection of DMI tables includes a few creative samples like "Jul 7 2016" or "09-16-08" which the kernel fails to parse.

So the default behavior at the driver level shouldn't be based on what older systems are most likely to enjoy. The default behavior must be the safest option, regardless of the age of the system.

...instead of open coding its functionality.

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

---
 drivers/acpi/sleep.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/acpi/sleep.c +0 -0

index 46cde0912762..b35923e3a926 100644
@@ -376,12 +376,10 @@ void __init acpi_sleep_no_blacklist(void)
 
 static void __init acpi_sleep_dmi_check(void)
 {

...instead of open coding its functionality.

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

---
 drivers/pci/pci.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

drivers/pci/pci.c +1 -5

index f6a4dd10d9b0..ae654e21439d 100644
@@ -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

Please use

PCI: Re-use ...

to match the prevailing drivers/pci style.

Quoted 1 line

...instead of open coding its functionality.

Same comment about making the changelog complete, independent of the subject.

Quoted 1 line

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

Acked-by Bjorn Helgaas <[email protected]>

Let me know if you want me to take the series. Otherwise I'll assume it
goes elsewhere.

Quoted 5 lines · Bjorn Helgaas

Quoted 1 line · Andy Shevchenko

...instead of open coding its functionality.

Same comment about making the changelog complete, independent of the subject.

Any suggestion how it would look like? (Same question for previous comment)

Quoted 10 lines · Andy Shevchenko

Quoted 6 lines · Bjorn Helgaas

Quoted 1 line · Andy Shevchenko

...instead of open coding its functionality.

Same comment about making the changelog complete, independent of the
subject.

Any suggestion how it would look like? (Same question for previous comment)

PCI: Re-use new dmi_get_bios_year() helper

Use new dmi_get_bios_year() helper instead of open-coding its functionality.

The usual document structure is something like:

TITLE

This abstract contains a summary of the entire document, in a few paragraphs of complete sentences.

Where "TITLE" makes sense all by itself, even without reading the body, and "Body" is a complete statement that also makes sense all by itself without having to read "TITLE" first.

Granted, it's trivial, but following the convention improves readability slightly because it fits the reader's expectations.

When the body is "...instead of open coding its functionality", it's a bit of a hiccup because I have to start over and look back up to the title to re-read the thing as a whole.

Quoted 19 lines · Bjorn Helgaas

Quoted 10 lines · Andy Shevchenko

Quoted 6 lines · Bjorn Helgaas

Quoted 1 line · Andy Shevchenko

...instead of open coding its functionality.

Same comment about making the changelog complete, independent of the
subject.

Any suggestion how it would look like? (Same question for previous comment)

PCI: Re-use new dmi_get_bios_year() helper

Use new dmi_get_bios_year() helper instead of open-coding its functionality.

The usual document structure is something like:

Quoted 9 lines

TITLE

This abstract contains a summary of the entire document, in a few paragraphs of complete sentences.

Where "TITLE" makes sense all by itself, even without reading the body, and "Body" is a complete statement that also makes sense all by itself without having to read "TITLE" first.

Thank you for a hint!

Quoted 2 lines

Granted, it's trivial, but following the convention improves readability slightly because it fits the reader's expectations.

Quoted 3 lines

When the body is "...instead of open coding its functionality", it's a bit of a hiccup because I have to start over and look back up to the title to re-read the thing as a whole.

OK, I got your point, though I don't like duplication in the subject and body.

...instead of open coding its functionality.

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

---
 drivers/pci/pci.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

drivers/pci/pci.c +0 -2

index f6a4dd10d9b0..ae654e21439d 100644
@@ -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

It's used in several places and more users may come. By using this helper they may create a slightly cleaner code.

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

---
 include/linux/dmi.h | 7 +++++++
 1 file changed, 7 insertions(+)

include/linux/dmi.h +0 -0

index 46e151172d95..241c27008c70 100644

include/linux/dmi.h +0 -0

 #endif	/* __DMI_H__ */

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 13 lines · Andy Shevchenko

It's used in several places and more users may come. By using this helper they may create a slightly cleaner code.

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

---
 include/linux/dmi.h | 7 +++++++
 1 file changed, 7 insertions(+)

include/linux/dmi.h +0 -0

index 46e151172d95..241c27008c70 100644

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

Quoted 9 lines

include/linux/dmi.h +6 -0

@@ -147,4 +147,11 @@ 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;
+}

I don't really care personally, and I assume this series will go via a non-PCI tree, but making this inline looks similar to this, which wasn't well-received:

https://lkml.kernel.org/r/CA+55aFypU331cQy-
[email protected]

Quoted 28 lines · Bjorn Helgaas

Quoted 13 lines · Andy Shevchenko

It's used in several places and more users may come. By using this helper they may create a slightly cleaner code.

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

---
 include/linux/dmi.h | 7 +++++++
 1 file changed, 7 insertions(+)

include/linux/dmi.h +0 -0

index 46e151172d95..241c27008c70 100644

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

Quoted 9 lines

include/linux/dmi.h +6 -0

@@ -147,4 +147,11 @@ 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;
+}

I don't really care personally, and I assume this series will go via a non-PCI tree, but making this inline looks similar to this, which wasn't well-received:

Quoted 2 lines

https://lkml.kernel.org/r/CA+55aFypU331cQy-
[email protected]

"Yes, that header file is already full of random inline functions, but they are generally wrapper functions that don't really do anything, ..."

I think the function above is exactly from the "wrapper that doesn't really do anything" category.

Quoted 13 lines · Andy Shevchenko

Quoted 5 lines · Bjorn Helgaas

but making this inline looks similar to this, which wasn't well-received:

https://lkml.kernel.org/r/CA+55aFypU331cQy-
[email protected]

"Yes, that header file is already full of random inline functions, but
they are generally wrapper functions that don't really do anything, ..."

Quoted 2 lines

I think the function above is exactly from the "wrapper that doesn't really do anything" category.

Yes, but honestly does it need to be inline even so?

Why don't you simply put the wrapper into dmi_scan.c and export it?

It's used in several places and more users may come. By using this helper they may create a slightly cleaner code.

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

---
 include/linux/dmi.h | 7 +++++++
 1 file changed, 7 insertions(+)

include/linux/dmi.h +0 -0

index 46e151172d95..241c27008c70 100644

include/linux/dmi.h +1 -0

+
 #endif	/* __DMI_H__ */

Hi Andy,

Quoted 2 lines · Andy Shevchenko

It's used in several places and more users may come. By using this helper they may create a slightly cleaner code.

In general I'm not a big fan of arbitrary API shortcuts. I won't object
if you think it's a good one to have, however I'm a bit concerned about
the silent handling of the "error case", which could cause unexpected default decisions as seen in patch 2/4 of this series.

The fact that
dmi_get_bios_year() returns 0 if there is no DMI BIOS date provided should at least be documented, to limit the risk.

I also wonder if dmi_get_date() itself couldn't be optimized a bit if it turns out that most callers are only interested in the year. Currently it will parse the whole string even if the caller isn't interested in the month and day.

The fact that dmi_get_date() returns true even if it couldn't parse the
date string at all is also strange, although unrelated with your current work.

checkpatch complains:

WARNING: Missing a blank line after declarations
#61: FILE: include/linux/dmi.h:153:

include/linux/dmi.h +2 -0

+	int year;
+	dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL);

And I would tend to agree. Just because it is an inline function in a header file doesn't mean we don't stick to the usual coding style policy.