Date: Fri, 04 Sep 2009 11:33:14 -0600 (MDT) From: "M. Warner Losh" <imp@bsdimp.com> To: attilio@FreeBSD.org Cc: 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: <20090904.113314.1979406201.imp@bsdimp.com> In-Reply-To: <3bbf2fe10909030646o2dc30166r303ee646572c741b@mail.gmail.com> References: <200909031340.n83Defkv034013@svn.freebsd.org> <3bbf2fe10909030646o2dc30166r303ee646572c741b@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
: 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...
The alternatives are likely worse. Please let me know what the plan
here is.
: In the while this semantic (used by many drivers) makes the
: single-transition state impraticable and so we will need to stick with
: that device states also in 9.0.
A lot of drivers use that idiom today. Are you signing up to make
sure they all work in the future?
Again: I wasn't consulted on this change, don't like it, and want to
talk more about it. It broke devinfo, and ignored code in cardbus and
pccard that likely needs to change to cope.
Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090904.113314.1979406201.imp>