Date: Tue, 28 Aug 2007 10:03:10 +0900 From: Pyun YongHyeon <pyunyh@gmail.com> To: Bill Paul <wpaul@FreeBSD.ORG> Cc: freebsd-net@FreeBSD.ORG, freebsd-current@FreeBSD.ORG Subject: Re: Bug in vr(4) driver Message-ID: <20070828010310.GA85263@cdnetworks.co.kr> In-Reply-To: <20070827201809.0367616A418@hub.freebsd.org> References: <20070827201809.0367616A418@hub.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--9jxsPFA5p3P2qPhR Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 --9jxsPFA5p3P2qPhR Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="vr.diff" 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); --9jxsPFA5p3P2qPhR--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070828010310.GA85263>