2.4.2 yenta_socket problems on ThinkPad 240

On 6-Jun-2001, I reported:

Arjan van de Ven of Red Hat tracked my problem down to a broken BIOS, which is not configuring the TI PC1211 CardBus bridge correctly. Even IBM's latest BIOS for the ThinkPad 240, IRET75WW released 17-May-2001, has this problem. Apparently IBM has issued fixes for other ThinkPads because the problem occurs with Windows 2000, but since Windows 2000 is not supported on the ThinkPad 240, it is unlikely that they will fix it.

A one line change to linux/include/asm-i386/pci.h fixes it:

-#define pcibios_assign_all_busses() 0
+#define pcibios_assign_all_busses() 1

Given that this macro exists, I surmise that other people have been bitten by similar problems. So now my question is:

Does it make sense to turn pcibios_assign_all_busses into a variable with a default value of zero, and implement a kernel argument to set it? That way people with broken BIOSes could solve it by simply adding the argument in their lilo.conf. In an ideal world the BIOSes would get fixed and such a workaround would be unnecessary, but in my experience trying to get a vendor to fix a BIOS problem is an exercise in futility.

If no one is opposed to my proposed change, I'll be happy to generate a suitable patch and send it in.

Thanks!
Eric Smith

I believe Alan had mentioned something on IRC about seeing a case where the CardBus bridge's secondary and subordinate bridge numbers were 1 on bootup, but 0 after the yenta driver got ahold of it. So, there is the potential that the yenta driver is not setting things up quite correctly.

To answer your question, I wouldn't mind at all having a kernel command line setting that turned the above into a variable...

I would love to just define it unconditionally for x86, but I believe Martin said that causes problems with some hardware, and the way the BIOS has set up that hardware. (details anyone?)

Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |

Im not sure unconditionally is wise. However turning it into a routine that walks the PCI bus tree and returns 1 if a duplicate is found seems to be a little bit less likely to cause suprises

That would work, but is really a bandaid because we don't know what the real problem is... this still smells vaguely like yenta and pci bus core should be more than just the kissing cousins they are now. OTOH I still don't like how much we trust firmware PCI bus setup on x86..

I am pretty lucky on Alpha, we already trust the kernel PCI code implicitly by unconditionally defining pcibios_assign_all_busses to one. :)

Jeff

Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |

The BIOS may make assumptions we dont know about such as the bus layout. What minimises the problem is effectively to validate the firmware provided PCI setup and if its crap, then do the job ourselves. That minimizes the problems

Hence I think it should not be a define but an __init validator for the bus setup

Yes.

Regardless, it would certainly make sense to have a manual override, with a kernel command line. If for no other reason than to allow for mistakes and let the user force the old/new behaviour.

So the #define should be a variable with a kernel command line override, along with a heuristic for the kernel to do a good guess on its own (and the heuristic should probably not be as global as the current "pcibios_assign_all_busses()" test - the heuristic will be able to tell on a bridge basis on whether that bridge may need assignment. This might imply giving the "pcibios_assign_all_busses()" thing the "dev" as an argument).

Linus

After some discussion of various alternatives, including always turning it on (bad for some systems), or writing a function to try to determine if the configuration left by the BIOS is sane (maybe OK, but could still need

Since I offered to generate a suitable patch, below find one against 2.4.5. This patch ONLY tries to provide for the command line override. I had to touch more files than I would have liked, and unfortunately can only test it on x86 even though it potentially affects other architectures. It seems to do the right thing on my IBM ThinkPad 240.

What I've done is:

1) Replaced the pcibios_assign_all_busses() macro in each architecture with a defined constant PCI_ASSIGN_ALL_BUSSES_DEFAULT. Note that some architectures were using a function instead of a macro, but the function just returned the contents of a variable.

2) Added the variable pci_assign_all_busses, which gets initialized from the defined constant PCI_ASSIGN_ALL_BUSSES_DEFAULT.

3) Added a __setup macro and function to handle a command line argument "pci_assign_all_busses=1" (or zero), which sets the pci_assign_all_busses variable.

4) Changed the code in drivers/pci/pci.c to use the variable rather than the old macro/function.

If code is added to attempt to automatically determine whether bus configuration is needed, and if that code runs after the command line is parsed, it might be necessary to add a second variable (or special values of the existing one) to avoid changing the variable if the user has specified a value on the command line, since the whole point is to allow the user to override the default behavior.

