Re[2]: pci_fixup_video change blows up on sparc64

[<eiichiro.oiwa.nm@hitachi.com>]

From: David Miller <davem@davemloft.net>
>From: <eiichiro.oiwa.nm@hitachi.com>
>Date: Fri, 20 Oct 2006 11:57:06 +0900
>
>> I am sorry. I assumed ROM base address will be 0xc0000 if VGA Enable
>> bit in Bridge Control Register set. This assuming is incorrect.
>
>Please also notice another point we are trying to explain in this
>thread, in fact several times.
>
>This "bridge control register" is only valid for "PCI to PCI" bridges.
>
>I repeat:
>
>	This "bridge control register" is only valid for "PCI to PCI"
>	bridges.
>
>It is not valid for "host to PCI" bridges, yet the pci_fixup_video()
>code is testing the bridge control register, blindly, in every bus
>device it sees as it walks up the device tree to the root.
>
>The bridge control register is valid for PCI header type BRIDGE, or
>CARDBUS.  Host to PCI controllers use PCI header type NORMAL.  For
>example, here is the lspci dump for my host bridge:
>
>0000:00:00.0 Host bridge: Sun Microsystems Computer Corp. Tomatillo PCI Bus Module
>00: 8e 10 01 a8 46 01 a0 22 00 00 00 06 00 40 00 00
>
>and here is a dump for a PCI->PCI bridge in the same machine:
>
>0000:00:02.0 PCI bridge: Texas Instruments PCI2250 PCI-to-PCI Bridge (rev 02)
>00: 4c 10 23 ac 07 00 10 02 02 00 04 06 10 40 01 00
>
>Clearly, the host bridge has PCI header type 0x00 (NORMAL) at offset
>0x0e, and the PCI-PCI bridge has header type 0x01 (BRIDGE).
>
>Back to the code, look at the loop:
>
>	bus = pdev->bus;
>	while (bus) {
>		bridge = bus->self;
>		if (bridge) {
>			pci_read_config_word(bridge, PCI_BRIDGE_CONTROL,
>						&config);
>			if (!(config & PCI_BRIDGE_CTL_VGA))
>				return;
>		}
>		bus = bus->parent;
>	}
>
>Where is it making sure that this is a PCI to PCI bus and not a
>host to PCI bus?  It's not, and that's a serious bug.  There should
>be a PCI header type check here, or similar.  The PCI device probing
>layer correctly checks the PCI header type before trying to access
>the bridge control register of any PCI device.
>
>And also, it is also true that the ioremap() calls done by
>pci_map_rom() for the "0xc0000" case are totally invalid.  For several
>reasons:
>
>1) That is going to be RAM, not I/O memory space, therefore accessing
>   it with ioremap() and asm/io.h accessors such as readl() and
>   memcpy_fromio() will result in bus errors and other faults.
>
>2) It is illegal to pass raw physical addresses to ioremap() as the
>   first argument.  The first argument to ioremap() is an architecture
>   defined opaque "address cookie" which by definition must be setup
>   by platform specific code.
>
>Just copying the x86 code over to IA64 to 'fix the problem' doesn't
>fix any of these bugs (illegal access to bridge control register on
>devices with PCI header type NORMAL) or portability problems (invalid
>first argument to ioremap()).
>
I am sorry again. I understood we need to fix pci_fixup_video and ...

Thank you for your good comments.

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