| Message ID | 20180222125923.57385-2-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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 >
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,
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.
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.
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.
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?
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,
...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(-)