Bug in vr(4) driver

[Pyun YongHyeon]

On Mon, Aug 27, 2007 at 08:18:08PM +0000, Bill Paul wrote:
 > 
 > I recently started writing a driver for the Via Rhine family of chips
 > for VxWorks (they turn up on various x86-based single board systems,
 > and I figured it'd be nice to actually support them out of the box),
 > and along the way, I noticed a subtle bug in the FreeBSD vr(4) driver.
 > 
 > The vr_attach() routine unconditionally does this for all supported
 > chips:
 > 
 >         /*
 >          * Windows may put the chip in suspend mode when it
 >          * shuts down. Be sure to kick it in the head to wake it
 >          * up again.
 >          */
 >         VR_CLRBIT(sc, VR_STICKHW, (VR_STICKHW_DS0|VR_STICKHW_DS1));
 > 
 > The problem is, the VR_STICKHW register is not valid on all Rhine
 > devices. The VT86C100A chip, which is present on the D-Link DFE-530TX
 > boards, doesn't support power management, and its register space is
 > only 128 bytes wide. The VR_STICKHW register offset falls outside this
 > range. This may go unnoticed in most scenarios, but if you happen to have
 > another PCI device in your system which is assigned the register
 > space immediately after that of the Rhine, the vr(4) driver will
 > incorrectly stomp it. In my case, the BIOS on my test board decided
 > to put the register space for my PRO/100 ethernet board right next
 > to the Rhine, and the Rhine driver ended up clobbering the IMR register
 > of the PRO/100 device. (Long story short: the board kept locking up on
 > boot. Took me the better part of the morning suss out why.)
 > 
 > The strictly correct thing to do would be to check the PCI config space
 > to make sure the device supports the power management capability and only
 > write to the VR_STICKHW register if it does. A less strictly correct
 > but equally effective thing to do would be:
 > 
 >         /*
 >          * Windows may put the chips that support power management into
 >          * suspend mode when it shuts down. Be sure to kick it in the
 >          * head to wake it up again.
 >          */
 > 	if (pci_get_device(dev) != VIA_DEVICEID_RHINE)
 >             VR_CLRBIT(sc, VR_STICKHW, (VR_STICKHW_DS0|VR_STICKHW_DS1));
 > 
 > This is basically the fix I put into my VxWorks driver. I suggest someone
 > update the FreeBSD driver as well.
 > 

Hi,

I don't have vr(4) hardwares(if I had I would have converted vr(4)
to use bus_dma(9)). Would you review/test the attached patch?

-- 
Regards,
Pyun YongHyeon
-------------- next part --------------
Index: if_vr.c
===================================================================
RCS file: /home/ncvs/src/sys/pci/if_vr.c,v
retrieving revision 1.126
diff -u -r1.126 if_vr.c
--- if_vr.c	23 Apr 2007 12:19:02 -0000	1.126
+++ if_vr.c	28 Aug 2007 01:00:34 -0000
@@ -90,6 +90,7 @@
 
 #include <dev/mii/miivar.h>
 
+#include <dev/pci/pcireg.h>
 #include <dev/pci/pcivar.h>
 
 #define VR_USEIOSPACE
@@ -513,6 +514,7 @@
 	struct ifnet		*ifp;
 	int			error = 0, rid;
 	struct vr_type		*t;
+	int			pmc;
 
 	sc = device_get_softc(dev);
 	sc->vr_dev = dev;
@@ -591,7 +593,8 @@
 	 * shuts down. Be sure to kick it in the head to wake it
 	 * up again.
 	 */
-	VR_CLRBIT(sc, VR_STICKHW, (VR_STICKHW_DS0|VR_STICKHW_DS1));
+	if (pci_find_extcap(dev, PCIY_PMG, &pmc) == 0)
+		VR_CLRBIT(sc, VR_STICKHW, (VR_STICKHW_DS0|VR_STICKHW_DS1));
 
 	/* Reset the adapter. */
 	vr_reset(sc);