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>