[PATCH 1/1] PCI: tune up ICH4 quirk for broken BIOSes

[PATCH 1/1] PCI: tune up ICH4 quirk for broken BIOSes

[Jiri Slaby]

There are BIOSes out there where they provide ACPI mapping that
conflicts with fixed one. Then e.g. an IDE controller doesn't work due
to I/O space conflict:
pci 0000:00:1f.1: address space collision: [io  0x0170-0x0177] conflicts with 0000:00:1f.0 [io  0x0100-0x017f]

0x0170-0x0177 for IDE controllers is created in pci_setup_device.
0x0100-0x017f for the ICH4 ISA bridge is created in quirk_ich4_lpc_acpi

The former is fixed, according to specs at that address.
The latter is read from the bridge confspace which contains:
00000000  86 80 a1 25 0f 00 80 02  02 00 01 06 00 00 80 00 |...%............|
00000010  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
00000020  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
00000030  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
00000040  01 01 00 00 10 00 00 00  00 00 00 00 00 00 01 00 |................|
00000050  00 00 00 00 00 00 00 00  01 05 00 00 10 00 00 00 |................|
...
I.e. 0x00000101 as a base (ORed by 1 meaning I/O space) for ACPI per
ICH4 specs.

Don't accept bases below 0x180 for this dynamic mapping.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/pci/quirks.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 53a786f..6d241eb 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -543,7 +543,14 @@ static void __devinit quirk_ich4_lpc_acpi(struct pci_dev *dev)
 	u32 region;
 
 	pci_read_config_dword(dev, 0x40, &region);
-	quirk_io_region(dev, region, 128, PCI_BRIDGE_RESOURCES, "ICH4 ACPI/GPIO/TCO");
+	/*
+	 * some BIOSes are bogus and create this dynamic mapping so that it
+	 * conflicts with fixed. There is no space below 0x180 for
+	 * ACPI/GPIO/TCO which is 128B long and 128B aligned.
+	 */
+	if (region >= 0x180)
+		quirk_io_region(dev, region, 128, PCI_BRIDGE_RESOURCES,
+				"ICH4 ACPI/GPIO/TCO");
 
 	pci_read_config_dword(dev, 0x58, &region);
 	quirk_io_region(dev, region, 64, PCI_BRIDGE_RESOURCES+1, "ICH4 GPIO");
-- 
1.7.3.4

Re: [PATCH 1/1] PCI: tune up ICH4 quirk for broken BIOSes

[Bjorn Helgaas]

On Thursday, January 06, 2011 08:17:46 am Jiri Slaby wrote:
> There are BIOSes out there where they provide ACPI mapping that
> conflicts with fixed one. Then e.g. an IDE controller doesn't work due
> to I/O space conflict:
> pci 0000:00:1f.1: address space collision: [io  0x0170-0x0177] conflicts with 0000:00:1f.0 [io  0x0100-0x017f]
> 
> 0x0170-0x0177 for IDE controllers is created in pci_setup_device.
> 0x0100-0x017f for the ICH4 ISA bridge is created in quirk_ich4_lpc_acpi

This might be the right thing to do, but it feels hacky to add more
magic numbers (">= 0x180"), so I'd like to understand it better.
(Is there a bugzilla for this?  Is it a regression?  Sounds like an
old machine where we should have seen this before.)

My guess is that we found this "conflict" and tried to move the
controller, but it's hardwired to stay at 0x170 when in compatibility
mode.  So the hardware is really still at 0x170, but Linux thinks it
moved it elsewhere, so it doesn't work.

The fixed IDE resources in pci_setup_device() make sense to me; I
think section 2.1 of the PCI IDE spec justifies specific OS knowledge
of the compatibility mode regions.

But I don't know the quirk_ich4_lpc_acpi() history, and this quirk
is the sort of information the OS should not have to know about.
Theoretically, ACPI tells us about the GPIO/TCO/etc. regions in a
generic way via namespace devices or something in the static tables.
Is that generic information missing, or is it there and Linux is
ignoring it?  If we're ignoring it, I'd rather fix that.

The main reason for claiming I/O regions is to keep us from placing
another device on top of them.  But we've had PCIBIOS_MIN_IO = 0x1000
forever, which should keep us from putting anything below that anyway
(at least for PCI devices).  So there must be some other reason for
claiming space in this quirk.

Bjorn

> The former is fixed, according to specs at that address.
> The latter is read from the bridge confspace which contains:
> 00000000  86 80 a1 25 0f 00 80 02  02 00 01 06 00 00 80 00 |...%............|
> 00000010  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
> 00000020  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
> 00000030  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
> 00000040  01 01 00 00 10 00 00 00  00 00 00 00 00 00 01 00 |................|
> 00000050  00 00 00 00 00 00 00 00  01 05 00 00 10 00 00 00 |................|
> ...
> I.e. 0x00000101 as a base (ORed by 1 meaning I/O space) for ACPI per
> ICH4 specs.
> 
> Don't accept bases below 0x180 for this dynamic mapping.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/pci/quirks.c |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 53a786f..6d241eb 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -543,7 +543,14 @@ static void __devinit quirk_ich4_lpc_acpi(struct pci_dev *dev)
>  	u32 region;
>  
>  	pci_read_config_dword(dev, 0x40, &region);
> -	quirk_io_region(dev, region, 128, PCI_BRIDGE_RESOURCES, "ICH4 ACPI/GPIO/TCO");
> +	/*
> +	 * some BIOSes are bogus and create this dynamic mapping so that it
> +	 * conflicts with fixed. There is no space below 0x180 for
> +	 * ACPI/GPIO/TCO which is 128B long and 128B aligned.
> +	 */
> +	if (region >= 0x180)
> +		quirk_io_region(dev, region, 128, PCI_BRIDGE_RESOURCES,
> +				"ICH4 ACPI/GPIO/TCO");
>  
>  	pci_read_config_dword(dev, 0x58, &region);
>  	quirk_io_region(dev, region, 64, PCI_BRIDGE_RESOURCES+1, "ICH4 GPIO");
> 

Re: [PATCH 1/1] PCI: tune up ICH4 quirk for broken BIOSes

[Jiri Slaby]

On 01/06/2011 08:24 PM, Bjorn Helgaas wrote:
> On Thursday, January 06, 2011 08:17:46 am Jiri Slaby wrote:
>> There are BIOSes out there where they provide ACPI mapping that
>> conflicts with fixed one. Then e.g. an IDE controller doesn't work due
>> to I/O space conflict:
>> pci 0000:00:1f.1: address space collision: [io  0x0170-0x0177] conflicts with 0000:00:1f.0 [io  0x0100-0x017f]
>>
>> 0x0170-0x0177 for IDE controllers is created in pci_setup_device.
>> 0x0100-0x017f for the ICH4 ISA bridge is created in quirk_ich4_lpc_acpi
> 
> This might be the right thing to do, but it feels hacky to add more
> magic numbers (">= 0x180"), so I'd like to understand it better.

Yes, definitely, it's a hack, but I haven't found anything better. So I
dropped the patch in for discussion.

> (Is there a bugzilla for this? 

Only a novell (opensuse) bugzilla:
https://bugzilla.novell.com/show_bug.cgi?id=558740

> Is it a regression?  Sounds like an
> old machine where we should have seen this before.)

Yes, a regression. 2.6.31 and newer doesn't work. 2.6.27 did. I don't
know what kernel exactly in between is first defunct.

> My guess is that we found this "conflict" and tried to move the
> controller, but it's hardwired to stay at 0x170 when in compatibility
> mode.  So the hardware is really still at 0x170, but Linux thinks it
> moved it elsewhere, so it doesn't work.

Sorry I didn't get this paragraph.

> The fixed IDE resources in pci_setup_device() make sense to me; I
> think section 2.1 of the PCI IDE spec justifies specific OS knowledge
> of the compatibility mode regions.
> 
> But I don't know the quirk_ich4_lpc_acpi() history, and this quirk
> is the sort of information the OS should not have to know about.

It's from ICH4 specs. For ICH4, the addresses of where to look for ACPI
and GPIO are specified in the PCI BUS conf space.

So we look at 0x40 in that space and find 0x101, so we reserve
0x100-0x17f which conflicts with legacy IDE which is 0x170-0x17f.

