On 29 May 2014 22:12, Kevin Hilman khilman@linaro.org wrote:
I'm going to nitpick some more on the changelog here, as well has a few comments. Sorry for the lag...
No worries, I was out on a week-long holiday, just back today.
All accepted.
NOTE: Following issue is still unresolved as this patchset isn't making it worse. But it should be fixed before addition of ONESHOT_STOPPED mode.
Few clockevent drivers have disabled clock events as soon as we enter their ->set_dev_mode() callbacks and so they stay disabled for unsupported modes as well.
All clockevent drivers are handling modes in ->set_dev_mode() with help of a 'switch' block. For the currently available modes none of them would ever get down to the 'default case' of switch block.
Not sure what you're saying heare about the default case. Do you mean currently, or after this series? A quick glance suggests that at least under arch/arm, there are several switch statments today that fall back to the default case, so I'm guessing you mean after this series? Please clarify.
Yeah, I meant after this series is applied.
And so we would never hit the BUG where we failed to change mode and still disabled clock events.
But as soon as any new mode would be introduced in clockevents core, some of these might behave unexpectedly. Like, core may expect events to be enabled as call to ->set_dev_mode() returned failure and may not try enabling events again.
Why would core expect events to be anbled upon ->set_dev_mode() failure?
I wasn't sure and so just mentioned that I can see this problem being there. Don't know how Thomas wants to see this :)
You're introducing this feature, so you get to decide what the core expects.
I wasn't talking about this feature but the default case in general, if something isn't supported we fail and core might/must believe nothing has changed..
IMO, since current behavior of most clockevent drivers is to leave clockevent device disabled for unsupported modes, it's perfectly reasonable for the core to assume that the clockevent driver is/remains disabled when ->set_dev_mode() fails.
Since this series (or probably the followup which adds ONESHOT_STOPPED) will be handling the error modes. It should just be clear that when ->set_dev_mode() fails, part of the error handling should be that the core must put the clockevent device into a working mode.
Probably yes. But would it be fine to have this NOTE in cover-letter for this series and ask for what's the right thing to do? And maybe give the options you gave and what I had ?