[PATCH v1 1/4] dmi: Introduce dmi_get_bios_year() helper

[PATCH v1 1/4] dmi: Introduce dmi_get_bios_year() helper

[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 <andriy.shevchenko@linux.intel.com>
---
 include/linux/dmi.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index 46e151172d95..241c27008c70 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -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;
+}
+
 #endif	/* __DMI_H__ */
-- 
2.16.1

[PATCH v1 2/4] x86/pci: Re-use new dmi_get_bios_year() helper

[Andy Shevchenko]

...instead of open coding its functionality.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 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(-)

diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index 7df49c40665e..00e60de30328 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -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);
 
diff --git a/arch/x86/pci/direct.c b/arch/x86/pci/direct.c
index 2d9503323d10..a51074c55982 100644
--- a/arch/x86/pci/direct.c
+++ b/arch/x86/pci/direct.c
@@ -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++) {
diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
index 96684d0adcf9..0b40482578b8 100644
--- a/arch/x86/pci/mmconfig-shared.c
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -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,
-- 
2.16.1

[tip:x86/platform] x86/pci: Simplify code by using the new dmi_get_bios_year() helper

[tip-bot for Andy Shevchenko]

Commit-ID:  69c42d493db452ea87c1ac56e83c978512f4e6ec
Gitweb:     https://git.kernel.org/tip/69c42d493db452ea87c1ac56e83c978512f4e6ec
Author:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
AuthorDate: Thu, 22 Feb 2018 14:59:21 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 23 Feb 2018 08:20:30 +0100

x86/pci: Simplify code by using the new dmi_get_bios_year() helper

...instead of open coding its functionality.

No changes in functionality.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Jean Delvare <jdelvare@suse.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-acpi@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Link: http://lkml.kernel.org/r/20180222125923.57385-2-andriy.shevchenko@linux.intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 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(-)

diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index 7df49c40665e..00e60de30328 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -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);
 
