Date: Tue, 02 Aug 2005 12:52:59 -0600 (MDT) From: "M. Warner Losh" <imp@bsdimp.com> To: scottl@samsco.org Cc: cvs-src@FreeBSD.org, src-committers@FreeBSD.org, emax@FreeBSD.org, cvs-all@FreeBSD.org, nate@root.org Subject: Re: cvs commit: src/sys/dev/an if_an.c Message-ID: <20050802.125259.02265276.imp@bsdimp.com> In-Reply-To: <42EFBB3E.4020508@samsco.org> References: <20050802160519.6A91016A431@hub.freebsd.org> <42EFB103.2070307@root.org> <42EFBB3E.4020508@samsco.org>
next in thread | previous in thread | raw e-mail | index | archive | help
In message: <42EFBB3E.4020508@samsco.org>
Scott Long <scottl@samsco.org> writes:
: Nate Lawson wrote:
: > Maksim Yevmenkin wrote:
: >
: >> emax 2005-08-02 16:03:51 UTC
: >>
: >> FreeBSD src repository
: >>
: >> Modified files:
: >> sys/dev/an if_an.c Log:
: >> Do not lock an to check gone flag. Only need to hold the lock to set
: >> the gone flag.
: >> Reviewed by: imp
: >> MFC after: 1 day
: >> Revision Changes Path
: >> 1.69 +1 -2 src/sys/dev/an/if_an.c
: >>
: >>
: >> Index: src/sys/dev/an/if_an.c
: >> diff -u src/sys/dev/an/if_an.c:1.68 src/sys/dev/an/if_an.c:1.69
: >> --- src/sys/dev/an/if_an.c:1.68 Wed Jul 27 21:03:35 2005
: >> +++ src/sys/dev/an/if_an.c Tue Aug 2 16:03:51 2005
: >> @@ -826,12 +826,11 @@
: >> struct an_softc *sc = device_get_softc(dev);
: >> struct ifnet *ifp = sc->an_ifp;
: >>
: >> - AN_LOCK(sc);
: >> if (sc->an_gone) {
: >> - AN_UNLOCK(sc);
: >> device_printf(dev,"already unloaded\n");
: >> return(0);
: >> }
: >> + AN_LOCK(sc);
: >> an_stop(sc);
: >> sc->an_gone = 1;
: >> ifmedia_removeall(&sc->an_ifmedia);
: >
: >
: > This commit is wrong. The race is:
: >
: > Process 1 Process 2
: > an_detach()
: > if (gone) ...
: > an_detach()
: > if (gone) ...
: > LOCK
: > an_stop()
: > gone = 1
: > UNLOCK
: > LOCK
: > an_stop() !!! called twice
: > gone = 1
: >
: > You really need to hold the lock over both the check and set, otherwise
: > it's not atomic.
: >
: > This driver also needs some mtx_asserts in an_reset() and other places
: > that must be called with the an lock held.
: >
:
: I agree with Nate. The single act of testing an integer usually doesn't
: require a mutex, but making a decision based on that test requires
: atomicity. I apologize for not noticing this is prior email.
However, nate is wrong here. You can't have two threads in a detach
at the same time.
Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20050802.125259.02265276.imp>