If I've done this in a suboptimal manner, I'd be happy to hear suggestions of better approaches. As submitting kernel patches goes, this is my first time, so please be gentle. :-)

Thanks,
Eric

diff -uNr linux-2.4.5.orig/arch/parisc/kernel/inventory.c linux-2.4.5/arch/parisc/kernel/inventory.c
@@ -138,6 +138,7 @@
 	ulong pcell_loc;
 
 	pdc_pat = (pdc_pat_cell_get_number(&pdc_result) == PDC_OK);
+	pci_assign_all_busses = pdc_pat;
 	if (!pdc_pat)
 	{
 		return 0;

diff -uNr linux-2.4.5.orig/arch/ppc/kernel/pci.c linux-2.4.5/arch/ppc/kernel/pci.c
@@ -47,10 +47,6 @@
 static void pcibios_fixup_cardbus(struct pci_dev* dev);
 #endif
 
-/* By default, we don't re-assign bus numbers. We do this only on
- * some pmacs
- */
-int pci_assign_all_busses;
 
 struct pci_controller* hose_head;
 struct pci_controller** hose_tail = &hose_head;

@@ -752,11 +748,6 @@
 		ppc_md.pcibios_after_init();
 }
 
-int __init
-pcibios_assign_all_busses(void)
-{
-	return pci_assign_all_busses;
-}
 
 void __init
 pcibios_fixup_pbus_ranges(struct pci_bus * bus, struct pbus_set_ranges_data * ranges)

diff -uNr linux-2.4.5.orig/drivers/pci/pci.c linux-2.4.5/drivers/pci/pci.c
@@ -37,6 +37,19 @@
 LIST_HEAD(pci_root_buses);
 LIST_HEAD(pci_devices);
 
+
+int pci_assign_all_busses = PCI_ASSIGN_ALL_BUSSES_DEFAULT;
+
+
+static int __init pci_assign_all_busses_setup(char *str)
+{
+	pci_assign_all_busses = simple_strtol(str,NULL,0);
+	return 1;
+}
+
+__setup("pci_assign_all_busses=", pci_assign_all_busses_setup);
+
+
 /**
  * pci_find_slot - locate PCI device from a given PCI slot
  * @bus: number of PCI bus on which desired PCI device resides

@@ -957,7 +970,7 @@
 
 	pci_read_config_dword(dev, PCI_PRIMARY_BUS, &buses);
 	DBG("Scanning behind PCI bridge %s, config %06x, pass %d\n", dev->slot_name, buses & 0xffffff, pass);
-	if ((buses & 0xffff00) && !pcibios_assign_all_busses()) {
+	if ((buses & 0xffff00) && !pci_assign_all_busses) {
 		/*
 		 * Bus already configured by firmware, process it in the first
 		 * pass and just note the configuration.

diff -uNr linux-2.4.5.orig/include/asm-alpha/pci.h linux-2.4.5/include/asm-alpha/pci.h
@@ -46,7 +46,7 @@
 /* Override the logic in pci_scan_bus for skipping already-configured
    bus numbers.  */
 
-#define pcibios_assign_all_busses()	1
+#define PCI_ASSIGN_ALL_BUSSES_DEFAULT 1
 
 #define PCIBIOS_MIN_IO		alpha_mv.min_io_address
 #define PCIBIOS_MIN_MEM		alpha_mv.min_mem_address

diff -uNr linux-2.4.5.orig/include/asm-arm/arch-ebsa285/hardware.h linux-2.4.5/include/asm-arm/arch-ebsa285/hardware.h
@@ -131,7 +131,7 @@
 extern void cpld_modify(int mask, int set);
 #endif
 
-#define pcibios_assign_all_busses()	1
+#define PCI_ASSIGN_ALL_BUSSES_DEFAULT 1
 
 #define PCIBIOS_MIN_IO		0x6000
 #define PCIBIOS_MIN_MEM 	0x40000000

diff -uNr linux-2.4.5.orig/include/asm-i386/pci.h linux-2.4.5/include/asm-i386/pci.h
@@ -7,7 +7,7 @@
    already-configured bus numbers - to be used for buggy BIOSes
    or architectures with incomplete PCI setup by the loader */
 