diff --git a/arch/x86/pci/direct.c b/arch/x86/pci/direct.c
index 2d9503323d10..a51074c55982 100644
--- a/arch/x86/pci/direct.c
+++ b/arch/x86/pci/direct.c
@@ -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++) {
diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
index 96684d0adcf9..0b40482578b8 100644
--- a/arch/x86/pci/mmconfig-shared.c
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -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,

Re: [PATCH v1 2/4] x86/pci: Re-use new dmi_get_bios_year() helper

[Bjorn Helgaas]

x86/PCI: Re-use ...

would match the arch/x86/pci/* convention.

On Thu, Feb 22, 2018 at 02:59:21PM +0200, Andy Shevchenko wrote:
> ...instead of open coding its functionality.

And I personally like it when changelogs are complete in themselves,
i.e., they aren't a continuation of the subject line.  It makes things
easier to read because the convention in written English is that the
title and the body are separate things.

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Assuming this goes via some other tree.

> ---
>  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(-)
> 
> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> index 7df49c40665e..00e60de30328 100644
> --- a/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -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);
>  
> diff --git a/arch/x86/pci/direct.c b/arch/x86/pci/direct.c
> index 2d9503323d10..a51074c55982 100644
> --- a/arch/x86/pci/direct.c
> +++ b/arch/x86/pci/direct.c
> @@ -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++) {
> diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
> index 96684d0adcf9..0b40482578b8 100644
> --- a/arch/x86/pci/mmconfig-shared.c
> +++ b/arch/x86/pci/mmconfig-shared.c
> @@ -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,
> -- 
> 2.16.1
> 

Re: [PATCH v1 2/4] x86/pci: Re-use new dmi_get_bios_year() helper

[Jean Delvare]

Hi Andy,

On Thu, 22 Feb 2018 14:59:21 +0200, Andy Shevchenko wrote:
> ...instead of open coding its functionality.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  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(-)
> 
> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> index 7df49c40665e..00e60de30328 100644
> --- a/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -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;

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.

Also there are more parentheses than needed.

>  
>  	dmi_check_system(pci_crs_quirks);
>  
> diff --git a/arch/x86/pci/direct.c b/arch/x86/pci/direct.c
> index 2d9503323d10..a51074c55982 100644
> --- a/arch/x86/pci/direct.c
> +++ b/arch/x86/pci/direct.c
> @@ -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++) {
> diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
> index 96684d0adcf9..0b40482578b8 100644
> --- a/arch/x86/pci/mmconfig-shared.c
> +++ b/arch/x86/pci/mmconfig-shared.c
> @@ -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;

Here too some parentheses are not needed.

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


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v1 2/4] x86/pci: Re-use new dmi_get_bios_year() helper

[Andy Shevchenko]

On Mon, 2018-02-26 at 17:28 +0100, Jean Delvare wrote:

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

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

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

Agree.

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

That's correct.

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

Re: [PATCH v1 2/4] x86/pci: Re-use new dmi_get_bios_year() helper

[Rafael J. Wysocki]

On Wed, Feb 28, 2018 at 11:29 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, 2018-02-26 at 17:28 +0100, Jean Delvare wrote:
>
>> > -   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;
>>
>> 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.
>
>> 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.

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.

Re: [PATCH v1 2/4] x86/pci: Re-use new dmi_get_bios_year() helper

[Jean Delvare]

On Wed, 28 Feb 2018 11:33:39 +0100, Rafael J. Wysocki wrote:
> On Wed, Feb 28, 2018 at 11:29 AM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > 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.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v1 2/4] x86/pci: Re-use new dmi_get_bios_year() helper

[Andy Shevchenko]

On Wed, 2018-02-28 at 20:21 +0100, Jean Delvare wrote:
> On Wed, 28 Feb 2018 11:33:39 +0100, Rafael J. Wysocki wrote:
> > On Wed, Feb 28, 2018 at 11:29 AM, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > 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.

Yep.

And here is a very good question which path is more safer: use _CRS, or
not?

Rafael, do you know any consequences of not using _CRS for PCI on older
and newer machines?

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

[PATCH v1 3/4] ACPI / sleep: Re-use new dmi_get_bios_year() helper

[Andy Shevchenko]

...instead of open coding its functionality.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/sleep.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 46cde0912762..b35923e3a926 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -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);
-- 
2.16.1

[tip:x86/platform] ACPI/sleep: Simplify code by using the new dmi_get_bios_year() helper

[tip-bot for Andy Shevchenko]

Commit-ID:  9c0a30b67bd916a7da1a7101dbf3115e5f10d852
Gitweb:     https://git.kernel.org/tip/9c0a30b67bd916a7da1a7101dbf3115e5f10d852
Author:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
AuthorDate: Thu, 22 Feb 2018 14:59:22 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 23 Feb 2018 08:20:30 +0100

ACPI/sleep: Simplify code by using the new dmi_get_bios_year() helper

...instead of open coding its functionality.

No changes in functionality.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Jean Delvare <jdelvare@suse.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-acpi@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Link: http://lkml.kernel.org/r/20180222125923.57385-3-andriy.shevchenko@linux.intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/acpi/sleep.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 46cde0912762..b35923e3a926 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -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);

Re: [PATCH v1 3/4] ACPI / sleep: Re-use new dmi_get_bios_year() helper

[Jean Delvare]

On Thu, 22 Feb 2018 14:59:22 +0200, Andy Shevchenko wrote:
> ...instead of open coding its functionality.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/acpi/sleep.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 46cde0912762..b35923e3a926 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -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);

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

-- 
Jean Delvare
SUSE L3 Support

[PATCH v1 4/4] pci: Re-use new dmi_get_bios_year() helper

[Andy Shevchenko]

...instead of open coding its functionality.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pci/pci.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index f6a4dd10d9b0..ae654e21439d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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;
 	}
 
-- 
2.16.1

[tip:x86/platform] pci: Simplify code by using the new dmi_get_bios_year() helper

[tip-bot for Andy Shevchenko]

Commit-ID:  ac95090a0440be1b17aa1b5d9462d9c67146ce0b
Gitweb:     https://git.kernel.org/tip/ac95090a0440be1b17aa1b5d9462d9c67146ce0b
Author:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
AuthorDate: Thu, 22 Feb 2018 14:59:23 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 23 Feb 2018 08:20:31 +0100

pci: Simplify code by using the new dmi_get_bios_year() helper

...instead of open coding its functionality.

No changes in functionality.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Jean Delvare <jdelvare@suse.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-acpi@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Link: http://lkml.kernel.org/r/20180222125923.57385-4-andriy.shevchenko@linux.intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/pci/pci.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index f6a4dd10d9b0..ae654e21439d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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;
 	}
 

Re: [PATCH v1 4/4] pci: Re-use new dmi_get_bios_year() helper

[Bjorn Helgaas]

Please use

  PCI: Re-use ...

to match the prevailing drivers/pci style.

On Thu, Feb 22, 2018 at 02:59:23PM +0200, Andy Shevchenko wrote:
> ...instead of open coding its functionality.

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

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

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

> ---
>  drivers/pci/pci.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index f6a4dd10d9b0..ae654e21439d 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -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;
>  	}
>  
> -- 
> 2.16.1
> 

Re: [PATCH v1 4/4] pci: Re-use new dmi_get_bios_year() helper

[Andy Shevchenko]

On Fri, 2018-02-23 at 15:40 -0600, Bjorn Helgaas wrote:
> Please use
> 
>   PCI: Re-use ...
> 
> to match the prevailing drivers/pci style.

Noted. (Same for x86/PCI part)

> On Thu, Feb 22, 2018 at 02:59:23PM +0200, Andy Shevchenko wrote:
> > ...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)

> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Thanks!

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

Ingo took them into x86/platform.

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

Re: [PATCH v1 4/4] pci: Re-use new dmi_get_bios_year() helper

[Bjorn Helgaas]

On Sun, Feb 25, 2018 at 03:27:04PM +0200, Andy Shevchenko wrote:
> On Fri, 2018-02-23 at 15:40 -0600, Bjorn Helgaas wrote:
> > On Thu, Feb 22, 2018 at 02:59:23PM +0200, Andy Shevchenko wrote:
> > > ...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.

Bjorn

Re: [PATCH v1 4/4] pci: Re-use new dmi_get_bios_year() helper

[Andy Shevchenko]

On Mon, 2018-02-26 at 12:19 -0600, Bjorn Helgaas wrote:
> On Sun, Feb 25, 2018 at 03:27:04PM +0200, Andy Shevchenko wrote:
> > On Fri, 2018-02-23 at 15:40 -0600, Bjorn Helgaas wrote:
> > > On Thu, Feb 22, 2018 at 02:59:23PM +0200, Andy Shevchenko wrote:
> > > > ...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.
> 

Thank you for a hint!

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

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


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

Re: [PATCH v1 4/4] pci: Re-use new dmi_get_bios_year() helper

[Bjorn Helgaas]

On Wed, Feb 28, 2018 at 12:12:22PM +0200, Andy Shevchenko wrote:
> On Mon, 2018-02-26 at 12:19 -0600, Bjorn Helgaas wrote:
> > On Sun, Feb 25, 2018 at 03:27:04PM +0200, Andy Shevchenko wrote:
> > > On Fri, 2018-02-23 at 15:40 -0600, Bjorn Helgaas wrote:
> > > > On Thu, Feb 22, 2018 at 02:59:23PM +0200, Andy Shevchenko wrote:
> > > > > ...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.
> > 
> 
> Thank you for a hint!
> 
> > 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.
> 
> OK, I got your point, though I don't like duplication in the subject and
> body.

Ah, I see.  I think of the subject and the body as serving two
distinct purposes, so for me there's no issue even if they happen to
contain exactly the same text.

Bjorn

Re: [PATCH v1 4/4] pci: Re-use new dmi_get_bios_year() helper

[Jean Delvare]

On Thu, 22 Feb 2018 14:59:23 +0200, Andy Shevchenko wrote:
> ...instead of open coding its functionality.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pci/pci.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index f6a4dd10d9b0..ae654e21439d 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -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;
>  	}
>  

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

-- 
Jean Delvare
SUSE L3 Support

[tip:x86/platform] dmi: Introduce the dmi_get_bios_year() helper function

[tip-bot for Andy Shevchenko]

Commit-ID:  492a1abd61e4b4f78c1c5804840a304a9e32da04
Gitweb:     https://git.kernel.org/tip/492a1abd61e4b4f78c1c5804840a304a9e32da04
Author:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
AuthorDate: Thu, 22 Feb 2018 14:59:20 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 23 Feb 2018 08:20:30 +0100

dmi: Introduce the dmi_get_bios_year() helper function

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 <andriy.shevchenko@linux.intel.com>
[ Minor stylistic cleanup. ]
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Jean Delvare <jdelvare@suse.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-acpi@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Link: http://lkml.kernel.org/r/20180222125923.57385-1-andriy.shevchenko@linux.intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/dmi.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index 46e151172d95..0bade156e908 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -147,4 +147,13 @@ 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__ */

Re: [PATCH v1 1/4] dmi: Introduce dmi_get_bios_year() helper

[Bjorn Helgaas]

On Thu, Feb 22, 2018 at 02:59:20PM +0200, Andy Shevchenko wrote:
> 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 <andriy.shevchenko@linux.intel.com>
> ---
>  include/linux/dmi.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
> index 46e151172d95..241c27008c70 100644
> --- a/include/linux/dmi.h
> +++ b/include/linux/dmi.h
> @@ -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-6ZJ6szF=2KVLqcbwCUGd+gTwPViRqRWN+g@mail.gmail.com

>  #endif	/* __DMI_H__ */
> -- 
> 2.16.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 1/4] dmi: Introduce dmi_get_bios_year() helper

