diff mbox

[RFC/PATCH,v2] ia64, SR870, EFI bug breaks ata_piix, uninitialized ICH4 IDE EXBAR mem resource

Message ID 20120924235457.GA31372@google.com
State Not Applicable
Headers show

Commit Message

Bjorn Helgaas Sept. 24, 2012, 11:54 p.m. UTC 
On Mon, Sep 24, 2012 at 07:09:12PM +0200, Stephan Schreiber wrote:
> Mpfff, there aren't many replies; seems I didn't satisfy what you
> want to have...
> 
> At first I want to mention that I just want to help the Debian
> project and started testing Debian Wheezy my old ia64 box.

Thanks, I really appreciate that, and you've done a huge amount of
debugging and testing already.  It's very normal to iterate on the
resolution as we're doing now.

> >The firmware left the memory BAR at 0x24 cleared (0x00000000), but it
> >also left the MEM bit in the command register disabled.  So it seems
> >like a Linux bug that we're trying to use that zero address from the
> >BAR.  If the firmware left the MEM or IO decode enable bit cleared,
> >why would we assume it put anything useful in the corresponding BARs?
> 
> Your idea would be a fundamental change in the Kernel; I just want
> to fix the ata_piix problem in Debian Wheezy.

Right.  I think you've tripped over a rather fundamental issue, and
I'm hoping we can fix that.  If we can, that will help many users,
not just the handful who have this ia64 box.

> If you would evaluate the command registers, which the BIOS or EFI
> has initialized, you would work around some wrong BARs. You might
> run into trouble due to wrong command register values instead.
> Are you sure that any BIOS or EFI sets the command registers correctly?

We can't be 100% sure about things like that, of course.  But we do
know that if the MEM or IO bits are set in the command register, the
device will claim transactions that match whatever is in the BARs.
So setting the MEM or IO bit is a pretty strong statement that the
BAR contains a valid address.  If the BIOS leaves those bits clear,
we really can't conclude anything about the BAR contents.

> Currently the Linux Kernel sets and clears the IORESOURCE_MEM and
> IORESOURCE_IO bits in the command registers as needed.
> Windows reconfigures any PCI device. The settings of the BIOS or EFI
> do not matter at all; the user doesn't experience any BIOS bug at
> all.

On x86, Windows normally doesn't reconfigure PCI devices unless it
finds a problem with the configuration done by the BIOS.  I suspect
it works similarly on ia64.  I would guess that Windows noticed that
the MEM bit was not set, and therefore ignored the MEM BAR contents.

Can you try the following patch?  It's based on 3.6-rc5+, but I think
it will apply to your 3.2.23 kernel with minor conflicts that shouldn't
be too hard to resolve.

It's not quite right because we really shouldn't turn on the MEM or IO
decode bit unless *all* of the corresponding BARs have been set, but
in your case, I think there is only one MEM BAR that is an issue.

Bjorn




commit 9038dd3b3c4c9e4c7ca0118c8df398c4c646ab58
Author: Bjorn Helgaas <bhelgaas@google.com>
Date:   Mon Sep 24 17:16:28 2012 -0600

    vsprintf: Add support for IORESOURCE_UNSET in %pR

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Stephan Schreiber Sept. 29, 2012, 10:09 p.m. UTC | #1 
Hello Bjorn,
thank you very much for the patch.
I tested it; it works.

(typing mistake: it must read PCI_COMMAND_MEMORY instead of  
PCI_COMMAND_MEM at one location;
some hunks of the patch couldn't be applied automatically on Kernel  
3.2.23 because some comments in the contexts are different)

