drivers: clock_control: clock_control_on is blocking #20710
Conversation
mbolivar
added
bug
mbolivar
requested review from
carlescufi,
henrikbrixandersen,
nashif,
nordic-krch,
pabigot and
tbursztyka
include/drivers/clock_control.h
Outdated
There was a problem hiding this comment.
That's actually wrong. See my comment on your issue.
Common pattern being: functions ending with _async are non blocking, others are.
The documentation was missing at that time, yes. But these were meant to be blocking and should be blocking or then they would be aliases to _async counterpart with a cb set as NULL... so they would be useless.
There was a problem hiding this comment.
Thanks for responding. I asked you on slack yesterday what you meant but got no answer :).
OK, so then the situation is that all the clock drivers which aren't blocking are buggy. Would you agree?
There was a problem hiding this comment.
functions ending with _async are non blocking, others are.
BTW, I fully agree with this, but nobody who responded so far seemed to, so I am glad we are on the same page.
There was a problem hiding this comment.
Would you agree?
I see from your comment on the issue that you do agree. Thanks!
There was a problem hiding this comment.
functions ending with _async are non blocking, others are.
BTW, I fully agree with this, but nobody who responded so far seemed to, so I am glad we are on the same page.
I also think this should be true, but see #15703 (comment). "Non-blocking" is subject to conditions, at least for some API.
There was a problem hiding this comment.
That comment seems to indicate that "non-blocking" is not the same as "queued", which makes sense.
There was a problem hiding this comment.
Thanks for responding. I asked you on slack yesterday what you meant but got no answer :).
Sorry I missed the message/notification
OK, so then the situation is that all the clock drivers which aren't blocking are buggy. Would you agree?
Yes, we are on same page.
mbolivar
force-pushed
the
clock-control-docs
branch
from
a1a8d9e to
2e168d4
Compare
mbolivar
changed the title
|
@tbursztyka I've rewritten this with the opposite sense. Please review and I guess we'll discuss at the next API meeting. |
nordic-krch
left a comment
nordic-krch
left a comment
There was a problem hiding this comment.
LGTM, 2 minor comments
include/drivers/clock_control.h
Outdated
There was a problem hiding this comment.
@ref clock_control_async_on()?
include/drivers/clock_control.h
Outdated
There was a problem hiding this comment.
It's not clear what it is actually. It's probably something that is vendor dependent.
There was a problem hiding this comment.
I don't understand this comment. That's what "opaque data" means, no?
There was a problem hiding this comment.
I've updated the clock disable comment to use the same language
include/drivers/clock_control.h
Outdated
There was a problem hiding this comment.
How should it behave when called from interrupt context? Do we forbid that or it should be blocking wait (with k_cpu_idle)?
There was a problem hiding this comment.
Yep, that is the 64 thousand dollar question.
We have a few choices which I was planning to discuss at API meeting.
There was a problem hiding this comment.
Why would it be any different from other blocking functions in this regard?
There was a problem hiding this comment.
Looks like we won't get to discuss in the API meeting. Here are some options:
- busy-wait in ISR context, sleep in thread context
- busy-wait no matter the context
- fail in ISR, sleep in thread
- ???
I'm not sure if there are other contexts we need to worry about. I don't know how meta IRQs work, for example.
Considering that some clocks take a long time to start up and stabilize, sleeping the thread seems desirable, if a bit more complex.
I'd like to discuss in the meeting to get as much feedback as possible from people who are interested.
There was a problem hiding this comment.
@carlescufi points out this is a special case of #6184.
There was a problem hiding this comment.
we can have k_cpu_idle() in ISR and semaphore in thread context.
Personally, I would prefer that function works in both context. I wouldn't like to have _isr, rather function should internally handle it.
There was a problem hiding this comment.
Updated following discussion in the API meeting. The intent has apparently been that unless specifically marked, all driver APIs are only safe to call from threads.
|
@mbolivar could you address the review by @nordic-krch ? |
mbolivar
force-pushed
the
clock-control-docs
branch
3 times, most recently
from
7c5c9a3 to
bf369bf
Compare
There was a problem hiding this comment.
I'm fine with this as a documentation fix for 2.1. As discussed in #20806 and suggested in #20796 (review) we should revisit this API in terms of naming, description of behavior, and functionality.
mbolivar
force-pushed
the
clock-control-docs
branch
from
bf369bf to
cfb9e7b
Compare
Document this behavior. This partially addresses zephyrproject-rtos#20708, but we'll have to deal with driver bugs case by case now that the desired behavior is clear. Signed-off-by: Martí Bolívar <[email protected]>
mbolivar
force-pushed
the
clock-control-docs
branch
from
cfb9e7b to
61d048b
Compare
11 participants
Document this behavior. This partially addresses #20708.