Regression in 8.2-STABLE bge code (from 7.4-STABLE)

[John Baldwin]

On Saturday, February 25, 2012 5:13:01 pm YongHyeon PYUN wrote:
> On Thu, Feb 23, 2012 at 09:46:20AM -0500, John Baldwin wrote:
> > On Tuesday, February 14, 2012 7:56:00 pm YongHyeon PYUN wrote:
> > > On Sat, Jan 28, 2012 at 09:24:53PM -0500, Michael L. Squires wrote:
> > > 
> > > Sorry for late reply.  Had been busy due to relocation.
> > > 
> > > > There is a bug in the Tyan S4881/S4882 PCI-X bridges that was fixed with a 
> > > > patch in 7.x (thank you very much).  This patch is not present in the 
> > > > 8.2-STABLE code and the symptoms (watchdog timeouts) have recurred.
> > > > 
> > > 
> > > Hmm, I thought the mailbox reordering bug was avoided by limiting
> > > DMA address space to 32bits but it seems it was not right workaround
> > > for AMD 8131 PCI-X Bridge.
> > > 
> > > > The watchdog timeouts do not appear to be present after I switched to an 
> > > > Intel gigabit PCI-X card.
> > > > 
> > > > I did a brute-force patch of the 8.2-STABLE bge code using the patches for
> > > > 7.4-STABLE; the resulting code compiled and, other than odd behavior at
> > > > startup, seems to be working normally.
> > > > 
> > > > This is using FreeBSD 8.2-STABLE amd64; I don't know what happens with 
> > > > i386.
> > > > 
> > > > Given the age of the boards it may be easier if I just continue using the
> > > > Intel gigabit card but am happy to test anything that comes my way.
> > > > 
> > > 
> > > Try attached patch and let me know how it goes.
> > > I didn't enable 64bit DMA addressing though. I think the AMD-8131
> > > PCI-X bridge needs both workarounds.
> > 
> > Eh, please don't do the thing where you walk all pcib devices.  Instead, walk 
> > up the tree like so:
> > 
> > static int
> > bge_mbox_reorder(struct bge_softc *sc)
> > {
> > 	devclass_t pcib, pci;
> > 	device_t dev, bus;
> > 
> > 	pci = devclass_find("pci");
> > 	pcib = devclass_find("pcib");
> > 	dev = sc->dev;
> > 	bus = device_get_parent(dev);
> > 	for (;;) {
> > 		dev = device_get_parent(bus);
> > 		bus = device_get_parent(dev);
> > 		if (device_get_devclass(dev) != pcib_devclass ||
> > 		    device_get_devclass(bus) != pci_devclass)
> > 			break;
> > 		/* Probe device ID. */
> > 	}
> > 	return (0);
> > }
> > 
> > It is not safe to use pci_get_vendor() with non-PCI devices (you may get
> > random junk, and Host-PCI bridges are not PCI devices).  Also, this will only
> > apply the quirk if a relevant bridge is in the bge device's path.
> > 
> 
> Thanks for reviewing and suggestion.
> Would you review updated one?

Looks good, thanks!  I wish there was some way to mark this quirk in the
PCI-PCI bridge driver so bge didn't have to walk up its tree, but I can't
really think of a better way.

-- 
John Baldwin