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.
In order to propagate error codes to callers of clockevents_set_mode(), its return type is also changed to 'int', but all the error handling is done inside clockevents_set_mode() currently as none of the currently available modes are allowed to fail.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org ---
Hi Daniel/Preeti/Others..
I need to get some level of reviews done for some of my patches because of this: https://lkml.org/lkml/2014/5/12/206
A reviewed-by: would be very helpful to get this upstreamed. Please review and point out any improvement in the patch or the log..
V1->V2: - Don't sprinkly WARN_ON() on call sites of clockevents_set_mode() - Handle errors returned from ->set_dev_mode() in clockevents_set_mode() only.
include/linux/clockchips.h | 7 +++++-- kernel/time/clockevents.c | 29 ++++++++++++++++++++++++----- kernel/time/timer_list.c | 9 +++++++-- 3 files changed, 36 insertions(+), 9 deletions(-)
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h index 2e4cb67..bf902bf 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 dev mode 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 *); @@ -160,7 +163,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..087a5aa 100644 --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -101,11 +101,20 @@ 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) { - dev->set_mode(mode, dev); + if (dev->set_dev_mode) { + int ret = dev->set_dev_mode(mode, dev); + + /* Currently available modes are not allowed to fail */ + if (WARN_ON(ret)) + return ret; + } else { + dev->set_mode(mode, dev); + } + dev->mode = mode;
/* @@ -119,6 +128,8 @@ void clockevents_set_mode(struct clock_event_device *dev, } } } + + return 0; }
/** @@ -441,15 +452,23 @@ 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); + if (dev->mode == CLOCK_EVT_MODE_PERIODIC) { + if (dev->set_dev_mode) { + ret = dev->set_dev_mode(CLOCK_EVT_MODE_PERIODIC, dev); + WARN_ON(ret); + } 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: ");
Hi Viresh,
On 05/13/2014 02:34 PM, Viresh Kumar wrote:
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.
In order to propagate error codes to callers of clockevents_set_mode(), its return type is also changed to 'int', but all the error handling is done inside clockevents_set_mode() currently as none of the currently available modes are allowed to fail.
Instead of *all the error handling*, you can perhaps say that we have added a WARN_ON() to begin with. We are ideally not doing any error handling other than a WARN_ON() right?
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Hi Daniel/Preeti/Others..
I need to get some level of reviews done for some of my patches because of this: https://lkml.org/lkml/2014/5/12/206
A reviewed-by: would be very helpful to get this upstreamed. Please review and point out any improvement in the patch or the log..
V1->V2:
- Don't sprinkly WARN_ON() on call sites of clockevents_set_mode()
- Handle errors returned from ->set_dev_mode() in clockevents_set_mode() only.
include/linux/clockchips.h | 7 +++++-- kernel/time/clockevents.c | 29 ++++++++++++++++++++++++----- kernel/time/timer_list.c | 9 +++++++-- 3 files changed, 36 insertions(+), 9 deletions(-)
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h index 2e4cb67..bf902bf 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 dev mode 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,
void (*set_mode)(enum clock_event_mode mode, struct clock_event_device *); void (*suspend)(struct clock_event_device *);struct clock_event_device *);
@@ -160,7 +163,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..087a5aa 100644 --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -101,11 +101,20 @@ 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) {
dev->set_mode(mode, dev);
if (dev->set_dev_mode) {
int ret = dev->set_dev_mode(mode, dev);
/* Currently available modes are not allowed to fail */
if (WARN_ON(ret))
return ret;
} else {
dev->set_mode(mode, dev);
}
dev->mode = mode;
/*
@@ -119,6 +128,8 @@ void clockevents_set_mode(struct clock_event_device *dev, } } }
- return 0;
}
/** @@ -441,15 +452,23 @@ 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);
- if (dev->mode == CLOCK_EVT_MODE_PERIODIC) {
if (dev->set_dev_mode) {
ret = dev->set_dev_mode(CLOCK_EVT_MODE_PERIODIC, dev);
WARN_ON(ret);
} 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: ");
Reviewed-by: Preeti U Murthy preeti@linux.vnet.ibm.com
On 13 May 2014 15:51, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
On 05/13/2014 02:34 PM, Viresh Kumar wrote:
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.
In order to propagate error codes to callers of clockevents_set_mode(), its return type is also changed to 'int', but all the error handling is done inside clockevents_set_mode() currently as none of the currently available modes are allowed to fail.
Instead of *all the error handling*, you can perhaps say that we have added a WARN_ON() to begin with.
Sure..
We are ideally not doing any error handling other than a WARN_ON() right?
Yep, we are just returning the error in that case..
Reviewed-by: Preeti U Murthy preeti@linux.vnet.ibm.com
Thanks :)
On 13 May 2014 15:51, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
On 05/13/2014 02:34 PM, Viresh Kumar wrote:
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.
In order to propagate error codes to callers of clockevents_set_mode(), its return type is also changed to 'int', but all the error handling is done inside clockevents_set_mode() currently as none of the currently available modes are allowed to fail.
Instead of *all the error handling*, you can perhaps say that we have added a WARN_ON() to begin with. We are ideally not doing any error handling other than a WARN_ON() right?
As suggested by you and Daniel (Over IRC), I have updated the logs. diff remains same:
Subject: [PATCH] clockevents: add ->set_dev_mode() to struct clock_event_device
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. Drivers that don't support it can return failure codes on a call to ->set_mode(), which has a return type of 'void' currently.
Following steps are suggested (by tglx) to get this fixed: 1. Add another callback ->set_dev_mode(), with return type 'int'. 2. Covert clockevent drivers to use ->set_dev_mode() instead of ->set_mode(). 3. Once all are converted, remove support for ->set_mode().
This patch implements 'part 1' of this approach. It also marks ->set_mode() deprecated as ->set_dev_mode() should be used instead.
In order to propagate error codes to callers of clockevents_set_mode(), change its return type to 'int'. Also add a WARN_ON() inside clockevents_set_mode() as none of the currently available modes are allowed to fail.
linaro-kernel@lists.linaro.org