On 23 May 2014 03:51, Kevin Hilman khilman@linaro.org wrote:
Viresh Kumar viresh.kumar@linaro.org writes:
Also it is made sure that drivers don't disable events for unsupported modes.
Why wouldn't you want that? Most drivers are leaving events disabled for unsupported modes currently.
You know why it took me 4 days to send this version because I wasn't able to decide what to do here..
So here is the problem: - There is no problem even after the current series as every driver is taking care of all currently available modes and so they would never goto default case..
- When we will send ONESHOT_STOPPED notification strange things might happen. Core would send ONESHOT_STOPPED notification and drivers would return -ENOSYS (unless they are updated to handle it). Additionally few of them would disable events, and core would never think that. It would still think drivers are running in the last configured mode as they returned error here..
Core wouldn't try to set mode again for them as it thinks they are still running in that mode.
For ONESHOT_STOPPED it may work without issues as core will call set_next_event() and they might enable events there..
But with another mode in future things might start to fail. Suppose we got a call for unsupported mode while we are in PERIODIC MODE. Driver will disable mode and return error, but core will expect it to generate events periodically.
So, I still feel these changes are required. But wasn't sure if I should send them separately before this series, with this series or after this one as a separate series.
Doing this seems to complicate things, and needlessly change/complicate current behavior, as well as make this patch more difficult to read/review.
So yes. Sending it here wasn't the best idea as anyway we aren't adding this bug in this series. Should have sent patches separately after this series.
In any case, I've given a quick skim over some of the driver changes below and found several (possible) issues.
Probably all related to above stuff only..
This will be tricky to get right on all platforms, so IMO the changes should be minimal.
Hmm, yes. Probably I will mention this problem in cover-letter and will mention that I will fix it separately before adding another mode.
diff --git a/arch/arm/mach-clps711x/common.c b/arch/arm/mach-clps711x/common.c index aee81fa..fa7c090 100644 --- a/arch/arm/mach-clps711x/common.c +++ b/arch/arm/mach-clps711x/common.c @@ -69,30 +69,30 @@ static u64 notrace clps711x_sched_clock_read(void) return ~readw_relaxed(CLPS711X_VIRT_BASE + TC1D); }
-static void clps711x_clockevent_set_mode(enum clock_event_mode mode, +static int clps711x_clockevent_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) {
disable_irq(IRQ_TC2OI);
You probably noticed this, but this step is now skipped for _PERIODIC. Since the IRQ is immediately re-enabled, it's probably not a problem, but it *is* a functional change that should be noted.
Review for these is difficult and to mention them in patches is also difficult. There are so many drivers per patch.
diff --git a/arch/arm/mach-cns3xxx/core.c b/arch/arm/mach-cns3xxx/core.c
+static int cns3xxx_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *clk) { unsigned long ctrl = readl(cns3xxx_tmr1 + TIMER1_2_CONTROL_OFFSET); @@ -132,11 +132,15 @@ static void cns3xxx_timer_set_mode(enum clock_event_mode mode, break; case CLOCK_EVT_MODE_UNUSED: case CLOCK_EVT_MODE_SHUTDOWN:
default:
case CLOCK_EVT_MODE_RESUME: ctrl = 0;
break;
default:
return -ENOSYS; } writel(ctrl, cns3xxx_tmr1 + TIMER1_2_CONTROL_OFFSET);
Hmm, looks like the timer is left disabled for the unsupported modes, which is the opposite of what's done elsewhere.
No. There is no writel() at top of this routine to disable events. We are doing it at then end for UNUSED/SHUTDOWN/RESUME but left it enabled for 'default case'..
return 0;
}
diff --git a/arch/arm/mach-omap1/timer32k.c b/arch/arm/mach-omap1/timer32k.c
+static int omap_32k_timer_set_mode(enum clock_event_mode mode, struct clock_event_device *evt) {
omap_32k_timer_stop();
switch (mode) { case CLOCK_EVT_MODE_PERIODIC: omap_32k_timer_start(OMAP_32K_TIMER_TICK_PERIOD);
Note that this is also a functional change because the timer doesn't get stopped before it is restarted in periodic mode. I can't remember if that's safe on OMAP1 (I think not), but it in any case, a functional change like should not be made without more thorough testing (IMO, this kind of functional change should not be part of this patch.)
I overlooked it. I thought it will work safely but probably it wouldn't.
Anyways, I am going to remove these kind of diffs now..
diff --git a/arch/arm/mach-pxa/time.c b/arch/arm/mach-pxa/time.c
pxa_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *dev) { switch (mode) { @@ -84,9 +84,11 @@ pxa_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *dev) break;
case CLOCK_EVT_MODE_RESUME:
case CLOCK_EVT_MODE_PERIODIC:
Hmm, you're changing periodic mode from supported to unsupported. Why?
This should have been mentioned in logs, forgot it. Actually some drivers don't support these modes, like pxa doesn't support periodic mode, And they either have a BUG/WARN/pr_err or a simple 'break;' to handle these. I have dropped them as Thomas asked me to get rid of these BUG/WARN/pr_err etc in this series.
OK, I give up for now. I've found enough issues to convince myself that this needs some closer attention, and possibly a rethink about how to handle unsupported modes.
Yeah.. Let me try another series, it should be much easier to review that :)
Just to mention, I have kept another branch with such changes on top as I knew I may have to send them separately. Good, less work for me :)