> Theoretically, ACPI tells us about the GPIO/TCO/etc. regions in a
> generic way via namespace devices or something in the static tables.
> Is that generic information missing, or is it there and Linux is
> ignoring it?  If we're ignoring it, I'd rather fix that.

It works for most boxes I would say. Try to google for "claimed by ICH4
ACPI/GPIO/TCO", it reports sane ranges like 0400-047f or 4000-407f.

> The main reason for claiming I/O regions is to keep us from placing
> another device on top of them.  But we've had PCIBIOS_MIN_IO = 0x1000
> forever, which should keep us from putting anything below that anyway
> (at least for PCI devices).  So there must be some other reason for
> claiming space in this quirk.

Anyway, this ACPI space may be below 0x1000 as can be seen above. From
the ICH standard, it can be anywhere...

thanks,
-- 
js

Re: [PATCH 1/1] PCI: tune up ICH4 quirk for broken BIOSes

[Jesse Barnes]

On Fri, 07 Jan 2011 21:44:35 +0100
Jiri Slaby <jirislaby@gmail.com> wrote:

> On 01/06/2011 08:24 PM, Bjorn Helgaas wrote:
> > Theoretically, ACPI tells us about the GPIO/TCO/etc. regions in a
> > generic way via namespace devices or something in the static tables.
> > Is that generic information missing, or is it there and Linux is
> > ignoring it?  If we're ignoring it, I'd rather fix that.
> 
> It works for most boxes I would say. Try to google for "claimed by ICH4
> ACPI/GPIO/TCO", it reports sane ranges like 0400-047f or 4000-407f.
> 
> > The main reason for claiming I/O regions is to keep us from placing
> > another device on top of them.  But we've had PCIBIOS_MIN_IO = 0x1000
> > forever, which should keep us from putting anything below that anyway
> > (at least for PCI devices).  So there must be some other reason for
> > claiming space in this quirk.
> 
> Anyway, this ACPI space may be below 0x1000 as can be seen above. From
> the ICH standard, it can be anywhere...

Is there some other resource we should be considering to avoid this
space?

And we know it won't work if we try to use the IDE controller in the
LPC allocated space?  Have you checked the ICH4 specs to see what the
decode rules are on this space?  I guess LPC has priority and doesn't
forward requests through to IDE?

Either way, it would be good to understand this issue a little better
before we start restricting the LPC space like this.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/1] PCI: tune up ICH4 quirk for broken BIOSes

[Jiri Slaby]

On 01/07/2011 11:37 PM, Jesse Barnes wrote:
> On Fri, 07 Jan 2011 21:44:35 +0100
> Jiri Slaby <jirislaby@gmail.com> wrote:
> 
>> On 01/06/2011 08:24 PM, Bjorn Helgaas wrote:
>>> Theoretically, ACPI tells us about the GPIO/TCO/etc. regions in a
>>> generic way via namespace devices or something in the static tables.
>>> Is that generic information missing, or is it there and Linux is
>>> ignoring it?  If we're ignoring it, I'd rather fix that.
>>
>> It works for most boxes I would say. Try to google for "claimed by ICH4
>> ACPI/GPIO/TCO", it reports sane ranges like 0400-047f or 4000-407f.
>>
>>> The main reason for claiming I/O regions is to keep us from placing
>>> another device on top of them.  But we've had PCIBIOS_MIN_IO = 0x1000
>>> forever, which should keep us from putting anything below that anyway
>>> (at least for PCI devices).  So there must be some other reason for
>>> claiming space in this quirk.
>>
>> Anyway, this ACPI space may be below 0x1000 as can be seen above. From
>> the ICH standard, it can be anywhere...
> 
> Is there some other resource we should be considering to avoid this
> space?

There are two kinds of address ranges (spaces) in ICH specs. Fixed
(address base is fixed) and Variable (base can be changed). And Fixed
space should not collide (the behaviour is explicitly unpredictable)
with Variable.

This ACPI space is defined as Variable. So it cannot collide with any
Fixed. Most of the Fixed are below 0x180, there is also 0x376, 0x3f6 for
IDE, 0x4D0-0x4D1 for PIC and 0xCF9 for reset generator.

> And we know it won't work if we try to use the IDE controller in the
> LPC allocated space?  Have you checked the ICH4 specs to see what the
> decode rules are on this space? I guess LPC has priority and doesn't
> forward requests through to IDE?

If we read/write to ports 0x170-0x17f, IDE ctrl responds on that
machine. I.e. if I remove the quirk, disks are found. (Otherwise they
are not due to the conflict.)

thanks,
-- 
js

Re: [PATCH 1/1] PCI: tune up ICH4 quirk for broken BIOSes

[Bjorn Helgaas]

On Friday, January 07, 2011 01:44:35 pm Jiri Slaby wrote:
> On 01/06/2011 08:24 PM, Bjorn Helgaas wrote:
> > On Thursday, January 06, 2011 08:17:46 am Jiri Slaby wrote:

> Only a novell (opensuse) bugzilla:
> https://bugzilla.novell.com/show_bug.cgi?id=558740
> 
> > Is it a regression?  Sounds like an
> > old machine where we should have seen this before.)
> 
> Yes, a regression. 2.6.31 and newer doesn't work. 2.6.27 did. I don't
> know what kernel exactly in between is first defunct.
> 
> > My guess is that we found this "conflict" and tried to move the
> > controller, but it's hardwired to stay at 0x170 when in compatibility
> > mode.  So the hardware is really still at 0x170, but Linux thinks it
> > moved it elsewhere, so it doesn't work.
> 
> Sorry I didn't get this paragraph.

IDE devices are special and are allowed to respond to the legacy
addresses, e.g., 0x170, even if those addresses don't appear in
the BARs.  That's why we have the special case in pci_setup_device().

But the code that detects the address space conflict and tries to
resolve it doesn't know about this hard-wired IDE address decoder.
Linux tries to move it by writing to a BAR, and that probably has
no effect on a compatibility-mode IDE device.

> > Theoretically, ACPI tells us about the GPIO/TCO/etc. regions in a
> > generic way via namespace devices or something in the static tables.
> > Is that generic information missing, or is it there and Linux is
> > ignoring it?  If we're ignoring it, I'd rather fix that.
> 
> It works for most boxes I would say. Try to google for "claimed by ICH4
> ACPI/GPIO/TCO", it reports sane ranges like 0400-047f or 4000-407f.

My point is that BIOS should be telling the OS about GPIO/TCO/etc.
regions via an ACPI mechanism, and, ideally, we would use that rather
than reading the address out of chipset-dependent registers.

Even though PMBASE says the ACPI registers occupy 128 bytes from
0x100-0x17f, it's likely there's no actual conflict between the
last 16 bytes and the IDE device.

ACPI probably reports this region via the FADT (the GPE PM register
blocks) and possibly a PNP0C02 device.  These will probably report
something that doesn't conflict with the legacy IDE ports, i.e., a
subset of the 0x100-0x17f range.

Ooooh, I notice in the bugzilla that something's wrong with SMBIOS
(comment 29) and ACPI is disabled because we couldn't find the
RSDP (dmesg in comment 27).  What sort of machine is this, anyway?
We didn't find PNPBIOS, either.

Bjorn

Re: [PATCH 1/1] PCI: tune up ICH4 quirk for broken BIOSes

[Jiri Slaby]

On 01/08/2011 12:03 AM, Bjorn Helgaas wrote:
> On Friday, January 07, 2011 01:44:35 pm Jiri Slaby wrote:
>> On 01/06/2011 08:24 PM, Bjorn Helgaas wrote:
>>> Theoretically, ACPI tells us about the GPIO/TCO/etc. regions in a
>>> generic way via namespace devices or something in the static tables.
>>> Is that generic information missing, or is it there and Linux is
>>> ignoring it?  If we're ignoring it, I'd rather fix that.
>>
>> It works for most boxes I would say. Try to google for "claimed by ICH4
>> ACPI/GPIO/TCO", it reports sane ranges like 0400-047f or 4000-407f.
> 
> My point is that BIOS should be telling the OS about GPIO/TCO/etc.
> regions via an ACPI mechanism, and, ideally, we would use that rather
> than reading the address out of chipset-dependent registers.
> 
> Even though PMBASE says the ACPI registers occupy 128 bytes from
> 0x100-0x17f, it's likely there's no actual conflict between the
> last 16 bytes and the IDE device.

