From owner-cvs-all@FreeBSD.ORG Tue Apr 29 11:28:18 2003 Return-Path: Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id D4DED37B401; Tue, 29 Apr 2003 11:28:17 -0700 (PDT) Received: from harmony.village.org (rover.bsdimp.com [204.144.255.66]) by mx1.FreeBSD.org (Postfix) with ESMTP id DA4B143F93; Tue, 29 Apr 2003 11:28:16 -0700 (PDT) (envelope-from imp@bsdimp.com) Received: from localhost (warner@rover2.village.org [10.0.0.1]) by harmony.village.org (8.12.8/8.12.3) with ESMTP id h3TISEA7090991; Tue, 29 Apr 2003 12:28:15 -0600 (MDT) (envelope-from imp@bsdimp.com) Date: Tue, 29 Apr 2003 12:28:08 -0600 (MDT) Message-Id: <20030429.122808.116092806.imp@bsdimp.com> To: nate@root.org From: "M. Warner Losh" In-Reply-To: References: <20030429054515.D74EF37B490@hub.freebsd.org> X-Mailer: Mew version 2.1 on Emacs 21.2 / Mule 5.0 (SAKAKI) Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit cc: cvs-src@FreeBSD.org cc: src-committers@FreeBSD.org cc: cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/fxp if_fxp.c if_fxpvar.h X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 29 Apr 2003 18:28:18 -0000 In message: Nate Lawson writes: : On Mon, 28 Apr 2003, Warner Losh wrote: : > Modified files: : > sys/dev/fxp if_fxp.c if_fxpvar.h : > Log: : > Fix 5 bugs: : > 1) always call fxp_stop in fxp_detach. Since we don't read from : > the card, there's no need to carefully look at things with : > bus_child_present. : : However, we do write to the card registers (i.e. to disable interrupt : generation). Since you were the one who suggested I should add these : calls, can you give more information about when bus_child_present should : be used (and update the man page if anything changed)? Writing to hardware that has recently departed is OK since the write will succeed and have no ill effects. Reading from hardware that isn't there is bogus because the bits returned are not meaningful (but typically 0xff). : > 2) Call FXP_UNLOCK() before calling bus_teardown_intr to avoid : > a possible deadlock reported by jhb. : : This adds a race since fxp_intr could occur after the unlock but before : the bus_teardown_intr call. The reason why I tore down the intr while : holding the lock is so fxp_intr would be prevented from accessing the : device until it has been disabled. Then the normal checks in fxp_intr : (IFF_OACTIVE or whatever) would show the card is gone and return without : accessing it. I guess this is ok since ether_ifdetach is still called : with the lock held (since it is what clears IFF_OACTIVE) but I'm : interested in your thoughts. That race is taken care with the dead flag. You can't hold the lock while calling bus_teardown_intr without introducing deadlocks. Also, looking at fxp_intr shows this to not be the case. You check for a number of things: + if (sc->gone) I added these. + return; FXP_LOCK(sc); #ifdef DEVICE_POLLING if (ifp->if_flags & IFF_POLLING) { FXP_UNLOCK(sc); return; } if (ether_poll_register(fxp_poll, ifp)) { /* disable interrupts */ CSR_WRITE_1(sc, FXP_CSR_SCB_INTRCNTL, FXP_SCB_INTR_DISABLE); fxp_poll(ifp, 0, 1); FXP_UNLOCK(sc); return; } #endif if (sc->suspended) { FXP_UNLOCK(sc); return; } Notice that we don't check to see if we're up or anything before we start to read the hardware. While we have a special check for 0xff here, which likely is a prior attempt to fix this problem. while ((statack = CSR_READ_1(sc, FXP_CSR_SCB_STATACK)) != 0) { /* ... check for 0xff here : > 3) add gone to the softc. Set it to true in detach. : > 4) Return immediately if gone is true in fxp_ioctl : > 5) Return immediately if gone is true in fxp_intr : : Not sure this approach is necessary. I am. Otherwise ioctl panics with recursive locks when the card is detached. A simple kld_unload if_fxp would provoke these races, including the recursive lock panic. Like I've said elsewhere, I'm cool with better solutions, but I have to be able to eject my fxp card without it panicing my system for those solutions to truly be "better." Cardbus, in its ISR, turns off delivery of the interrupt when it notices that the 'status' of the card has changed. However, the races that are there are still present with normal pci cards and the kld_unload case (and the future detach command I'm working on). This is a race in device/bus layer's interaction with the interrupt code. Warner