From owner-freebsd-current Thu Mar 20 21:24:11 2003 Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 16D0A37B401; Thu, 20 Mar 2003 21:24:10 -0800 (PST) Received: from harmony.village.org (rover.bsdimp.com [204.144.255.66]) by mx1.FreeBSD.org (Postfix) with ESMTP id E527C43FAF; Thu, 20 Mar 2003 21:24:08 -0800 (PST) (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 h2L5O7A7045550; Thu, 20 Mar 2003 22:24:07 -0700 (MST) (envelope-from imp@bsdimp.com) Date: Thu, 20 Mar 2003 22:23:52 -0700 (MST) Message-Id: <20030320.222352.122290134.imp@bsdimp.com> To: nate@root.org Cc: current@FreeBSD.ORG, wpaul@FreeBSD.ORG Subject: Re: Updated if_* attach/detach patches From: "M. Warner Losh" In-Reply-To: References: 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 Sender: owner-freebsd-current@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG In message: Nate Lawson writes: : - dc: move interrupt allocation back where it was before. It was unnecessary : to move it Why's that? If dc is on a shared interrupt line, then dc_intr is going to be called, potentially, before the rest of the attach routine finishes. Keep in mind that attach routines run with interrupts enabled in many interesting cases (including cardbus). You can't call bus_setup_intr until the very end of attach. Since there's no locking here, bad things would happen if an interrupt fired, no? It looks like dc might be safe (since it returns right away if DC_ISR is zero for the bits it knows about)... : - pcn: add missing bzero of softc softc is automatically bzero'd. : - rl: move irq allocation before ether_ifattach. Problems could have been : caused by allocating the irq after enabling interrupts on the card. Same problem as the dc driver. ether_ifattach isn't going to start the interface, so you are safe waiting until after ether_ifattach to do this. And for the rl driver there's no check to see if the RL_ISR has bits set before we acquire the lock... Even with that, it might be safe, but I'm less sure. Why are you checking device_is_alive() in detach? detach won't get called if that isn't the case. Oh, I see, you've added calls... In general, in the drivers I've written I have a foo_alloc() and foo_dealloc() to get and release the resources rather than overloading detach to do this. Well, foo_alloc isn't always possible in the newer world order where locking matters. foo_dalloc can safely be written, however. You want to device_delete_child before calling bus_generic_detatch(). but the old driver does it backwards, so that's not a huge deal. For dc and rl you might want to call bus_child_present(dev) in the detach routines and *NOT* call foo_stop() if the card isn't present. wi actually sets gone in detach: /* check if device was removed */ sc->wi_gone = !bus_child_present(dev); which might not be a horrible idea. Warner To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-current" in the body of the message