I wouldn't say so. According to the datasheet 0x60-0x7f of the space
(i.e. 0x160-0x17f here) is for TCO registers. There:
0x10 -- Software IRQ Generation Register (i.e. 0x170)
0x11-0x1f -- reserved (0x171-0x17f)

So at least 0x170 should be conflicting. Unless TCO is unused/disabled
and not mapped there at all. May be that the case?

> ACPI probably reports this region via the FADT (the GPE PM register
> blocks) and possibly a PNP0C02 device.  These will probably report
> something that doesn't conflict with the legacy IDE ports, i.e., a
> subset of the 0x100-0x17f range.
> 
> Ooooh, I notice in the bugzilla that something's wrong with SMBIOS
> (comment 29) and ACPI is disabled because we couldn't find the
> RSDP (dmesg in comment 27).  What sort of machine is this, anyway?
> We didn't find PNPBIOS, either.

Hmm, it looks like some old crap. What exact information you would like
to know? I've just asked if ACPI is not disabled in BIOS. There should
be no machine without ACPI running still in the 21st century, I think.

thanks,
-- 
js

Re: [PATCH 1/1] PCI: tune up ICH4 quirk for broken BIOSes

[Bjorn Helgaas]

On Friday, January 07, 2011 04:29:00 pm Jiri Slaby wrote:
> On 01/08/2011 12:03 AM, Bjorn Helgaas wrote:
> > On Friday, January 07, 2011 01:44:35 pm Jiri Slaby wrote:
> >> On 01/06/2011 08:24 PM, Bjorn Helgaas wrote:
> >>> Theoretically, ACPI tells us about the GPIO/TCO/etc. regions in a
> >>> generic way via namespace devices or something in the static tables.
> >>> Is that generic information missing, or is it there and Linux is
> >>> ignoring it?  If we're ignoring it, I'd rather fix that.
> >>
> >> It works for most boxes I would say. Try to google for "claimed by ICH4
> >> ACPI/GPIO/TCO", it reports sane ranges like 0400-047f or 4000-407f.
> > 
> > My point is that BIOS should be telling the OS about GPIO/TCO/etc.
> > regions via an ACPI mechanism, and, ideally, we would use that rather
> > than reading the address out of chipset-dependent registers.
> > 
> > Even though PMBASE says the ACPI registers occupy 128 bytes from
> > 0x100-0x17f, it's likely there's no actual conflict between the
> > last 16 bytes and the IDE device.
> 
> I wouldn't say so. According to the datasheet 0x60-0x7f of the space
> (i.e. 0x160-0x17f here) is for TCO registers. There:
> 0x10 -- Software IRQ Generation Register (i.e. 0x170)
> 0x11-0x1f -- reserved (0x171-0x17f)
> 
> So at least 0x170 should be conflicting. Unless TCO is unused/disabled
> and not mapped there at all. May be that the case?

Maybe.  All your patch does is avoid reserving this 0x100-0x1f7
region; it doesn't actually *move* anything.  And the IDE device
apparently works at the 0x170 compatibility address.  So the
ICH ACPI stuff is still at 0x100-0x17f, so apparently they don't
conflict or maybe the ICH ACPI stuff is disabled.  If the box
doesn't even have ACPI, I suppose there would be no reason to
have the ACPI registers enabled.  Is there something in ICH
that tells us whether they're enabled?

> > ACPI probably reports this region via the FADT (the GPE PM register
> > blocks) and possibly a PNP0C02 device.  These will probably report
> > something that doesn't conflict with the legacy IDE ports, i.e., a
> > subset of the 0x100-0x17f range.
> > 
> > Ooooh, I notice in the bugzilla that something's wrong with SMBIOS
> > (comment 29) and ACPI is disabled because we couldn't find the
> > RSDP (dmesg in comment 27).  What sort of machine is this, anyway?
> > We didn't find PNPBIOS, either.
> 
> Hmm, it looks like some old crap. What exact information you would like
> to know? I've just asked if ACPI is not disabled in BIOS. There should
> be no machine without ACPI running still in the 21st century, I think.

I'm just wondering if the machine actually does have ACPI, but
there's some Linux problem related to finding the tables.  If it's
really old enough, that wouldn't be so surprising, but I see USB
and gigabit NIC hardware, so it's not truly ancient.

Bjorn

Re: [PATCH 1/1] PCI: tune up ICH4 quirk for broken BIOSes

[Jiri Slaby]

On 01/08/2011 01:16 AM, Bjorn Helgaas wrote:
> On Friday, January 07, 2011 04:29:00 pm Jiri Slaby wrote:
>> On 01/08/2011 12:03 AM, Bjorn Helgaas wrote:
>>> On Friday, January 07, 2011 01:44:35 pm Jiri Slaby wrote:
>>>> On 01/06/2011 08:24 PM, Bjorn Helgaas wrote:
>>>>> Theoretically, ACPI tells us about the GPIO/TCO/etc. regions in a
>>>>> generic way via namespace devices or something in the static tables.
>>>>> Is that generic information missing, or is it there and Linux is
>>>>> ignoring it?  If we're ignoring it, I'd rather fix that.
>>>>
>>>> It works for most boxes I would say. Try to google for "claimed by ICH4
>>>> ACPI/GPIO/TCO", it reports sane ranges like 0400-047f or 4000-407f.
>>>
>>> My point is that BIOS should be telling the OS about GPIO/TCO/etc.
>>> regions via an ACPI mechanism, and, ideally, we would use that rather
>>> than reading the address out of chipset-dependent registers.
>>>
>>> Even though PMBASE says the ACPI registers occupy 128 bytes from
>>> 0x100-0x17f, it's likely there's no actual conflict between the
>>> last 16 bytes and the IDE device.
>>
>> I wouldn't say so. According to the datasheet 0x60-0x7f of the space
>> (i.e. 0x160-0x17f here) is for TCO registers. There:
>> 0x10 -- Software IRQ Generation Register (i.e. 0x170)
>> 0x11-0x1f -- reserved (0x171-0x17f)
>>
>> So at least 0x170 should be conflicting. Unless TCO is unused/disabled
>> and not mapped there at all. May be that the case?
> 
> Maybe.  All your patch does is avoid reserving this 0x100-0x1f7
> region; it doesn't actually *move* anything.  And the IDE device
> apparently works at the 0x170 compatibility address.  So the
> ICH ACPI stuff is still at 0x100-0x17f, so apparently they don't
> conflict or maybe the ICH ACPI stuff is disabled.  If the box
> doesn't even have ACPI, I suppose there would be no reason to
> have the ACPI registers enabled.  Is there something in ICH
> that tells us whether they're enabled?

Hmm, there is:
bit 4: ACPI Enable (ACPI_EN) — R/W.
  0 = Disable.
  1 = Decode of the I/O range pointed to by the ACPI Base register is
enabled, and the ACPI power management function is enabled. Note that
the APM power management ranges (B2/B3h) are always enabled and are not
affected by this bit.

at 0x44 in the bridge conf space. So we should definitely check the value.

I don't have the actual value in that register when ACPI is disabled in
BIOS. From the run where acpi=off was passed to the kernel, there is
0x10 (i.e. ACPI_EN=1). However I don't know whether ACPI was disabled in
BIOS at that time.

>>> ACPI probably reports this region via the FADT (the GPE PM register
>>> blocks) and possibly a PNP0C02 device.  These will probably report
>>> something that doesn't conflict with the legacy IDE ports, i.e., a
>>> subset of the 0x100-0x17f range.
>>>
>>> Ooooh, I notice in the bugzilla that something's wrong with SMBIOS
>>> (comment 29) and ACPI is disabled because we couldn't find the
>>> RSDP (dmesg in comment 27).  What sort of machine is this, anyway?
>>> We didn't find PNPBIOS, either.
>>
>> Hmm, it looks like some old crap. What exact information you would like
>> to know? I've just asked if ACPI is not disabled in BIOS. There should
>> be no machine without ACPI running still in the 21st century, I think.
> 
> I'm just wondering if the machine actually does have ACPI, but
> there's some Linux problem related to finding the tables.  If it's
> really old enough, that wouldn't be so surprising, but I see USB
> and gigabit NIC hardware, so it's not truly ancient.

