[PATCH v3 0/2] x86, dmi: Uninline dmi_get_bios_year()

[PATCH v3 0/2] x86, dmi: Uninline dmi_get_bios_year()

[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.

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(-)

-- 
2.16.2

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

[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 <jdelvare@suse.de>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/pci/acpi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index 00e60de30328..5559dcaddd5e 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -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);
-- 
2.16.2

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

[Jean Delvare]

Hi Andy,

On Tue, 20 Mar 2018 15:54:09 +0200, Andy Shevchenko wrote:
> 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")

> Reported-by: Jean Delvare <jdelvare@suse.de>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  arch/x86/pci/acpi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> index 00e60de30328..5559dcaddd5e 100644
> --- a/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -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 <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

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

[Andy Shevchenko]

On Tue, 2018-03-20 at 16:32 +0100, Jean Delvare wrote:
> Hi Andy,
> 
> On Tue, 20 Mar 2018 15:54:09 +0200, Andy Shevchenko wrote:
> > 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")

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.

> > Reported-by: Jean Delvare <jdelvare@suse.de>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  arch/x86/pci/acpi.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> > index 00e60de30328..5559dcaddd5e 100644
> > --- a/arch/x86/pci/acpi.c
> > +++ b/arch/x86/pci/acpi.c
> > @@ -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".

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.

> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Thomas Gleixner]

On Tue, 20 Mar 2018, Andy Shevchenko wrote:
> On Tue, 2018-03-20 at 16:32 +0100, Jean Delvare wrote:
> > Hi Andy,
> > 
> > On Tue, 20 Mar 2018 15:54:09 +0200, Andy Shevchenko wrote:
> > > 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")
> 
> 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

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

[Andy Shevchenko]

On Tue, 2018-03-20 at 17:47 +0100, Thomas Gleixner wrote:
> On Tue, 20 Mar 2018, Andy Shevchenko wrote:
> > On Tue, 2018-03-20 at 16:32 +0100, Jean Delvare wrote:

> > > 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.

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

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Jean Delvare]

On Tue, 20 Mar 2018 17:47:41 +0100 (CET), Thomas Gleixner wrote:
> On Tue, 20 Mar 2018, Andy Shevchenko wrote:
> > On Tue, 2018-03-20 at 16:32 +0100, Jean Delvare wrote:  
> > > 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.

+1

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,
-- 
Jean Delvare
SUSE L3 Support

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

[Rafael J. Wysocki]

On Tuesday, March 20, 2018 2:54:09 PM CET Andy Shevchenko wrote:
> 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 <jdelvare@suse.de>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  arch/x86/pci/acpi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> index 00e60de30328..5559dcaddd5e 100644
> --- a/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -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 <rafael.j.wysocki@intel.com>

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

[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.

Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/firmware/dmi_scan.c | 20 ++++++++++++++++++++
 include/linux/dmi.h         | 11 ++---------
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index ae4f1b181c23..bfb6ce6abc83 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -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
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index b5b8c26b04fb..c46fdb36700b 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -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__ */
-- 
2.16.2

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

[Jean Delvare]

Hi Andy,

On Tue, 20 Mar 2018 15:54:10 +0200, Andy Shevchenko wrote:
> 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.

> 
> Additionally, during the move, add a bit of documentation.
> 
> Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/firmware/dmi_scan.c | 20 ++++++++++++++++++++
>  include/linux/dmi.h         | 11 ++---------
>  2 files changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index ae4f1b181c23..bfb6ce6abc83 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -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
> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
> index b5b8c26b04fb..c46fdb36700b 100644
> --- a/include/linux/dmi.h
> +++ b/include/linux/dmi.h
> @@ -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__ */

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

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

[Rafael J. Wysocki]

On Tuesday, March 20, 2018 2:54:10 PM CET Andy Shevchenko wrote:
> 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.
> 
> Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/firmware/dmi_scan.c | 20 ++++++++++++++++++++
>  include/linux/dmi.h         | 11 ++---------
>  2 files changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index ae4f1b181c23..bfb6ce6abc83 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -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
> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
> index b5b8c26b04fb..c46fdb36700b 100644
> --- a/include/linux/dmi.h
> +++ b/include/linux/dmi.h
> @@ -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__ */
> 

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>