clockevents: add ->set_dev_mode() to struct clock_event_device
cover clockevents: add ->set_dev_mode() to struct clock_event_device
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(-)
Signature · 1 line
2.0.0.rc2
RFC 1/2 clockevents: return 'int' from clockevents_set_mode()
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.
--- 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(-)
arch/arm/common/bL_switcher.c +3 -2
@@ -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 ); }
include/linux/clockchips.h +1 -1
@@ -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);
kernel/time/clockevents.c +5 -3
@@ -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); }
kernel/time/tick-broadcast.c +16 -9
@@ -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 );
kernel/time/tick-common.c +3 -3
@@ -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 )
kernel/time/tick-oneshot.c +3 -3
@@ -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 ; }
Signature · 1 line
2.0.0.rc2
Re: [RFC 1/2] clockevents: return 'int' from clockevents_set_mode()
Quoted 9 lines · Viresh Kumar
clockevents_set_mode()callsdev->set_mode()which has return type of 'void' currently. Later patches would add another interfacedev->set_dev_mode(), which will return int andclockevents_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 ifclockevents_set_mode()fails, do issue aWARN()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
Re: [RFC 1/2] clockevents: return 'int' from clockevents_set_mode()
Quoted 2 lines · Thomas Gleixner
Does that
handle_return_value()mean that you sprinkleWARN_ONsall 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 :(
Quoted 3 lines
Does it mean, that you change the return value
semantics of functions which happen to callclock_events_set_mode()just because it now has a return value?
I didn't do it, probably yet another bad/confusing log by me :(
Quoted 1 line
Quoted 1 line
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 ..
Quoted 2 lines
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
RFC 2/2 clockevents: add ->set_dev_mode() to struct clock_event_device
In reply to Viresh KumarThere 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.
--- include/linux/clockchips.h | 5 ++++- kernel/time/clockevents.c | 26 ++++++++++++++++++++------ kernel/time/timer_list.c | 9 +++++++-- 3 files changed, 31 insertions(+), 9 deletions(-)
include/linux/clockchips.h +4 -1
@@ -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 *);
kernel/time/clockevents.c +20 -6
@@ -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; }/**
kernel/time/timer_list.c +7 -2
@@ -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: " );
Signature · 1 line
2.0.0.rc2