Date: Tue, 29 Apr 2003 14:44:27 -0400 (EDT) From: John Baldwin <jhb@FreeBSD.org> To: Nate Lawson <nate@root.org> Cc: cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/fxp if_fxp.c if_fxpvar.h Message-ID: <XFMail.20030429144427.jhb@FreeBSD.org> In-Reply-To: <Pine.BSF.4.21.0304291101001.75697-100000@root.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 29-Apr-2003 Nate Lawson wrote:
> 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)?
>
>> 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.
>
>> 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.
It is quite necessary:
/*
* If the interrupt thread is already running, then just mark this
* handler as being dead and let the ithread do the actual removal.
*/
mtx_lock_spin(&sched_lock);
if (!TD_AWAITING_INTR(ithread->it_td)) {
handler->ih_flags |= IH_DEAD;
...
mtx_unlock_spin(&sched_lock);
if ((handler->ih_flags & IH_DEAD) != 0)
msleep(handler, &ithread->it_lock, PUSER, "itrmh", 0);
and in ithread_loop():
if ((ih->ih_flags & IH_DEAD) != 0) {
mtx_lock(&ithd->it_lock);
TAILQ_REMOVE(&ithd->it_handlers, ih,
ih_next);
wakeup(ih);
mtx_unlock(&ithd->it_lock);
goto restart;
}
This is how we ensure that the interrupt handler is never executed
once bus_teardown_intr() returns.
--
John Baldwin <jhb@FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve!" - http://www.FreeBSD.org/
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?XFMail.20030429144427.jhb>