-#define pcibios_assign_all_busses()	0
+#define PCI_ASSIGN_ALL_BUSSES_DEFAULT 0
 
 extern unsigned long pci_mem_start;
 #define PCIBIOS_MIN_IO		0x1000

diff -uNr linux-2.4.5.orig/include/asm-ia64/pci.h linux-2.4.5/include/asm-ia64/pci.h
@@ -14,7 +14,7 @@
  * already-configured bus numbers - to be used for buggy BIOSes or
  * architectures with incomplete PCI setup by the loader.
  */
-#define pcibios_assign_all_busses()     0
+#define PCI_ASSIGN_ALL_BUSSES_DEFAULT 0
 
 #define PCIBIOS_MIN_IO		0x1000
 #define PCIBIOS_MIN_MEM		0x10000000

diff -uNr linux-2.4.5.orig/include/asm-m68k/pci.h linux-2.4.5/include/asm-m68k/pci.h
@@ -33,7 +33,7 @@
 	void (*conf_device)(unsigned char bus, unsigned char device_fn);
 };
 
-#define pcibios_assign_all_busses()	0
+#define PCI_ASSIGN_ALL_BUSSES_DEFAULT 0
 
 extern inline void pcibios_set_master(struct pci_dev *dev)
 {

diff -uNr linux-2.4.5.orig/include/asm-mips/pci.h linux-2.4.5/include/asm-mips/pci.h
@@ -13,7 +13,7 @@
    already-configured bus numbers - to be used for buggy BIOSes
    or architectures with incomplete PCI setup by the loader */
 
-#define pcibios_assign_all_busses()	0
+#define PCI_ASSIGN_ALL_BUSSES_DEFAULT 0
 
 #define PCIBIOS_MIN_IO		0x1000
 #define PCIBIOS_MIN_MEM		0x10000000

diff -uNr linux-2.4.5.orig/include/asm-mips64/pci.h linux-2.4.5/include/asm-mips64/pci.h
@@ -13,7 +13,7 @@
    already-configured bus numbers - to be used for buggy BIOSes
    or architectures with incomplete PCI setup by the loader */
 
-#define pcibios_assign_all_busses()	0
+#define PCI_ASSIGN_ALL_BUSSES_DEFAULT 0
 
 #define PCIBIOS_MIN_IO		0x1000
 #define PCIBIOS_MIN_MEM		0x10000000

diff -uNr linux-2.4.5.orig/include/asm-parisc/pci.h linux-2.4.5/include/asm-parisc/pci.h
@@ -204,9 +204,8 @@
 */
 #ifdef __LP64__
 extern int pdc_pat;  /* arch/parisc/kernel/inventory.c */
-#define pcibios_assign_all_busses()	pdc_pat
-#else
-#define pcibios_assign_all_busses()	0
+extern int pci_assign_all_busses;  /* drivers/pci/pci.c */
+#define PCI_ASSIGN_ALL_BUSSES_DEFAULT 0
 #endif
 
 #define PCIBIOS_MIN_IO          0x10

diff -uNr linux-2.4.5.orig/include/asm-ppc/pci.h linux-2.4.5/include/asm-ppc/pci.h
@@ -13,7 +13,11 @@
 #define IOBASE_ISA_MEM		4
 
 
-extern int pcibios_assign_all_busses(void);
+/* By default, we don't re-assign bus numbers. We do this only on
+ * some pmacs
+ */
+#define PCI_ASSIGN_ALL_BUSSES_DEFAULT 0
+extern int pci_assign_all_busses;  /* drivers/pci/pci.c */
 
 #define PCIBIOS_MIN_IO		0x1000
 #define PCIBIOS_MIN_MEM		0x10000000

diff -uNr linux-2.4.5.orig/include/asm-sh/pci.h linux-2.4.5/include/asm-sh/pci.h
@@ -7,7 +7,7 @@
    already-configured bus numbers - to be used for buggy BIOSes
    or architectures with incomplete PCI setup by the loader */
 
-#define pcibios_assign_all_busses()	1
+#define PCI_ASSIGN_ALL_BUSSES_DEFAULT 1
 
 /* These are currently the correct values for the STM overdrive board. 
  * We need some way of setting this on a board specific way, it will 

diff -uNr linux-2.4.5.orig/include/asm-sparc/pci.h linux-2.4.5/include/asm-sparc/pci.h
@@ -7,7 +7,7 @@
  * already-configured bus numbers - to be used for buggy BIOSes
  * or architectures with incomplete PCI setup by the loader.
  */
-#define pcibios_assign_all_busses()	0
+#define PCI_ASSIGN_ALL_BUSSES_DEFAULT 0
 
 #define PCIBIOS_MIN_IO		0UL
 #define PCIBIOS_MIN_MEM		0UL

diff -uNr linux-2.4.5.orig/include/asm-sparc64/pci.h linux-2.4.5/include/asm-sparc64/pci.h
@@ -10,7 +10,7 @@
  * already-configured bus numbers - to be used for buggy BIOSes
  * or architectures with incomplete PCI setup by the loader.
  */
-#define pcibios_assign_all_busses()	0
+#define PCI_ASSIGN_ALL_BUSSES_DEFAULT 0
 
 #define PCIBIOS_MIN_IO		0UL
 #define PCIBIOS_MIN_MEM		0UL

You should try 2.4.6-pre5, it already includes a patch for you :)

