[PATCH] x86, HPET: ignore any PCI BARs that match an HPET we already know about

[PATCH] x86, HPET: ignore any PCI BARs that match an HPET we already know about

[Bjorn Helgaas]

We often discover the HPET early, via the static ACPI HPET table, before
enumerating PCI devices.  If the HPET is implemented as a PCI function,
we will discover it again during PCI device enumeration.  We must ignore
the PCI function so we don't inadvertently move it out from under the
driver.

I think it's better to ignore *any* PCI BAR that matches a previously
discovered HPET; that way we don't need platform-specific knowledge,
and we won't have to add more quirks for future machines.

This is for a regression from 2.6.34, but the reporter has been
unable to test it yet.

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=18482
Reported-by: Simon Arlott <simon@fire.lp0.eu>
Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---

 arch/x86/kernel/quirks.c |   21 +++++++++++++++++++++
 arch/x86/pci/fixup.c     |   28 ----------------------------
 2 files changed, 21 insertions(+), 28 deletions(-)


diff --git a/arch/x86/kernel/quirks.c b/arch/x86/kernel/quirks.c
index 939b9e9..99a7d4b 100644
--- a/arch/x86/kernel/quirks.c
+++ b/arch/x86/kernel/quirks.c
@@ -507,6 +507,27 @@ static void force_disable_hpet_msi(struct pci_dev *unused)
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_SBX00_SMBUS,
 			 force_disable_hpet_msi);
 
+static void disable_pci_hpet(struct pci_dev *dev)
+{
+	int i;
+
+	if (!hpet_address)
+		return;
+
+	for (i = 0; i < 6; i++) {
+		struct resource *res = &dev->resource[i];
+
+		if (resource_type(res) == IORESOURCE_MEM &&
+		    res->start == hpet_address) {
+			dev_info(&dev->dev, "BAR %d: %pR is an HPET we found earlier; ignoring this BAR\n",
+				 i, res);
+			res->flags = 0;
+			res->start = 0;
+			res->end = 0;
+		}
+	}
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, disable_pci_hpet);
 #endif
 
 #if defined(CONFIG_PCI) && defined(CONFIG_NUMA)
diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index 6dd8955..08eba69 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -493,31 +493,3 @@ static void __devinit pci_siemens_interrupt_controller(struct pci_dev *dev)
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SIEMENS, 0x0015,
 			  pci_siemens_interrupt_controller);
-
-/*
- * SB600: Disable BAR1 on device 14.0 to avoid HPET resources from
- * confusing the PCI engine:
- */
-static void sb600_disable_hpet_bar(struct pci_dev *dev)
-{
-	u8 val;
-
-	/*
-	 * The SB600 and SB700 both share the same device
-	 * ID, but the PM register 0x55 does something different
-	 * for the SB700, so make sure we are dealing with the
-	 * SB600 before touching the bit:
-	 */
-
-	pci_read_config_byte(dev, 0x08, &val);
-
-	if (val < 0x2F) {
-		outb(0x55, 0xCD6);
-		val = inb(0xCD7);
-
-		/* Set bit 7 in PM register 0x55 */
-		outb(0x55, 0xCD6);
-		outb(val | 0x80, 0xCD7);
-	}
-}
-DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x4385, sb600_disable_hpet_bar);

Re: [PATCH] x86, HPET: ignore any PCI BARs that match an HPET we already know about

[Bjorn Helgaas]

On Wednesday, September 22, 2010 02:15:47 pm Bjorn Helgaas wrote:
> 
> We often discover the HPET early, via the static ACPI HPET table, before
> enumerating PCI devices.  If the HPET is implemented as a PCI function,
> we will discover it again during PCI device enumeration.  We must ignore
> the PCI function so we don't inadvertently move it out from under the
> driver.
> 
> I think it's better to ignore *any* PCI BAR that matches a previously
> discovered HPET; that way we don't need platform-specific knowledge,
> and we won't have to add more quirks for future machines.
> 
> This is for a regression from 2.6.34, but the reporter has been
> unable to test it yet.
> 
> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=18482