The dmesg output:
[    0.000000] Initializing cgroup subsys cpuset
[    0.000000] Initializing cgroup subsys cpu
[    0.000000] Linux version 3.2.0-3-mckinley (Debian 3.2.23-1)  
(debian-kernel@lists.debian.org) (gcc version 4.6.3 (Debian 4.6.3-8) )  
#1 SMP Fri Sep 28 21:57:11 CEST 2012
...
[    0.065510] pci 0000:00:1f.1: [8086:24cb] type 0 class 0x000101
[    0.065524] pci 0000:00:1f.1: reg 10: [io  0x0000-0x0007]
[    0.065535] pci 0000:00:1f.1: reg 14: [io  0x0000-0x0003]
[    0.065546] pci 0000:00:1f.1: reg 18: [io  0x0000-0x0007]
[    0.065556] pci 0000:00:1f.1: reg 1c: [io  0x0000-0x0003]
[    0.065567] pci 0000:00:1f.1: reg 20: [io  0x1000-0x100f]
[    0.065578] pci 0000:00:1f.1: reg 24: [mem 0x00000000-0x000003ff unset]
...
[    1.391380] libata version 3.00 loaded.
[    1.391922] ata_piix 0000:00:1f.1: version 2.13
[    1.391938] ata_piix 0000:00:1f.1: can't derive routing for PCI INT A
[    1.392493] scsi0 : ata_piix
[    1.392886] scsi1 : ata_piix
[    1.393018] ata1: PATA max UDMA/100 cmd 0x1f0 ctl 0x3f6 bmdma 0x1000 irq 34
[    1.393066] ata2: PATA max UDMA/100 cmd 0x170 ctl 0x376 bmdma 0x1008 irq 33
[    1.557756] ata1.00: ATAPI: HL-DT-ST DVDRAM GSA-T40N, JR03, max UDMA/33
[    1.573616] ata1.00: configured for UDMA/33
[    1.579147] scsi 0:0:0:0: CD-ROM            HL-DT-ST DVDRAM  
GSA-T40N  JR03 PQ: 0 ANSI: 5
[    1.590806] sr0: scsi3-mmc drive: 24x/24x writer dvd-ram cd/rw  
xa/form2 cdda tray
[    1.590872] cdrom: Uniform CD-ROM driver Revision: 3.20
[    1.591272] sr 0:0:0:0: Attached scsi CD-ROM sr0
[    1.593910] sr 0:0:0:0: Attached scsi generic sg0 type 5
...


How is the chance to get this patch or a similar one into linux-next?



> On x86, Windows normally doesn't reconfigure PCI devices unless it
> finds a problem with the configuration done by the BIOS.  I suspect
> it works similarly on ia64.  I would guess that Windows noticed that
> the MEM bit was not set, and therefore ignored the MEM BAR contents.

Since I have the four Windows versions 'for Itanium Based Systems' on  
that box as well (XP, Server 2003, 2008, 2008 R2), I can tell you more:
The Device Manager shows a memory range FFBFFC00-FFBFFFFF for the  
"Intel 82801DB Ultra ATA Storage Controller-24CB" - on any of these  
Windows versions.


Stephan



--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Oct. 1, 2012, 4:28 p.m. UTC | #2 
On Sat, Sep 29, 2012 at 4:09 PM, Stephan Schreiber <info@fs-driver.org> wrote:
> Hello Bjorn,
> thank you very much for the patch.
> I tested it; it works.
>
> (typing mistake: it must read PCI_COMMAND_MEMORY instead of PCI_COMMAND_MEM
> at one location;
> some hunks of the patch couldn't be applied automatically on Kernel 3.2.23
> because some comments in the contexts are different)

Thanks a lot for testing this!  I'll fix up this typo and work on
getting something like this merged.

> The dmesg output:
>
> [    0.000000] Initializing cgroup subsys cpuset
> [    0.000000] Initializing cgroup subsys cpu
> [    0.000000] Linux version 3.2.0-3-mckinley (Debian 3.2.23-1)
> (debian-kernel@lists.debian.org) (gcc version 4.6.3 (Debian 4.6.3-8) ) #1
> SMP Fri Sep 28 21:57:11 CEST 2012
> ...
> [    0.065510] pci 0000:00:1f.1: [8086:24cb] type 0 class 0x000101
> [    0.065524] pci 0000:00:1f.1: reg 10: [io  0x0000-0x0007]
> [    0.065535] pci 0000:00:1f.1: reg 14: [io  0x0000-0x0003]
> [    0.065546] pci 0000:00:1f.1: reg 18: [io  0x0000-0x0007]
> [    0.065556] pci 0000:00:1f.1: reg 1c: [io  0x0000-0x0003]
> [    0.065567] pci 0000:00:1f.1: reg 20: [io  0x1000-0x100f]
> [    0.065578] pci 0000:00:1f.1: reg 24: [mem 0x00000000-0x000003ff unset]
> ...
> [    1.391380] libata version 3.00 loaded.
> [    1.391922] ata_piix 0000:00:1f.1: version 2.13
> [    1.391938] ata_piix 0000:00:1f.1: can't derive routing for PCI INT A
> [    1.392493] scsi0 : ata_piix
> [    1.392886] scsi1 : ata_piix
> [    1.393018] ata1: PATA max UDMA/100 cmd 0x1f0 ctl 0x3f6 bmdma 0x1000 irq
> 34
> [    1.393066] ata2: PATA max UDMA/100 cmd 0x170 ctl 0x376 bmdma 0x1008 irq
> 33
> [    1.557756] ata1.00: ATAPI: HL-DT-ST DVDRAM GSA-T40N, JR03, max UDMA/33
> [    1.573616] ata1.00: configured for UDMA/33
> [    1.579147] scsi 0:0:0:0: CD-ROM            HL-DT-ST DVDRAM GSA-T40N
> JR03 PQ: 0 ANSI: 5
> [    1.590806] sr0: scsi3-mmc drive: 24x/24x writer dvd-ram cd/rw xa/form2
> cdda tray
> [    1.590872] cdrom: Uniform CD-ROM driver Revision: 3.20
> [    1.591272] sr 0:0:0:0: Attached scsi CD-ROM sr0
> [    1.593910] sr 0:0:0:0: Attached scsi generic sg0 type 5
> ...