pci=assign-busses on the command line.

Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |

Well, the _real_ advantage on the alpha side is that there are only a handful of systems, and those systems tend to be designed by an even smaller number of companies.

That, in turn, makes it so easy to trust the kernel to have enough knowledge.

Linus

That could hurt.

I have a device that can serve as a bridge, and also as a network card. Sometimes the bridge feature is not used, so the bus numbers get set to 0xfd, 0xfe, 0xff or just all 0xff. Multiple cards get the same values just to keep them out of the way.

(this is a PCI-over-network thing)

Depending on EEPROM config data, it might not have a bridge class code at boot. It has to be turned into a bridge via config space writes though, so that the network feature will work. This means I need to allocate some bus numbers after boot, perhaps renumbering other bridges to make room. (BTW if there is an API for this that I missed, please let me know)

In case I do want to really use the bridge feature, there is a little bug to deal with. The primary bus number must be set equal to the secondary bus number. No problem I hope?

Right now Linux seems happy, with lspci complaining a bit.

If the generic code were to "fix" my bus number assignment, all Hell would break loose.

Cardbus shouldn't be touching the bus stuff at all, BUT there may be strange hardware that doesn't like the following:

        config_writeb(socket, PCI_SEC_LATENCY_TIMER, 176); 
	config_writeb(socket, PCI_PRIMARY_BUS, dev->bus->number);
        config_writeb(socket, PCI_SECONDARY_BUS, dev->subordinate->number);
        config_writeb(socket, PCI_SUBORDINATE_BUS, dev->subordinate->number);

I have heard rumors of PCI devices that want all these set with a single double-word write.

That's how drivers/pci/pci.c does it, and it kind of makes sense (setting all bus details in one atomic go, so that there can be no question of how the config space accesses get forwarded or what the bus routing is).

So it is probable that yenta _should_ do

	unsigned int buses =
		(latency << 24) |
		(subordinate << 16) |
		(secondary << 8) |
		primary;
	config_writel(socket, PCI_PRIMARY_BUS, buses);

although I believe that any device that gets this wrong is really broken, as it just re-programs the old values (needed in case of a suspend event).

Agreed. That would make sense regardless.

One of the main problems we used to have was (and will still probably be) bridges that we don't recognize as such or that are invisible for some other reason. The BIOS has set them up correctly, and if we reprogram other stuff, we may have serious problems.

In particular, a lot of PCI bridges - notably the host bridges - have "extra" bridging information in the extended part of the PCI config space. Just see the problems we had with serverworks and compaq host bridges and the "last bus" information.

