Date: Sat, 5 Sep 2009 17:12:17 +0200 From: Attilio Rao <attilio@freebsd.org> To: "M. Warner Losh" <imp@bsdimp.com> Cc: arch@freebsd.org Subject: Re: NEWBUS states Message-ID: <3bbf2fe10909050812l4340f679h6a4d7dae1daa3bf8@mail.gmail.com> In-Reply-To: <20090904.172310.-1939841993.imp@bsdimp.com> References: <3bbf2fe10909041455u552b0dbdm1708ea0a26365149@mail.gmail.com> <20090904.161634.-217944108.imp@bsdimp.com> <3bbf2fe10909041546y2b5633e1ue063955568df1a06@mail.gmail.com> <20090904.172310.-1939841993.imp@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
2009/9/5 M. Warner Losh <imp@bsdimp.com>:
> In message: <3bbf2fe10909041546y2b5633e1ue063955568df1a06@mail.gmail.com>
> Attilio Rao <attilio@FreeBSD.org> writes:
> : 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.
>
> You can't do just that. There's the "DIS_foo vs DS_foo" issue as
> well, which cannot be solved with typedefs. Some mapping is
> required.. this is a sill API, one could argue, but it is the one we
> have today...
Bah, sorry, I kept reading it as DS_ rather than DIS_ .
Anyways, what do you think about, for 9.0, just having one interface
and remove the DIS_* bloat?
> :
> : 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.
>
> Yes. I understand that. I'm specifically suggesting that we only MFC
> the changes to the ABI today, and MFC the changes to the code when it
> is complete.
Sure, and that's what I did. The small parts in subr_bus.c aren't that
much important but they can be used as a reminder. If you feel
strongly against it I can also review and eventually drop them.
> : 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.
>
> True. However, this device_is_attached stuff is fundamentally broken
> as it is today. I took a closer look at the typical usage, and it
> stands in as a proxy for 'did all the resources get allocated' and
> even that's just the bus_resouce* stuff...
Of course. Such paradigms are only sane (in particular from the
standpoint of the locking) only when you can make some assumptions
about a known context, and you should also know the state of your
device there.
> : The right fix, as pointed out in other e-mails, is to not use
>
> e-mails that I wasn't cc'd on since this discussion happened in
> private. I hate to keep harping on this point, but there's been too
> much of that lately...
Sorry for that, but in this case I was referring to e-mail sent to
public mailing list. My current english and expressive skillset is not
that much developed so far though, so it is likely I did express the
concept with cryptic/incorrect words as often happens, so I can't
blame anyone else than me for that.
> : 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
>
> This would be transitioning to using a common set of code for
> release_resources, ala the other most common driver idiom...
I agree. There are various ways to fix this that we can discuss and
put in place during 9.0 timelife.
> : - 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.
>
> Yes. The problem is that we're trying to guess what the right locking
> approach will be at the 11th hour, and I'm worried we will guess wrong.
>
> : I'm sorry if these points weren't clear before.
>
> OK. Let me ponder based on that... It might be better for this round
> of changes to leverage off the device 'flags' field to indicate that
> we're attaching/detaching. This would not break the
> device_is_attached() usage, and would solve the interlock problem
> nicely. While it isn't as aesthetically pleasing as the new states,
> it would allow us to easily MFC it without API/ABI breakage. This
> field surely would be covered by the same set of locks as the state
> field.
>
> I know that there's a good aesthetic argument to be made against this,
> but on the other hand 'compatibility' hacks can violate one's
> aesthetics. We can migrate to a more pleasing state-based model in 9
> and reduce the risk to other code from changing its semantics at this
> late date.
That was exactly the original point of my commit.
> : > 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
>
> http://people.freebsd.org/~imp/newbus-20090904.diff
>
> sorry about that...
So I see in this patch you are also implementing the idea to offer
rooms in the enum intra-existing-states that kib proposed. That is a
good idea to do now. However, the one-state transition can't be
implemented in this patch as you accepted few lines above.
> : 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.
>
> Why have them at all? We're just speculating about what the protocol
> will be, rather than abstracting down from a known-to-be-good
> implementation. This sort of thing has bit us in the past before.
> Since at best we're speculating about what the best approach is, I'd
> prefer to keep the leaps of faith as small as possible.
Ok, if you are strongly against it, we can remove them, I just think
they will be harmless and a good reminder.
Thanks,
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?3bbf2fe10909050812l4340f679h6a4d7dae1daa3bf8>