I've tried hard to find somebody who can test this, but nobody who
can reproduce the original failure has been able to test it.  I
propose that we put it in linux-next and see what happens there.

Bjorn

Re: [PATCH] x86, HPET: ignore any PCI BARs that match an HPET we already know about

[H. Peter Anvin]

On 09/22/2010 01:19 PM, Bjorn Helgaas wrote:
> On Wednesday, September 22, 2010 02:15:47 pm Bjorn Helgaas wrote:
>>
>> We often discover the HPET early, via the static ACPI HPET table, before
>> enumerating PCI devices.  If the HPET is implemented as a PCI function,
>> we will discover it again during PCI device enumeration.  We must ignore
>> the PCI function so we don't inadvertently move it out from under the
>> driver.
>>
>> I think it's better to ignore *any* PCI BAR that matches a previously
>> discovered HPET; that way we don't need platform-specific knowledge,
>> and we won't have to add more quirks for future machines.
>>
>> This is for a regression from 2.6.34, but the reporter has been
>> unable to test it yet.
>>
>> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=18482
> 
> I've tried hard to find somebody who can test this, but nobody who
> can reproduce the original failure has been able to test it.  I
> propose that we put it in linux-next and see what happens there.
> 

Makes sense to me.

	-hpa

Re: [PATCH] x86, HPET: ignore any PCI BARs that match an HPET we already know about

[H. Peter Anvin]

On 09/22/2010 01:21 PM, H. Peter Anvin wrote:
> On 09/22/2010 01:19 PM, Bjorn Helgaas wrote:
>> On Wednesday, September 22, 2010 02:15:47 pm Bjorn Helgaas wrote:
>>>
>>> We often discover the HPET early, via the static ACPI HPET table, before
>>> enumerating PCI devices.  If the HPET is implemented as a PCI function,
>>> we will discover it again during PCI device enumeration.  We must ignore
>>> the PCI function so we don't inadvertently move it out from under the
>>> driver.
>>>
>>> I think it's better to ignore *any* PCI BAR that matches a previously
>>> discovered HPET; that way we don't need platform-specific knowledge,
>>> and we won't have to add more quirks for future machines.
>>>
>>> This is for a regression from 2.6.34, but the reporter has been
>>> unable to test it yet.
>>>
>>> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=18482
>>
>> I've tried hard to find somebody who can test this, but nobody who
>> can reproduce the original failure has been able to test it.  I
>> propose that we put it in linux-next and see what happens there.
>>
> 
> Makes sense to me.
> 

Actually, this ties into something that I have pointed out in the past:
there will be devices used by the firmware or the platform that has
corresponding PCI BARs. Most of them can be spotted by being a PCI BAR
pointing into reserved memory.

When we find a PCI BAR pointing into reserved memory it really should be
considered a fixed resource, period, full stop.  This is critical for
the proper operation of the platform.

	-hpa

[tip:x86/pci] x86, pci: Ignore any PCI BARs that match an HPET we already know about

[tip-bot for Bjorn Helgaas]

Commit-ID:  e72a6c8f7ac715a9cdb473afbc30743348d0a1e2
Gitweb:     http://git.kernel.org/tip/e72a6c8f7ac715a9cdb473afbc30743348d0a1e2
Author:     Bjorn Helgaas <bjorn.helgaas@hp.com>
AuthorDate: Wed, 22 Sep 2010 14:15:47 -0600
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Wed, 22 Sep 2010 13:29:39 -0700

x86, pci: Ignore any PCI BARs that match an HPET we already know about

We often discover the HPET early, via the static ACPI HPET table,
before enumerating PCI devices.  If the HPET is implemented as a PCI
function, we will discover it again during PCI device enumeration.  We
must ignore the PCI function so we don't inadvertently move it out
from under the driver.