Generic code generally cannot get that right, and without full knowledge of all bridges (which we'll never have even in theory - just think of trying to boot a kernel on a chipset that is newer than the kernel) we're much better off using the BIOS setup at least as a very strong HINT.

That said, we already have heuristics for stuff that we notice is broken and we fix it up in that case. We could just try to add a new heuristic, namely "if we notice that two PCI bridges have the same bus number, we'd better allocate a new one somewhere".

Linus

That would be consistent with the behaviour in the bugzilla report - it went from 0,0,0,1 to 176,0,0,0...

Alan

Hmm. It might be equally possible that we just get a bus number wrong somewhere. In fact, looking at it more I think the yenta code is just wrong - it uses the wrong bus numbers as far as I can see.

(And also, the generic PCI code _will_ write some of the bus information with byte writes, so the "use a dword write" thing is not necessarily a major requirement. See how the pci.c code writes the fixed-up subordinate bus, for example).

As far as I can tell, the yenta code should _really_ do something like

	PCI_PROMARY_BUS:	dev->subordinate->primary
	PCI_SECONDARY_BUS:	dev->subordinate->secondary
	PCI_SUBORDINATE_BUS:	dev->subordinate->subordinate
	PCI_SEC_LATENCY_TIMER:	preferably settable, not just hardcoded to 176

instead of playing with "dev->bus->number" (which _should_ be the same as it is now), and "subordinate->number" (which is _not_ the same as "subordinate->subordinate".

So I think the bugzilla report just reflects the fact that we got "PCI_SUBORDINATE_BUS" wrong.

Now, that is pretty much meaningless, as far as I can tell, because the original 0,0,0,1 is _also_ bogus. No way should "secondary" be 0, as far as I can tell - that would cause two buses to have bus # 0.

I suspect that the "fix" for this may be simpler than it initially appears:

- fix yenta.c to write the right values (ie get "subordinate" right - this will really only matter if/once we get PCI:PCI bridges on a cardbus card, and this is probably why nobody really reacted before). See above for the proper values.

- Fix the "do I need to assign this bus" test in drivers/pci/pci.c. Right now it is:

	if ((buses & 0xffff00) && !pcibios_assign_all_busses()) {

and from what I can tell it should really be more along the lines of

	primary = buses & 0xff;
	secondary = (buses >> 8) & 0xff;
	subordinate = (buses >> 16) & 0xff;

	assign = pcibios_assign_all_busses();

	/* Bus 0 should be the host bus */
	if (!secondary || !subordinate)
		assign = 1;

	/* Primary should be the same as the bus the bridge is on */
	if (primary != dev->bus->number)
		assing = 1;

	/* Subordinate should be larger or equal to secondary */
	if (secondary > subordinate)
		assign = 1;

#if 0
	/*
	 * We don't actually have full parent range - most of the time we
	 * don't understand host bridge ranges. Eventually we can do:
	 */

	/* The range [secondary:subordinate] must not contain the parent bus */
	if (secondary <= dev->bus->number && subordinate >= dev->bus->number)
		assign = 1;

	/* The range [secondary:subordinate] should be in the parent range */
	if (secondary < dev->bus->secondary)
		assign = 1;
	if (subordinate > dev->bus->subordinate)
		assign = 1;
#else
	/*
	 * In the meantime, we'll just _assume_ that all PCI bus numbers
	 * should be assigned in increasing order from the parent
	 */
	if (secondary <= dev->bus->number)
		assign = 1;

#endif

But the above is just my quick off-the-cuff "these are better sanity-tests" thing, somebody else with PCI bus understanding should check it out even more.

Jeff? Comments?

Linus

Ah, nice. That produces numbers on my laptop that look a bit better. Patch attached (which conflicts with the previous yenta.c patch).

I left 176 hardcoded for now, pending thinking on the rest of your message...

Jeff Garzik      | Andre the Giant has a posse.
Building 1024    |
MandrakeSoft     |

Index: drivers/pcmcia/yenta.c
===================================================================
RCS file: /cvsroot/gkernel/linux_2_4/drivers/pcmcia/yenta.c,v retrieving revision 1.1.1.25.4.1

diff -u -r1.1.1.25.4.1 yenta.c
@@ -644,9 +644,9 @@
 	config_writeb(socket, PCI_LATENCY_TIMER, 168);
 	config_writel(socket, PCI_PRIMARY_BUS,
 		(176 << 24) |			   /* sec. latency timer */
-		(dev->subordinate->number << 16) | /* subordinate bus */
-		(dev->subordinate->number << 8) |  /* secondary bus */
-		dev->bus->number);		   /* primary bus */
+		(dev->subordinate->subordinate << 16) | /* subordinate bus */
+		(dev->subordinate->secondary << 8) |  /* secondary bus */
+		dev->subordinate->primary);		   /* primary bus */
 
 	/*
 	 * Set up the bridging state: