Re: NEWBUS states
- From: Attilio Rao <attilio@xxxxxxxxxxx>
- Date: Sat, 5 Sep 2009 17:12:17 +0200
2009/9/5 M. Warner Losh <imp@xxxxxxxxxx>:
In message: <3bbf2fe10909041546y2b5633e1ue063955568df1a06@xxxxxxxxxxxxxx>
Attilio Rao <attilio@xxxxxxxxxxx> writes:
: 2009/9/5 M. Warner Losh <imp@xxxxxxxxxx>:
: > [[ redirected to arch@ since too much of this discussion has been private ]]
: >
: > In message: <3bbf2fe10909041455u552b0dbdm1708ea0a26365149@xxxxxxxxxxxxxx>
: > Attilio Rao <attilio@xxxxxxxxxxx> writes:
: > : 2009/9/4 M. Warner Losh <imp@xxxxxxxxxx>:
: > : > In message: <200909031340.n83Defkv034013@xxxxxxxxxxxxxxx>
: > : > Attilio Rao <attilio@xxxxxxxxxxx> 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
_______________________________________________
freebsd-arch@xxxxxxxxxxx mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "freebsd-arch-unsubscribe@xxxxxxxxxxx"
- Follow-Ups:
- Re: NEWBUS states
- From: M. Warner Losh
- Re: NEWBUS states
- References:
- NEWBUS states (was Re: svn commit: r196779 - in head/sys: kern sys)
- From: M. Warner Losh
- Re: NEWBUS states (was Re: svn commit: r196779 - in head/sys: kern sys)
- From: Attilio Rao
- Re: NEWBUS states
- From: M. Warner Losh
- NEWBUS states (was Re: svn commit: r196779 - in head/sys: kern sys)
- Prev by Date: Re: NEWBUS states
- Next by Date: Re: NEWBUS states
- Previous by thread: Re: NEWBUS states
- Next by thread: Re: NEWBUS states
- Index(es):
Relevant Pages
|