[Andy Shevchenko]

On Fri, 2018-02-23 at 15:35 -0600, Bjorn Helgaas wrote:
> On Thu, Feb 22, 2018 at 02:59:20PM +0200, Andy Shevchenko wrote:
> > 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 <andriy.shevchenko@linux.intel.com>
> > ---
> >  include/linux/dmi.h | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/include/linux/dmi.h b/include/linux/dmi.h
> > index 46e151172d95..241c27008c70 100644
> > --- a/include/linux/dmi.h
> > +++ b/include/linux/dmi.h
> > @@ -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-
> 6ZJ6szF=2KVLqcbwCUGd+gTwPViRqRWN+g@mail.gmail.com

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

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

Re: [PATCH v1 1/4] dmi: Introduce dmi_get_bios_year() helper

[Rafael J. Wysocki]

On Sun, Feb 25, 2018 at 2:23 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Fri, 2018-02-23 at 15:35 -0600, Bjorn Helgaas wrote:
>> On Thu, Feb 22, 2018 at 02:59:20PM +0200, Andy Shevchenko wrote:
>> > 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 <andriy.shevchenko@linux.intel.com>
>> > ---
>> >  include/linux/dmi.h | 7 +++++++
>> >  1 file changed, 7 insertions(+)
>> >
>> > diff --git a/include/linux/dmi.h b/include/linux/dmi.h
>> > index 46e151172d95..241c27008c70 100644
>> > --- a/include/linux/dmi.h
>> > +++ b/include/linux/dmi.h
>> > @@ -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-
>> 6ZJ6szF=2KVLqcbwCUGd+gTwPViRqRWN+g@mail.gmail.com
>
> "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.

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?