I think it's better to ignore *any* PCI BAR that matches a previously
discovered HPET; that way we don't need platform-specific knowledge,
and we won't have to add more quirks for future machines.

This is for a regression from 2.6.34, but the reporter has been
unable to test it yet.

[ hpa: if this fixes the regression, it should be promoted to x86/urgent ]

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=18482
Reported-by: Simon Arlott <simon@fire.lp0.eu>
Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
LKML-Reference: <20100922201547.3197.33702.stgit@bob.kio>
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 arch/x86/kernel/quirks.c |   21 +++++++++++++++++++++
 arch/x86/pci/fixup.c     |   28 ----------------------------
 2 files changed, 21 insertions(+), 28 deletions(-)

diff --git a/arch/x86/kernel/quirks.c b/arch/x86/kernel/quirks.c
index 939b9e9..99a7d4b 100644
--- a/arch/x86/kernel/quirks.c
+++ b/arch/x86/kernel/quirks.c
@@ -507,6 +507,27 @@ static void force_disable_hpet_msi(struct pci_dev *unused)
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_SBX00_SMBUS,
 			 force_disable_hpet_msi);
 
+static void disable_pci_hpet(struct pci_dev *dev)
+{
+	int i;
+
+	if (!hpet_address)
+		return;
+
+	for (i = 0; i < 6; i++) {
+		struct resource *res = &dev->resource[i];
+
+		if (resource_type(res) == IORESOURCE_MEM &&
+		    res->start == hpet_address) {
+			dev_info(&dev->dev, "BAR %d: %pR is an HPET we found earlier; ignoring this BAR\n",
+				 i, res);
+			res->flags = 0;
+			res->start = 0;
+			res->end = 0;
+		}
+	}
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, disable_pci_hpet);
 #endif
 
 #if defined(CONFIG_PCI) && defined(CONFIG_NUMA)
diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index 6dd8955..08eba69 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -493,31 +493,3 @@ static void __devinit pci_siemens_interrupt_controller(struct pci_dev *dev)
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SIEMENS, 0x0015,
 			  pci_siemens_interrupt_controller);
-
-/*
- * SB600: Disable BAR1 on device 14.0 to avoid HPET resources from
- * confusing the PCI engine:
- */
-static void sb600_disable_hpet_bar(struct pci_dev *dev)
-{
-	u8 val;
-
-	/*
-	 * The SB600 and SB700 both share the same device
-	 * ID, but the PM register 0x55 does something different
-	 * for the SB700, so make sure we are dealing with the
-	 * SB600 before touching the bit:
-	 */
-
-	pci_read_config_byte(dev, 0x08, &val);
-
-	if (val < 0x2F) {
-		outb(0x55, 0xCD6);
-		val = inb(0xCD7);
-
-		/* Set bit 7 in PM register 0x55 */
-		outb(0x55, 0xCD6);
-		outb(val | 0x80, 0xCD7);
-	}
-}
-DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x4385, sb600_disable_hpet_bar);

Re: [tip:x86/pci] x86, pci: Ignore any PCI BARs that match an HPET we already know about

[Yinghai Lu]

