Date: Fri, 04 Sep 2009 11:29:19 -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.112919.1521002024.imp@bsdimp.com> In-Reply-To: <200909031340.n83Defkv034013@svn.freebsd.org> References: <200909031340.n83Defkv034013@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
In message: <200909031340.n83Defkv034013@svn.freebsd.org>
Attilio Rao <attilio@FreeBSD.org> writes:
: 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.
That appropriate moment better be ASAP. At least for the change I've
highlighed below, since it changes the ABI. Btw, this change also
breaks devinfo.
: Modified: head/sys/sys/bus.h
: ==============================================================================
: --- head/sys/sys/bus.h Thu Sep 3 12:41:00 2009 (r196778)
: +++ head/sys/sys/bus.h Thu Sep 3 13:40:41 2009 (r196779)
: @@ -52,8 +52,11 @@ struct u_businfo {
: typedef enum device_state {
: DS_NOTPRESENT, /**< @brief not probed or probe failed */
: DS_ALIVE, /**< @brief probe succeeded */
: + DS_ATTACHING, /**< @brief attaching is in progress */
: DS_ATTACHED, /**< @brief attach method called */
: - DS_BUSY /**< @brief device is open */
: + DS_BUSY, /**< @brief device is open */
: + DS_DETACHING /**< @brief detaching is in progress */
: +
: } device_state_t;
device_state_t is exported to userland via devctl. Well, that's not
entirely true... It isn't used EXACTLY, but there's this in
devinfo.h:
/*
* State of the device.
*/
/* XXX not sure if I want a copy here, or expose sys/bus.h */
typedef enum devinfo_state {
DIS_NOTPRESENT, /* not probed or probe failed */
DIS_ALIVE, /* probe succeeded */
DIS_ATTACHED, /* attach method called */
DIS_BUSY /* device is open */
} devinfo_state_t;
which is why devinfo is broken.
Also, DS_BUSY is used in many drivers to PREVENT detaching. So the
change is bad from that point of view, since DS_DETACHING is now >
DS_BUSY. There's really a partial ordering relationship now where
before there was a total ordering (DS_BUSY is > DS_ATTACHED and
DS_DETACHING is > DS_ATTACH, but DS_DETACHING isn't > DS_BUSY and
DS_BUSY isn't > DS_DETACHING). I think that you've destroyed
information here by unconditionally setting it:
- if ((error = DEVICE_DETACH(dev)) != 0)
+ dev->state = DS_DETACHING;
+ if ((error = DEVICE_DETACH(dev)) != 0) {
+ KASSERT(dev->state == DS_DETACHING,
+ ("%s: %p device state must not been changing", __func__,
+ dev));
+ dev->state = DS_ATTACHED;
return (error);
+ }
+ KASSERT(dev->state == DS_DETACHING,
+ ("%s: %p device state must not been changing", __func__, dev));
And this looks racy between the check earlier and this setting.
Properly locked, this wouldn't destroy information...
At the very least cardbus/cardbus.c and pccard/pccard.c need to be
looked at since they both have code that looks like:
for (tmp = 0; tmp < numdevs; tmp++) {
struct cardbus_devinfo *dinfo = device_get_ivars(devlist[tmp]);
int status = device_get_state(devlist[tmp]);
if (dinfo->pci.cfg.dev != devlist[tmp])
device_printf(cbdev, "devinfo dev mismatch\n");
if (status == DS_ATTACHED || status == DS_BUSY)
device_detach(devlist[tmp]);
cardbus_release_all_resources(cbdev, dinfo);
cardbus_device_destroy(dinfo);
device_delete_child(cbdev, devlist[tmp]);
pci_freecfg((struct pci_devinfo *)dinfo);
}
which does ignore errors returned by device_detach for the DS_BUSY
case because there's not currently a good way to tell device_detach
that it *MUST* detach the device *NOW* without any possibility of veto
by the driver. The above code also isn't DS_DETACHING aware, and may
be wrong in the face of this new state.
Of course, grepping the tree does show one or two places where DS_BUSY
is used inappropriately:
rp.c:
static int
rp_pcidetach(device_t dev)
{
CONTROLLER_t *ctlp;
if (device_get_state(dev) == DS_BUSY)
return (EBUSY);
is one example. The above check should just be removed (ditto for its
SHUTDOWN) routine.
So I think we should fix rp.c, but we need to talk through this change
a little more. I'm surprised I wasn't even pinged about it, since it
hits code that I maintain and a simple grep would have found...
I'm not yet asking for it to be backed out, but I don't like it on its
surface and want to talk about it in more detail.
Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090904.112919.1521002024.imp>