Hi Thomas,
As suggested by you (https://lkml.org/lkml/2014/5/10/86), here is an attempt to implement first two steps for fixing the bigger problem.
This patchset is about adding ->set_dev_mode() to struct clock_event_device with return type 'int'. To achieve that return type of clockevents_set_mode() is also changed to propagate failures.
Let me know if this is what you wanted to see or I screwed up again :(
I will then start updating drivers one by one (100 drivers AFAICT). I will try to fix them in separate patches and will post them together in a single patchset. And because the number is going to be over 100, let me know how do you want me to send that..
Viresh Kumar (2): clockevents: return 'int' from clockevents_set_mode() clockevents: add ->set_dev_mode() to struct clock_event_device
arch/arm/common/bL_switcher.c | 5 +++-- include/linux/clockchips.h | 7 +++++-- kernel/time/clockevents.c | 34 +++++++++++++++++++++++++--------- kernel/time/tick-broadcast.c | 25 ++++++++++++++++--------- kernel/time/tick-common.c | 6 +++--- kernel/time/tick-oneshot.c | 6 +++--- kernel/time/timer_list.c | 9 +++++++-- 7 files changed, 62 insertions(+), 30 deletions(-)
clockevents_set_mode() calls dev->set_mode() which has return type of 'void' currently. Later patches would add another interface dev->set_dev_mode(), which will return int and clockevents_set_mode() must propagate failures returned from it.
This patch changes prototype of clockevents_set_mode() to return 'int' and fix caller sites as well. As most of the call sites maynot work if clockevents_set_mode() fails, do issue a WARN() on failures. All other are updated to return error codes.
Cc: Nicolas Pitre nicolas.pitre@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- arch/arm/common/bL_switcher.c | 5 +++-- include/linux/clockchips.h | 2 +- kernel/time/clockevents.c | 8 +++++--- kernel/time/tick-broadcast.c | 25 ++++++++++++++++--------- kernel/time/tick-common.c | 6 +++--- kernel/time/tick-oneshot.c | 6 +++--- 6 files changed, 31 insertions(+), 21 deletions(-)
diff --git a/arch/arm/common/bL_switcher.c b/arch/arm/common/bL_switcher.c index f01c0ee..bf95a6a 100644 --- a/arch/arm/common/bL_switcher.c +++ b/arch/arm/common/bL_switcher.c @@ -234,7 +234,8 @@ static int bL_switch_to(unsigned int new_cluster_id) tdev = NULL; if (tdev) { tdev_mode = tdev->evtdev->mode; - clockevents_set_mode(tdev->evtdev, CLOCK_EVT_MODE_SHUTDOWN); + WARN_ON(clockevents_set_mode(tdev->evtdev, + CLOCK_EVT_MODE_SHUTDOWN)); }
ret = cpu_pm_enter(); @@ -262,7 +263,7 @@ static int bL_switch_to(unsigned int new_cluster_id) ret = cpu_pm_exit();
if (tdev) { - clockevents_set_mode(tdev->evtdev, tdev_mode); + WARN_ON(clockevents_set_mode(tdev->evtdev, tdev_mode)); clockevents_program_event(tdev->evtdev, tdev->evtdev->next_event, 1); } diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h index 2e4cb67..4e7a4d3 100644 --- a/include/linux/clockchips.h +++ b/include/linux/clockchips.h @@ -160,7 +160,7 @@ extern int clockevents_update_freq(struct clock_event_device *ce, u32 freq);
extern void clockevents_exchange_device(struct clock_event_device *old, struct clock_event_device *new); -extern void clockevents_set_mode(struct clock_event_device *dev, +extern int clockevents_set_mode(struct clock_event_device *dev, enum clock_event_mode mode); extern int clockevents_program_event(struct clock_event_device *dev, ktime_t expires, bool force); diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c index ad362c2..8665660 100644 --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -101,7 +101,7 @@ EXPORT_SYMBOL_GPL(clockevent_delta2ns); * * Must be called with interrupts disabled ! */ -void clockevents_set_mode(struct clock_event_device *dev, +int clockevents_set_mode(struct clock_event_device *dev, enum clock_event_mode mode) { if (dev->mode != mode) { @@ -119,6 +119,8 @@ void clockevents_set_mode(struct clock_event_device *dev, } } } + + return 0; }
/** @@ -127,7 +129,7 @@ void clockevents_set_mode(struct clock_event_device *dev, */ void clockevents_shutdown(struct clock_event_device *dev) { - clockevents_set_mode(dev, CLOCK_EVT_MODE_SHUTDOWN); + WARN_ON(clockevents_set_mode(dev, CLOCK_EVT_MODE_SHUTDOWN)); dev->next_event.tv64 = KTIME_MAX; }
@@ -503,7 +505,7 @@ void clockevents_exchange_device(struct clock_event_device *old, */ if (old) { module_put(old->owner); - clockevents_set_mode(old, CLOCK_EVT_MODE_UNUSED); + WARN_ON(clockevents_set_mode(old, CLOCK_EVT_MODE_UNUSED)); list_del(&old->list); list_add(&old->list, &clockevents_released); } diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c index 64c5990..fb83fa1 100644 --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -464,7 +464,8 @@ int tick_resume_broadcast(void) bc = tick_broadcast_device.evtdev;
if (bc) { - clockevents_set_mode(bc, CLOCK_EVT_MODE_RESUME); + if (clockevents_set_mode(bc, CLOCK_EVT_MODE_RESUME)) + goto unlock;
switch (tick_broadcast_device.mode) { case TICKDEV_MODE_PERIODIC: @@ -479,6 +480,7 @@ int tick_resume_broadcast(void) break; } } +unlock: raw_spin_unlock_irqrestore(&tick_broadcast_lock, flags);
return broadcast; @@ -532,8 +534,11 @@ static int tick_broadcast_set_event(struct clock_event_device *bc, int cpu, { int ret;
- if (bc->mode != CLOCK_EVT_MODE_ONESHOT) - clockevents_set_mode(bc, CLOCK_EVT_MODE_ONESHOT); + if (bc->mode != CLOCK_EVT_MODE_ONESHOT) { + ret = clockevents_set_mode(bc, CLOCK_EVT_MODE_ONESHOT); + if (ret) + return ret; + }
ret = clockevents_program_event(bc, expires, force); if (!ret) @@ -543,8 +548,7 @@ static int tick_broadcast_set_event(struct clock_event_device *bc, int cpu,
int tick_resume_broadcast_oneshot(struct clock_event_device *bc) { - clockevents_set_mode(bc, CLOCK_EVT_MODE_ONESHOT); - return 0; + return clockevents_set_mode(bc, CLOCK_EVT_MODE_ONESHOT); }
/* @@ -562,8 +566,8 @@ void tick_check_oneshot_broadcast_this_cpu(void) * switched over, leave the device alone. */ if (td->mode == TICKDEV_MODE_ONESHOT) { - clockevents_set_mode(td->evtdev, - CLOCK_EVT_MODE_ONESHOT); + WARN_ON(clockevents_set_mode(td->evtdev, + CLOCK_EVT_MODE_ONESHOT)); } } } @@ -741,7 +745,9 @@ int tick_broadcast_oneshot_control(unsigned long reason) cpumask_clear_cpu(cpu, tick_broadcast_oneshot_mask); } else { if (cpumask_test_and_clear_cpu(cpu, tick_broadcast_oneshot_mask)) { - clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT); + ret = clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT); + if (ret) + goto out; /* * The cpu which was handling the broadcast * timer marked this cpu in the broadcast @@ -858,7 +864,8 @@ void tick_broadcast_setup_oneshot(struct clock_event_device *bc) tick_broadcast_oneshot_mask, tmpmask);
if (was_periodic && !cpumask_empty(tmpmask)) { - clockevents_set_mode(bc, CLOCK_EVT_MODE_ONESHOT); + WARN_ON(clockevents_set_mode(bc, + CLOCK_EVT_MODE_ONESHOT)); tick_broadcast_init_next_event(tmpmask, tick_next_period); tick_broadcast_set_event(bc, cpu, tick_next_period, 1); diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c index 0a0608e..7b2cf15 100644 --- a/kernel/time/tick-common.c +++ b/kernel/time/tick-common.c @@ -140,7 +140,7 @@ void tick_setup_periodic(struct clock_event_device *dev, int broadcast)
if ((dev->features & CLOCK_EVT_FEAT_PERIODIC) && !tick_broadcast_oneshot_active()) { - clockevents_set_mode(dev, CLOCK_EVT_MODE_PERIODIC); + WARN_ON(clockevents_set_mode(dev, CLOCK_EVT_MODE_PERIODIC)); } else { unsigned long seq; ktime_t next; @@ -150,7 +150,7 @@ void tick_setup_periodic(struct clock_event_device *dev, int broadcast) next = tick_next_period; } while (read_seqretry(&jiffies_lock, seq));
- clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT); + WARN_ON(clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT));
for (;;) { if (!clockevents_program_event(dev, next, false)) @@ -384,7 +384,7 @@ void tick_resume(void) struct tick_device *td = &__get_cpu_var(tick_cpu_device); int broadcast = tick_resume_broadcast();
- clockevents_set_mode(td->evtdev, CLOCK_EVT_MODE_RESUME); + WARN_ON(clockevents_set_mode(td->evtdev, CLOCK_EVT_MODE_RESUME));
if (!broadcast) { if (td->mode == TICKDEV_MODE_PERIODIC) diff --git a/kernel/time/tick-oneshot.c b/kernel/time/tick-oneshot.c index 8241090..b525f97 100644 --- a/kernel/time/tick-oneshot.c +++ b/kernel/time/tick-oneshot.c @@ -38,7 +38,7 @@ void tick_resume_oneshot(void) { struct clock_event_device *dev = __this_cpu_read(tick_cpu_device.evtdev);
- clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT); + WARN_ON(clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT)); clockevents_program_event(dev, ktime_get(), true); }
@@ -50,7 +50,7 @@ void tick_setup_oneshot(struct clock_event_device *newdev, ktime_t next_event) { newdev->event_handler = handler; - clockevents_set_mode(newdev, CLOCK_EVT_MODE_ONESHOT); + WARN_ON(clockevents_set_mode(newdev, CLOCK_EVT_MODE_ONESHOT)); clockevents_program_event(newdev, next_event, true); }
@@ -81,7 +81,7 @@ int tick_switch_to_oneshot(void (*handler)(struct clock_event_device *))
td->mode = TICKDEV_MODE_ONESHOT; dev->event_handler = handler; - clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT); + WARN_ON(clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT)); tick_broadcast_switch_to_oneshot(); return 0; }
On Mon, 12 May 2014, Viresh Kumar wrote:
clockevents_set_mode() calls dev->set_mode() which has return type of 'void' currently. Later patches would add another interface dev->set_dev_mode(), which will return int and clockevents_set_mode() must propagate failures returned from it.
This patch changes prototype of clockevents_set_mode() to return 'int' and fix caller sites as well. As most of the call sites maynot work if clockevents_set_mode() fails, do issue a WARN() on failures. All other are updated to return error codes.
Is this a speed coding contest? And did you even think about what I suggested:
if (dev->set_dev_mode) { ret = dev->set_dev_mode(dev, mode); handle_return_value(ret);
Does that handle_return_value() mean that you sprinkle WARN_ONs all over the place? Does it mean, that you change the return value semantics of functions which happen to call clock_events_set_mode() just because it now has a return value?
Hell no.
handle_return_value() means handle the return value right there. We know what we want to set and we know what may fail and what not, so we can explicitely WARN right there. And for those things which might fail, we return the failure and handle it at the call site.
I've warned you and I'm seriosly grumpy by now. You are just wasting my time and I'm tired of that.
Find someone competent who reviews your patches, deals with you and when they make sense sends them to me. Or just stay away from kernel/* and find something else to wreckage.
Thanks,
tglx
On 12 May 2014 15:49, Thomas Gleixner tglx@linutronix.de wrote:
Does that handle_return_value() mean that you sprinkle WARN_ONs all over the place?
Yeah, probably it should have been done this way:
- dev->set_mode(mode, dev); + if (dev->set_dev_mode) { + int ret = dev->set_dev_mode(mode, dev); + + /* Currently available modes are not expected to fail */ + if (WARN_ON(ret)) + return ret; + } else { + dev->set_mode(mode, dev); + } +
My fault and a big one :(
Does it mean, that you change the return value semantics of functions which happen to call clock_events_set_mode() just because it now has a return value?
I didn't do it, probably yet another bad/confusing log by me :(
All other are updated to return error codes.
When I read it now, I can see why it was confusing :(. What I meant was:
"routines which had capability to return errors would now return them on failure of clockevents_set_mode() as well.."
But even that isn't required and handling should be added only to callers of clockevents_set_mode() which are issuing ONESHOT_STOPPED ..
Find someone competent who reviews your patches, deals with you and when they make sense sends them to me.
I will make sure to get some Reviewed-by/Signed-off-by for my patches from someone with good understanding of these frameworks before sending them out again..
Sorry for the noise :(
-- viresh
There is a requirement to add another mode: CLOCK_EVT_MODE_ONESHOT_STOPPED (lkml.org/lkml/2014/5/9/508) to clockevent devices and clockevent-drivers may or maynot support it. And so can return failure codes on a call to ->set_mode(), which has a return type of 'void' as of now.
To fix that, add another callback ->set_dev_mode(), with return type 'int'. All clockevent drivers will be migrated to use this new interface later. Also mark ->set_mode() deprecated.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- include/linux/clockchips.h | 5 ++++- kernel/time/clockevents.c | 26 ++++++++++++++++++++------ kernel/time/timer_list.c | 9 +++++++-- 3 files changed, 31 insertions(+), 9 deletions(-)
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h index 4e7a4d3..e15836f 100644 --- a/include/linux/clockchips.h +++ b/include/linux/clockchips.h @@ -81,7 +81,8 @@ enum clock_event_mode { * @mode: operating mode assigned by the management code * @features: features * @retries: number of forced programming retries - * @set_mode: set mode function + * @set_dev_mode: set state function + * @set_mode: set mode function (deprecated, use set_dev_mode instead) * @broadcast: function to broadcast events * @min_delta_ticks: minimum delta value in ticks stored for reconfiguration * @max_delta_ticks: maximum delta value in ticks stored for reconfiguration @@ -109,6 +110,8 @@ struct clock_event_device { unsigned long retries;
void (*broadcast)(const struct cpumask *mask); + int (*set_dev_mode)(enum clock_event_mode mode, + struct clock_event_device *); void (*set_mode)(enum clock_event_mode mode, struct clock_event_device *); void (*suspend)(struct clock_event_device *); diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c index 8665660..3ef5997 100644 --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -104,8 +104,17 @@ EXPORT_SYMBOL_GPL(clockevent_delta2ns); int clockevents_set_mode(struct clock_event_device *dev, enum clock_event_mode mode) { + int ret = 0; + if (dev->mode != mode) { - dev->set_mode(mode, dev); + if (dev->set_dev_mode) + ret = dev->set_dev_mode(mode, dev); + else + dev->set_mode(mode, dev); + + if (unlikely(ret)) + return ret; + dev->mode = mode;
/* @@ -443,15 +452,20 @@ EXPORT_SYMBOL_GPL(clockevents_config_and_register);
int __clockevents_update_freq(struct clock_event_device *dev, u32 freq) { + int ret = 0; + clockevents_config(dev, freq);
if (dev->mode == CLOCK_EVT_MODE_ONESHOT) - return clockevents_program_event(dev, dev->next_event, false); - - if (dev->mode == CLOCK_EVT_MODE_PERIODIC) - dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev); + ret = clockevents_program_event(dev, dev->next_event, false); + else if (dev->mode == CLOCK_EVT_MODE_PERIODIC) { + if (dev->set_dev_mode) + ret = dev->set_dev_mode(CLOCK_EVT_MODE_PERIODIC, dev); + else + dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev); + }
- return 0; + return ret; }
/** diff --git a/kernel/time/timer_list.c b/kernel/time/timer_list.c index 61ed862..3d854aa 100644 --- a/kernel/time/timer_list.c +++ b/kernel/time/timer_list.c @@ -228,8 +228,13 @@ print_tickdevice(struct seq_file *m, struct tick_device *td, int cpu) print_name_offset(m, dev->set_next_event); SEQ_printf(m, "\n");
- SEQ_printf(m, " set_mode: "); - print_name_offset(m, dev->set_mode); + if (dev->set_dev_mode) { + SEQ_printf(m, " set_dev_mode: "); + print_name_offset(m, dev->set_dev_mode); + } else { + SEQ_printf(m, " set_mode: "); + print_name_offset(m, dev->set_mode); + } SEQ_printf(m, "\n");
SEQ_printf(m, " event_handler: ");
linaro-kernel@lists.linaro.org
-
Thomas Gleixner -
Viresh Kumar