On 09/22/2010 02:22 PM, tip-bot for Bjorn Helgaas wrote:
> Commit-ID:  e72a6c8f7ac715a9cdb473afbc30743348d0a1e2
> Gitweb:     http://git.kernel.org/tip/e72a6c8f7ac715a9cdb473afbc30743348d0a1e2
> Author:     Bjorn Helgaas <bjorn.helgaas@hp.com>
> AuthorDate: Wed, 22 Sep 2010 14:15:47 -0600
> Committer:  H. Peter Anvin <hpa@linux.intel.com>
> CommitDate: Wed, 22 Sep 2010 13:29:39 -0700
> 
> x86, pci: Ignore any PCI BARs that match an HPET we already know about
> 
> We often discover the HPET early, via the static ACPI HPET table,
> before enumerating PCI devices.  If the HPET is implemented as a PCI
> function, we will discover it again during PCI device enumeration.  We
> must ignore the PCI function so we don't inadvertently move it out
> from under the driver.
> 
> I think it's better to ignore *any* PCI BAR that matches a previously
> discovered HPET; that way we don't need platform-specific knowledge,
> and we won't have to add more quirks for future machines.
> 
> This is for a regression from 2.6.34, but the reporter has been
> unable to test it yet.
> 
> [ hpa: if this fixes the regression, it should be promoted to x86/urgent ]
> 
> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=18482
> Reported-by: Simon Arlott <simon@fire.lp0.eu>
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> LKML-Reference: <20100922201547.3197.33702.stgit@bob.kio>
> Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
> ---
>  arch/x86/kernel/quirks.c |   21 +++++++++++++++++++++
>  arch/x86/pci/fixup.c     |   28 ----------------------------
>  2 files changed, 21 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/x86/kernel/quirks.c b/arch/x86/kernel/quirks.c
> index 939b9e9..99a7d4b 100644
> --- a/arch/x86/kernel/quirks.c
> +++ b/arch/x86/kernel/quirks.c
> @@ -507,6 +507,27 @@ static void force_disable_hpet_msi(struct pci_dev *unused)
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_SBX00_SMBUS,
>  			 force_disable_hpet_msi);
>  
> +static void disable_pci_hpet(struct pci_dev *dev)
> +{
> +	int i;
> +
> +	if (!hpet_address)
> +		return;
> +
> +	for (i = 0; i < 6; i++) {
> +		struct resource *res = &dev->resource[i];
> +
> +		if (resource_type(res) == IORESOURCE_MEM &&
> +		    res->start == hpet_address) {
> +			dev_info(&dev->dev, "BAR %d: %pR is an HPET we found earlier; ignoring this BAR\n",
> +				 i, res);
> +			res->flags = 0;
> +			res->start = 0;
> +			res->end = 0;
> +		}
> +	}
> +}

It seems there is some problem here:

hpet_res is inserted to resource with late_initcall().
so if you don't touch that hpet address in bar and not put in the resource.
kernel could allocate that address to other devices that doesn't get resource from BIOS.

Yinghai


> +DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, disable_pci_hpet);
>  #endif
>  
>  #if defined(CONFIG_PCI) && defined(CONFIG_NUMA)
> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index 6dd8955..08eba69 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -493,31 +493,3 @@ static void __devinit pci_siemens_interrupt_controller(struct pci_dev *dev)
>  }
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SIEMENS, 0x0015,
>  			  pci_siemens_interrupt_controller);
> -
> -/*
> - * SB600: Disable BAR1 on device 14.0 to avoid HPET resources from
> - * confusing the PCI engine:
> - */
> -static void sb600_disable_hpet_bar(struct pci_dev *dev)
> -{
> -	u8 val;
> -
> -	/*
> -	 * The SB600 and SB700 both share the same device
> -	 * ID, but the PM register 0x55 does something different
> -	 * for the SB700, so make sure we are dealing with the
> -	 * SB600 before touching the bit:
> -	 */
> -
> -	pci_read_config_byte(dev, 0x08, &val);
> -
> -	if (val < 0x2F) {
> -		outb(0x55, 0xCD6);
> -		val = inb(0xCD7);
> -
> -		/* Set bit 7 in PM register 0x55 */
> -		outb(0x55, 0xCD6);
> -		outb(val | 0x80, 0xCD7);
> -	}
> -}
> -DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x4385, sb600_disable_hpet_bar);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tip-commits" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] x86, HPET: ignore any PCI BARs that match an HPET we already know about

[Bjorn Helgaas]

