svn commit: r196779 - in head/sys: kern sys
John Baldwin
jhb at freebsd.org
Fri Sep 4 17:51:24 UTC 2009
On Friday 04 September 2009 1:33:14 pm M. Warner Losh wrote:
> In message: <3bbf2fe10909030646o2dc30166r303ee646572c741b at mail.gmail.com>
> Attilio Rao <attilio at FreeBSD.org> writes:
> : 2009/9/3 Attilio Rao <attilio at 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
More information about the svn-src-all
mailing list