Date: Sat, 5 Sep 2009 00:46:03 +0200 From: Attilio Rao <attilio@freebsd.org> To: "M. Warner Losh" <imp@bsdimp.com> Cc: arch@freebsd.org Subject: Re: NEWBUS states (was Re: svn commit: r196779 - in head/sys: kern sys) Message-ID: <3bbf2fe10909041546y2b5633e1ue063955568df1a06@mail.gmail.com> In-Reply-To: <20090904.161634.-217944108.imp@bsdimp.com> References: <200909031340.n83Defkv034013@svn.freebsd.org> <20090904.112919.1521002024.imp@bsdimp.com> <3bbf2fe10909041455u552b0dbdm1708ea0a26365149@mail.gmail.com> <20090904.161634.-217944108.imp@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
2009/9/5 M. Warner Losh <imp@bsdimp.com>:
> [[ redirected to arch@ since too much of this discussion has been private ]]
>
> In message: <3bbf2fe10909041455u552b0dbdm1708ea0a26365149@mail.gmail.com>
> Attilio Rao <attilio@freebsd.org> writes:
> : 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.
>
> There's a lot of possible fixes. It is broken right now. Your commit
> broke it.
>
> The problem is that we have multiple names for the same things, and
> that's a defined API/ABI today.... So having a _bus.h won't solve
> this problem without introducing name space pollution (well, I suppose
> you could have __DS_NOTPRESENT, __DS_ALIVE, etc and then have
> devinfo.h and bus.h map those in, but that still wouldn't totally
> solve the problem).
What I would like to do is simply to typedef the enum I get from the
_bus.h. I'm not sure if nothing else is still required.
> : > 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?
>
> I think it is a minor concern. There's no locking now to prevent
> multiple threads doing stuff. I think it would be better to
> completely omit this stuff than to put it in 1/2 way.
The point for this change is to add now parts which could introduce,
in the future, ABI compatibility breakage so that we can MFC the
newbus locking to 8.1. Obviously that is not a finished patch, because
it still misses of the full context.
> : > 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.
>
> I explained about partial ordering already. Let me try again.
>
> DS_BUSY isn't compatible with this definition. It would be better to
> merge DS_ATTACHING and DS_DETACHING into one state, say DS_TRANSITION.
> Then we'd do the attach sequence as DS_ALIVE -> DS_TRANSITION ->
> DS_ATTACHED (and later DS_BUSY maybe). Then detach would go from
> DS_ATTACHED -> DS_TRANSITION to tear it down. There's a strong
> ordering expected in the code, and finding all the subtle places that
> this extra state beyond DS_BUSY causes problems will be hard.
We all agreed the one-state was the better option but it can't be done
in this way because of the device_is_attached() used in the detach
virtual functions. Using just one transition state will break
device_is_attached() in those parts.
The right fix, as pointed out in other e-mails, is to not use
device_is_attached() in detach virtual functions. The better fix, in
my idea would involve:
- replace the device_is_attached() usage in detach virtual functions,
with a more functional support
- use one-state transition
But that is just too much job to push in before then 8.0-REL and if
that would mean to not commit a patch and make impossible a future
MFC, I prefer to go with a lesser-perfect-but-still-working-approach.
I'm sorry if these points weren't clear before.
> In short, I think that http://people.freebsd.org/~imp/newbus-20090904
> should be committed and MFC'd. I've not addressed the devinfo
> breakage yet...
404
Btw, I don't agree about removing the changes within subr_bus.c. They
are harmless (KASSERT and further checks)
and will help as a reminder.
For the moment the only thing I still see to be fixed is devinfo. I
will provide a patch before the end of the day.
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?3bbf2fe10909041546y2b5633e1ue063955568df1a06>