From owner-svn-src-head@FreeBSD.ORG Fri Sep 4 17:51:24 2009 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 7796D1065676; Fri, 4 Sep 2009 17:51:24 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 31C318FC19; Fri, 4 Sep 2009 17:51:24 +0000 (UTC) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id B845C46B2C; Fri, 4 Sep 2009 13:51:23 -0400 (EDT) Received: from jhbbsd.hudson-trading.com (unknown [209.249.190.8]) by bigwig.baldwin.cx (Postfix) with ESMTPA id C945B8A043; Fri, 4 Sep 2009 13:51:22 -0400 (EDT) From: John Baldwin To: "M. Warner Losh" Date: Fri, 4 Sep 2009 13:49:53 -0400 User-Agent: KMail/1.9.7 References: <200909031340.n83Defkv034013@svn.freebsd.org> <3bbf2fe10909030646o2dc30166r303ee646572c741b@mail.gmail.com> <20090904.113314.1979406201.imp@bsdimp.com> In-Reply-To: <20090904.113314.1979406201.imp@bsdimp.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200909041349.53872.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.0.1 (bigwig.baldwin.cx); Fri, 04 Sep 2009 13:51:22 -0400 (EDT) X-Virus-Scanned: clamav-milter 0.95.1 at bigwig.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-1.6 required=4.2 tests=AWL,BAYES_00,RDNS_NONE autolearn=no version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on bigwig.baldwin.cx Cc: attilio@freebsd.org, svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r196779 - in head/sys: kern sys X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 04 Sep 2009 17:51:24 -0000 On Friday 04 September 2009 1:33:14 pm M. Warner Losh wrote: > In message: <3bbf2fe10909030646o2dc30166r303ee646572c741b@mail.gmail.com> > Attilio Rao writes: > : 2009/9/3 Attilio Rao : > : > Author: attilio > : > Date: Thu Sep 3 13:40:41 2009 > : > New Revision: 196779 > : > URL: http://svn.freebsd.org/changeset/base/196779 > : > > : > Log: > : > Add intermediate states for attaching and detaching that will be > : > reused by the enhached newbus locking once it is checked in. > : > This change can be easilly MFCed to STABLE_8 at the appropriate moment. > : > : About this commit, there are a few of things which worths nothing: > : - This has been committed separately from the new-bus locking because > : it is planned to be MFCed before 8.0 goes out in order to offer the > : correct ABI compatibility for merging, while 8.1-REL, the newbus > : locking > > I understand why the change is made here, but I don't think it is the > right one. Or at least I don't think I adequately understand its > motivation and need to understand that better since it doesn't make > sense to me. > > : - Probabilly what we really wanted here is just a transitioning state > : instead than both DS_ATTACHING and DS_DETACHING. Some consumers would > : eventually understand what of the 2 is that and one of these consumers > : is device_is_attached(). That function is used improperly by many > : detach handlers in a construct like: > > I don't understand the point you are trying to make here... I'm not > sure that piggybacking this information on the state is the right > approach. We need a single state (though not really 2 IMO) so we can run device attach and detach routines w/o holding the new-bus lock (which it turns out we need to do as attach and detach routines can do too many things that aren't safe to do while holding the new-bus lock). > : int > : foo_attach(device_t dev) > : { > : ... > : if (error != 0) > : foo_detach(dev); > : ... > : } > : > : int > : foo_detach(device_dev) > : { > : ... > : if (device_is_attached(dev)) { > : /* do some handover */ > : } > : ... > : } > : > : That is an incorrect behaviour which needs to be discouraged and that > : will be fixed during 9.0. > > Why is that bad? There are a lot of drivers that do that. I don't > understand the point you are making here... I think it is sloppy/lazy at best. A detach routine should really only be used for detaching. > The alternatives are likely worse. Please let me know what the plan > here is. I think a very sane alternative is to split these detach routines up. All the bits that aren't conditional on 'device_is_attached()' would move into a 'foo_release_resources()' that foo_attach() calls on error. foo_detach() would then do all the work that is currently conditional on 'device_is_attached()' unconditionally and then invoke foo_release_resources(). A larger demo: int foo_attach() { sc->ifp = if_alloc(); ... ether_ifattach(sc->ifp); return (0); error: foo_detach(); } int foo_detach() { if (device_is_attached()) { ether_ifdetach(sc->ifp); } ... if_free(sc->ifp); } would now become this: int foo_attach() { sc->ifp = if_alloc(); ... ether_ifattach(sc->ifp); return (0); error: foo_release_resources(); } int foo_detach() { ether_ifdetach(sc->ifp); foo_release_resources(); } int foo_release_resources() { ... if_free(sc->ifp); } -- John Baldwin