On Wednesday, September 22, 2010 02:56:35 pm H. Peter Anvin wrote:
> On 09/22/2010 01:21 PM, H. Peter Anvin wrote:
> > On 09/22/2010 01:19 PM, Bjorn Helgaas wrote:
> >> On Wednesday, September 22, 2010 02:15:47 pm Bjorn Helgaas wrote:
> >>>
> >>> We often discover the HPET early, via the static ACPI HPET table, before
> >>> enumerating PCI devices.  If the HPET is implemented as a PCI function,
> >>> we will discover it again during PCI device enumeration.  We must ignore
> >>> the PCI function so we don't inadvertently move it out from under the
> >>> driver.
> >>>
> >>> I think it's better to ignore *any* PCI BAR that matches a previously
> >>> discovered HPET; that way we don't need platform-specific knowledge,
> >>> and we won't have to add more quirks for future machines.
> >>>
> >>> This is for a regression from 2.6.34, but the reporter has been
> >>> unable to test it yet.
> >>>
> >>> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=18482
> >>
> >> I've tried hard to find somebody who can test this, but nobody who
> >> can reproduce the original failure has been able to test it.  I
> >> propose that we put it in linux-next and see what happens there.
> > 
> > Makes sense to me.
> 
> Actually, this ties into something that I have pointed out in the past:
> there will be devices used by the firmware or the platform that has
> corresponding PCI BARs. Most of them can be spotted by being a PCI BAR
> pointing into reserved memory.
> 
> When we find a PCI BAR pointing into reserved memory it really should be
> considered a fixed resource, period, full stop.  This is critical for
> the proper operation of the platform.

Yes, I like that idea a lot.  It would save us from the current HPET
bug, where we have this situation:

   BIOS-e820: 00000000fec00000 - 0000000100000000 (reserved)
  ACPI: HPET id: 0x10b9a201 base: 0xfed00000
  pci 0000:00:14.0: no compatible bridge window for [mem 0xfed00000-0xfed003ff]
  pci 0000:00:14.0: BAR 1: assigned [mem 0x80000000-0x800003ff]

The only thing I *don't* like is that Windows doesn't work that way.
I set up this scenario in qemu (this is with a VGA device, not an
HPET device):

   BIOS-e820: [mem 0x00000000e8000000-0x00000000f1ffffff] reserved
  pci_root PNP0A03:00: host bridge window [mem 0xe0000000-0xefffffff]
  pci 0000:00:02.0: BAR 0: [mem 0xf0000000-0xf1ffffff pref]

According to E820, BAR 0 is in a reserved area, but Windows 7 still
moved it to [mem 0xec000000-0xedffffff].  Windows moved it because
it was outside the host bridge window (same reason Linux moved the
HPET in the original bug).  There was plenty of space available
that's *not* reserved (0xe0000000-0xe7ffffff), but for whatever
reason, Windows put it in the reserved part of the window.

We don't necessarily need to slavishly copy what Windows does, but
I'd feel better if we understood how it avoided the problem on the
Gigabyte board so we could make a more informed decision.

It really doesn't look like Windows, and therefore BIOS writers,
share your expectations about PCI BARs in E820 reserved areas.
It's likely still *safe* to make them fixed resources, but we might
be able to fix more issues if we knew how Windows avoids the problem.

Bjorn

Re: [PATCH] x86, HPET: ignore any PCI BARs that match an HPET we already know about

[H. Peter Anvin]

On 09/22/2010 02:52 PM, Bjorn Helgaas wrote:
> 
> It really doesn't look like Windows, and therefore BIOS writers,
> share your expectations about PCI BARs in E820 reserved areas.
> It's likely still *safe* to make them fixed resources, but we might
> be able to fix more issues if we knew how Windows avoids the problem.
> 

*Which Windows*!?  Each version of Windows works differently, with
different quirks and bugs, and the BIOS vendors only ever test the
currently shipping version (except sometimes on server boards.)

	-hpa

Re: [PATCH] x86, HPET: ignore any PCI BARs that match an HPET we already know about

[H. Peter Anvin]