The box is:
http://www.xembedded.com/content/vme/processors/xvme-690.php
and has ACPI, but the user disabled ACPI (I don't know why yet).

thanks,
-- 
js

Re: [PATCH 1/1] PCI: tune up ICH4 quirk for broken BIOSes

[Bjorn Helgaas]

On Saturday, January 08, 2011 02:58:01 am Jiri Slaby wrote:
> On 01/08/2011 01:16 AM, Bjorn Helgaas wrote:
> > On Friday, January 07, 2011 04:29:00 pm Jiri Slaby wrote:
> >> On 01/08/2011 12:03 AM, Bjorn Helgaas wrote:
> >>> On Friday, January 07, 2011 01:44:35 pm Jiri Slaby wrote:
> >>>> On 01/06/2011 08:24 PM, Bjorn Helgaas wrote:
> >>>>> Theoretically, ACPI tells us about the GPIO/TCO/etc. regions in a
> >>>>> generic way via namespace devices or something in the static tables.
> >>>>> Is that generic information missing, or is it there and Linux is
> >>>>> ignoring it?  If we're ignoring it, I'd rather fix that.
> >>>>
> >>>> It works for most boxes I would say. Try to google for "claimed by ICH4
> >>>> ACPI/GPIO/TCO", it reports sane ranges like 0400-047f or 4000-407f.
> >>>
> >>> My point is that BIOS should be telling the OS about GPIO/TCO/etc.
> >>> regions via an ACPI mechanism, and, ideally, we would use that rather
> >>> than reading the address out of chipset-dependent registers.
> >>>
> >>> Even though PMBASE says the ACPI registers occupy 128 bytes from
> >>> 0x100-0x17f, it's likely there's no actual conflict between the
> >>> last 16 bytes and the IDE device.
> >>
> >> I wouldn't say so. According to the datasheet 0x60-0x7f of the space
> >> (i.e. 0x160-0x17f here) is for TCO registers. There:
> >> 0x10 -- Software IRQ Generation Register (i.e. 0x170)
> >> 0x11-0x1f -- reserved (0x171-0x17f)
> >>
> >> So at least 0x170 should be conflicting. Unless TCO is unused/disabled
> >> and not mapped there at all. May be that the case?
> > 
> > Maybe.  All your patch does is avoid reserving this 0x100-0x1f7
> > region; it doesn't actually *move* anything.  And the IDE device
> > apparently works at the 0x170 compatibility address.  So the
> > ICH ACPI stuff is still at 0x100-0x17f, so apparently they don't
> > conflict or maybe the ICH ACPI stuff is disabled.  If the box
> > doesn't even have ACPI, I suppose there would be no reason to
> > have the ACPI registers enabled.  Is there something in ICH
> > that tells us whether they're enabled?
> 
> Hmm, there is:
> bit 4: ACPI Enable (ACPI_EN) — R/W.
>   0 = Disable.
>   1 = Decode of the I/O range pointed to by the ACPI Base register is
> enabled, and the ACPI power management function is enabled. Note that
> the APM power management ranges (B2/B3h) are always enabled and are not
> affected by this bit.
> 
> at 0x44 in the bridge conf space. So we should definitely check the value.
> 
> I don't have the actual value in that register when ACPI is disabled in
> BIOS. From the run where acpi=off was passed to the kernel, there is
> 0x10 (i.e. ACPI_EN=1). However I don't know whether ACPI was disabled in
> BIOS at that time.

Checking ACPI_EN before doing anything in the quirk looks like
the simplest thing (if the BIOS actually sets ACPI_EN=0 when
it disables ACPI).

Bjorn

Re: [PATCH 1/1] PCI: tune up ICH4 quirk for broken BIOSes

[Jiri Slaby]

On 01/10/2011 07:40 PM, Bjorn Helgaas wrote:
> On Saturday, January 08, 2011 02:58:01 am Jiri Slaby wrote:
>> On 01/08/2011 01:16 AM, Bjorn Helgaas wrote:
>>> On Friday, January 07, 2011 04:29:00 pm Jiri Slaby wrote:
>>>> On 01/08/2011 12:03 AM, Bjorn Helgaas wrote:
>>>>> On Friday, January 07, 2011 01:44:35 pm Jiri Slaby wrote:
>>>>>> On 01/06/2011 08:24 PM, Bjorn Helgaas wrote:
>>>>>>> Theoretically, ACPI tells us about the GPIO/TCO/etc. regions in a
>>>>>>> generic way via namespace devices or something in the static tables.
>>>>>>> Is that generic information missing, or is it there and Linux is
>>>>>>> ignoring it?  If we're ignoring it, I'd rather fix that.
>>>>>>
>>>>>> It works for most boxes I would say. Try to google for "claimed by ICH4
>>>>>> ACPI/GPIO/TCO", it reports sane ranges like 0400-047f or 4000-407f.
>>>>>
>>>>> My point is that BIOS should be telling the OS about GPIO/TCO/etc.
>>>>> regions via an ACPI mechanism, and, ideally, we would use that rather
>>>>> than reading the address out of chipset-dependent registers.
>>>>>
>>>>> Even though PMBASE says the ACPI registers occupy 128 bytes from
>>>>> 0x100-0x17f, it's likely there's no actual conflict between the
>>>>> last 16 bytes and the IDE device.
>>>>
>>>> I wouldn't say so. According to the datasheet 0x60-0x7f of the space
>>>> (i.e. 0x160-0x17f here) is for TCO registers. There:
>>>> 0x10 -- Software IRQ Generation Register (i.e. 0x170)
>>>> 0x11-0x1f -- reserved (0x171-0x17f)
>>>>
>>>> So at least 0x170 should be conflicting. Unless TCO is unused/disabled
>>>> and not mapped there at all. May be that the case?
>>>
>>> Maybe.  All your patch does is avoid reserving this 0x100-0x1f7
>>> region; it doesn't actually *move* anything.  And the IDE device
>>> apparently works at the 0x170 compatibility address.  So the
>>> ICH ACPI stuff is still at 0x100-0x17f, so apparently they don't
>>> conflict or maybe the ICH ACPI stuff is disabled.  If the box
>>> doesn't even have ACPI, I suppose there would be no reason to
>>> have the ACPI registers enabled.  Is there something in ICH
>>> that tells us whether they're enabled?
>>
>> Hmm, there is:
>> bit 4: ACPI Enable (ACPI_EN) — R/W.
>>   0 = Disable.
>>   1 = Decode of the I/O range pointed to by the ACPI Base register is
>> enabled, and the ACPI power management function is enabled. Note that
>> the APM power management ranges (B2/B3h) are always enabled and are not
>> affected by this bit.
>>
>> at 0x44 in the bridge conf space. So we should definitely check the value.
>>
>> I don't have the actual value in that register when ACPI is disabled in
>> BIOS. From the run where acpi=off was passed to the kernel, there is
>> 0x10 (i.e. ACPI_EN=1). However I don't know whether ACPI was disabled in
>> BIOS at that time.
> 
> Checking ACPI_EN before doing anything in the quirk looks like
> the simplest thing (if the BIOS actually sets ACPI_EN=0 when
> it disables ACPI).

Unfortunately, they double checked and the BIOS leaves ACPI_EN=1 even
when ACPI is disabled.
From hexdump -Cv /sys/bus/pci/devices/0000\:00\:1f.0/config:
00000040  01 01 00 00 10 00 00 00  00 00 00 00 00 00 01 00
          ^base addr^|^-- 4th bit ~ ACPI_EN

But I think we still should add the check in any case, the same for GPIO
(there is GPIO_EN) and maybe for newer ICHs. What do you think?

The problem is we can't add a quirk based on DMI for this one, since
there is no DMI table.

I'm out of ideas now.

thanks,
-- 
js

Re: [PATCH 1/1] PCI: tune up ICH4 quirk for broken BIOSes

[Bjorn Helgaas]

On Thursday, January 13, 2011 03:07:24 am Jiri Slaby wrote:
> On 01/10/2011 07:40 PM, Bjorn Helgaas wrote:
> > On Saturday, January 08, 2011 02:58:01 am Jiri Slaby wrote:
> >> On 01/08/2011 01:16 AM, Bjorn Helgaas wrote:
> >>> On Friday, January 07, 2011 04:29:00 pm Jiri Slaby wrote:
> >>>> On 01/08/2011 12:03 AM, Bjorn Helgaas wrote:
> >>>>> On Friday, January 07, 2011 01:44:35 pm Jiri Slaby wrote:
> >>>>>> On 01/06/2011 08:24 PM, Bjorn Helgaas wrote:
> >>>>>>> Theoretically, ACPI tells us about the GPIO/TCO/etc. regions in a
> >>>>>>> generic way via namespace devices or something in the static tables.
> >>>>>>> Is that generic information missing, or is it there and Linux is
> >>>>>>> ignoring it?  If we're ignoring it, I'd rather fix that.
> >>>>>>
> >>>>>> It works for most boxes I would say. Try to google for "claimed by ICH4
> >>>>>> ACPI/GPIO/TCO", it reports sane ranges like 0400-047f or 4000-407f.
> >>>>>
> >>>>> My point is that BIOS should be telling the OS about GPIO/TCO/etc.
> >>>>> regions via an ACPI mechanism, and, ideally, we would use that rather
> >>>>> than reading the address out of chipset-dependent registers.
> >>>>>
> >>>>> Even though PMBASE says the ACPI registers occupy 128 bytes from
> >>>>> 0x100-0x17f, it's likely there's no actual conflict between the
> >>>>> last 16 bytes and the IDE device.
> >>>>
> >>>> I wouldn't say so. According to the datasheet 0x60-0x7f of the space
> >>>> (i.e. 0x160-0x17f here) is for TCO registers. There:
> >>>> 0x10 -- Software IRQ Generation Register (i.e. 0x170)
> >>>> 0x11-0x1f -- reserved (0x171-0x17f)
> >>>>
> >>>> So at least 0x170 should be conflicting. Unless TCO is unused/disabled
> >>>> and not mapped there at all. May be that the case?
> >>>
> >>> Maybe.  All your patch does is avoid reserving this 0x100-0x1f7
> >>> region; it doesn't actually *move* anything.  And the IDE device
> >>> apparently works at the 0x170 compatibility address.  So the
> >>> ICH ACPI stuff is still at 0x100-0x17f, so apparently they don't
> >>> conflict or maybe the ICH ACPI stuff is disabled.  If the box
> >>> doesn't even have ACPI, I suppose there would be no reason to
> >>> have the ACPI registers enabled.  Is there something in ICH
> >>> that tells us whether they're enabled?
> >>
> >> Hmm, there is:
> >> bit 4: ACPI Enable (ACPI_EN) — R/W.
> >>   0 = Disable.
> >>   1 = Decode of the I/O range pointed to by the ACPI Base register is
> >> enabled, and the ACPI power management function is enabled. Note that
> >> the APM power management ranges (B2/B3h) are always enabled and are not
> >> affected by this bit.
> >>
> >> at 0x44 in the bridge conf space. So we should definitely check the value.
> >>
> >> I don't have the actual value in that register when ACPI is disabled in
> >> BIOS. From the run where acpi=off was passed to the kernel, there is
> >> 0x10 (i.e. ACPI_EN=1). However I don't know whether ACPI was disabled in
> >> BIOS at that time.
> > 
> > Checking ACPI_EN before doing anything in the quirk looks like
> > the simplest thing (if the BIOS actually sets ACPI_EN=0 when
> > it disables ACPI).
> 
> Unfortunately, they double checked and the BIOS leaves ACPI_EN=1 even
> when ACPI is disabled.
> From hexdump -Cv /sys/bus/pci/devices/0000\:00\:1f.0/config:
> 00000040  01 01 00 00 10 00 00 00  00 00 00 00 00 00 01 00
>           ^base addr^|^-- 4th bit ~ ACPI_EN
> 
> But I think we still should add the check in any case, the same for GPIO
> (there is GPIO_EN) and maybe for newer ICHs. What do you think?
> 
> The problem is we can't add a quirk based on DMI for this one, since
> there is no DMI table.
> 
> I'm out of ideas now.

I think we're back to the question of why we have the ICH4 quirk in
the first place, and I don't know the answer to that.

If we didn't have the quirk at all, this machine would work.  Other
machines *should* work without, but maybe there's a BIOS or old Linux
bug that the quirk covers up.  I added Linus because he added some
similar "should be unnecessary" code for ICH7+ (894886e5d).

Maybe you could make the ICH4 quirk only claim the region if it's
above PCIBIOS_MIN_IO, i.e., if it's in the area where we could put
another PCI device on top of it?

Bjorn

Re: [PATCH 1/1] PCI: tune up ICH4 quirk for broken BIOSes

[Linus Torvalds]

On Thu, Jan 13, 2011 at 3:19 PM, Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
>
> I think we're back to the question of why we have the ICH4 quirk in
> the first place, and I don't know the answer to that.

Iirc, there were several laptops that didn't have the ACPI region
mentioned in any of the regular places, and we'd allocate the PCMCIA
IO region on top of them. The machine would boot, but if anybody ever
inserted a PCCard into the machine, the first access to the IO region
would generally just halt it (because it was trying to read the
PCCard, but the APCI region decodes first, and then the read from that
usually put the CPU in a sleep state that it would never wake up from
for obvious reasons).

So we do want the ICH4 quirk.

> Maybe you could make the ICH4 quirk only claim the region if it's
> above PCIBIOS_MIN_IO, i.e., if it's in the area where we could put
> another PCI device on top of it?

That may well be the best solution - we'd only mark it reserved if it
might conflict with a dynamic allocation, and if it conflicts with
other motherboard resources (like the IDE ports), then we don't know
what the decode priority is anyway, so..

In this case, it looks like the legacy IDE ports get decoded before
the dynamic BAR's. Which is not too much of a surprise (fixed
addresses tend to be the first to get decoded).

                    Linus

