From owner-cvs-all@FreeBSD.ORG Thu Apr 10 17:13:17 2003
Return-Path: <owner-cvs-all@FreeBSD.ORG>
Delivered-To: cvs-all@freebsd.org
Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125])
by hub.freebsd.org (Postfix) with ESMTP
id 799E337B41D; Thu, 10 Apr 2003 17:13:17 -0700 (PDT)
Received: from elvis.mu.org (elvis.mu.org [192.203.228.196])
by mx1.FreeBSD.org (Postfix) with ESMTP
id AEA5243F85; Thu, 10 Apr 2003 17:13:16 -0700 (PDT)
(envelope-from mux@freebsd.org)
Received: by elvis.mu.org (Postfix, from userid 1920)
id 98DAC2ED40B; Thu, 10 Apr 2003 17:13:16 -0700 (PDT)
Date: Fri, 11 Apr 2003 02:13:16 +0200
From: Maxime Henrion <mux@freebsd.org>
To: Nate Lawson <nate@root.org>
Message-ID: <20030411001316.GN1750@elvis.mu.org>
References: <20030410231519.3348737B4A5@hub.freebsd.org>
<Pine.BSF.4.21.0304101655330.32612-100000@root.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Pine.BSF.4.21.0304101655330.32612-100000@root.org>
User-Agent: Mutt/1.4.1i
cc: cvs-src@FreeBSD.org
cc: src-committers@FreeBSD.org
cc: cvs-all@FreeBSD.org
Subject: Re: cvs commit: src/sys/dev/fxp if_fxp.c
X-BeenThere: cvs-all@freebsd.org
X-Mailman-Version: 2.1.1
Precedence: list
List-Id: CVS commit messages for the entire tree <cvs-all.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/cvs-all>,
<mailto:cvs-all-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/cvs-all>
List-Post: <mailto:cvs-all@freebsd.org>
List-Help: <mailto:cvs-all-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/cvs-all>,
<mailto:cvs-all-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Fri, 11 Apr 2003 00:13:17 -0000
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@.
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