Re: [PATCH v1 1/4] dmi: Introduce dmi_get_bios_year() helper

[Andy Shevchenko]

On Mon, 2018-02-26 at 10:29 +0100, Rafael J. Wysocki wrote:
> On Sun, Feb 25, 2018 at 2:23 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, 2018-02-23 at 15:35 -0600, Bjorn Helgaas wrote:

> > > but making this inline looks similar to this, which
> > > wasn't well-received:
> > > 
> > > https://lkml.kernel.org/r/CA+55aFypU331cQy-
> > > 6ZJ6szF=2KVLqcbwCUGd+gTwPViRqRWN+g@mail.gmail.com
> > 
> > "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.
> 
> 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?

Taking into consideration that there are few comments to be addressed
and series had been applied, I will send a fix up series where I move
this to be a normal function.

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

Re: [PATCH v1 1/4] dmi: Introduce dmi_get_bios_year() helper

[Jean Delvare]

Hi Andy,

On Thu, 22 Feb 2018 14:59:20 +0200, Andy Shevchenko wrote:
> 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.

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/linux/dmi.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
> index 46e151172d95..241c27008c70 100644
> --- a/include/linux/dmi.h
> +++ b/include/linux/dmi.h
> @@ -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;
> +}

checkpatch complains:

WARNING: Missing a blank line after declarations
#61: FILE: include/linux/dmi.h:153:
+	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.

> +
>  #endif	/* __DMI_H__ */

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v1 1/4] dmi: Introduce dmi_get_bios_year() helper

[Andy Shevchenko]

On Tue, 2018-02-27 at 10:14 +0100, Jean Delvare wrote:
> Hi Andy,
> 
> On Thu, 22 Feb 2018 14:59:20 +0200, Andy Shevchenko wrote:
> > 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.

Yes, we better to return -ERRNO in such case and allow callers to behave
correspondingly.

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

So, what I consider is to
- move inline function to be regular one
- optimize it with current dmi_get_date()
- return error code when year is not parsable
- consider current use cases where we do compare for less than a given
year

Does it sound a correct approach?

> 
> checkpatch complains:
> 
> WARNING: Missing a blank line after declarations
> #61: FILE: include/linux/dmi.h:153:
> +	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.

Ingo fixed that.

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