>> On x86, Windows normally doesn't reconfigure PCI devices unless it
>> finds a problem with the configuration done by the BIOS.  I suspect
>> it works similarly on ia64.  I would guess that Windows noticed that
>> the MEM bit was not set, and therefore ignored the MEM BAR contents.
>
>
> Since I have the four Windows versions 'for Itanium Based Systems' on that
> box as well (XP, Server 2003, 2008, 2008 R2), I can tell you more:
> The Device Manager shows a memory range FFBFFC00-FFBFFFFF for the "Intel
> 82801DB Ultra ATA Storage Controller-24CB" - on any of these Windows
> versions.

Oh, that's good data, thanks!  It looks like Windows noticed that the
BAR was invalid and assigned a valid resource to it.  That's in the
third aperture below:

ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-01])
pci_root PNP0A03:00: host bridge window [mem 0x000a0000-0x000fffff]
pci_root PNP0A03:00: host bridge window [mem 0xfa000000-0xfbffffff]
pci_root PNP0A03:00: host bridge window [mem 0xff000000-0xffffffff]
pci_root PNP0A03:00: host bridge window [mem 0xfec00000-0xfec0ffff]

Linux *should* probably do the same (though at a different actual
address because we assign bottom-up instead of top-down as Windows
does).  I don't know off the top of my head whether we actually do in
this case or not.

What's the output of "dmesg | grep 0000:00:1f.1; lspci -vs00:1f.1"?
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephan Schreiber Oct. 2, 2012, 7:48 p.m. UTC | #3 
> Thanks a lot for testing this!  I'll fix up this typo and work on
> getting something like this merged.

Thank you very much!
If you want me to test anything, don't hesitate to contact me.


> What's the output of "dmesg | grep 0000:00:1f.1; lspci -vs00:1f.1"?

[    0.065506] pci 0000:00:1f.1: [8086:24cb] type 0 class 0x000101
[    0.065520] pci 0000:00:1f.1: reg 10: [io  0x0000-0x0007]
[    0.065531] pci 0000:00:1f.1: reg 14: [io  0x0000-0x0003]
[    0.065542] pci 0000:00:1f.1: reg 18: [io  0x0000-0x0007]
[    0.065553] pci 0000:00:1f.1: reg 1c: [io  0x0000-0x0003]
[    0.065564] pci 0000:00:1f.1: reg 20: [io  0x1000-0x100f]
[    0.065575] pci 0000:00:1f.1: reg 24: [mem 0x00000000-0x000003ff unset]
[    1.415250] ata_piix 0000:00:1f.1: version 2.13
[    1.415276] ata_piix 0000:00:1f.1: can't derive routing for PCI INT A
00:1f.1 IDE interface: Intel Corporation 82801DB (ICH4) IDE Controller  
(rev 02) (prog-if 8a [Master SecP PriP])
	Subsystem: Intel Corporation Device 3404
	Flags: bus master, medium devsel, latency 0
	I/O ports at 01f0 [size=8]
	I/O ports at 03f4 [size=1]
	I/O ports at 0170 [size=8]
	I/O ports at 0374 [size=1]
	I/O ports at 1000 [size=16]
	Kernel driver in use: ata_piix




Stephan