On 09/22/2010 02:52 PM, Bjorn Helgaas wrote:
> 
> We don't necessarily need to slavishly copy what Windows does, but
> I'd feel better if we understood how it avoided the problem on the
> Gigabyte board so we could make a more informed decision.
> 
> It really doesn't look like Windows, and therefore BIOS writers,
> share your expectations about PCI BARs in E820 reserved areas.
> It's likely still *safe* to make them fixed resources, but we might
> be able to fix more issues if we knew how Windows avoids the problem.
> 

Keep in mind that Windows -- the version they tested against -- might
just work by accident.  That's a "ship it" condition for the BIOS for
almost every vendor.

Also, do note that the reservations don't necessarily need to come from
the BIOS; we can mark the HPET area internally reserved, for example,
when we discover it.

	-hpa

Re: [PATCH] x86, HPET: ignore any PCI BARs that match an HPET we already know about

[Bjorn Helgaas]

On Wednesday, September 22, 2010 04:10:36 pm H. Peter Anvin wrote:
> On 09/22/2010 02:52 PM, Bjorn Helgaas wrote:
> > 
> > We don't necessarily need to slavishly copy what Windows does, but
> > I'd feel better if we understood how it avoided the problem on the
> > Gigabyte board so we could make a more informed decision.
> > 
> > It really doesn't look like Windows, and therefore BIOS writers,
> > share your expectations about PCI BARs in E820 reserved areas.
> > It's likely still *safe* to make them fixed resources, but we might
> > be able to fix more issues if we knew how Windows avoids the problem.
> > 
> 
> Keep in mind that Windows -- the version they tested against -- might
> just work by accident.  That's a "ship it" condition for the BIOS for
> almost every vendor.

Yep.  I really wish I had a board to play with to find out.  I'm sure
we'd learn something useful.

> Also, do note that the reservations don't necessarily need to come from
> the BIOS; we can mark the HPET area internally reserved, for example,
> when we discover it.

That actually raises another question I had: we currently look at the
HPET table pretty early, but I don't know whether the earliness is a
requirement.  It would be cleaner if we could ignore the table and
discover the HPET later in normal ways like pnp_register_driver() and
pci_register_driver().  Then I could imagine someday dealing with the
resources in a more generic way, i.e., in the PCI and PNP cores.

Bjorn

Re: [PATCH] x86, HPET: ignore any PCI BARs that match an HPET we already know about

[H. Peter Anvin]

On 09/22/2010 03:37 PM, Bjorn Helgaas wrote:
> 
> That actually raises another question I had: we currently look at the
> HPET table pretty early, but I don't know whether the earliness is a
> requirement.  It would be cleaner if we could ignore the table and
> discover the HPET later in normal ways like pnp_register_driver() and
> pci_register_driver().  Then I could imagine someday dealing with the
> resources in a more generic way, i.e., in the PCI and PNP cores.
> 

Well, depends on how early we need timers...
	
	-hpa

Re: [tip:x86/pci] x86, pci: Ignore any PCI BARs that match an HPET we already know about

[Bjorn Helgaas]

