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

[M. Warner Losh]

In message: <Pine.BSF.4.21.0304291101001.75697-100000 at root.org>
            Nate Lawson <nate at root.org> 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