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

[Attilio Rao]

2009/9/4 M. Warner Losh <imp at bsdimp.com>:
> In message: <200909031340.n83Defkv034013 at svn.freebsd.org>
>            Attilio Rao <attilio at FreeBSD.org> writes:
> : 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.

I think the right fix here is to maintain in sync devinfo.h and bus.h
definition by just having one. I see that the devices states are
redefined in devinfo.h in order to avoid namespace pollution and this
is a good point. What I propose is to add a new header (_bus.h,  to be
included in both bus.h and devinfo.h) which just containst the device
states.

> 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...

Sorry, I really don't understand what point are you making here, and
what the scaring words of "destroying", "racy", etc. means. Can you
explain better that part?

> 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.

How DS_DETACHING can cause problems here? device_detach() simply won't
run if the state is DS_DETACHING as expected (another thread is alredy
detaching and there is no need for it to detach).
Also, please note that in this case, for the state == DS_BUSY he
device_detach() won't do anything. You can't simply skip the return
value and anything else, but the reality is still the operation won't
happen.

> 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...

Still, I don't see a problem with the codes you mentioned (if not in
the consumers, which were alredy "broken" before this change and which
situation is not worse now), unless the devinfo breakage.

Attilio


-- 
Peace can only be achieved by understanding - A. Einstein