From owner-cvs-src@FreeBSD.ORG Tue Aug 2 18:54:08 2005
Return-Path: <owner-cvs-src@FreeBSD.ORG>
X-Original-To: cvs-src@FreeBSD.org
Delivered-To: cvs-src@FreeBSD.org
Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125])
by hub.freebsd.org (Postfix) with ESMTP id 2476E16A41F;
Tue, 2 Aug 2005 18:54:08 +0000 (GMT) (envelope-from imp@bsdimp.com)
Received: from harmony.bsdimp.com (vc4-2-0-87.dsl.netrack.net [199.45.160.85])
by mx1.FreeBSD.org (Postfix) with ESMTP id 7CF6743D4C;
Tue, 2 Aug 2005 18:54:07 +0000 (GMT) (envelope-from imp@bsdimp.com)
Received: from localhost (localhost.village.org [127.0.0.1] (may be forged))
by harmony.bsdimp.com (8.13.3/8.13.3) with ESMTP id j72Iq3ip029204;
Tue, 2 Aug 2005 12:52:03 -0600 (MDT) (envelope-from imp@bsdimp.com)
Date: Tue, 02 Aug 2005 12:52:59 -0600 (MDT)
Message-Id: <20050802.125259.02265276.imp@bsdimp.com>
To: scottl@samsco.org
From: "M. Warner Losh" <imp@bsdimp.com>
In-Reply-To: <42EFBB3E.4020508@samsco.org>
References: <20050802160519.6A91016A431@hub.freebsd.org>
<42EFB103.2070307@root.org> <42EFBB3E.4020508@samsco.org>
X-Mailer: Mew version 3.3 on Emacs 21.3 / Mule 5.0 (SAKAKI)
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-2.0
(harmony.bsdimp.com [127.0.0.1]);
Tue, 02 Aug 2005 12:52:04 -0600 (MDT)
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
X-BeenThere: cvs-src@freebsd.org
X-Mailman-Version: 2.1.5
Precedence: list
List-Id: CVS commit messages for the src tree <cvs-src.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/cvs-src>,
<mailto:cvs-src-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/cvs-src>
List-Post: <mailto:cvs-src@freebsd.org>
List-Help: <mailto:cvs-src-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/cvs-src>,
<mailto:cvs-src-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Tue, 02 Aug 2005 18:54:08 -0000
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