On Wednesday, September 22, 2010 03:48:44 pm Yinghai Lu wrote:
> On 09/22/2010 02:22 PM, tip-bot for Bjorn Helgaas wrote:
> > Commit-ID:  e72a6c8f7ac715a9cdb473afbc30743348d0a1e2
> > Gitweb:     http://git.kernel.org/tip/e72a6c8f7ac715a9cdb473afbc30743348d0a1e2
> > Author:     Bjorn Helgaas <bjorn.helgaas@hp.com>
> > AuthorDate: Wed, 22 Sep 2010 14:15:47 -0600
> > Committer:  H. Peter Anvin <hpa@linux.intel.com>
> > CommitDate: Wed, 22 Sep 2010 13:29:39 -0700
> > 
> > x86, pci: Ignore any PCI BARs that match an HPET we already know about
> > 
> > We often discover the HPET early, via the static ACPI HPET table,
> > before enumerating PCI devices.  If the HPET is implemented as a PCI
> > function, we will discover it again during PCI device enumeration.  We
> > must ignore the PCI function so we don't inadvertently move it out
> > from under the driver.
> > 
> > I think it's better to ignore *any* PCI BAR that matches a previously
> > discovered HPET; that way we don't need platform-specific knowledge,
> > and we won't have to add more quirks for future machines.
> > 
> > This is for a regression from 2.6.34, but the reporter has been
> > unable to test it yet.
> > 
> > [ hpa: if this fixes the regression, it should be promoted to x86/urgent ]
> > 
> > Reference: https://bugzilla.kernel.org/show_bug.cgi?id=18482
> > Reported-by: Simon Arlott <simon@fire.lp0.eu>
> > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> > LKML-Reference: <20100922201547.3197.33702.stgit@bob.kio>
> > Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
> > ---
> >  arch/x86/kernel/quirks.c |   21 +++++++++++++++++++++
> >  arch/x86/pci/fixup.c     |   28 ----------------------------
> >  2 files changed, 21 insertions(+), 28 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/quirks.c b/arch/x86/kernel/quirks.c
> > index 939b9e9..99a7d4b 100644
> > --- a/arch/x86/kernel/quirks.c
> > +++ b/arch/x86/kernel/quirks.c
> > @@ -507,6 +507,27 @@ static void force_disable_hpet_msi(struct pci_dev *unused)
> >  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_SBX00_SMBUS,
> >  			 force_disable_hpet_msi);
> >  
> > +static void disable_pci_hpet(struct pci_dev *dev)
> > +{
> > +	int i;
> > +
> > +	if (!hpet_address)
> > +		return;
> > +
> > +	for (i = 0; i < 6; i++) {
> > +		struct resource *res = &dev->resource[i];
> > +
> > +		if (resource_type(res) == IORESOURCE_MEM &&
> > +		    res->start == hpet_address) {
> > +			dev_info(&dev->dev, "BAR %d: %pR is an HPET we found earlier; ignoring this BAR\n",
> > +				 i, res);
> > +			res->flags = 0;
> > +			res->start = 0;
> > +			res->end = 0;
> > +		}
> > +	}
> > +}
> 
> It seems there is some problem here:
> 
> hpet_res is inserted to resource with late_initcall().
> so if you don't touch that hpet address in bar and not put in the resource.
> kernel could allocate that address to other devices that doesn't get resource from BIOS.

Hmm, yes, that's a problem.  Let me look into it tomorrow; maybe we
can do something like setting IORESOURCE_PCI_FIXED instead of just
clearing out the resource.

Bjorn

Re: [tip:x86/pci] x86, pci: Ignore any PCI BARs that match an HPET we already know about

[H. Peter Anvin]

On 09/22/2010 03:41 PM, Bjorn Helgaas wrote:
>>
>> It seems there is some problem here:
>>
>> hpet_res is inserted to resource with late_initcall().
>> so if you don't touch that hpet address in bar and not put in the resource.
>> kernel could allocate that address to other devices that doesn't get resource from BIOS.
> 
> Hmm, yes, that's a problem.  Let me look into it tomorrow; maybe we
> can do something like setting IORESOURCE_PCI_FIXED instead of just
> clearing out the resource.
> 
> Bjorn

OK, dropping the patch for now.

	-hpa

Re: [tip:x86/pci] x86, pci: Ignore any PCI BARs that match an HPET we already know about

[Bjorn Helgaas]