Re: [PATCH 1/1] PCI: tune up ICH4 quirk for broken BIOSes

[Jiri Slaby]

On 01/14/2011 01:15 AM, Linus Torvalds wrote:
> On Thu, Jan 13, 2011 at 3:19 PM, Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
>>
>> I think we're back to the question of why we have the ICH4 quirk in
>> the first place, and I don't know the answer to that.
> 
> Iirc, there were several laptops that didn't have the ACPI region
> mentioned in any of the regular places, and we'd allocate the PCMCIA
> IO region on top of them. The machine would boot, but if anybody ever
> inserted a PCCard into the machine, the first access to the IO region
> would generally just halt it (because it was trying to read the
> PCCard, but the APCI region decodes first, and then the read from that
> usually put the CPU in a sleep state that it would never wake up from
> for obvious reasons).
> 
> So we do want the ICH4 quirk.

Yes, this is an "official" way how ICH4 (and later) advertises the region.

The original commit says:
commit df4ba87cfc2239aa85e21178fca35cf6644a6dd1
Author: Linus Torvalds <torvalds@home.osdl.org>
Date:   Thu Oct 23 17:52:25 2003 -0700

    Add a quirk for the Intel ICH-[45] to add special ACPI regions.

    This fixes resource conflicts due to IO decode that doesn't show
    up with a normal PCI probe (we do similar quirks for most other
    chipsets). Without it, the kernel doesn't know about some magic
    IO decodes for the chips.

>> Maybe you could make the ICH4 quirk only claim the region if it's
>> above PCIBIOS_MIN_IO, i.e., if it's in the area where we could put
>> another PCI device on top of it?
> 
> That may well be the best solution - we'd only mark it reserved if it
> might conflict with a dynamic allocation, and if it conflicts with
> other motherboard resources (like the IDE ports), then we don't know
> what the decode priority is anyway, so..
> 
> In this case, it looks like the legacy IDE ports get decoded before
> the dynamic BAR's. Which is not too much of a surprise (fixed
> addresses tend to be the first to get decoded).

I will attach 2 patches for this as a reply. I created 2 variants:
A is for all quirk
B is only for ICH4 and ICH6

thanks,
-- 
js

[PATCH 1/2] PCI: add more checking to ICH region quirks

[Jiri Slaby]

Per ICH4 and ICH6 specs, ACPI and GPIO regions are valid iff ACPI_EN
and GPIO_EN bits are set to 1. Add checks for these bits into the
quirks prior to the region creation.

While at it, name the constants by macros.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Bjorn Helgaas <bjorn.helgaas@hp.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Thomas Renninger <trenn@suse.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
---
 drivers/pci/quirks.c |   45 +++++++++++++++++++++++++++++++++++++--------
 1 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 53a786f..8db2426 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -533,6 +533,17 @@ static void __devinit quirk_piix4_acpi(struct pci_dev *dev)
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_82371AB_3,	quirk_piix4_acpi);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_82443MX_3,	quirk_piix4_acpi);
 
