Date: Fri, 4 Sep 2009 23:55:52 +0200 From: Attilio Rao <attilio@freebsd.org> To: "M. Warner Losh" <imp@bsdimp.com> 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: <3bbf2fe10909041455u552b0dbdm1708ea0a26365149@mail.gmail.com> In-Reply-To: <20090904.112919.1521002024.imp@bsdimp.com> References: <200909031340.n83Defkv034013@svn.freebsd.org> <20090904.112919.1521002024.imp@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
2009/9/4 M. Warner Losh <imp@bsdimp.com>:
> In message: <200909031340.n83Defkv034013@svn.freebsd.org>
> Attilio Rao <attilio@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3bbf2fe10909041455u552b0dbdm1708ea0a26365149>