Re: [PATCH 2/2] PM-runtime: allow userspace to monitor runtime_status changes

["Rafael J. Wysocki" <rafael@kernel.org>]

On Sun, Sep 1, 2019 at 4:09 PM Akinobu Mita <akinobu.mita@gmail.com> wrote:
>
> This enables the /sys/devices/.../power/runtime_status attribute to
> allow the user space to get notifications via poll/select when the device
> runtime PM status is changed.
>
> An example use case is to avoid unnecessary accesses for device statistics
> (e.g. diskstats for block devices) while the device is in runtime suspend
> by user space LED device actitity trigger.
>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Dominik Brodowski <linux@dominikbrodowski.net>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  Documentation/ABI/testing/sysfs-devices-power | 2 ++
>  drivers/base/power/power.h                    | 1 +
>  drivers/base/power/runtime.c                  | 1 +
>  drivers/base/power/sysfs.c                    | 5 +++++
>  4 files changed, 9 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-devices-power b/Documentation/ABI/testing/sysfs-devices-power
> index 3e50536..47dc357 100644
> --- a/Documentation/ABI/testing/sysfs-devices-power
> +++ b/Documentation/ABI/testing/sysfs-devices-power
> @@ -269,3 +269,5 @@ Description:
>                 the current runtime PM status of the device, which may be
>                 "suspended", "suspending", "resuming", "active", "error" (fatal
>                 error), or "unsupported" (runtime PM is disabled).
> +               This attribute allows the user space to get notifications via
> +               poll/select when the device runtime PM status is changed.
> diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
> index ec33fbdb..8891bf4 100644
> --- a/drivers/base/power/power.h
> +++ b/drivers/base/power/power.h
> @@ -74,6 +74,7 @@ extern int pm_qos_sysfs_add_flags(struct device *dev);
>  extern void pm_qos_sysfs_remove_flags(struct device *dev);
>  extern int pm_qos_sysfs_add_latency_tolerance(struct device *dev);
>  extern void pm_qos_sysfs_remove_latency_tolerance(struct device *dev);
> +extern void sysfs_notify_runtime_status(struct device *dev);
>
>  #else /* CONFIG_PM */
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index b753355..3a3e413 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -94,6 +94,7 @@ static void __update_runtime_status(struct device *dev, enum rpm_status status)
>  {
>         update_pm_runtime_accounting(dev);
>         dev->power.runtime_status = status;
> +       sysfs_notify_runtime_status(dev);

There are concerns about this.

First off, it adds overhead for devices that change the PM-runtime
status relatively often.  I'm not sure if that's sufficiently
justified.

Second, it is called for status changes from "active" to "suspending"
and from "suspending" to "suspended" (and analogously for resume)
which may not be particularly useful.  At least, user space may not
have enough time to act on such notifications.

Finally, it is racy, because at the time user space does something on
a device PM-runtime status change, it very well may have changed the
other way around already.

>  }
>
>  static u64 rpm_get_accounted_time(struct device *dev, bool suspended)
> diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> index 1b9c281c..e86d8cb 100644
> --- a/drivers/base/power/sysfs.c
> +++ b/drivers/base/power/sysfs.c
> @@ -734,3 +734,8 @@ void dpm_sysfs_remove(struct device *dev)
>         sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
>         sysfs_remove_group(&dev->kobj, &pm_attr_group);
>  }
> +
> +void sysfs_notify_runtime_status(struct device *dev)
> +{
> +       sysfs_notify(&dev->kobj, "power", "runtime_status");
> +}
> --
> 2.7.4
>