svn commit: r196779 - in head/sys: kern sys

[John Baldwin]

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