On Wednesday, September 22, 2010 03:48:44 pm Yinghai Lu wrote:
> On 09/22/2010 02:22 PM, tip-bot for Bjorn Helgaas wrote:
> > Commit-ID:  e72a6c8f7ac715a9cdb473afbc30743348d0a1e2
> > Gitweb:     http://git.kernel.org/tip/e72a6c8f7ac715a9cdb473afbc30743348d0a1e2
> > Author:     Bjorn Helgaas <bjorn.helgaas@hp.com>
> > AuthorDate: Wed, 22 Sep 2010 14:15:47 -0600
> > Committer:  H. Peter Anvin <hpa@linux.intel.com>
> > CommitDate: Wed, 22 Sep 2010 13:29:39 -0700
> > 
> > x86, pci: Ignore any PCI BARs that match an HPET we already know about
> > 
> > We often discover the HPET early, via the static ACPI HPET table,
> > before enumerating PCI devices.  If the HPET is implemented as a PCI
> > function, we will discover it again during PCI device enumeration.  We
> > must ignore the PCI function so we don't inadvertently move it out
> > from under the driver.
> > 
> > I think it's better to ignore *any* PCI BAR that matches a previously
> > discovered HPET; that way we don't need platform-specific knowledge,
> > and we won't have to add more quirks for future machines.
> > 
> > This is for a regression from 2.6.34, but the reporter has been
> > unable to test it yet.
> > 
> > [ hpa: if this fixes the regression, it should be promoted to x86/urgent ]
> > 
> > Reference: https://bugzilla.kernel.org/show_bug.cgi?id=18482
> > Reported-by: Simon Arlott <simon@fire.lp0.eu>
> > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> > LKML-Reference: <20100922201547.3197.33702.stgit@bob.kio>
> > Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
> > ---
> >  arch/x86/kernel/quirks.c |   21 +++++++++++++++++++++
> >  arch/x86/pci/fixup.c     |   28 ----------------------------
> >  2 files changed, 21 insertions(+), 28 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/quirks.c b/arch/x86/kernel/quirks.c
> > index 939b9e9..99a7d4b 100644
> > --- a/arch/x86/kernel/quirks.c
> > +++ b/arch/x86/kernel/quirks.c
> > @@ -507,6 +507,27 @@ static void force_disable_hpet_msi(struct pci_dev *unused)
> >  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_SBX00_SMBUS,
> >  			 force_disable_hpet_msi);
> >  
> > +static void disable_pci_hpet(struct pci_dev *dev)
> > +{
> > +	int i;
> > +
> > +	if (!hpet_address)
> > +		return;
> > +
> > +	for (i = 0; i < 6; i++) {
> > +		struct resource *res = &dev->resource[i];
> > +
> > +		if (resource_type(res) == IORESOURCE_MEM &&
> > +		    res->start == hpet_address) {
> > +			dev_info(&dev->dev, "BAR %d: %pR is an HPET we found earlier; ignoring this BAR\n",
> > +				 i, res);
> > +			res->flags = 0;
> > +			res->start = 0;
> > +			res->end = 0;
> > +		}
> > +	}
> > +}
> 
> It seems there is some problem here:
> 
> hpet_res is inserted to resource with late_initcall().
> so if you don't touch that hpet address in bar and not put in the resource.
> kernel could allocate that address to other devices that doesn't get resource from BIOS.

Marking the BAR as IORESOURCE_PCI_FIXED would fix part of the problem.
It would prevent us from moving the BAR, which means the HPET driver
should keep working.

And in some cases, it will let us claim the address space to keep the
kernel from putting another device there.

In other cases (like bug 18482), we won't claim the address space
because the BAR is not inside a host bridge window, and we'll return
from pci_claim_resource() before requesting the BAR because we didn't
find a parent to allocate from.  However, we will never place another
PCI device there, because we only allocate PCI space from PCI host
bridge windows.

Outside of the host bridge windows, it feels like no-man's land --
there's nothing to protect the resources used by PCI *or* ACPI devices
unless they happen to be in E820 reserved area.  I think we're just
being lucky now because we basically never move ACPI devices.

Bottom line: even though IORESOURCE_PCI_FIXED isn't perfect, I think
it's a better idea than my first patch, and I'll post a revision.

Bjorn