Re: [PATCH 4/7] pm: runtime: Mark last busy stamp in pm_runtime_put_sync_autosuspend()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sakari,

Thank you for the patch.

On Thu, Apr 10, 2025 at 06:31:03PM +0300, Sakari Ailus wrote:
> Set device's last busy timestamp to current time in
> pm_runtime_put_sync_autosuspend().
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I was a bit puzzled by why this function exists. Reading
Documentation/power/runtime_pm.rst answered that question: if I
understand it correctly, the function is meant to be used by code that
doesn't know whether or not autosuspend has been enabled for a device,
such as core code in subsystems.

I looked at usage patterns, and found the function being used in drivers
as well, for instance in drivers/media/i2c/tc358746.c. Given that the
driver unconditionally enabled autosuspend, is this incorrect usage of
the API ?

> ---
>  Documentation/power/runtime_pm.rst |  3 ++-
>  include/linux/pm_runtime.h         | 11 +++++++----
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst
> index e7bbdc66d64c..9c21c913f9cf 100644
> --- a/Documentation/power/runtime_pm.rst
> +++ b/Documentation/power/runtime_pm.rst
> @@ -428,7 +428,8 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h:
>        pm_runtime_suspend(dev) and return its result
>  
>    `int pm_runtime_put_sync_autosuspend(struct device *dev);`
> -    - decrement the device's usage counter; if the result is 0 then run
> +    - set the power.last_busy field to the current time and decrement the
> +      device's usage counter; if the result is 0 then run
>        pm_runtime_autosuspend(dev) and return its result
>  
>    `void pm_runtime_enable(struct device *dev);`
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index 0ade3f75d903..e26caf2c0552 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -645,12 +645,14 @@ static inline int pm_runtime_put_sync_suspend(struct device *dev)
>  }
>  
>  /**
> - * pm_runtime_put_sync_autosuspend - Drop device usage counter and autosuspend if 0.
> + * pm_runtime_put_sync_autosuspend - Update the last access time of a device,
> + * 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).
> + * Update the last access time of @dev, 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 runtime PM usage counter of @dev remains decremented in all cases, even
>   * if it returns an error code.
> @@ -670,6 +672,7 @@ static inline int pm_runtime_put_sync_suspend(struct device *dev)
>   */
>  static inline int pm_runtime_put_sync_autosuspend(struct device *dev)
>  {
> +	pm_runtime_mark_last_busy(dev);
>  	return __pm_runtime_suspend(dev, RPM_GET_PUT | RPM_AUTO);
>  }
>  

-- 
Regards,

Laurent Pinchart