In message: <200909031340.n83defkv034...@svn.freebsd.org>
            Attilio Rao <atti...@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
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Reply via email to