| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <54DD72A4.8040503@linux.vnet.ibm.com>
Date: Fri, 13 Feb 2015 09:12:28 +0530
From: Preeti U Murthy <preeti@...ux.vnet.ibm.com>
To: Viresh Kumar <viresh.kumar@...aro.org>,
Thomas Gleixner <tglx@...utronix.de>
CC: linaro-kernel@...ts.linaro.org, linux-kernel@...r.kernel.org,
Kevin Hilman <khilman@...aro.org>,
Frederic Weisbecker <fweisbec@...il.com>,
Daniel Lezcano <daniel.lezcano@...aro.org>,
linaro-networking@...aro.org, peterz@...radead.org
Subject: Re: [PATCH v2] clockevents: Introduce mode specific callbacks
On 02/13/2015 06:24 AM, Viresh Kumar wrote:
> It is not possible for the clockevents core to know which modes (other than
> those with a corresponding feature flag) are supported by a particular
> implementation. And drivers are expected to handle transition to all modes
> elegantly, as ->set_mode() would be issued for them unconditionally.
>
> Now, adding support for a new mode complicates things a bit if we want to use
> the legacy ->set_mode() callback. We need to closely review all clockevents
> drivers to see if they would break on addition of a new mode. And after such
> reviews, it is found that we have to do non-trivial changes to most of the
> drivers [1].
>
> Introduce mode-specific set_mode_*() callbacks, some of which the drivers may or
> may not implement. A missing callback would clearly convey the message that the
> corresponding mode isn't supported.
>
> A driver may still choose to keep supporting the legacy ->set_mode() callback,
> but ->set_mode() wouldn't be supporting any new modes beyond RESUME. If a driver
> wants to get benefited by using a new mode, it would be required to migrate to
> the mode specific callbacks.
>
> The legacy ->set_mode() callback and the newly introduced mode-specific
> callbacks are mutually exclusive. Only one of them should be supported by the
> driver.
>
> Sanity check is done at the time of registration to distinguish between optional
> and required callbacks and to make error recovery and handling simpler. If the
> legacy ->set_mode() callback is provided, all mode specific ones would be
> ignored by the core but a warning is thrown if they are present.
>
> Call sites calling ->set_mode() directly are also updated to use
> __clockevents_set_mode() instead, as ->set_mode() may not be available anymore
> for few drivers.
>
> [1] https://lkml.org/lkml/2014/12/9/605
> [2] https://lkml.org/lkml/2015/1/23/255
>
> Suggested-by: Thomas Gleixner <tglx@...utronix.de> [2]
> Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
> ---
> V1->V2: Stricter sanity checks.
>
> include/linux/clockchips.h | 21 +++++++++--
> kernel/time/clockevents.c | 88 ++++++++++++++++++++++++++++++++++++++++++++--
> kernel/time/timer_list.c | 32 +++++++++++++++--
> 3 files changed, 134 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
> index 2e4cb67f6e56..59af26b54d15 100644
> --- a/include/linux/clockchips.h
> +++ b/include/linux/clockchips.h
> @@ -39,6 +39,8 @@ enum clock_event_mode {
> CLOCK_EVT_MODE_PERIODIC,
> CLOCK_EVT_MODE_ONESHOT,
> CLOCK_EVT_MODE_RESUME,
> +
> + /* Legacy ->set_mode() callback doesn't support below modes */
> };
>
> /*
> @@ -81,7 +83,11 @@ 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_mode: legacy set mode function, only for modes <= CLOCK_EVT_MODE_RESUME.
> + * @set_mode_periodic: switch mode to periodic, if !set_mode
> + * @set_mode_oneshot: switch mode to oneshot, if !set_mode
> + * @set_mode_shutdown: switch mode to shutdown, if !set_mode
> + * @set_mode_resume: resume clkevt device, if !set_mode
> * @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
> @@ -108,9 +114,20 @@ struct clock_event_device {
> unsigned int features;
> unsigned long retries;
>
> - void (*broadcast)(const struct cpumask *mask);
> + /*
> + * Mode transition callback(s): Only one of the two groups should be
> + * defined:
> + * - set_mode(), only for modes <= CLOCK_EVT_MODE_RESUME.
> + * - set_mode_{shutdown|periodic|oneshot|resume}().
> + */
> void (*set_mode)(enum clock_event_mode mode,
> struct clock_event_device *);
> + int (*set_mode_periodic)(struct clock_event_device *);
> + int (*set_mode_oneshot)(struct clock_event_device *);
> + int (*set_mode_shutdown)(struct clock_event_device *);
> + int (*set_mode_resume)(struct clock_event_device *);
> +
> + void (*broadcast)(const struct cpumask *mask);
> void (*suspend)(struct clock_event_device *);
> void (*resume)(struct clock_event_device *);
> unsigned long min_delta_ticks;
> diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
> index 55449909f114..489642b08d64 100644
> --- a/kernel/time/clockevents.c
> +++ b/kernel/time/clockevents.c
> @@ -94,6 +94,57 @@ u64 clockevent_delta2ns(unsigned long latch, struct clock_event_device *evt)
> }
> EXPORT_SYMBOL_GPL(clockevent_delta2ns);
>
> +static int __clockevents_set_mode(struct clock_event_device *dev,
> + enum clock_event_mode mode)
> +{
> + /* Transition with legacy set_mode() callback */
> + if (dev->set_mode) {
> + /* Legacy callback doesn't support new modes */
> + if (mode > CLOCK_EVT_MODE_RESUME)
> + return -ENOSYS;
> + dev->set_mode(mode, dev);
> + return 0;
> + }
> +
> + if (dev->features & CLOCK_EVT_FEAT_DUMMY)
> + return 0;
> +
> + /* Transition with new mode-specific callbacks */
> + switch (mode) {
> + case CLOCK_EVT_MODE_UNUSED:
> + /*
> + * This is an internal state, which is guaranteed to go from
> + * SHUTDOWN to UNUSED. No driver interaction required.
> + */
> + return 0;
> +
> + case CLOCK_EVT_MODE_SHUTDOWN:
> + return dev->set_mode_shutdown(dev);
> +
> + case CLOCK_EVT_MODE_PERIODIC:
> + /* Core internal bug */
> + if (!(dev->features & CLOCK_EVT_FEAT_PERIODIC))
> + return -ENOSYS;
> + return dev->set_mode_periodic(dev);
> +
> + case CLOCK_EVT_MODE_ONESHOT:
> + /* Core internal bug */
> + if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT))
> + return -ENOSYS;
> + return dev->set_mode_oneshot(dev);
> +
> + case CLOCK_EVT_MODE_RESUME:
> + /* Optional callback */
> + if (dev->set_mode_resume)
> + return dev->set_mode_resume(dev);
> + else
> + return 0;
> +
> + default:
> + return -ENOSYS;
> + }
> +}
> +
> /**
> * clockevents_set_mode - set the operating mode of a clock event device
> * @dev: device to modify
> @@ -105,7 +156,9 @@ void clockevents_set_mode(struct clock_event_device *dev,
> enum clock_event_mode mode)
> {
> if (dev->mode != mode) {
> - dev->set_mode(mode, dev);
> + if (__clockevents_set_mode(dev, mode))
> + return;
> +
> dev->mode = mode;
>
> /*
> @@ -373,6 +426,35 @@ int clockevents_unbind_device(struct clock_event_device *ced, int cpu)
> }
> EXPORT_SYMBOL_GPL(clockevents_unbind);
>
> +/* Sanity check of mode transition callbacks */
> +static int clockevents_sanity_check(struct clock_event_device *dev)
> +{
> + /* Legacy set_mode() callback */
> + if (dev->set_mode) {
> + /* We shouldn't be supporting new modes now */
> + WARN_ON(dev->set_mode_periodic || dev->set_mode_oneshot ||
> + dev->set_mode_shutdown || dev->set_mode_resume);
> + return 0;
> + }
> +
> + if (dev->features & CLOCK_EVT_FEAT_DUMMY)
> + return 0;
> +
> + /* New mode-specific callbacks */
> + if (!dev->set_mode_shutdown)
> + return -EINVAL;
> +
> + if ((dev->features & CLOCK_EVT_FEAT_PERIODIC) &&
> + !dev->set_mode_periodic)
> + return -EINVAL;
> +
> + if ((dev->features & CLOCK_EVT_FEAT_ONESHOT) &&
> + !dev->set_mode_oneshot)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> /**
> * clockevents_register_device - register a clock event device
> * @dev: device to register
> @@ -382,6 +464,8 @@ void clockevents_register_device(struct clock_event_device *dev)
> unsigned long flags;
>
> BUG_ON(dev->mode != CLOCK_EVT_MODE_UNUSED);
> + BUG_ON(clockevents_sanity_check(dev));
> +
> if (!dev->cpumask) {
> WARN_ON(num_possible_cpus() > 1);
> dev->cpumask = cpumask_of(smp_processor_id());
> @@ -449,7 +533,7 @@ int __clockevents_update_freq(struct clock_event_device *dev, u32 freq)
> return clockevents_program_event(dev, dev->next_event, false);
>
> if (dev->mode == CLOCK_EVT_MODE_PERIODIC)
> - dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev);
> + return __clockevents_set_mode(dev, CLOCK_EVT_MODE_PERIODIC);
>
> return 0;
> }
> diff --git a/kernel/time/timer_list.c b/kernel/time/timer_list.c
> index 61ed862cdd37..2cfd19485824 100644
> --- a/kernel/time/timer_list.c
> +++ b/kernel/time/timer_list.c
> @@ -228,9 +228,35 @@ 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);
> - SEQ_printf(m, "\n");
> + if (dev->set_mode) {
> + SEQ_printf(m, " set_mode: ");
> + print_name_offset(m, dev->set_mode);
> + SEQ_printf(m, "\n");
> + } else {
> + if (dev->set_mode_shutdown) {
> + SEQ_printf(m, " shutdown: ");
> + print_name_offset(m, dev->set_mode_shutdown);
> + SEQ_printf(m, "\n");
> + }
> +
> + if (dev->set_mode_periodic) {
> + SEQ_printf(m, " periodic: ");
> + print_name_offset(m, dev->set_mode_periodic);
> + SEQ_printf(m, "\n");
> + }
> +
> + if (dev->set_mode_oneshot) {
> + SEQ_printf(m, " oneshot: ");
> + print_name_offset(m, dev->set_mode_oneshot);
> + SEQ_printf(m, "\n");
> + }
> +
> + if (dev->set_mode_resume) {
> + SEQ_printf(m, " resume: ");
> + print_name_offset(m, dev->set_mode_resume);
> + SEQ_printf(m, "\n");
> + }
> + }
>
> SEQ_printf(m, " event_handler: ");
> print_name_offset(m, dev->event_handler);
>
Reviewed-by: Preeti U Murthy <preeti@...ux.vnet.ibm.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists