cvs commit: src/sys/dev/fxp if_fxp.c if_fxpvar.h

[John Baldwin]

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 at FreeBSD.org>  <><  http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve!"  -  http://www.FreeBSD.org/