| Message ID | 1465506139-25981-3-git-send-email-dbasehore@chromium.org (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
On Thu, 9 Jun 2016, dbasehore@chromium.org wrote: > > +/* > + * Clockevent device may run during freeze > + */ > +# define CLOCK_EVT_FEAT_FREEZE 0x000100 This is a bad name and a horrible comment. The device does not freeze. It is able to run during suspend. Hint: We have CLOCK_SOURCE_SUSPEND_NONSTOP which is self explanatory. > /** > * struct clock_event_device - clock event device descriptor > * @event_handler: Assigned by the framework to be called by the low > * level handler of the event source > * @set_next_event: set next event function using a clocksource delta > * @set_next_ktime: set next event function using a direct ktime value > + * @event_pending: check if the programmed event is still pending. Used > + * for freeze events when timekeeping is suspended and > + * irqs are disabled. > * @next_event: local storage for the next event in oneshot mode > * @max_delta_ns: maximum delta value in ns > * @min_delta_ns: minimum delta value in ns > @@ -100,7 +108,9 @@ struct clock_event_device { > void (*event_handler)(struct clock_event_device *); > int (*set_next_event)(unsigned long evt, struct clock_event_device *); > int (*set_next_ktime)(ktime_t expires, struct clock_event_device *); > + bool (*event_expired)(struct clock_event_device *); > ktime_t next_event; > + bool freeze_event_programmed; I really don't like that flag. Why do you need it at all? > u64 max_delta_ns; > u64 min_delta_ns; > +static int clockevents_program_freeze_event(struct clock_event_device *dev, > + ktime_t delta) > +{ > + int64_t delta_ns = ktime_to_ns(delta); Why int? Please use u64 and spare all the silly type casts you have in your code. > + unsigned long long clc; What's wrong with u64? > + int ret; > + > + if (delta_ns > (int64_t) dev->max_delta_ns) { > + printk_deferred(KERN_WARNING > + "Freeze event time longer than max delta\n"); > + delta_ns = (int64_t) dev->max_delta_ns; What's the point of this? Tell the caller that it does not work and be done with it. -ERANGE or something like that. > + } > + clockevents_tick_resume(dev); > + clockevents_switch_state(dev, CLOCK_EVT_STATE_ONESHOT); > + delta_ns = max_t(int64_t, delta_ns, dev->min_delta_ns); You're comparing signed and insigned. Use u64 and max()... Also you really should tell the caller, that providing a timeout that small is silly. > + clc = ((unsigned long long) delta_ns * dev->mult) >> dev->shift; Sigh. > + ret = dev->set_next_event((unsigned long) clc, dev); > + if (ret < 0) { > + printk_deferred(KERN_WARNING > + "Failed to program freeze event\n"); > + clockevents_shutdown(dev); > + } else { > + dev->freeze_event_programmed = true; I'm still not seing why you need that flag. > + } > + > + return ret; > +} > + > +static bool clockevents_freeze_event_expired(struct clock_event_device *dev) > +{ > + if (dev->freeze_event_programmed) > + return dev->event_expired(dev); So this will oops from deep inside suspend when the clock event does not have the callback .... > + return false; > +} > + > +static void clockevents_cleanup_freeze_event(struct clock_event_device *dev) > +{ > + if (!(dev->features & CLOCK_EVT_FEAT_FREEZE)) > + return; What's that check for? This is only called from the code below in a section which cannot be reached when the flag is not set. > + clockevents_shutdown(dev); You can open code this line at the call site because that's all you need. > + dev->freeze_event_programmed = false; > +} > +/** > + * timed_freeze - Enter freeze on a CPU for a timed duration > + * @ops: Pointers for enter freeze and callback functions. > + * @data: Pointer to pass arguments to the function pointers. > + * @delta: Time to freeze for. If this amount of time passes in freeze, the > + * callback in ops will be called. > + * > + * Returns the value from ops->enter_freeze or ops->callback on success, -EERROR > + * otherwise. If an error is encountered while setting up the clock event, > + * freeze with still be entered, but it will not be timed nor will the callback > + * function be run. That logic makes no sense at all. > +int timed_freeze(struct timed_freeze_ops *ops, void *data, ktime_t delta) > +{ > + int cpu = smp_processor_id(); > + struct tick_device *td = tick_get_device(cpu); > + struct clock_event_device *dev; > + int ret; > + > + if (!ops || !ops->enter_freeze) { > + printk_deferred(KERN_ERR > + "[%s] called with invalid ops\n", __func__); > + return -EINVAL; > + } > + > + if (!td || !td->evtdev || td is always valid, because it's a pointer to a per cpu variable. > + !(td->evtdev->features & CLOCK_EVT_FEAT_FREEZE)) { > + printk_deferred(KERN_WARNING > + "[%s] called with invalid clock event device\n", The function is not called with an invalid clock event device. There is either no clock event device or the device does not support this. > + __func__); > + ret = -ENOSYS; > + goto freeze_no_check; > + } > + > + dev = td->evtdev; > + if (!clockevent_state_shutdown(dev)) { > + printk_deferred(KERN_WARNING > + "[%s] called while clock event device in use\n", > + __func__); > + ret = -EBUSY; > + goto freeze_no_check; > + } > + > + ret = clockevents_program_freeze_event(dev, delta); > + if (ret < 0) > + goto freeze_no_check; > + > + ret = ops->enter_freeze(data); > + if (ret < 0) > + goto out; > + > + if (ops->callback && clockevents_freeze_event_expired(dev)) > + ret = ops->callback(data); This callback thing is just wrong here. If that fails then how is the call site supposed to figure out where the error came from? From your printks? The correct way to do this is: int tick_set_frozen_event(ktime_t delta) { if (....) return -EINVAL; if (....) return -ENOSYS; return program_event(dev, delta); } and: int tick_clear_frozen_event() { ret = event_expired(dev); clockevents_shutdown(dev); return ret; } So no ops, nothing nada. And at the call site you do: ret = tick_set_frozen_event(delta); if (ret) goto deal_with_ret; ret = freeze(); if (ret) { tick_clear_frozen_event(); goto deal_with_freeze_abort; } ret = tick_clear_frozen_event(); do_something_sensible(ret); That's actually understandable and debugable code. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi,
[auto build test ERROR on pm/linux-next]
[also build test ERROR on v4.7-rc2 next-20160609]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/dbasehore-chromium-org/Add-suspend-to-idle-validation-for-Intel-SoCs/20160610-053246
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: i386-randconfig-h0-06112342 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
>> kernel/time/clockevents.c:403:5: error: redefinition of 'timed_freeze'
int timed_freeze(struct timed_freeze_ops *ops, void *data, ktime_t delta)
^~~~~~~~~~~~
In file included from kernel/time/clockevents.c:20:0:
include/linux/suspend.h:291:19: note: previous definition of 'timed_freeze' was here
static inline int timed_freeze(struct timed_freeze_ops *ops, void *data,
^~~~~~~~~~~~
vim +/timed_freeze +403 kernel/time/clockevents.c
397 *
398 * Returns the value from ops->enter_freeze or ops->callback on success, -EERROR
399 * otherwise. If an error is encountered while setting up the clock event,
400 * freeze with still be entered, but it will not be timed nor will the callback
401 * function be run.
402 */
> 403 int timed_freeze(struct timed_freeze_ops *ops, void *data, ktime_t delta)
404 {
405 int cpu = smp_processor_id();
406 struct tick_device *td = tick_get_device(cpu);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Thu, Jun 9, 2016 at 3:43 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > On Thu, 9 Jun 2016, dbasehore@chromium.org wrote: >> >> +/* >> + * Clockevent device may run during freeze >> + */ >> +# define CLOCK_EVT_FEAT_FREEZE 0x000100 > > This is a bad name and a horrible comment. The device does not freeze. It is > able to run during suspend. > > Hint: We have CLOCK_SOURCE_SUSPEND_NONSTOP which is self explanatory. > >> /** >> * struct clock_event_device - clock event device descriptor >> * @event_handler: Assigned by the framework to be called by the low >> * level handler of the event source >> * @set_next_event: set next event function using a clocksource delta >> * @set_next_ktime: set next event function using a direct ktime value >> + * @event_pending: check if the programmed event is still pending. Used >> + * for freeze events when timekeeping is suspended and >> + * irqs are disabled. >> * @next_event: local storage for the next event in oneshot mode >> * @max_delta_ns: maximum delta value in ns >> * @min_delta_ns: minimum delta value in ns >> @@ -100,7 +108,9 @@ struct clock_event_device { >> void (*event_handler)(struct clock_event_device *); >> int (*set_next_event)(unsigned long evt, struct clock_event_device *); >> int (*set_next_ktime)(ktime_t expires, struct clock_event_device *); >> + bool (*event_expired)(struct clock_event_device *); >> ktime_t next_event; >> + bool freeze_event_programmed; > > I really don't like that flag. Why do you need it at all? Probably left over from when I tried doing this a different way. I'll remove it. > >> u64 max_delta_ns; >> u64 min_delta_ns; > >> +static int clockevents_program_freeze_event(struct clock_event_device *dev, >> + ktime_t delta) >> +{ >> + int64_t delta_ns = ktime_to_ns(delta); > > Why int? Please use u64 and spare all the silly type casts you have in your > code. > >> + unsigned long long clc; > > What's wrong with u64? Will change. > >> + int ret; >> + >> + if (delta_ns > (int64_t) dev->max_delta_ns) { >> + printk_deferred(KERN_WARNING >> + "Freeze event time longer than max delta\n"); >> + delta_ns = (int64_t) dev->max_delta_ns; > > What's the point of this? Tell the caller that it does not work and be done > with it. -ERANGE or something like that. > Okay. >> + } > >> + clockevents_tick_resume(dev); >> + clockevents_switch_state(dev, CLOCK_EVT_STATE_ONESHOT); >> + delta_ns = max_t(int64_t, delta_ns, dev->min_delta_ns); > > You're comparing signed and insigned. Use u64 and max()... Also you really > should tell the caller, that providing a timeout that small is silly. > >> + clc = ((unsigned long long) delta_ns * dev->mult) >> dev->shift; > > Sigh. > >> + ret = dev->set_next_event((unsigned long) clc, dev); >> + if (ret < 0) { >> + printk_deferred(KERN_WARNING >> + "Failed to program freeze event\n"); >> + clockevents_shutdown(dev); >> + } else { >> + dev->freeze_event_programmed = true; > > I'm still not seing why you need that flag. > >> + } >> + >> + return ret; >> +} >> + >> +static bool clockevents_freeze_event_expired(struct clock_event_device *dev) >> +{ >> + if (dev->freeze_event_programmed) >> + return dev->event_expired(dev); > > So this will oops from deep inside suspend when the clock event does not have > the callback .... > >> + return false; >> +} >> + >> +static void clockevents_cleanup_freeze_event(struct clock_event_device *dev) >> +{ >> + if (!(dev->features & CLOCK_EVT_FEAT_FREEZE)) >> + return; > > What's that check for? This is only called from the code below in a section > which cannot be reached when the flag is not set. > >> + clockevents_shutdown(dev); > > You can open code this line at the call site because that's all you need. > >> + dev->freeze_event_programmed = false; >> +} > >> +/** >> + * timed_freeze - Enter freeze on a CPU for a timed duration >> + * @ops: Pointers for enter freeze and callback functions. >> + * @data: Pointer to pass arguments to the function pointers. >> + * @delta: Time to freeze for. If this amount of time passes in freeze, the >> + * callback in ops will be called. >> + * >> + * Returns the value from ops->enter_freeze or ops->callback on success, -EERROR >> + * otherwise. If an error is encountered while setting up the clock event, >> + * freeze with still be entered, but it will not be timed nor will the callback >> + * function be run. > > That logic makes no sense at all. > >> +int timed_freeze(struct timed_freeze_ops *ops, void *data, ktime_t delta) >> +{ >> + int cpu = smp_processor_id(); >> + struct tick_device *td = tick_get_device(cpu); >> + struct clock_event_device *dev; >> + int ret; >> + >> + if (!ops || !ops->enter_freeze) { >> + printk_deferred(KERN_ERR >> + "[%s] called with invalid ops\n", __func__); >> + return -EINVAL; >> + } >> + >> + if (!td || !td->evtdev || > > td is always valid, because it's a pointer to a per cpu variable. > >> + !(td->evtdev->features & CLOCK_EVT_FEAT_FREEZE)) { >> + printk_deferred(KERN_WARNING >> + "[%s] called with invalid clock event device\n", > > The function is not called with an invalid clock event device. There is either > no clock event device or the device does not support this. > >> + __func__); >> + ret = -ENOSYS; >> + goto freeze_no_check; >> + } >> + >> + dev = td->evtdev; >> + if (!clockevent_state_shutdown(dev)) { >> + printk_deferred(KERN_WARNING >> + "[%s] called while clock event device in use\n", >> + __func__); >> + ret = -EBUSY; >> + goto freeze_no_check; >> + } >> + >> + ret = clockevents_program_freeze_event(dev, delta); >> + if (ret < 0) >> + goto freeze_no_check; >> + >> + ret = ops->enter_freeze(data); >> + if (ret < 0) >> + goto out; >> + >> + if (ops->callback && clockevents_freeze_event_expired(dev)) >> + ret = ops->callback(data); > > This callback thing is just wrong here. If that fails then how is the call > site supposed to figure out where the error came from? From your printks? > > The correct way to do this is: > > int tick_set_frozen_event(ktime_t delta) > { > if (....) > return -EINVAL; > if (....) > return -ENOSYS; > > return program_event(dev, delta); > } > > and: > > int tick_clear_frozen_event() > { > ret = event_expired(dev); > clockevents_shutdown(dev); > return ret; > } > > So no ops, nothing nada. > > And at the call site you do: > > ret = tick_set_frozen_event(delta); > if (ret) > goto deal_with_ret; > > ret = freeze(); > if (ret) { > tick_clear_frozen_event(); > goto deal_with_freeze_abort; > } > > ret = tick_clear_frozen_event(); > > do_something_sensible(ret); > > That's actually understandable and debugable code. > > Thanks, > > tglx > > I'll address these in another patch to be sent out soon. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h index 0d442e3..e1b66d4 100644 --- a/include/linux/clockchips.h +++ b/include/linux/clockchips.h @@ -66,12 +66,20 @@ enum clock_event_state { */ # define CLOCK_EVT_FEAT_HRTIMER 0x000080 +/* + * Clockevent device may run during freeze + */ +# define CLOCK_EVT_FEAT_FREEZE 0x000100 + /** * struct clock_event_device - clock event device descriptor * @event_handler: Assigned by the framework to be called by the low * level handler of the event source * @set_next_event: set next event function using a clocksource delta * @set_next_ktime: set next event function using a direct ktime value + * @event_pending: check if the programmed event is still pending. Used + * for freeze events when timekeeping is suspended and + * irqs are disabled. * @next_event: local storage for the next event in oneshot mode * @max_delta_ns: maximum delta value in ns * @min_delta_ns: minimum delta value in ns @@ -100,7 +108,9 @@ struct clock_event_device { void (*event_handler)(struct clock_event_device *); int (*set_next_event)(unsigned long evt, struct clock_event_device *); int (*set_next_ktime)(ktime_t expires, struct clock_event_device *); + bool (*event_expired)(struct clock_event_device *); ktime_t next_event; + bool freeze_event_programmed; u64 max_delta_ns; u64 min_delta_ns; u32 mult; diff --git a/include/linux/suspend.h b/include/linux/suspend.h index 8b6ec7e..d8d4296 100644 --- a/include/linux/suspend.h +++ b/include/linux/suspend.h @@ -194,6 +194,11 @@ struct platform_freeze_ops { void (*end)(void); }; +struct timed_freeze_ops { + int (*enter_freeze)(void *); + int (*callback)(void *); +}; + #ifdef CONFIG_SUSPEND /** * suspend_set_ops - set platform dependent suspend operations @@ -246,6 +251,9 @@ static inline bool idle_should_freeze(void) return unlikely(suspend_freeze_state == FREEZE_STATE_ENTER); } +extern int timed_freeze(struct timed_freeze_ops *ops, void *data, + ktime_t delta); + extern void freeze_set_ops(const struct platform_freeze_ops *ops); extern void freeze_wake(void); @@ -280,6 +288,8 @@ static inline bool pm_resume_via_firmware(void) { return false; } static inline void suspend_set_ops(const struct platform_suspend_ops *ops) {} static inline int pm_suspend(suspend_state_t state) { return -ENOSYS; } static inline bool idle_should_freeze(void) { return false; } +static inline int timed_freeze(struct timed_freeze_ops *ops, void *data, + ktime_t delta) { return -ENOSYS; } static inline void freeze_set_ops(const struct platform_freeze_ops *ops) {} static inline void freeze_wake(void) {} #endif /* !CONFIG_SUSPEND */ diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c index a9b76a4..e7ca673 100644 --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -17,6 +17,7 @@ #include <linux/module.h> #include <linux/smp.h> #include <linux/device.h> +#include <linux/suspend.h> #include "tick-internal.h" @@ -341,6 +342,122 @@ int clockevents_program_event(struct clock_event_device *dev, ktime_t expires, return (rc && force) ? clockevents_program_min_delta(dev) : rc; } +static int clockevents_program_freeze_event(struct clock_event_device *dev, + ktime_t delta) +{ + int64_t delta_ns = ktime_to_ns(delta); + unsigned long long clc; + int ret; + + if (delta_ns > (int64_t) dev->max_delta_ns) { + printk_deferred(KERN_WARNING + "Freeze event time longer than max delta\n"); + delta_ns = (int64_t) dev->max_delta_ns; + } + + clockevents_tick_resume(dev); + clockevents_switch_state(dev, CLOCK_EVT_STATE_ONESHOT); + delta_ns = max_t(int64_t, delta_ns, dev->min_delta_ns); + clc = ((unsigned long long) delta_ns * dev->mult) >> dev->shift; + ret = dev->set_next_event((unsigned long) clc, dev); + if (ret < 0) { + printk_deferred(KERN_WARNING + "Failed to program freeze event\n"); + clockevents_shutdown(dev); + } else { + dev->freeze_event_programmed = true; + } + + return ret; +} + +static bool clockevents_freeze_event_expired(struct clock_event_device *dev) +{ + if (dev->freeze_event_programmed) + return dev->event_expired(dev); + + return false; +} + +static void clockevents_cleanup_freeze_event(struct clock_event_device *dev) +{ + if (!(dev->features & CLOCK_EVT_FEAT_FREEZE)) + return; + + clockevents_shutdown(dev); + dev->freeze_event_programmed = false; +} + +/** + * timed_freeze - Enter freeze on a CPU for a timed duration + * @ops: Pointers for enter freeze and callback functions. + * @data: Pointer to pass arguments to the function pointers. + * @delta: Time to freeze for. If this amount of time passes in freeze, the + * callback in ops will be called. + * + * Returns the value from ops->enter_freeze or ops->callback on success, -EERROR + * otherwise. If an error is encountered while setting up the clock event, + * freeze with still be entered, but it will not be timed nor will the callback + * function be run. + */ +int timed_freeze(struct timed_freeze_ops *ops, void *data, ktime_t delta) +{ + int cpu = smp_processor_id(); + struct tick_device *td = tick_get_device(cpu); + struct clock_event_device *dev; + int ret; + + if (!ops || !ops->enter_freeze) { + printk_deferred(KERN_ERR + "[%s] called with invalid ops\n", __func__); + return -EINVAL; + } + + if (!td || !td->evtdev || + !(td->evtdev->features & CLOCK_EVT_FEAT_FREEZE)) { + printk_deferred(KERN_WARNING + "[%s] called with invalid clock event device\n", + __func__); + ret = -ENOSYS; + goto freeze_no_check; + } + + dev = td->evtdev; + if (!clockevent_state_shutdown(dev)) { + printk_deferred(KERN_WARNING + "[%s] called while clock event device in use\n", + __func__); + ret = -EBUSY; + goto freeze_no_check; + } + + ret = clockevents_program_freeze_event(dev, delta); + if (ret < 0) + goto freeze_no_check; + + ret = ops->enter_freeze(data); + if (ret < 0) + goto out; + + if (ops->callback && clockevents_freeze_event_expired(dev)) + ret = ops->callback(data); + +out: + clockevents_cleanup_freeze_event(dev); + return ret; + +freeze_no_check: + /* + * If an error happens before enter_freeze, enter freeze normally and + * return an error. The called can't tell if freeze should be entered on + * an error (since errors can happen after returning from freeze), so + * just handle it here. + */ + ops->enter_freeze(data); + return ret; +} +EXPORT_SYMBOL_GPL(timed_freeze); + /* * Called after a notify add to make devices available which were * released from the notifier call.