2020-07-31 17:04:22

by Rafael J. Wysocki

[permalink] [raw]
Subject: [PATCH] PM: runtime: Add kerneldoc comments to multiple helpers

From: Rafael J. Wysocki <[email protected]>

Add kerneldoc comments to multiple PM-runtime helper functions
defined as static inline wrappers around lower-level routines to
provide quick reference decumentation of their behavior.

Some of them are similar to each other with subtle differences only
and the behavior of some of them may appear as counter-intuitive, so
clarify all that to avoid confusion.

Signed-off-by: Rafael J. Wysocki <[email protected]>
---
include/linux/pm_runtime.h | 246 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 246 insertions(+)

Index: linux-pm/include/linux/pm_runtime.h
===================================================================
--- linux-pm.orig/include/linux/pm_runtime.h
+++ linux-pm/include/linux/pm_runtime.h
@@ -60,58 +60,151 @@ extern void pm_runtime_put_suppliers(str
extern void pm_runtime_new_link(struct device *dev);
extern void pm_runtime_drop_link(struct device *dev);

+/**
+ * pm_runtime_get_if_in_use - Conditionally bump up runtime PM usage counter.
+ * @dev: Target device.
+ *
+ * Increment the runtime PM usage counter of @dev if its runtime PM status is
+ * %RPM_ACTIVE and its runtime PM usage counter is greater than 0.
+ */
static inline int pm_runtime_get_if_in_use(struct device *dev)
{
return pm_runtime_get_if_active(dev, false);
}

+/**
+ * pm_suspend_ignore_children - Set runtime PM behavior regarding children.
+ * @dev: Target device.
+ * @enable: Whether or not to ignore possible dependencies on children.
+ *
+ * The dependencies of @dev on its children will not be taken into account by
+ * the runtime PM framework going forward if @enable is %true, or they will
+ * be taken into account otherwise.
+ */
static inline void pm_suspend_ignore_children(struct device *dev, bool enable)
{
dev->power.ignore_children = enable;
}

+/**
+ * pm_runtime_get_noresume - Bump up runtime PM usage counter of a device.
+ * @dev: Target device.
+ */
static inline void pm_runtime_get_noresume(struct device *dev)
{
atomic_inc(&dev->power.usage_count);
}

+/**
+ * pm_runtime_put_noidle - Drop runtime PM usage counter of a device.
+ * @dev: Target device.
+ *
+ * Decrement the runtime PM usage counter of @dev unless it is 0 already.
+ */
static inline void pm_runtime_put_noidle(struct device *dev)
{
atomic_add_unless(&dev->power.usage_count, -1, 0);
}

+/**
+ * pm_runtime_suspended - Check whether or not a device is runtime-suspended.
+ * @dev: Target device.
+ *
+ * Return %true if runtime PM is enabled for @dev and its runtime PM status is
+ * %RPM_SUSPENDED, or %false otherwise.
+ *
+ * Note that the return value of this function can only be trusted if it is
+ * called under the runtime PM lock of @dev or under conditions in which
+ * runtime PM cannot be either disabled or enabled for @dev and its runtime PM
+ * status cannot change.
+ */
static inline bool pm_runtime_suspended(struct device *dev)
{
return dev->power.runtime_status == RPM_SUSPENDED
&& !dev->power.disable_depth;
}

+/**
+ * pm_runtime_active - Check whether or not a device is runtime-active.
+ * @dev: Target device.
+ *
+ * Return %true if runtime PM is enabled for @dev and its runtime PM status is
+ * %RPM_ACTIVE, or %false otherwise.
+ *
+ * Note that the return value of this function can only be trusted if it is
+ * called under the runtime PM lock of @dev or under conditions in which
+ * runtime PM cannot be either disabled or enabled for @dev and its runtime PM
+ * status cannot change.
+ */
static inline bool pm_runtime_active(struct device *dev)
{
return dev->power.runtime_status == RPM_ACTIVE
|| dev->power.disable_depth;
}

+/**
+ * pm_runtime_status_suspended - Check if runtime PM status is "suspended".
+ * @dev: Target device.
+ *
+ * Return %true if the runtime PM status of @dev is %RPM_SUSPENDED, or %false
+ * otherwise, regardless of whether or not runtime PM has been enabled for @dev.
+ *
+ * Note that the return value of this function can only be trusted if it is
+ * called under the runtime PM lock of @dev or under conditions in which the
+ * runtime PM status of @dev cannot change.
+ */
static inline bool pm_runtime_status_suspended(struct device *dev)
{
return dev->power.runtime_status == RPM_SUSPENDED;
}

+/**
+ * pm_runtime_enabled - Check if runtime PM is enabled.
+ * @dev: Target device.
+ *
+ * Return %true if runtime PM is enabled for @dev or %false otherwise.
+ *
+ * Note that the return value of this function can only be trusted if it is
+ * called under the runtime PM lock of @dev or under conditions in which
+ * runtime PM cannot be either disabled or enabled for @dev.
+ */
static inline bool pm_runtime_enabled(struct device *dev)
{
return !dev->power.disable_depth;
}

+/**
+ * pm_runtime_has_no_callbacks - Check if runtime PM callbacks may be present.
+ * @dev: Target device.
+ *
+ * Return %true if @dev is a special device without runtime PM callbacks or
+ * %false otherwise.
+ */
static inline bool pm_runtime_has_no_callbacks(struct device *dev)
{
return dev->power.no_callbacks;
}

+/**
+ * pm_runtime_mark_last_busy - Update the last access time of a device.
+ * @dev: Target device.
+ *
+ * Update the last access time of @dev used by the runtime PM autosuspend
+ * mechanism to the current time as returned by ktime_get_mono_fast_ns().
+ */
static inline void pm_runtime_mark_last_busy(struct device *dev)
{
WRITE_ONCE(dev->power.last_busy, ktime_get_mono_fast_ns());
}

+/**
+ * pm_runtime_is_irq_safe - Check if runtime PM can work in interrupt context.
+ * @dev: Target device.
+ *
+ * Return %true if @dev has been marked as an "IRQ-safe" device (with respect
+ * to runtime PM), in which case its runtime PM callabcks can be expected to
+ * work correctly when invoked from interrupt handlers.
+ */
static inline bool pm_runtime_is_irq_safe(struct device *dev)
{
return dev->power.irq_safe;
@@ -191,97 +284,250 @@ static inline void pm_runtime_drop_link(

#endif /* !CONFIG_PM */

+/**
+ * pm_runtime_idle - Conditionally set up autosuspend of a device or suspend it.
+ * @dev: Target device.
+ *
+ * Invoke the "idle check" callback of @dev and, depending on its return value,
+ * set up autosuspend of @dev or suspend it (depending on whether or not
+ * autosuspend has been enabled for it).
+ */
static inline int pm_runtime_idle(struct device *dev)
{
return __pm_runtime_idle(dev, 0);
}

+/**
+ * pm_runtime_suspend - Suspend a device synchronously.
+ * @dev: Target device.
+ */
static inline int pm_runtime_suspend(struct device *dev)
{
return __pm_runtime_suspend(dev, 0);
}

+/**
+ * pm_runtime_autosuspend - Set up autosuspend of a device or suspend it.
+ * @dev: Target device.
+ *
+ * Set up autosuspend of @dev or suspend it (depending on whether or not
+ * autosuspend is enabled for it) without engaging its "idle check" callback.
+ */
static inline int pm_runtime_autosuspend(struct device *dev)
{
return __pm_runtime_suspend(dev, RPM_AUTO);
}

+/**
+ * pm_runtime_resume - Resume a device synchronously.
+ * @dev: Target device.
+ */
static inline int pm_runtime_resume(struct device *dev)
{
return __pm_runtime_resume(dev, 0);
}

+/**
+ * pm_request_idle - Queue up "idle check" execution for a device.
+ * @dev: Target device.
+ *
+ * Queue up a work item to run an equivalent of pm_runtime_idle() for @dev
+ * asynchronously.
+ */
static inline int pm_request_idle(struct device *dev)
{
return __pm_runtime_idle(dev, RPM_ASYNC);
}

+/**
+ * pm_request_resume - Queue up runtime-resume of a device.
+ * @dev: Target device.
+ */
static inline int pm_request_resume(struct device *dev)
{
return __pm_runtime_resume(dev, RPM_ASYNC);
}

+/**
+ * pm_request_autosuspend - Queue up autosuspend of a device.
+ * @dev: Target device.
+ *
+ * Queue up a work item to run an equivalent pm_runtime_autosuspend() for @dev
+ * asynchronously.
+ */
static inline int pm_request_autosuspend(struct device *dev)
{
return __pm_runtime_suspend(dev, RPM_ASYNC | RPM_AUTO);
}

+/**
+ * pm_runtime_get - Bump up usage counter and queue up resume of a device.
+ * @dev: Target device.
+ *
+ * Bump up the runtime PM usage counter of @dev and queue up a work item to
+ * carry out runtime-resume of it.
+ */
static inline int pm_runtime_get(struct device *dev)
{
return __pm_runtime_resume(dev, RPM_GET_PUT | RPM_ASYNC);
}

+/**
+ * pm_runtime_get_sync - Bump up usage counter of a device and resume it.
+ * @dev: Target device.
+ *
+ * Bump up the runtime PM usage counter of @dev and carry out runtime-resume of
+ * it synchronously.
+ *
+ * The possible return values of this function are the same as for
+ * pm_runtime_resume() and the runtime PM usage counter of @dev remains
+ * incremented in all cases, even if it returns an error code.
+ */
static inline int pm_runtime_get_sync(struct device *dev)
{
return __pm_runtime_resume(dev, RPM_GET_PUT);
}

+/**
+ * pm_runtime_put - Drop device usage counter and queue up "idle check" if 0.
+ * @dev: Target device.
+ *
+ * Decrement the runtime PM usage counter of @dev and if it turns out to be
+ * equal to 0, queue up a work item for @dev like in pm_request_idle().
+ */
static inline int pm_runtime_put(struct device *dev)
{
return __pm_runtime_idle(dev, RPM_GET_PUT | RPM_ASYNC);
}

+/**
+ * pm_runtime_put_autosuspend - Drop device usage counter and queue autosuspend if 0.
+ * @dev: Target device.
+ *
+ * Decrement the runtime PM usage counter of @dev and if it turns out to be
+ * equal to 0, queue up a work item for @dev like in pm_request_autosuspend().
+ */
static inline int pm_runtime_put_autosuspend(struct device *dev)
{
return __pm_runtime_suspend(dev,
RPM_GET_PUT | RPM_ASYNC | RPM_AUTO);
}

+/**
+ * pm_runtime_put_sync - Drop device usage counter and run "idle check" if 0.
+ * @dev: Target device.
+ *
+ * Decrement the runtime PM usage counter of @dev and if it turns out to be
+ * equal to 0, invoke the "idle check" callback of @dev and, depending on its
+ * return value, set up autosuspend of @dev or suspend it (depending on whether
+ * or not autosuspend has been enabled for it).
+ *
+ * The possible return values of this function are the same as for
+ * pm_runtime_idle() and the runtime PM usage counter of @dev remains
+ * decremented in all cases, even if it returns an error code.
+ */
static inline int pm_runtime_put_sync(struct device *dev)
{
return __pm_runtime_idle(dev, RPM_GET_PUT);
}

+/**
+ * pm_runtime_put_sync_suspend - Drop device usage counter and suspend if 0.
+ * @dev: Target device.
+ *
+ * Decrement the runtime PM usage counter of @dev and if it turns out to be
+ * equal to 0, carry out runtime-suspend of @dev synchronously.
+ *
+ * The possible return values of this function are the same as for
+ * pm_runtime_suspend() and the runtime PM usage counter of @dev remains
+ * decremented in all cases, even if it returns an error code.
+ */
static inline int pm_runtime_put_sync_suspend(struct device *dev)
{
return __pm_runtime_suspend(dev, RPM_GET_PUT);
}

+/**
+ * pm_runtime_put_sync_autosuspend - Drop device usage counter and autosuspend if 0.
+ * @dev: Target device.
+ *
+ * Decrement the runtime PM usage counter of @dev and if it turns out to be
+ * equal to 0, set up autosuspend of @dev or suspend it synchronously (depending
+ * on whether or not autosuspend has been enabled for it).
+ *
+ * The possible return values of this function are the same as for
+ * pm_runtime_autosuspend() and the runtime PM usage counter of @dev remains
+ * decremented in all cases, even if it returns an error code.
+ */
static inline int pm_runtime_put_sync_autosuspend(struct device *dev)
{
return __pm_runtime_suspend(dev, RPM_GET_PUT | RPM_AUTO);
}

+/**
+ * pm_runtime_set_active - Set runtime PM status to "active".
+ * @dev: Target device.
+ *
+ * Set the runtime PM status of @dev to %RPM_ACTIVE and ensure that dependencies
+ * of it will be taken into account.
+ *
+ * It is not valid to call this function for devices with runtime PM enabled.
+ */
static inline int pm_runtime_set_active(struct device *dev)
{
return __pm_runtime_set_status(dev, RPM_ACTIVE);
}

+/**
+ * pm_runtime_set_suspended - Set runtime PM status to "active".
+ * @dev: Target device.
+ *
+ * Set the runtime PM status of @dev to %RPM_SUSPENDED and ensure that
+ * dependencies of it will be taken into account.
+ *
+ * It is not valid to call this function for devices with runtime PM enabled.
+ */
static inline int pm_runtime_set_suspended(struct device *dev)
{
return __pm_runtime_set_status(dev, RPM_SUSPENDED);
}

+/**
+ * pm_runtime_disable - Disable runtime PM for a device.
+ * @dev: Target device.
+ *
+ * Prevent the runtime PM framework from working with @dev (by incrementing its
+ * "blocking" counter).
+ *
+ * For each invocation of this function for @dev there must be a matching
+ * pm_runtime_enable() call in order for runtime PM to be enabled for it.
+ */
static inline void pm_runtime_disable(struct device *dev)
{
__pm_runtime_disable(dev, true);
}

+/**
+ * pm_runtime_use_autosuspend - Allow autosuspend to be used for a device.
+ * @dev: Target device.
+ *
+ * Allow the runtime PM autosuspend mechanism to be used for @dev whenever
+ * requested (or "autosuspend" will be handled as direct runtime-suspend for
+ * it).
+ */
static inline void pm_runtime_use_autosuspend(struct device *dev)
{
__pm_runtime_use_autosuspend(dev, true);
}

+/**
+ * pm_runtime_dont_use_autosuspend - Prevent autosuspend from being used.
+ * @dev: Target device.
+ *
+ * Prevent the runtime PM autosuspend mechanism from being used for @dev which
+ * means that "autosuspend" will be handled as direct runtime-suspend for it
+ * going forward.
+ */
static inline void pm_runtime_dont_use_autosuspend(struct device *dev)
{
__pm_runtime_use_autosuspend(dev, false);




2020-07-31 19:03:55

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH] PM: runtime: Add kerneldoc comments to multiple helpers

On Fri, Jul 31, 2020 at 07:03:26PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Add kerneldoc comments to multiple PM-runtime helper functions
> defined as static inline wrappers around lower-level routines to
> provide quick reference decumentation of their behavior.
>
> Some of them are similar to each other with subtle differences only
> and the behavior of some of them may appear as counter-intuitive, so
> clarify all that to avoid confusion.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>

Acked-by: Alan Stern <[email protected]>

2020-08-03 08:55:26

by Sakari Ailus

[permalink] [raw]
Subject: Re: [PATCH] PM: runtime: Add kerneldoc comments to multiple helpers

Hi Rafael,

Thanks for the patch.

On Fri, Jul 31, 2020 at 07:03:26PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Add kerneldoc comments to multiple PM-runtime helper functions
> defined as static inline wrappers around lower-level routines to
> provide quick reference decumentation of their behavior.
>
> Some of them are similar to each other with subtle differences only
> and the behavior of some of them may appear as counter-intuitive, so
> clarify all that to avoid confusion.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> include/linux/pm_runtime.h | 246 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 246 insertions(+)
>
> Index: linux-pm/include/linux/pm_runtime.h
> ===================================================================
> --- linux-pm.orig/include/linux/pm_runtime.h
> +++ linux-pm/include/linux/pm_runtime.h
> @@ -60,58 +60,151 @@ extern void pm_runtime_put_suppliers(str
> extern void pm_runtime_new_link(struct device *dev);
> extern void pm_runtime_drop_link(struct device *dev);
>
> +/**
> + * pm_runtime_get_if_in_use - Conditionally bump up runtime PM usage counter.
> + * @dev: Target device.
> + *
> + * Increment the runtime PM usage counter of @dev if its runtime PM status is
> + * %RPM_ACTIVE and its runtime PM usage counter is greater than 0.

The implementation of the non-runtime PM variants (used when CONFIG_PM is
disabled) isn't here but I think it'd be nice if their behaviour was also
documented here. pm_runtime_get_if_in_use() returns -EINVAL if CONFIG_PM is
disabled, for instance.

pm_runtime_disable() is defined here but the documentation in corresponding
pm_runtime_enable() in drivers/base/power/runtime.c is rather terse. It'd
be nice to improve that now (or separately).

--
Kind regards,

Sakari Ailus

2020-08-03 11:40:55

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH] PM: runtime: Add kerneldoc comments to multiple helpers

Hi Sakari,

On Mon, Aug 3, 2020 at 10:53 AM Sakari Ailus
<[email protected]> wrote:
>
> Hi Rafael,
>
> Thanks for the patch.
>
> On Fri, Jul 31, 2020 at 07:03:26PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > Add kerneldoc comments to multiple PM-runtime helper functions
> > defined as static inline wrappers around lower-level routines to
> > provide quick reference decumentation of their behavior.
>
> > Some of them are similar to each other with subtle differences only
> > and the behavior of some of them may appear as counter-intuitive, so
> > clarify all that to avoid confusion.
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> > include/linux/pm_runtime.h | 246 +++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 246 insertions(+)
> >
> > Index: linux-pm/include/linux/pm_runtime.h
> > ===================================================================
> > --- linux-pm.orig/include/linux/pm_runtime.h
> > +++ linux-pm/include/linux/pm_runtime.h
> > @@ -60,58 +60,151 @@ extern void pm_runtime_put_suppliers(str
> > extern void pm_runtime_new_link(struct device *dev);
> > extern void pm_runtime_drop_link(struct device *dev);
> >
> > +/**
> > + * pm_runtime_get_if_in_use - Conditionally bump up runtime PM usage counter.
> > + * @dev: Target device.
> > + *
> > + * Increment the runtime PM usage counter of @dev if its runtime PM status is
> > + * %RPM_ACTIVE and its runtime PM usage counter is greater than 0.
>
> The implementation of the non-runtime PM variants (used when CONFIG_PM is
> disabled) isn't here but I think it'd be nice if their behaviour was also
> documented here. pm_runtime_get_if_in_use() returns -EINVAL if CONFIG_PM is
> disabled, for instance.

These kerneldoc comments cover the CONFIG_PM case only. The behavior
for !CONFIG_PM needs to be figured out from the code, if it matters.

I'm not sure why it would matter for pm_runtime_get_if_in_use(), in particular?

> pm_runtime_disable() is defined here but the documentation in corresponding
> pm_runtime_enable() in drivers/base/power/runtime.c is rather terse. It'd
> be nice to improve that now (or separately).

Yes, separately.

Thanks!

2020-08-03 23:06:45

by Sakari Ailus

[permalink] [raw]
Subject: Re: [PATCH] PM: runtime: Add kerneldoc comments to multiple helpers

Hi Rafael,

One more comment below.

On Fri, Jul 31, 2020 at 07:03:26PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Add kerneldoc comments to multiple PM-runtime helper functions
> defined as static inline wrappers around lower-level routines to
> provide quick reference decumentation of their behavior.
>
> Some of them are similar to each other with subtle differences only
> and the behavior of some of them may appear as counter-intuitive, so
> clarify all that to avoid confusion.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> include/linux/pm_runtime.h | 246 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 246 insertions(+)
>
> Index: linux-pm/include/linux/pm_runtime.h
> ===================================================================
> --- linux-pm.orig/include/linux/pm_runtime.h
> +++ linux-pm/include/linux/pm_runtime.h
> @@ -60,58 +60,151 @@ extern void pm_runtime_put_suppliers(str
> extern void pm_runtime_new_link(struct device *dev);
> extern void pm_runtime_drop_link(struct device *dev);
>
> +/**
> + * pm_runtime_get_if_in_use - Conditionally bump up runtime PM usage counter.
> + * @dev: Target device.
> + *
> + * Increment the runtime PM usage counter of @dev if its runtime PM status is
> + * %RPM_ACTIVE and its runtime PM usage counter is greater than 0.
> + */
> static inline int pm_runtime_get_if_in_use(struct device *dev)
> {
> return pm_runtime_get_if_active(dev, false);
> }
>
> +/**
> + * pm_suspend_ignore_children - Set runtime PM behavior regarding children.
> + * @dev: Target device.
> + * @enable: Whether or not to ignore possible dependencies on children.
> + *
> + * The dependencies of @dev on its children will not be taken into account by
> + * the runtime PM framework going forward if @enable is %true, or they will
> + * be taken into account otherwise.
> + */
> static inline void pm_suspend_ignore_children(struct device *dev, bool enable)
> {
> dev->power.ignore_children = enable;
> }
>
> +/**
> + * pm_runtime_get_noresume - Bump up runtime PM usage counter of a device.
> + * @dev: Target device.
> + */
> static inline void pm_runtime_get_noresume(struct device *dev)
> {
> atomic_inc(&dev->power.usage_count);
> }
>
> +/**
> + * pm_runtime_put_noidle - Drop runtime PM usage counter of a device.
> + * @dev: Target device.
> + *
> + * Decrement the runtime PM usage counter of @dev unless it is 0 already.
> + */
> static inline void pm_runtime_put_noidle(struct device *dev)
> {
> atomic_add_unless(&dev->power.usage_count, -1, 0);
> }
>
> +/**
> + * pm_runtime_suspended - Check whether or not a device is runtime-suspended.
> + * @dev: Target device.
> + *
> + * Return %true if runtime PM is enabled for @dev and its runtime PM status is
> + * %RPM_SUSPENDED, or %false otherwise.
> + *
> + * Note that the return value of this function can only be trusted if it is
> + * called under the runtime PM lock of @dev or under conditions in which
> + * runtime PM cannot be either disabled or enabled for @dev and its runtime PM
> + * status cannot change.
> + */
> static inline bool pm_runtime_suspended(struct device *dev)
> {
> return dev->power.runtime_status == RPM_SUSPENDED
> && !dev->power.disable_depth;
> }
>
> +/**
> + * pm_runtime_active - Check whether or not a device is runtime-active.
> + * @dev: Target device.
> + *
> + * Return %true if runtime PM is enabled for @dev and its runtime PM status is
> + * %RPM_ACTIVE, or %false otherwise.
> + *
> + * Note that the return value of this function can only be trusted if it is
> + * called under the runtime PM lock of @dev or under conditions in which
> + * runtime PM cannot be either disabled or enabled for @dev and its runtime PM
> + * status cannot change.
> + */
> static inline bool pm_runtime_active(struct device *dev)
> {
> return dev->power.runtime_status == RPM_ACTIVE
> || dev->power.disable_depth;
> }
>
> +/**
> + * pm_runtime_status_suspended - Check if runtime PM status is "suspended".
> + * @dev: Target device.
> + *
> + * Return %true if the runtime PM status of @dev is %RPM_SUSPENDED, or %false
> + * otherwise, regardless of whether or not runtime PM has been enabled for @dev.
> + *
> + * Note that the return value of this function can only be trusted if it is
> + * called under the runtime PM lock of @dev or under conditions in which the
> + * runtime PM status of @dev cannot change.
> + */
> static inline bool pm_runtime_status_suspended(struct device *dev)
> {
> return dev->power.runtime_status == RPM_SUSPENDED;
> }
>
> +/**
> + * pm_runtime_enabled - Check if runtime PM is enabled.
> + * @dev: Target device.
> + *
> + * Return %true if runtime PM is enabled for @dev or %false otherwise.
> + *
> + * Note that the return value of this function can only be trusted if it is
> + * called under the runtime PM lock of @dev or under conditions in which
> + * runtime PM cannot be either disabled or enabled for @dev.
> + */
> static inline bool pm_runtime_enabled(struct device *dev)
> {
> return !dev->power.disable_depth;
> }
>
> +/**
> + * pm_runtime_has_no_callbacks - Check if runtime PM callbacks may be present.
> + * @dev: Target device.
> + *
> + * Return %true if @dev is a special device without runtime PM callbacks or
> + * %false otherwise.
> + */
> static inline bool pm_runtime_has_no_callbacks(struct device *dev)
> {
> return dev->power.no_callbacks;
> }
>
> +/**
> + * pm_runtime_mark_last_busy - Update the last access time of a device.
> + * @dev: Target device.
> + *
> + * Update the last access time of @dev used by the runtime PM autosuspend
> + * mechanism to the current time as returned by ktime_get_mono_fast_ns().
> + */
> static inline void pm_runtime_mark_last_busy(struct device *dev)
> {
> WRITE_ONCE(dev->power.last_busy, ktime_get_mono_fast_ns());
> }
>
> +/**
> + * pm_runtime_is_irq_safe - Check if runtime PM can work in interrupt context.
> + * @dev: Target device.
> + *
> + * Return %true if @dev has been marked as an "IRQ-safe" device (with respect
> + * to runtime PM), in which case its runtime PM callabcks can be expected to
> + * work correctly when invoked from interrupt handlers.
> + */
> static inline bool pm_runtime_is_irq_safe(struct device *dev)
> {
> return dev->power.irq_safe;
> @@ -191,97 +284,250 @@ static inline void pm_runtime_drop_link(
>
> #endif /* !CONFIG_PM */
>
> +/**
> + * pm_runtime_idle - Conditionally set up autosuspend of a device or suspend it.
> + * @dev: Target device.
> + *
> + * Invoke the "idle check" callback of @dev and, depending on its return value,
> + * set up autosuspend of @dev or suspend it (depending on whether or not
> + * autosuspend has been enabled for it).
> + */
> static inline int pm_runtime_idle(struct device *dev)
> {
> return __pm_runtime_idle(dev, 0);
> }
>
> +/**
> + * pm_runtime_suspend - Suspend a device synchronously.
> + * @dev: Target device.
> + */
> static inline int pm_runtime_suspend(struct device *dev)
> {
> return __pm_runtime_suspend(dev, 0);
> }
>
> +/**
> + * pm_runtime_autosuspend - Set up autosuspend of a device or suspend it.
> + * @dev: Target device.
> + *
> + * Set up autosuspend of @dev or suspend it (depending on whether or not
> + * autosuspend is enabled for it) without engaging its "idle check" callback.
> + */
> static inline int pm_runtime_autosuspend(struct device *dev)
> {
> return __pm_runtime_suspend(dev, RPM_AUTO);
> }
>
> +/**
> + * pm_runtime_resume - Resume a device synchronously.
> + * @dev: Target device.
> + */
> static inline int pm_runtime_resume(struct device *dev)
> {
> return __pm_runtime_resume(dev, 0);
> }
>
> +/**
> + * pm_request_idle - Queue up "idle check" execution for a device.
> + * @dev: Target device.
> + *
> + * Queue up a work item to run an equivalent of pm_runtime_idle() for @dev
> + * asynchronously.
> + */
> static inline int pm_request_idle(struct device *dev)
> {
> return __pm_runtime_idle(dev, RPM_ASYNC);
> }
>
> +/**
> + * pm_request_resume - Queue up runtime-resume of a device.
> + * @dev: Target device.
> + */
> static inline int pm_request_resume(struct device *dev)
> {
> return __pm_runtime_resume(dev, RPM_ASYNC);
> }
>
> +/**
> + * pm_request_autosuspend - Queue up autosuspend of a device.
> + * @dev: Target device.
> + *
> + * Queue up a work item to run an equivalent pm_runtime_autosuspend() for @dev
> + * asynchronously.
> + */
> static inline int pm_request_autosuspend(struct device *dev)
> {
> return __pm_runtime_suspend(dev, RPM_ASYNC | RPM_AUTO);
> }
>
> +/**
> + * pm_runtime_get - Bump up usage counter and queue up resume of a device.
> + * @dev: Target device.
> + *
> + * Bump up the runtime PM usage counter of @dev and queue up a work item to
> + * carry out runtime-resume of it.
> + */
> static inline int pm_runtime_get(struct device *dev)
> {
> return __pm_runtime_resume(dev, RPM_GET_PUT | RPM_ASYNC);
> }
>
> +/**
> + * pm_runtime_get_sync - Bump up usage counter of a device and resume it.
> + * @dev: Target device.
> + *
> + * Bump up the runtime PM usage counter of @dev and carry out runtime-resume of
> + * it synchronously.
> + *
> + * The possible return values of this function are the same as for
> + * pm_runtime_resume() and the runtime PM usage counter of @dev remains

I think it's fine to refer to return values of pm_runtime_resume(), but the
return values of pm_runtime_resume() are not documented above. Could you
add them?

Reviewed-by: Sakari Ailus <[email protected]>

--
Kind regards,

Sakari Ailus

2020-08-03 23:08:19

by Sakari Ailus

[permalink] [raw]
Subject: Re: [PATCH] PM: runtime: Add kerneldoc comments to multiple helpers

Hi Rafael,

On Mon, Aug 03, 2020 at 01:36:52PM +0200, Rafael J. Wysocki wrote:
> Hi Sakari,
>
> On Mon, Aug 3, 2020 at 10:53 AM Sakari Ailus
> <[email protected]> wrote:
> >
> > Hi Rafael,
> >
> > Thanks for the patch.
> >
> > On Fri, Jul 31, 2020 at 07:03:26PM +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <[email protected]>
> > >
> > > Add kerneldoc comments to multiple PM-runtime helper functions
> > > defined as static inline wrappers around lower-level routines to
> > > provide quick reference decumentation of their behavior.
> >
> > > Some of them are similar to each other with subtle differences only
> > > and the behavior of some of them may appear as counter-intuitive, so
> > > clarify all that to avoid confusion.
> > >
> > > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > > ---
> > > include/linux/pm_runtime.h | 246 +++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 246 insertions(+)
> > >
> > > Index: linux-pm/include/linux/pm_runtime.h
> > > ===================================================================
> > > --- linux-pm.orig/include/linux/pm_runtime.h
> > > +++ linux-pm/include/linux/pm_runtime.h
> > > @@ -60,58 +60,151 @@ extern void pm_runtime_put_suppliers(str
> > > extern void pm_runtime_new_link(struct device *dev);
> > > extern void pm_runtime_drop_link(struct device *dev);
> > >
> > > +/**
> > > + * pm_runtime_get_if_in_use - Conditionally bump up runtime PM usage counter.
> > > + * @dev: Target device.
> > > + *
> > > + * Increment the runtime PM usage counter of @dev if its runtime PM status is
> > > + * %RPM_ACTIVE and its runtime PM usage counter is greater than 0.
> >
> > The implementation of the non-runtime PM variants (used when CONFIG_PM is
> > disabled) isn't here but I think it'd be nice if their behaviour was also
> > documented here. pm_runtime_get_if_in_use() returns -EINVAL if CONFIG_PM is
> > disabled, for instance.
>
> These kerneldoc comments cover the CONFIG_PM case only. The behavior
> for !CONFIG_PM needs to be figured out from the code, if it matters.
>
> I'm not sure why it would matter for pm_runtime_get_if_in_use(), in particular?

Just as an example. It depends on the use case, but there have been bugs
related to these (e.g. commit 4d471563d87b2b83e73b8abffb9273950e6d2e36),
likely at least partly because it's extra manual work to figure out what a
given API function could return when it's not documented.

>
> > pm_runtime_disable() is defined here but the documentation in corresponding
> > pm_runtime_enable() in drivers/base/power/runtime.c is rather terse. It'd
> > be nice to improve that now (or separately).
>
> Yes, separately.

Thanks!

--
Regards,

Sakari Ailus

2020-08-04 10:07:06

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH] PM: runtime: Add kerneldoc comments to multiple helpers

Hi Sakari,

On Tue, Aug 4, 2020 at 1:03 AM Sakari Ailus
<[email protected]> wrote:
>
> Hi Rafael,
>
> One more comment below.
>
> On Fri, Jul 31, 2020 at 07:03:26PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > Add kerneldoc comments to multiple PM-runtime helper functions
> > defined as static inline wrappers around lower-level routines to
> > provide quick reference decumentation of their behavior.
> >
> > Some of them are similar to each other with subtle differences only
> > and the behavior of some of them may appear as counter-intuitive, so
> > clarify all that to avoid confusion.
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> > include/linux/pm_runtime.h | 246 +++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 246 insertions(+)
> >
> > Index: linux-pm/include/linux/pm_runtime.h
> > ===================================================================
> > --- linux-pm.orig/include/linux/pm_runtime.h
> > +++ linux-pm/include/linux/pm_runtime.h
> > @@ -60,58 +60,151 @@ extern void pm_runtime_put_suppliers(str
> > extern void pm_runtime_new_link(struct device *dev);
> > extern void pm_runtime_drop_link(struct device *dev);
> >
> > +/**
> > + * pm_runtime_get_if_in_use - Conditionally bump up runtime PM usage counter.
> > + * @dev: Target device.
> > + *
> > + * Increment the runtime PM usage counter of @dev if its runtime PM status is
> > + * %RPM_ACTIVE and its runtime PM usage counter is greater than 0.
> > + */
> > static inline int pm_runtime_get_if_in_use(struct device *dev)
> > {
> > return pm_runtime_get_if_active(dev, false);
> > }
> >
> > +/**
> > + * pm_suspend_ignore_children - Set runtime PM behavior regarding children.
> > + * @dev: Target device.
> > + * @enable: Whether or not to ignore possible dependencies on children.
> > + *
> > + * The dependencies of @dev on its children will not be taken into account by
> > + * the runtime PM framework going forward if @enable is %true, or they will
> > + * be taken into account otherwise.
> > + */
> > static inline void pm_suspend_ignore_children(struct device *dev, bool enable)
> > {
> > dev->power.ignore_children = enable;
> > }
> >
> > +/**
> > + * pm_runtime_get_noresume - Bump up runtime PM usage counter of a device.
> > + * @dev: Target device.
> > + */
> > static inline void pm_runtime_get_noresume(struct device *dev)
> > {
> > atomic_inc(&dev->power.usage_count);
> > }
> >
> > +/**
> > + * pm_runtime_put_noidle - Drop runtime PM usage counter of a device.
> > + * @dev: Target device.
> > + *
> > + * Decrement the runtime PM usage counter of @dev unless it is 0 already.
> > + */
> > static inline void pm_runtime_put_noidle(struct device *dev)
> > {
> > atomic_add_unless(&dev->power.usage_count, -1, 0);
> > }
> >
> > +/**
> > + * pm_runtime_suspended - Check whether or not a device is runtime-suspended.
> > + * @dev: Target device.
> > + *
> > + * Return %true if runtime PM is enabled for @dev and its runtime PM status is
> > + * %RPM_SUSPENDED, or %false otherwise.
> > + *
> > + * Note that the return value of this function can only be trusted if it is
> > + * called under the runtime PM lock of @dev or under conditions in which
> > + * runtime PM cannot be either disabled or enabled for @dev and its runtime PM
> > + * status cannot change.
> > + */
> > static inline bool pm_runtime_suspended(struct device *dev)
> > {
> > return dev->power.runtime_status == RPM_SUSPENDED
> > && !dev->power.disable_depth;
> > }
> >
> > +/**
> > + * pm_runtime_active - Check whether or not a device is runtime-active.
> > + * @dev: Target device.
> > + *
> > + * Return %true if runtime PM is enabled for @dev and its runtime PM status is
> > + * %RPM_ACTIVE, or %false otherwise.
> > + *
> > + * Note that the return value of this function can only be trusted if it is
> > + * called under the runtime PM lock of @dev or under conditions in which
> > + * runtime PM cannot be either disabled or enabled for @dev and its runtime PM
> > + * status cannot change.
> > + */
> > static inline bool pm_runtime_active(struct device *dev)
> > {
> > return dev->power.runtime_status == RPM_ACTIVE
> > || dev->power.disable_depth;
> > }
> >
> > +/**
> > + * pm_runtime_status_suspended - Check if runtime PM status is "suspended".
> > + * @dev: Target device.
> > + *
> > + * Return %true if the runtime PM status of @dev is %RPM_SUSPENDED, or %false
> > + * otherwise, regardless of whether or not runtime PM has been enabled for @dev.
> > + *
> > + * Note that the return value of this function can only be trusted if it is
> > + * called under the runtime PM lock of @dev or under conditions in which the
> > + * runtime PM status of @dev cannot change.
> > + */
> > static inline bool pm_runtime_status_suspended(struct device *dev)
> > {
> > return dev->power.runtime_status == RPM_SUSPENDED;
> > }
> >
> > +/**
> > + * pm_runtime_enabled - Check if runtime PM is enabled.
> > + * @dev: Target device.
> > + *
> > + * Return %true if runtime PM is enabled for @dev or %false otherwise.
> > + *
> > + * Note that the return value of this function can only be trusted if it is
> > + * called under the runtime PM lock of @dev or under conditions in which
> > + * runtime PM cannot be either disabled or enabled for @dev.
> > + */
> > static inline bool pm_runtime_enabled(struct device *dev)
> > {
> > return !dev->power.disable_depth;
> > }
> >
> > +/**
> > + * pm_runtime_has_no_callbacks - Check if runtime PM callbacks may be present.
> > + * @dev: Target device.
> > + *
> > + * Return %true if @dev is a special device without runtime PM callbacks or
> > + * %false otherwise.
> > + */
> > static inline bool pm_runtime_has_no_callbacks(struct device *dev)
> > {
> > return dev->power.no_callbacks;
> > }
> >
> > +/**
> > + * pm_runtime_mark_last_busy - Update the last access time of a device.
> > + * @dev: Target device.
> > + *
> > + * Update the last access time of @dev used by the runtime PM autosuspend
> > + * mechanism to the current time as returned by ktime_get_mono_fast_ns().
> > + */
> > static inline void pm_runtime_mark_last_busy(struct device *dev)
> > {
> > WRITE_ONCE(dev->power.last_busy, ktime_get_mono_fast_ns());
> > }
> >
> > +/**
> > + * pm_runtime_is_irq_safe - Check if runtime PM can work in interrupt context.
> > + * @dev: Target device.
> > + *
> > + * Return %true if @dev has been marked as an "IRQ-safe" device (with respect
> > + * to runtime PM), in which case its runtime PM callabcks can be expected to
> > + * work correctly when invoked from interrupt handlers.
> > + */
> > static inline bool pm_runtime_is_irq_safe(struct device *dev)
> > {
> > return dev->power.irq_safe;
> > @@ -191,97 +284,250 @@ static inline void pm_runtime_drop_link(
> >
> > #endif /* !CONFIG_PM */
> >
> > +/**
> > + * pm_runtime_idle - Conditionally set up autosuspend of a device or suspend it.
> > + * @dev: Target device.
> > + *
> > + * Invoke the "idle check" callback of @dev and, depending on its return value,
> > + * set up autosuspend of @dev or suspend it (depending on whether or not
> > + * autosuspend has been enabled for it).
> > + */
> > static inline int pm_runtime_idle(struct device *dev)
> > {
> > return __pm_runtime_idle(dev, 0);
> > }
> >
> > +/**
> > + * pm_runtime_suspend - Suspend a device synchronously.
> > + * @dev: Target device.
> > + */
> > static inline int pm_runtime_suspend(struct device *dev)
> > {
> > return __pm_runtime_suspend(dev, 0);
> > }
> >
> > +/**
> > + * pm_runtime_autosuspend - Set up autosuspend of a device or suspend it.
> > + * @dev: Target device.
> > + *
> > + * Set up autosuspend of @dev or suspend it (depending on whether or not
> > + * autosuspend is enabled for it) without engaging its "idle check" callback.
> > + */
> > static inline int pm_runtime_autosuspend(struct device *dev)
> > {
> > return __pm_runtime_suspend(dev, RPM_AUTO);
> > }
> >
> > +/**
> > + * pm_runtime_resume - Resume a device synchronously.
> > + * @dev: Target device.
> > + */
> > static inline int pm_runtime_resume(struct device *dev)
> > {
> > return __pm_runtime_resume(dev, 0);
> > }
> >
> > +/**
> > + * pm_request_idle - Queue up "idle check" execution for a device.
> > + * @dev: Target device.
> > + *
> > + * Queue up a work item to run an equivalent of pm_runtime_idle() for @dev
> > + * asynchronously.
> > + */
> > static inline int pm_request_idle(struct device *dev)
> > {
> > return __pm_runtime_idle(dev, RPM_ASYNC);
> > }
> >
> > +/**
> > + * pm_request_resume - Queue up runtime-resume of a device.
> > + * @dev: Target device.
> > + */
> > static inline int pm_request_resume(struct device *dev)
> > {
> > return __pm_runtime_resume(dev, RPM_ASYNC);
> > }
> >
> > +/**
> > + * pm_request_autosuspend - Queue up autosuspend of a device.
> > + * @dev: Target device.
> > + *
> > + * Queue up a work item to run an equivalent pm_runtime_autosuspend() for @dev
> > + * asynchronously.
> > + */
> > static inline int pm_request_autosuspend(struct device *dev)
> > {
> > return __pm_runtime_suspend(dev, RPM_ASYNC | RPM_AUTO);
> > }
> >
> > +/**
> > + * pm_runtime_get - Bump up usage counter and queue up resume of a device.
> > + * @dev: Target device.
> > + *
> > + * Bump up the runtime PM usage counter of @dev and queue up a work item to
> > + * carry out runtime-resume of it.
> > + */
> > static inline int pm_runtime_get(struct device *dev)
> > {
> > return __pm_runtime_resume(dev, RPM_GET_PUT | RPM_ASYNC);
> > }
> >
> > +/**
> > + * pm_runtime_get_sync - Bump up usage counter of a device and resume it.
> > + * @dev: Target device.
> > + *
> > + * Bump up the runtime PM usage counter of @dev and carry out runtime-resume of
> > + * it synchronously.
> > + *
> > + * The possible return values of this function are the same as for
> > + * pm_runtime_resume() and the runtime PM usage counter of @dev remains
>
> I think it's fine to refer to return values of pm_runtime_resume(), but the
> return values of pm_runtime_resume() are not documented above.

Yes, because it is a wrapper around __pm_runtime_resume() and the
return values of that should be documented in its kerneldoc.

Documenting them in two places would require manual synchronization of
the kerneldoc comments on every future update of them.

> Could you add them?

I'm going to update the __pm_runtime_resume() kerneldoc to list all of them.

> Reviewed-by: Sakari Ailus <[email protected]>

Thank you!

2020-08-04 10:16:05

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH] PM: runtime: Add kerneldoc comments to multiple helpers

Hi Sakari,

On Tue, Aug 4, 2020 at 1:05 AM Sakari Ailus
<[email protected]> wrote:
>
> Hi Rafael,
>
> On Mon, Aug 03, 2020 at 01:36:52PM +0200, Rafael J. Wysocki wrote:
> > Hi Sakari,
> >
> > On Mon, Aug 3, 2020 at 10:53 AM Sakari Ailus
> > <[email protected]> wrote:
> > >
> > > Hi Rafael,
> > >
> > > Thanks for the patch.
> > >
> > > On Fri, Jul 31, 2020 at 07:03:26PM +0200, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <[email protected]>
> > > >
> > > > Add kerneldoc comments to multiple PM-runtime helper functions
> > > > defined as static inline wrappers around lower-level routines to
> > > > provide quick reference decumentation of their behavior.
> > >
> > > > Some of them are similar to each other with subtle differences only
> > > > and the behavior of some of them may appear as counter-intuitive, so
> > > > clarify all that to avoid confusion.
> > > >
> > > > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > > > ---
> > > > include/linux/pm_runtime.h | 246 +++++++++++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 246 insertions(+)
> > > >
> > > > Index: linux-pm/include/linux/pm_runtime.h
> > > > ===================================================================
> > > > --- linux-pm.orig/include/linux/pm_runtime.h
> > > > +++ linux-pm/include/linux/pm_runtime.h
> > > > @@ -60,58 +60,151 @@ extern void pm_runtime_put_suppliers(str
> > > > extern void pm_runtime_new_link(struct device *dev);
> > > > extern void pm_runtime_drop_link(struct device *dev);
> > > >
> > > > +/**
> > > > + * pm_runtime_get_if_in_use - Conditionally bump up runtime PM usage counter.
> > > > + * @dev: Target device.
> > > > + *
> > > > + * Increment the runtime PM usage counter of @dev if its runtime PM status is
> > > > + * %RPM_ACTIVE and its runtime PM usage counter is greater than 0.
> > >
> > > The implementation of the non-runtime PM variants (used when CONFIG_PM is
> > > disabled) isn't here but I think it'd be nice if their behaviour was also
> > > documented here. pm_runtime_get_if_in_use() returns -EINVAL if CONFIG_PM is
> > > disabled, for instance.
> >
> > These kerneldoc comments cover the CONFIG_PM case only. The behavior
> > for !CONFIG_PM needs to be figured out from the code, if it matters.
> >
> > I'm not sure why it would matter for pm_runtime_get_if_in_use(), in particular?
>
> Just as an example. It depends on the use case, but there have been bugs
> related to these (e.g. commit 4d471563d87b2b83e73b8abffb9273950e6d2e36),
> likely at least partly because it's extra manual work to figure out what a
> given API function could return when it's not documented.

If it is a static inline wrapper around another exported function,
whoever uses it should look at the documentation of the function being
wrapped anyway, so IMO it is sufficient to document the return values
in there and also (as stated in another message) this avoids the need
to manually synchronize the kerneldoc comments every time a new return
value is added or removed.

In the particular case above it might be useful to change
pm_runtime_get_if_active() to return bool, make it return "false" if
PM-runtime is disabled for the device and update the callers
accordingly (some of them still appear to be doing the wrong thing).

IOW, it would return "true" only if the usage counter has been
incremented and so it needs to be decremented.

Cheers!

2020-08-05 08:52:25

by Sakari Ailus

[permalink] [raw]
Subject: Re: [PATCH] PM: runtime: Add kerneldoc comments to multiple helpers

Hi Rafael,

On Tue, Aug 04, 2020 at 12:15:03PM +0200, Rafael J. Wysocki wrote:
> Hi Sakari,
>
> On Tue, Aug 4, 2020 at 1:05 AM Sakari Ailus
> <[email protected]> wrote:
> >
> > Hi Rafael,
> >
> > On Mon, Aug 03, 2020 at 01:36:52PM +0200, Rafael J. Wysocki wrote:
> > > Hi Sakari,
> > >
> > > On Mon, Aug 3, 2020 at 10:53 AM Sakari Ailus
> > > <[email protected]> wrote:
> > > >
> > > > Hi Rafael,
> > > >
> > > > Thanks for the patch.
> > > >
> > > > On Fri, Jul 31, 2020 at 07:03:26PM +0200, Rafael J. Wysocki wrote:
> > > > > From: Rafael J. Wysocki <[email protected]>
> > > > >
> > > > > Add kerneldoc comments to multiple PM-runtime helper functions
> > > > > defined as static inline wrappers around lower-level routines to
> > > > > provide quick reference decumentation of their behavior.
> > > >
> > > > > Some of them are similar to each other with subtle differences only
> > > > > and the behavior of some of them may appear as counter-intuitive, so
> > > > > clarify all that to avoid confusion.
> > > > >
> > > > > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > > > > ---
> > > > > include/linux/pm_runtime.h | 246 +++++++++++++++++++++++++++++++++++++++++++++
> > > > > 1 file changed, 246 insertions(+)
> > > > >
> > > > > Index: linux-pm/include/linux/pm_runtime.h
> > > > > ===================================================================
> > > > > --- linux-pm.orig/include/linux/pm_runtime.h
> > > > > +++ linux-pm/include/linux/pm_runtime.h
> > > > > @@ -60,58 +60,151 @@ extern void pm_runtime_put_suppliers(str
> > > > > extern void pm_runtime_new_link(struct device *dev);
> > > > > extern void pm_runtime_drop_link(struct device *dev);
> > > > >
> > > > > +/**
> > > > > + * pm_runtime_get_if_in_use - Conditionally bump up runtime PM usage counter.
> > > > > + * @dev: Target device.
> > > > > + *
> > > > > + * Increment the runtime PM usage counter of @dev if its runtime PM status is
> > > > > + * %RPM_ACTIVE and its runtime PM usage counter is greater than 0.
> > > >
> > > > The implementation of the non-runtime PM variants (used when CONFIG_PM is
> > > > disabled) isn't here but I think it'd be nice if their behaviour was also
> > > > documented here. pm_runtime_get_if_in_use() returns -EINVAL if CONFIG_PM is
> > > > disabled, for instance.
> > >
> > > These kerneldoc comments cover the CONFIG_PM case only. The behavior
> > > for !CONFIG_PM needs to be figured out from the code, if it matters.
> > >
> > > I'm not sure why it would matter for pm_runtime_get_if_in_use(), in particular?
> >
> > Just as an example. It depends on the use case, but there have been bugs
> > related to these (e.g. commit 4d471563d87b2b83e73b8abffb9273950e6d2e36),
> > likely at least partly because it's extra manual work to figure out what a
> > given API function could return when it's not documented.
>
> If it is a static inline wrapper around another exported function,
> whoever uses it should look at the documentation of the function being
> wrapped anyway, so IMO it is sufficient to document the return values
> in there and also (as stated in another message) this avoids the need
> to manually synchronize the kerneldoc comments every time a new return
> value is added or removed.
>
> In the particular case above it might be useful to change
> pm_runtime_get_if_active() to return bool, make it return "false" if
> PM-runtime is disabled for the device and update the callers
> accordingly (some of them still appear to be doing the wrong thing).
>
> IOW, it would return "true" only if the usage counter has been
> incremented and so it needs to be decremented.

In the case of above commit, the driver is interested in knowing whether
the device is powered on, and so accessible. That's the case if PM is
disabled, so it should return true. Then we do lose the information whether
the counter was touched. I guess we should keep it as-is.

--
Sakari Ailus

2020-08-05 16:56:30

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH] PM: runtime: Add kerneldoc comments to multiple helpers

On Wednesday, August 5, 2020 10:49:28 AM CEST Sakari Ailus wrote:
> Hi Rafael,
>
> On Tue, Aug 04, 2020 at 12:15:03PM +0200, Rafael J. Wysocki wrote:
> > Hi Sakari,
> >
> > On Tue, Aug 4, 2020 at 1:05 AM Sakari Ailus
> > <[email protected]> wrote:
> > >
> > > Hi Rafael,
> > >
> > > On Mon, Aug 03, 2020 at 01:36:52PM +0200, Rafael J. Wysocki wrote:
> > > > Hi Sakari,
> > > >
> > > > On Mon, Aug 3, 2020 at 10:53 AM Sakari Ailus
> > > > <[email protected]> wrote:
> > > > >
> > > > > Hi Rafael,
> > > > >
> > > > > Thanks for the patch.
> > > > >
> > > > > On Fri, Jul 31, 2020 at 07:03:26PM +0200, Rafael J. Wysocki wrote:
> > > > > > From: Rafael J. Wysocki <[email protected]>
> > > > > >
> > > > > > Add kerneldoc comments to multiple PM-runtime helper functions
> > > > > > defined as static inline wrappers around lower-level routines to
> > > > > > provide quick reference decumentation of their behavior.
> > > > >
> > > > > > Some of them are similar to each other with subtle differences only
> > > > > > and the behavior of some of them may appear as counter-intuitive, so
> > > > > > clarify all that to avoid confusion.
> > > > > >
> > > > > > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > > > > > ---
> > > > > > include/linux/pm_runtime.h | 246 +++++++++++++++++++++++++++++++++++++++++++++
> > > > > > 1 file changed, 246 insertions(+)
> > > > > >
> > > > > > Index: linux-pm/include/linux/pm_runtime.h
> > > > > > ===================================================================
> > > > > > --- linux-pm.orig/include/linux/pm_runtime.h
> > > > > > +++ linux-pm/include/linux/pm_runtime.h
> > > > > > @@ -60,58 +60,151 @@ extern void pm_runtime_put_suppliers(str
> > > > > > extern void pm_runtime_new_link(struct device *dev);
> > > > > > extern void pm_runtime_drop_link(struct device *dev);
> > > > > >
> > > > > > +/**
> > > > > > + * pm_runtime_get_if_in_use - Conditionally bump up runtime PM usage counter.
> > > > > > + * @dev: Target device.
> > > > > > + *
> > > > > > + * Increment the runtime PM usage counter of @dev if its runtime PM status is
> > > > > > + * %RPM_ACTIVE and its runtime PM usage counter is greater than 0.
> > > > >
> > > > > The implementation of the non-runtime PM variants (used when CONFIG_PM is
> > > > > disabled) isn't here but I think it'd be nice if their behaviour was also
> > > > > documented here. pm_runtime_get_if_in_use() returns -EINVAL if CONFIG_PM is
> > > > > disabled, for instance.
> > > >
> > > > These kerneldoc comments cover the CONFIG_PM case only. The behavior
> > > > for !CONFIG_PM needs to be figured out from the code, if it matters.
> > > >
> > > > I'm not sure why it would matter for pm_runtime_get_if_in_use(), in particular?
> > >
> > > Just as an example. It depends on the use case, but there have been bugs
> > > related to these (e.g. commit 4d471563d87b2b83e73b8abffb9273950e6d2e36),
> > > likely at least partly because it's extra manual work to figure out what a
> > > given API function could return when it's not documented.
> >
> > If it is a static inline wrapper around another exported function,
> > whoever uses it should look at the documentation of the function being
> > wrapped anyway, so IMO it is sufficient to document the return values
> > in there and also (as stated in another message) this avoids the need
> > to manually synchronize the kerneldoc comments every time a new return
> > value is added or removed.
> >
> > In the particular case above it might be useful to change
> > pm_runtime_get_if_active() to return bool, make it return "false" if
> > PM-runtime is disabled for the device and update the callers
> > accordingly (some of them still appear to be doing the wrong thing).
> >
> > IOW, it would return "true" only if the usage counter has been
> > incremented and so it needs to be decremented.
>
> In the case of above commit, the driver is interested in knowing whether
> the device is powered on, and so accessible. That's the case if PM is
> disabled, so it should return true. Then we do lose the information whether
> the counter was touched. I guess we should keep it as-is.

Fair enough.



2020-08-12 09:28:34

by Ulf Hansson

[permalink] [raw]
Subject: Re: [PATCH] PM: runtime: Add kerneldoc comments to multiple helpers

On Fri, 31 Jul 2020 at 19:03, Rafael J. Wysocki <[email protected]> wrote:
>
> From: Rafael J. Wysocki <[email protected]>
>
> Add kerneldoc comments to multiple PM-runtime helper functions
> defined as static inline wrappers around lower-level routines to
> provide quick reference decumentation of their behavior.
>
> Some of them are similar to each other with subtle differences only
> and the behavior of some of them may appear as counter-intuitive, so
> clarify all that to avoid confusion.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>

Reviewed-by: Ulf Hansson <[email protected]>

Kind regards
Uffe

> ---
> include/linux/pm_runtime.h | 246 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 246 insertions(+)
>
> Index: linux-pm/include/linux/pm_runtime.h
> ===================================================================
> --- linux-pm.orig/include/linux/pm_runtime.h
> +++ linux-pm/include/linux/pm_runtime.h
> @@ -60,58 +60,151 @@ extern void pm_runtime_put_suppliers(str
> extern void pm_runtime_new_link(struct device *dev);
> extern void pm_runtime_drop_link(struct device *dev);
>
> +/**
> + * pm_runtime_get_if_in_use - Conditionally bump up runtime PM usage counter.
> + * @dev: Target device.
> + *
> + * Increment the runtime PM usage counter of @dev if its runtime PM status is
> + * %RPM_ACTIVE and its runtime PM usage counter is greater than 0.
> + */
> static inline int pm_runtime_get_if_in_use(struct device *dev)
> {
> return pm_runtime_get_if_active(dev, false);
> }
>
> +/**
> + * pm_suspend_ignore_children - Set runtime PM behavior regarding children.
> + * @dev: Target device.
> + * @enable: Whether or not to ignore possible dependencies on children.
> + *
> + * The dependencies of @dev on its children will not be taken into account by
> + * the runtime PM framework going forward if @enable is %true, or they will
> + * be taken into account otherwise.
> + */
> static inline void pm_suspend_ignore_children(struct device *dev, bool enable)
> {
> dev->power.ignore_children = enable;
> }
>
> +/**
> + * pm_runtime_get_noresume - Bump up runtime PM usage counter of a device.
> + * @dev: Target device.
> + */
> static inline void pm_runtime_get_noresume(struct device *dev)
> {
> atomic_inc(&dev->power.usage_count);
> }
>
> +/**
> + * pm_runtime_put_noidle - Drop runtime PM usage counter of a device.
> + * @dev: Target device.
> + *
> + * Decrement the runtime PM usage counter of @dev unless it is 0 already.
> + */
> static inline void pm_runtime_put_noidle(struct device *dev)
> {
> atomic_add_unless(&dev->power.usage_count, -1, 0);
> }
>
> +/**
> + * pm_runtime_suspended - Check whether or not a device is runtime-suspended.
> + * @dev: Target device.
> + *
> + * Return %true if runtime PM is enabled for @dev and its runtime PM status is
> + * %RPM_SUSPENDED, or %false otherwise.
> + *
> + * Note that the return value of this function can only be trusted if it is
> + * called under the runtime PM lock of @dev or under conditions in which
> + * runtime PM cannot be either disabled or enabled for @dev and its runtime PM
> + * status cannot change.
> + */
> static inline bool pm_runtime_suspended(struct device *dev)
> {
> return dev->power.runtime_status == RPM_SUSPENDED
> && !dev->power.disable_depth;
> }
>
> +/**
> + * pm_runtime_active - Check whether or not a device is runtime-active.
> + * @dev: Target device.
> + *
> + * Return %true if runtime PM is enabled for @dev and its runtime PM status is
> + * %RPM_ACTIVE, or %false otherwise.
> + *
> + * Note that the return value of this function can only be trusted if it is
> + * called under the runtime PM lock of @dev or under conditions in which
> + * runtime PM cannot be either disabled or enabled for @dev and its runtime PM
> + * status cannot change.
> + */
> static inline bool pm_runtime_active(struct device *dev)
> {
> return dev->power.runtime_status == RPM_ACTIVE
> || dev->power.disable_depth;
> }
>
> +/**
> + * pm_runtime_status_suspended - Check if runtime PM status is "suspended".
> + * @dev: Target device.
> + *
> + * Return %true if the runtime PM status of @dev is %RPM_SUSPENDED, or %false
> + * otherwise, regardless of whether or not runtime PM has been enabled for @dev.
> + *
> + * Note that the return value of this function can only be trusted if it is
> + * called under the runtime PM lock of @dev or under conditions in which the
> + * runtime PM status of @dev cannot change.
> + */
> static inline bool pm_runtime_status_suspended(struct device *dev)
> {
> return dev->power.runtime_status == RPM_SUSPENDED;
> }
>
> +/**
> + * pm_runtime_enabled - Check if runtime PM is enabled.
> + * @dev: Target device.
> + *
> + * Return %true if runtime PM is enabled for @dev or %false otherwise.
> + *
> + * Note that the return value of this function can only be trusted if it is
> + * called under the runtime PM lock of @dev or under conditions in which
> + * runtime PM cannot be either disabled or enabled for @dev.
> + */
> static inline bool pm_runtime_enabled(struct device *dev)
> {
> return !dev->power.disable_depth;
> }
>
> +/**
> + * pm_runtime_has_no_callbacks - Check if runtime PM callbacks may be present.
> + * @dev: Target device.
> + *
> + * Return %true if @dev is a special device without runtime PM callbacks or
> + * %false otherwise.
> + */
> static inline bool pm_runtime_has_no_callbacks(struct device *dev)
> {
> return dev->power.no_callbacks;
> }
>
> +/**
> + * pm_runtime_mark_last_busy - Update the last access time of a device.
> + * @dev: Target device.
> + *
> + * Update the last access time of @dev used by the runtime PM autosuspend
> + * mechanism to the current time as returned by ktime_get_mono_fast_ns().
> + */
> static inline void pm_runtime_mark_last_busy(struct device *dev)
> {
> WRITE_ONCE(dev->power.last_busy, ktime_get_mono_fast_ns());
> }
>
> +/**
> + * pm_runtime_is_irq_safe - Check if runtime PM can work in interrupt context.
> + * @dev: Target device.
> + *
> + * Return %true if @dev has been marked as an "IRQ-safe" device (with respect
> + * to runtime PM), in which case its runtime PM callabcks can be expected to
> + * work correctly when invoked from interrupt handlers.
> + */
> static inline bool pm_runtime_is_irq_safe(struct device *dev)
> {
> return dev->power.irq_safe;
> @@ -191,97 +284,250 @@ static inline void pm_runtime_drop_link(
>
> #endif /* !CONFIG_PM */
>
> +/**
> + * pm_runtime_idle - Conditionally set up autosuspend of a device or suspend it.
> + * @dev: Target device.
> + *
> + * Invoke the "idle check" callback of @dev and, depending on its return value,
> + * set up autosuspend of @dev or suspend it (depending on whether or not
> + * autosuspend has been enabled for it).
> + */
> static inline int pm_runtime_idle(struct device *dev)
> {
> return __pm_runtime_idle(dev, 0);
> }
>
> +/**
> + * pm_runtime_suspend - Suspend a device synchronously.
> + * @dev: Target device.
> + */
> static inline int pm_runtime_suspend(struct device *dev)
> {
> return __pm_runtime_suspend(dev, 0);
> }
>
> +/**
> + * pm_runtime_autosuspend - Set up autosuspend of a device or suspend it.
> + * @dev: Target device.
> + *
> + * Set up autosuspend of @dev or suspend it (depending on whether or not
> + * autosuspend is enabled for it) without engaging its "idle check" callback.
> + */
> static inline int pm_runtime_autosuspend(struct device *dev)
> {
> return __pm_runtime_suspend(dev, RPM_AUTO);
> }
>
> +/**
> + * pm_runtime_resume - Resume a device synchronously.
> + * @dev: Target device.
> + */
> static inline int pm_runtime_resume(struct device *dev)
> {
> return __pm_runtime_resume(dev, 0);
> }
>
> +/**
> + * pm_request_idle - Queue up "idle check" execution for a device.
> + * @dev: Target device.
> + *
> + * Queue up a work item to run an equivalent of pm_runtime_idle() for @dev
> + * asynchronously.
> + */
> static inline int pm_request_idle(struct device *dev)
> {
> return __pm_runtime_idle(dev, RPM_ASYNC);
> }
>
> +/**
> + * pm_request_resume - Queue up runtime-resume of a device.
> + * @dev: Target device.
> + */
> static inline int pm_request_resume(struct device *dev)
> {
> return __pm_runtime_resume(dev, RPM_ASYNC);
> }
>
> +/**
> + * pm_request_autosuspend - Queue up autosuspend of a device.
> + * @dev: Target device.
> + *
> + * Queue up a work item to run an equivalent pm_runtime_autosuspend() for @dev
> + * asynchronously.
> + */
> static inline int pm_request_autosuspend(struct device *dev)
> {
> return __pm_runtime_suspend(dev, RPM_ASYNC | RPM_AUTO);
> }
>
> +/**
> + * pm_runtime_get - Bump up usage counter and queue up resume of a device.
> + * @dev: Target device.
> + *
> + * Bump up the runtime PM usage counter of @dev and queue up a work item to
> + * carry out runtime-resume of it.
> + */
> static inline int pm_runtime_get(struct device *dev)
> {
> return __pm_runtime_resume(dev, RPM_GET_PUT | RPM_ASYNC);
> }
>
> +/**
> + * pm_runtime_get_sync - Bump up usage counter of a device and resume it.
> + * @dev: Target device.
> + *
> + * Bump up the runtime PM usage counter of @dev and carry out runtime-resume of
> + * it synchronously.
> + *
> + * The possible return values of this function are the same as for
> + * pm_runtime_resume() and the runtime PM usage counter of @dev remains
> + * incremented in all cases, even if it returns an error code.
> + */
> static inline int pm_runtime_get_sync(struct device *dev)
> {
> return __pm_runtime_resume(dev, RPM_GET_PUT);
> }
>
> +/**
> + * pm_runtime_put - Drop device usage counter and queue up "idle check" if 0.
> + * @dev: Target device.
> + *
> + * Decrement the runtime PM usage counter of @dev and if it turns out to be
> + * equal to 0, queue up a work item for @dev like in pm_request_idle().
> + */
> static inline int pm_runtime_put(struct device *dev)
> {
> return __pm_runtime_idle(dev, RPM_GET_PUT | RPM_ASYNC);
> }
>
> +/**
> + * pm_runtime_put_autosuspend - Drop device usage counter and queue autosuspend if 0.
> + * @dev: Target device.
> + *
> + * Decrement the runtime PM usage counter of @dev and if it turns out to be
> + * equal to 0, queue up a work item for @dev like in pm_request_autosuspend().
> + */
> static inline int pm_runtime_put_autosuspend(struct device *dev)
> {
> return __pm_runtime_suspend(dev,
> RPM_GET_PUT | RPM_ASYNC | RPM_AUTO);
> }
>
> +/**
> + * pm_runtime_put_sync - Drop device usage counter and run "idle check" if 0.
> + * @dev: Target device.
> + *
> + * Decrement the runtime PM usage counter of @dev and if it turns out to be
> + * equal to 0, invoke the "idle check" callback of @dev and, depending on its
> + * return value, set up autosuspend of @dev or suspend it (depending on whether
> + * or not autosuspend has been enabled for it).
> + *
> + * The possible return values of this function are the same as for
> + * pm_runtime_idle() and the runtime PM usage counter of @dev remains
> + * decremented in all cases, even if it returns an error code.
> + */
> static inline int pm_runtime_put_sync(struct device *dev)
> {
> return __pm_runtime_idle(dev, RPM_GET_PUT);
> }
>
> +/**
> + * pm_runtime_put_sync_suspend - Drop device usage counter and suspend if 0.
> + * @dev: Target device.
> + *
> + * Decrement the runtime PM usage counter of @dev and if it turns out to be
> + * equal to 0, carry out runtime-suspend of @dev synchronously.
> + *
> + * The possible return values of this function are the same as for
> + * pm_runtime_suspend() and the runtime PM usage counter of @dev remains
> + * decremented in all cases, even if it returns an error code.
> + */
> static inline int pm_runtime_put_sync_suspend(struct device *dev)
> {
> return __pm_runtime_suspend(dev, RPM_GET_PUT);
> }
>
> +/**
> + * pm_runtime_put_sync_autosuspend - Drop device usage counter and autosuspend if 0.
> + * @dev: Target device.
> + *
> + * Decrement the runtime PM usage counter of @dev and if it turns out to be
> + * equal to 0, set up autosuspend of @dev or suspend it synchronously (depending
> + * on whether or not autosuspend has been enabled for it).
> + *
> + * The possible return values of this function are the same as for
> + * pm_runtime_autosuspend() and the runtime PM usage counter of @dev remains
> + * decremented in all cases, even if it returns an error code.
> + */
> static inline int pm_runtime_put_sync_autosuspend(struct device *dev)
> {
> return __pm_runtime_suspend(dev, RPM_GET_PUT | RPM_AUTO);
> }
>
> +/**
> + * pm_runtime_set_active - Set runtime PM status to "active".
> + * @dev: Target device.
> + *
> + * Set the runtime PM status of @dev to %RPM_ACTIVE and ensure that dependencies
> + * of it will be taken into account.
> + *
> + * It is not valid to call this function for devices with runtime PM enabled.
> + */
> static inline int pm_runtime_set_active(struct device *dev)
> {
> return __pm_runtime_set_status(dev, RPM_ACTIVE);
> }
>
> +/**
> + * pm_runtime_set_suspended - Set runtime PM status to "active".
> + * @dev: Target device.
> + *
> + * Set the runtime PM status of @dev to %RPM_SUSPENDED and ensure that
> + * dependencies of it will be taken into account.
> + *
> + * It is not valid to call this function for devices with runtime PM enabled.
> + */
> static inline int pm_runtime_set_suspended(struct device *dev)
> {
> return __pm_runtime_set_status(dev, RPM_SUSPENDED);
> }
>
> +/**
> + * pm_runtime_disable - Disable runtime PM for a device.
> + * @dev: Target device.
> + *
> + * Prevent the runtime PM framework from working with @dev (by incrementing its
> + * "blocking" counter).
> + *
> + * For each invocation of this function for @dev there must be a matching
> + * pm_runtime_enable() call in order for runtime PM to be enabled for it.
> + */
> static inline void pm_runtime_disable(struct device *dev)
> {
> __pm_runtime_disable(dev, true);
> }
>
> +/**
> + * pm_runtime_use_autosuspend - Allow autosuspend to be used for a device.
> + * @dev: Target device.
> + *
> + * Allow the runtime PM autosuspend mechanism to be used for @dev whenever
> + * requested (or "autosuspend" will be handled as direct runtime-suspend for
> + * it).
> + */
> static inline void pm_runtime_use_autosuspend(struct device *dev)
> {
> __pm_runtime_use_autosuspend(dev, true);
> }
>
> +/**
> + * pm_runtime_dont_use_autosuspend - Prevent autosuspend from being used.
> + * @dev: Target device.
> + *
> + * Prevent the runtime PM autosuspend mechanism from being used for @dev which
> + * means that "autosuspend" will be handled as direct runtime-suspend for it
> + * going forward.
> + */
> static inline void pm_runtime_dont_use_autosuspend(struct device *dev)
> {
> __pm_runtime_use_autosuspend(dev, false);
>
>
>