Re: Bug in vr(4) driver

On Monday 27 August 2007 09:03:10 pm Pyun YongHyeon wrote:
> 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?

Pyun,

I'd say to go ahead and commit the patch.

-- 
John Baldwin