+#define ICH_PMBASE	0x40
+#define ICH_ACPI_CNTL	0x44
+#define  ICH4_ACPI_EN	0x10
+#define  ICH6_ACPI_EN	0x80
+#define ICH4_GPIOBASE	0x58
+#define ICH4_GPIO_CNTL	0x5c
+#define  ICH4_GPIO_EN	0x10
+#define ICH6_GPIOBASE	0x48
+#define ICH6_GPIO_CNTL	0x4c
+#define  ICH6_GPIO_EN	0x10
+
 /*
  * ICH4, ICH4-M, ICH5, ICH5-M ACPI: Three IO regions pointed to by longwords at
  *	0x40 (128 bytes of ACPI, GPIO & TCO registers)
@@ -541,12 +552,21 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,	PCI_DEVICE_ID_INTEL_82443MX_3,	qui
 static void __devinit quirk_ich4_lpc_acpi(struct pci_dev *dev)
 {
 	u32 region;
+	u8 enable;
 
-	pci_read_config_dword(dev, 0x40, &region);
-	quirk_io_region(dev, region, 128, PCI_BRIDGE_RESOURCES, "ICH4 ACPI/GPIO/TCO");
+	pci_read_config_byte(dev, ICH_ACPI_CNTL, &enable);
+	if (enable & ICH4_ACPI_EN) {
+		pci_read_config_dword(dev, ICH_PMBASE, &region);
+		quirk_io_region(dev, region, 128, PCI_BRIDGE_RESOURCES,
+				"ICH4 ACPI/GPIO/TCO");
+	}
 
-	pci_read_config_dword(dev, 0x58, &region);
-	quirk_io_region(dev, region, 64, PCI_BRIDGE_RESOURCES+1, "ICH4 GPIO");
+	pci_read_config_byte(dev, ICH4_GPIO_CNTL, &enable);
+	if (enable & ICH4_GPIO_EN) {
+		pci_read_config_dword(dev, ICH4_GPIOBASE, &region);
+		quirk_io_region(dev, region, 64, PCI_BRIDGE_RESOURCES + 1,
+				"ICH4 GPIO");
+	}
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,    PCI_DEVICE_ID_INTEL_82801AA_0,		quirk_ich4_lpc_acpi);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,    PCI_DEVICE_ID_INTEL_82801AB_0,		quirk_ich4_lpc_acpi);
@@ -562,12 +582,21 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,    PCI_DEVICE_ID_INTEL_ESB_1,		qui
 static void __devinit ich6_lpc_acpi_gpio(struct pci_dev *dev)
 {
 	u32 region;
+	u8 enable;
 
-	pci_read_config_dword(dev, 0x40, &region);
-	quirk_io_region(dev, region, 128, PCI_BRIDGE_RESOURCES, "ICH6 ACPI/GPIO/TCO");
+	pci_read_config_byte(dev, ICH_ACPI_CNTL, &enable);
+	if (enable & ICH6_ACPI_EN) {
+		pci_read_config_dword(dev, ICH_PMBASE, &region);
+		quirk_io_region(dev, region, 128, PCI_BRIDGE_RESOURCES,
+				"ICH6 ACPI/GPIO/TCO");
+	}
 
-	pci_read_config_dword(dev, 0x48, &region);
-	quirk_io_region(dev, region, 64, PCI_BRIDGE_RESOURCES+1, "ICH6 GPIO");
+	pci_read_config_byte(dev, ICH6_GPIO_CNTL, &enable);
+	if (enable & ICH4_GPIO_EN) {
+		pci_read_config_dword(dev, ICH6_GPIOBASE, &region);
+		quirk_io_region(dev, region, 64, PCI_BRIDGE_RESOURCES + 1,
+				"ICH6 GPIO");
+	}
 }
 
 static void __devinit ich6_lpc_generic_decode(struct pci_dev *dev, unsigned reg, const char *name, int dynsize)
-- 
1.7.3.4

[PATCH option B 2/2] PCI: do not create quirk I/O regions below PCIBIOS_MIN_IO for ICH

[Jiri Slaby]

Some broken BIOSes on ICH4 chipset report an ACPI region which is in
conflict with legacy IDE ports when ACPI is disabled. Even though the
regions overlap, IDE ports are working correctly (we cannot find out
the decoding rules on chipsets).

So the only problem is the reported region itself, if we don't reserve
the region in the quirk everything works as expected.

This patch avoids reserving any quirk regions below PCIBIOS_MIN_IO
which is 0x1000. Some regions might be (and are by a fast google
query) below this border, but the only difference is that they won't
be reserved anymore. They should still work though the same as before.

The conflicts look like (1f.0 is bridge, 1f.1 is IDE ctrl):
pci 0000:00:1f.1: address space collision: [io 0x0170-0x0177] conflicts with 0000:00:1f.0 [io  0x0100-0x017f]

At 0x0100 a 128 bytes long ACPI region is reported in the quirk for
ICH4. ata_piix then fails to find disks because the IDE legacy ports
are zeroed:
ata_piix 0000:00:1f.1: device not available (can't reserve [io 0x0000-0x0007])

References: https://bugzilla.novell.com/show_bug.cgi?id=558740
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Bjorn Helgaas <bjorn.helgaas@hp.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Thomas Renninger <trenn@suse.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
---
 drivers/pci/quirks.c |   32 ++++++++++++++++++++++++--------
 1 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 8db2426..b3ab2f7 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -554,18 +554,30 @@ static void __devinit quirk_ich4_lpc_acpi(struct pci_dev *dev)
 	u32 region;
 	u8 enable;
 
+	/*
+	 * The check for PCIBIOS_MIN_IO is to ensure we won't create a conflict
+	 * with low legacy (and fixed) ports. We don't know the decoding
+	 * priority and can't tell whether the legacy device or the one created
+	 * here is really at that address.  This happens on boards with broken
+	 * BIOSes.
+	*/
+
 	pci_read_config_byte(dev, ICH_ACPI_CNTL, &enable);
 	if (enable & ICH4_ACPI_EN) {
 		pci_read_config_dword(dev, ICH_PMBASE, &region);
-		quirk_io_region(dev, region, 128, PCI_BRIDGE_RESOURCES,
-				"ICH4 ACPI/GPIO/TCO");
+		region &= PCI_BASE_ADDRESS_IO_MASK;
+		if (region >= PCIBIOS_MIN_IO)
+			quirk_io_region(dev, region, 128, PCI_BRIDGE_RESOURCES,
+					"ICH4 ACPI/GPIO/TCO");
 	}
 
 	pci_read_config_byte(dev, ICH4_GPIO_CNTL, &enable);
 	if (enable & ICH4_GPIO_EN) {
 		pci_read_config_dword(dev, ICH4_GPIOBASE, &region);
-		quirk_io_region(dev, region, 64, PCI_BRIDGE_RESOURCES + 1,
-				"ICH4 GPIO");
+		region &= PCI_BASE_ADDRESS_IO_MASK;
+		if (region >= PCIBIOS_MIN_IO)
+			quirk_io_region(dev, region, 64,
+					PCI_BRIDGE_RESOURCES + 1, "ICH4 GPIO");
 	}
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,    PCI_DEVICE_ID_INTEL_82801AA_0,		quirk_ich4_lpc_acpi);
@@ -587,15 +599,19 @@ static void __devinit ich6_lpc_acpi_gpio(struct pci_dev *dev)
 	pci_read_config_byte(dev, ICH_ACPI_CNTL, &enable);
 	if (enable & ICH6_ACPI_EN) {
 		pci_read_config_dword(dev, ICH_PMBASE, &region);
-		quirk_io_region(dev, region, 128, PCI_BRIDGE_RESOURCES,
-				"ICH6 ACPI/GPIO/TCO");
+		region &= PCI_BASE_ADDRESS_IO_MASK;
+		if (region >= PCIBIOS_MIN_IO)
+			quirk_io_region(dev, region, 128, PCI_BRIDGE_RESOURCES,
+					"ICH6 ACPI/GPIO/TCO");
 	}
 
 	pci_read_config_byte(dev, ICH6_GPIO_CNTL, &enable);
 	if (enable & ICH4_GPIO_EN) {
 		pci_read_config_dword(dev, ICH6_GPIOBASE, &region);
-		quirk_io_region(dev, region, 64, PCI_BRIDGE_RESOURCES + 1,
-				"ICH6 GPIO");
+		region &= PCI_BASE_ADDRESS_IO_MASK;
+		if (region >= PCIBIOS_MIN_IO)
+			quirk_io_region(dev, region, 64,
+					PCI_BRIDGE_RESOURCES + 1, "ICH6 GPIO");
 	}
 }
 
-- 
1.7.3.4

Re: [PATCH option B 2/2] PCI: do not create quirk I/O regions below PCIBIOS_MIN_IO for ICH

[Sergei Shtylyov]

Hello.

On 14-01-2011 13:32, Jiri Slaby wrote:

> Some broken BIOSes on ICH4 chipset report an ACPI region which is in
> conflict with legacy IDE ports when ACPI is disabled. Even though the
> regions overlap, IDE ports are working correctly (we cannot find out
> the decoding rules on chipsets).
>
> So the only problem is the reported region itself, if we don't reserve
> the region in the quirk everything works as expected.

> This patch avoids reserving any quirk regions below PCIBIOS_MIN_IO
> which is 0x1000. Some regions might be (and are by a fast google
> query) below this border, but the only difference is that they won't
> be reserved anymore. They should still work though the same as before.

> The conflicts look like (1f.0 is bridge, 1f.1 is IDE ctrl):
> pci 0000:00:1f.1: address space collision: [io 0x0170-0x0177] conflicts with 0000:00:1f.0 [io  0x0100-0x017f]

> At 0x0100 a 128 bytes long ACPI region is reported in the quirk for
> ICH4. ata_piix then fails to find disks because the IDE legacy ports
> are zeroed:
> ata_piix 0000:00:1f.1: device not available (can't reserve [io 0x0000-0x0007])

> References: https://bugzilla.novell.com/show_bug.cgi?id=558740
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Bjorn Helgaas <bjorn.helgaas@hp.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Thomas Renninger <trenn@suse.de>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
[...]

> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 8db2426..b3ab2f7 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -554,18 +554,30 @@ static void __devinit quirk_ich4_lpc_acpi(struct pci_dev *dev)
>   	u32 region;
>   	u8 enable;
>
> +	/*
> +	 * The check for PCIBIOS_MIN_IO is to ensure we won't create a conflict
> +	 * with low legacy (and fixed) ports. We don't know the decoding
> +	 * priority and can't tell whether the legacy device or the one created
> +	 * here is really at that address.  This happens on boards with broken
> +	 * BIOSes.
> +	*/
> +
>   	pci_read_config_byte(dev, ICH_ACPI_CNTL,&enable);
>   	if (enable&  ICH4_ACPI_EN) {
>   		pci_read_config_dword(dev, ICH_PMBASE,&region);
> -		quirk_io_region(dev, region, 128, PCI_BRIDGE_RESOURCES,
> -				"ICH4 ACPI/GPIO/TCO");
> +		region &= PCI_BASE_ADDRESS_IO_MASK;

    Why don't you do the masking right in the patch #1? And is it really 
necessary if the region size is known to be 128 bytes?

WBR, Sergei

Re: [PATCH option B 2/2] PCI: do not create quirk I/O regions below PCIBIOS_MIN_IO for ICH

[Jiri Slaby]

On 02/11/2011 01:09 PM, Sergei Shtylyov wrote:
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index 8db2426..b3ab2f7 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -554,18 +554,30 @@ static void __devinit quirk_ich4_lpc_acpi(struct
>> pci_dev *dev)
>>       u32 region;
>>       u8 enable;
>>
>> +    /*
>> +     * The check for PCIBIOS_MIN_IO is to ensure we won't create a
>> conflict
>> +     * with low legacy (and fixed) ports. We don't know the decoding
>> +     * priority and can't tell whether the legacy device or the one
>> created
>> +     * here is really at that address.  This happens on boards with
>> broken
>> +     * BIOSes.
>> +    */
>> +
>>       pci_read_config_byte(dev, ICH_ACPI_CNTL,&enable);
>>       if (enable&  ICH4_ACPI_EN) {
>>           pci_read_config_dword(dev, ICH_PMBASE,&region);
>> -        quirk_io_region(dev, region, 128, PCI_BRIDGE_RESOURCES,
>> -                "ICH4 ACPI/GPIO/TCO");
>> +        region &= PCI_BASE_ADDRESS_IO_MASK;
> 
>    Why don't you do the masking right in the patch #1? And is it really
> necessary if the region size is known to be 128 bytes?

The region here contains also the low flag bits. Yes, it can be a part
of 1/1. But I don't think it matters.

regards,
-- 
js
suse labs

[PATCH option A 2/2] PCI: do not create quirk I/O regions below PCIBIOS_MIN_IO

[Jiri Slaby]

Some broken BIOSes on ICH4 chipset report an ACPI region which is in
conflict with legacy IDE ports when ACPI is disabled. Even though the
regions overlap, IDE ports are working correctly (we cannot find out
the decoding rules on chipsets).

So the only problem is the reported region itself, if we don't reserve
the region in the quirk everything works as expected.

This patch avoids reserving any quirk regions below PCIBIOS_MIN_IO
which is 0x1000. Some regions might be (and are by a fast google
query) below this border, but the only difference is that they won't
be reserved anymore. They should still work though the same as before.

The conflicts look like (1f.0 is bridge, 1f.1 is IDE ctrl):
pci 0000:00:1f.1: address space collision: [io 0x0170-0x0177] conflicts with 0000:00:1f.0 [io  0x0100-0x017f]

At 0x0100 a 128 bytes long ACPI region is reported in the quirk for
ICH4. ata_piix then fails to find disks because the IDE legacy ports
are zeroed:
ata_piix 0000:00:1f.1: device not available (can't reserve [io 0x0000-0x0007])

References: https://bugzilla.novell.com/show_bug.cgi?id=558740
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Bjorn Helgaas <bjorn.helgaas@hp.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Thomas Renninger <trenn@suse.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
---
 drivers/pci/quirks.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 8db2426..cfedf2b 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -388,7 +388,14 @@ static void __devinit quirk_io_region(struct pci_dev *dev, unsigned region,
 	unsigned size, int nr, const char *name)
 {
 	region &= ~(size-1);
-	if (region) {
+	/*
+	 * The check for PCIBIOS_MIN_IO is to ensure we won't create a conflict
+	 * with low legacy (and fixed) ports. We don't know the decoding
+	 * priority and can't tell whether the legacy device or the one created
+	 * here is really at that address. This happens on boards with broken
+	 * BIOSes. At least one known is with ICH4.
+	 */
+	if (region && region >= PCIBIOS_MIN_IO) {
 		struct pci_bus_region bus_region;
 		struct resource *res = dev->resource + nr;
 
-- 
1.7.3.4

Re: [PATCH 1/1] PCI: tune up ICH4 quirk for broken BIOSes

[Bjorn Helgaas]

On Friday, January 14, 2011 03:31:16 am Jiri Slaby wrote:
> On 01/14/2011 01:15 AM, Linus Torvalds wrote:
> > On Thu, Jan 13, 2011 at 3:19 PM, Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
> >>
> >> I think we're back to the question of why we have the ICH4 quirk in
> >> the first place, and I don't know the answer to that.
> > 
> > Iirc, there were several laptops that didn't have the ACPI region
> > mentioned in any of the regular places, and we'd allocate the PCMCIA
> > IO region on top of them. The machine would boot, but if anybody ever
> > inserted a PCCard into the machine, the first access to the IO region
> > would generally just halt it (because it was trying to read the
> > PCCard, but the APCI region decodes first, and then the read from that
> > usually put the CPU in a sleep state that it would never wake up from
> > for obvious reasons).
> > 
> > So we do want the ICH4 quirk.
> 
> Yes, this is an "official" way how ICH4 (and later) advertises the region.

The quirk is a bug workaround, *not* the "official, planned" way to
deal with these regions.  The official way is to use ACPI, because
that's a generic way that doesn't require changes for new versions
of ICH.

The bug might be that BIOS didn't mention the region in the expected
places.  Or it might be that the BIOS mentioned it, but Linux didn't
deal with it correctly.  For example, the Linux ACPI/PNP core mostly
ignores the resources mentioned in ACPI _CRS methods.  We only look
at those in individual drivers.  The PNP system.c driver reserves
PNP0C01/02 resources, but the BIOS could use other PNP IDs, and Linux
would ignore them (PNPACPI parses them, but doesn't reserve them in
any way).

Bjorn

Re: [PATCH 1/1] PCI: tune up ICH4 quirk for broken BIOSes

[Robert Hancock]

On 01/14/2011 10:10 AM, Bjorn Helgaas wrote:
> On Friday, January 14, 2011 03:31:16 am Jiri Slaby wrote:
>> On 01/14/2011 01:15 AM, Linus Torvalds wrote:
>>> On Thu, Jan 13, 2011 at 3:19 PM, Bjorn Helgaas<bjorn.helgaas@hp.com>  wrote:
>>>>
>>>> I think we're back to the question of why we have the ICH4 quirk in
>>>> the first place, and I don't know the answer to that.
>>>
>>> Iirc, there were several laptops that didn't have the ACPI region
>>> mentioned in any of the regular places, and we'd allocate the PCMCIA
>>> IO region on top of them. The machine would boot, but if anybody ever
>>> inserted a PCCard into the machine, the first access to the IO region
>>> would generally just halt it (because it was trying to read the
>>> PCCard, but the APCI region decodes first, and then the read from that
>>> usually put the CPU in a sleep state that it would never wake up from
>>> for obvious reasons).
>>>
>>> So we do want the ICH4 quirk.
>>
>> Yes, this is an "official" way how ICH4 (and later) advertises the region.
>
> The quirk is a bug workaround, *not* the "official, planned" way to
> deal with these regions.  The official way is to use ACPI, because
> that's a generic way that doesn't require changes for new versions
> of ICH.
>
> The bug might be that BIOS didn't mention the region in the expected
> places.  Or it might be that the BIOS mentioned it, but Linux didn't
> deal with it correctly.  For example, the Linux ACPI/PNP core mostly
> ignores the resources mentioned in ACPI _CRS methods.  We only look
> at those in individual drivers.  The PNP system.c driver reserves
> PNP0C01/02 resources, but the BIOS could use other PNP IDs, and Linux
> would ignore them (PNPACPI parses them, but doesn't reserve them in
> any way).

The patch to add the quirk was added back in 2003, and I believe using 
ACPI in Linux (other than for HT sibling discovery) was an uncommon 
configuration back then. It's quite possible the resources were reserved 
in ACPI on such systems and the quirk was only needed for systems with 
ACPI disabled in either the BIOS or the kernel.

Re: [PATCH 1/1] PCI: tune up ICH4 quirk for broken BIOSes

[Jiri Slaby]

On 01/14/2011 05:10 PM, Bjorn Helgaas wrote:
> On Friday, January 14, 2011 03:31:16 am Jiri Slaby wrote:
>> On 01/14/2011 01:15 AM, Linus Torvalds wrote:
>>> On Thu, Jan 13, 2011 at 3:19 PM, Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
>>>>
>>>> I think we're back to the question of why we have the ICH4 quirk in
>>>> the first place, and I don't know the answer to that.
>>>
>>> Iirc, there were several laptops that didn't have the ACPI region
>>> mentioned in any of the regular places, and we'd allocate the PCMCIA
>>> IO region on top of them. The machine would boot, but if anybody ever
>>> inserted a PCCard into the machine, the first access to the IO region
>>> would generally just halt it (because it was trying to read the
>>> PCCard, but the APCI region decodes first, and then the read from that
>>> usually put the CPU in a sleep state that it would never wake up from
>>> for obvious reasons).
>>>
>>> So we do want the ICH4 quirk.
>>
>> Yes, this is an "official" way how ICH4 (and later) advertises the region.
> 
> The quirk is a bug workaround, *not* the "official, planned" way to
> deal with these regions.  The official way is to use ACPI, because
> that's a generic way that doesn't require changes for new versions
> of ICH.

Ok, I understand that. For non-ACPI setups this is probably the only
place to look at.

Anyway, has anybody had a chance to look at the patches? Any comments,
nacks/acks?

https://lkml.org/lkml/2011/1/14/115
https://lkml.org/lkml/2011/1/14/113
https://lkml.org/lkml/2011/1/14/114

thanks,
-- 
js

Re: [PATCH 1/1] PCI: tune up ICH4 quirk for broken BIOSes

[Jesse Barnes]

On Tue, 08 Feb 2011 10:55:01 +0100
Jiri Slaby <jirislaby@gmail.com> wrote:

> On 01/14/2011 05:10 PM, Bjorn Helgaas wrote:
> > On Friday, January 14, 2011 03:31:16 am Jiri Slaby wrote:
> >> On 01/14/2011 01:15 AM, Linus Torvalds wrote:
> >>> On Thu, Jan 13, 2011 at 3:19 PM, Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
> >>>>
> >>>> I think we're back to the question of why we have the ICH4 quirk in
> >>>> the first place, and I don't know the answer to that.
> >>>
> >>> Iirc, there were several laptops that didn't have the ACPI region
> >>> mentioned in any of the regular places, and we'd allocate the PCMCIA
> >>> IO region on top of them. The machine would boot, but if anybody ever
> >>> inserted a PCCard into the machine, the first access to the IO region
> >>> would generally just halt it (because it was trying to read the
> >>> PCCard, but the APCI region decodes first, and then the read from that
> >>> usually put the CPU in a sleep state that it would never wake up from
> >>> for obvious reasons).
> >>>
> >>> So we do want the ICH4 quirk.
> >>
> >> Yes, this is an "official" way how ICH4 (and later) advertises the region.
> > 
> > The quirk is a bug workaround, *not* the "official, planned" way to
> > deal with these regions.  The official way is to use ACPI, because
> > that's a generic way that doesn't require changes for new versions
> > of ICH.
> 
> Ok, I understand that. For non-ACPI setups this is probably the only
> place to look at.
> 
> Anyway, has anybody had a chance to look at the patches? Any comments,
> nacks/acks?
> 
> https://lkml.org/lkml/2011/1/14/115
> https://lkml.org/lkml/2011/1/14/113
> https://lkml.org/lkml/2011/1/14/114

I don't have a problem making the quirk quirkier, but it would be nice
to get rid of the need for it entirely (though we can leave that to
Bjorn :). Can you re-submit these three against my linux-next branch?

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/1] PCI: tune up ICH4 quirk for broken BIOSes

[Jiri Slaby]

On 02/08/2011 10:20 PM, Jesse Barnes wrote:
> On Tue, 08 Feb 2011 10:55:01 +0100
> Jiri Slaby <jirislaby@gmail.com> wrote:
> 
>> On 01/14/2011 05:10 PM, Bjorn Helgaas wrote:
>>> On Friday, January 14, 2011 03:31:16 am Jiri Slaby wrote:
>>>> On 01/14/2011 01:15 AM, Linus Torvalds wrote:
>>>>> On Thu, Jan 13, 2011 at 3:19 PM, Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
>>>>>>
>>>>>> I think we're back to the question of why we have the ICH4 quirk in
>>>>>> the first place, and I don't know the answer to that.
>>>>>
>>>>> Iirc, there were several laptops that didn't have the ACPI region
>>>>> mentioned in any of the regular places, and we'd allocate the PCMCIA
>>>>> IO region on top of them. The machine would boot, but if anybody ever
>>>>> inserted a PCCard into the machine, the first access to the IO region
>>>>> would generally just halt it (because it was trying to read the
>>>>> PCCard, but the APCI region decodes first, and then the read from that
>>>>> usually put the CPU in a sleep state that it would never wake up from
>>>>> for obvious reasons).
>>>>>
>>>>> So we do want the ICH4 quirk.
>>>>
>>>> Yes, this is an "official" way how ICH4 (and later) advertises the region.
>>>
>>> The quirk is a bug workaround, *not* the "official, planned" way to
>>> deal with these regions.  The official way is to use ACPI, because
>>> that's a generic way that doesn't require changes for new versions
>>> of ICH.
>>
>> Ok, I understand that. For non-ACPI setups this is probably the only
>> place to look at.
>>
>> Anyway, has anybody had a chance to look at the patches? Any comments,
>> nacks/acks?
>>
>> https://lkml.org/lkml/2011/1/14/115
>> https://lkml.org/lkml/2011/1/14/113
>> https://lkml.org/lkml/2011/1/14/114
> 
> I don't have a problem making the quirk quirkier, but it would be nice
> to get rid of the need for it entirely (though we can leave that to
> Bjorn :). Can you re-submit these three against my linux-next branch?

Ok, I can. But do you want solution 113 or 114 -- they solve the same,
but in a different manner?

thanks,
-- 
js