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

[Maxime Henrion]

Nate Lawson wrote:
> On Thu, 10 Apr 2003, Maxime Henrion wrote:
> >   Modified files:
> >     sys/dev/fxp          if_fxp.c 
> >   Log:
> >   - Clean up the fxp_release() and fxp_detach() functions.
> 
> There's a version of this in the diff I just posted to current at .

Feel free to commit it.

> >   - Be sure to teardown the interrupt first so that "kldunload if_fxp"
> >     doesn't panic the box.  It's now deadlocking rather than crashing,
> >     which isn't really better, but I'm unsure this is fxp(4)'s fault.
> 
> There's also a version of this in my diff.

Same.

> I have been testing my diff by loading and unloading fxp while doing a
> large transfer and I cannot replicate this.  Are you sure it's not a local
> problem?  I have never had a deadlock or a crash and loading fxp again
> always works.

I have no idea, this is why I was saying it's maybe not fxp(4)'s fault
and it may indeed be a local problem.

> > @@ -878,20 +874,23 @@
> >  
> >  	s = splimp();
> >  
> > -	/*
> > -	 * Stop DMA and drop transmit queue.
> > -	 */
> > -	fxp_stop(sc);
> > -
> > -	/*
> > -	 * Close down routes etc.
> > -	 */
> > -	ether_ifdetach(&sc->arpcom.ac_if);
> > -
> > -	/*
> > -	 * Free all media structures.
> > -	 */
> > -	ifmedia_removeall(&sc->sc_media);
> > +	if (device_is_alive(dev)) {
> > +		/*
> > +		 * Stop DMA and drop transmit queue.
> > +		 */
> > +		if (bus_child_present(dev))
> > +			fxp_stop(sc);
> > +		/*
> > +		 * Close down routes etc.
> > +		 */
> > +		ether_ifdetach(&sc->arpcom.ac_if);
> > +		device_delete_child(dev, sc->miibus);
> > +		bus_generic_detach(dev);
> > +		/*
> > +		 * Free all media structures.
> > +		 */
> > +		ifmedia_removeall(&sc->sc_media);
> > +	}
> >  
> >  	splx(s);
> 
> Um, fxp_detach() should not be called for any case where the device isn't
> alive.  fxp_detach should ONLY be called once attach has succeeded which
> by definition means the device is alive.  bus_child_present() is the
> bus-specific method to see that the hardware is actually there;
> device_is_alive only tells you that the device_t node is present in the
> tree.  fxp_release may be called in error cases.  Rather than working
> around your problem this way, please find what is calling fxp_detach when
> the device is not alive.

This way of doing things (ie: using device_is_alive()) is an almost
verbatim copy of what you did in every network driver in sys/pci/.
Now if it's wrong, feel free to change it, but I guess you'll have
to change all those drivers too then.

Cheers,
Maxime