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

M. Warner Losh imp at bsdimp.com
Fri Sep 4 17:31:05 UTC 2009


In message: <200909031340.n83Defkv034013 at svn.freebsd.org>
            Attilio Rao <attilio at 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


More information about the svn-src-all mailing list