Date: Fri, 4 Sep 2009 13:49:53 -0400 From: John Baldwin <jhb@freebsd.org> To: "M. Warner Losh" <imp@bsdimp.com> 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 Message-ID: <200909041349.53872.jhb@freebsd.org> In-Reply-To: <20090904.113314.1979406201.imp@bsdimp.com> References: <200909031340.n83Defkv034013@svn.freebsd.org> <3bbf2fe10909030646o2dc30166r303ee646572c741b@mail.gmail.com> <20090904.113314.1979406201.imp@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Friday 04 September 2009 1:33:14 pm M. Warner Losh wrote: > In message: <3bbf2fe10909030646o2dc30166r303ee646572c741b@mail.gmail.com> > Attilio Rao <attilio@FreeBSD.org> writes: > : 2009/9/3 Attilio Rao <attilio@freebsd.org>: > : > 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200909041349.53872.jhb>