--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 0e33754..b6ceeee 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -600,7 +600,7 @@  char *resource_string(char *buf, char *end, struct resource *res,
 	 * 64-bit res (sizeof==8): 20 chars in dec, 18 in hex ("0x" + 16) */
 #define RSRC_BUF_SIZE		((2 * sizeof(resource_size_t)) + 4)
 #define FLAG_BUF_SIZE		(2 * sizeof(res->flags))
-#define DECODED_BUF_SIZE	sizeof("[mem - 64bit pref window disabled]")
+#define DECODED_BUF_SIZE	sizeof("[mem - 64bit pref window unset disabled]")
 #define RAW_BUF_SIZE		sizeof("[mem - flags 0x]")
 	char sym[max(2*RSRC_BUF_SIZE + DECODED_BUF_SIZE,
 		     2*RSRC_BUF_SIZE + FLAG_BUF_SIZE + RAW_BUF_SIZE)];
@@ -642,6 +642,8 @@  char *resource_string(char *buf, char *end, struct resource *res,
 			p = string(p, pend, " pref", str_spec);
 		if (res->flags & IORESOURCE_WINDOW)
 			p = string(p, pend, " window", str_spec);
+		if (res->flags & IORESOURCE_UNSET)
+			p = string(p, pend, " unset", str_spec);
 		if (res->flags & IORESOURCE_DISABLED)
 			p = string(p, pend, " disabled", str_spec);
 	} else {

commit f4795a79dc370b6f4106768b16a4a9edba4df933
Author: Bjorn Helgaas <bhelgaas@google.com>
Date:   Mon Sep 24 17:15:30 2012 -0600

    PCI: Ignore BAR contents when firmware left decoding disabled

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 2396111..6926dcb 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -175,9 +175,10 @@  int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 
 	mask = type ? PCI_ROM_ADDRESS_MASK : ~0;
 
+	pci_read_config_word(dev, PCI_COMMAND, &orig_cmd);
+
 	/* No printks while decoding is disabled! */
 	if (!dev->mmio_always_on) {
-		pci_read_config_word(dev, PCI_COMMAND, &orig_cmd);
 		pci_write_config_word(dev, PCI_COMMAND,
 			orig_cmd & ~(PCI_COMMAND_MEMORY | PCI_COMMAND_IO));
 	}
@@ -211,9 +212,13 @@  int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		if (res->flags & IORESOURCE_IO) {
 			l &= PCI_BASE_ADDRESS_IO_MASK;
 			mask = PCI_BASE_ADDRESS_IO_MASK & (u32) IO_SPACE_LIMIT;
+			if (!(orig_cmd & PCI_COMMAND_IO))
+				res->flags |= IORESOURCE_UNSET;
 		} else {
 			l &= PCI_BASE_ADDRESS_MEM_MASK;
 			mask = (u32)PCI_BASE_ADDRESS_MEM_MASK;
+			if (!(orig_cmd & PCI_COMMAND_MEM))
+				res->flags |= IORESOURCE_UNSET;
 		}
 	} else {
 		res->flags |= (l & IORESOURCE_ROM_ENABLE);
@@ -248,6 +253,7 @@  int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 			/* Address above 32-bit boundary; disable the BAR */
 			pci_write_config_dword(dev, pos, 0);
 			pci_write_config_dword(dev, pos + 4, 0);
+			res->flags |= IORESOURCE_UNSET;
 			region.start = 0;
 			region.end = sz64;
 			pcibios_bus_to_resource(dev, res, &region);

commit f90b82ab54efa462681d5dd99fd926f2563f7a93
Author: Bjorn Helgaas <bhelgaas@google.com>
Date:   Mon Sep 24 17:28:21 2012 -0600

    PCI: Don't claim BARs that haven't been assigned

diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 81b88bd..9f9e31a 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -113,6 +113,9 @@  int pci_claim_resource(struct pci_dev *dev, int resource)
 	struct resource *res = &dev->resource[resource];
 	struct resource *root, *conflict;
 
+	if (res->flags & IORESOURCE_UNSET)
+		return 0;
+
 	root = pci_find_parent_resource(dev, res);
 	if (!root) {
 		dev_info(&dev->dev, "no compatible bridge window for %pR\n",
@@ -334,6 +337,8 @@  int pci_enable_resources(struct pci_dev *dev, int mask)
 
 		if (!(r->flags & (IORESOURCE_IO | IORESOURCE_MEM)))
 			continue;
+		if (r->flags & IORESOURCE_UNSET)
+			continue;
 		if ((i == PCI_ROM_RESOURCE) &&
 				(!(r->flags & IORESOURCE_ROM_ENABLE)))
 			continue;