[PATCH 0/2] Synchronize PM runtime enable state with parent

[PATCH 0/2] Synchronize PM runtime enable state with parent

[Mårten Lindahl]

Hi!

A device that creates a child character device with cdev_device_add by
default creates a PM sysfs group with power attributes for userspace
control. This means that the power attributes monitors the child device
only and thus does not reflect the parent device PM runtime behavior.

But as the PM runtime framework (rpm_suspend/rpm_resume) operates not
only on a single device that has been enabled for runtime PM, but also
on its parent, it should be possible to synchronize the child and the
parent so that the power attribute monitoring reflects the child and
the parent as one.

As an example, if an i2c_client device registers an iio_device, the
iio_device will create sysfs power/attribute nodes for userspace
control. But if the dev_pm_ops with resume/suspend callbacks is
attached to the struct i2c_driver.driver.pm, the PM runtime needs to be
enabled for the i2c_client device and not for the child iio_device.

In this case PM runtime can be enabled for the i2c_client device and
suspend/resume callbacks will be triggered, but the child sysfs power
attributes will be visible but marked as 'unsupported' and can not be
used for control or monitoring. This can be confusing as the sysfs
device node presents the i2c_client and the iio_device as one device.

A solution to this is to enable runtime PM not only for the i2c_device
but for the iio_device child also. As there already exists a link from
the child to its parent and both are enabled, all sysfs
control/monitoring now reflects both devices, which from a userspace
perspective makes more sense.

These patches [1] adds a function to the PM runtime framework to
synchronize the PM runtime enable state of a device with its parent,
and [2] applies it to an iio device, where the described issue exists.

Kind regards
Mårten Lindahl

Mårten Lindahl (2):
  PM: runtime: Synchronize PM runtime enable state with parent
  iio: light: vcnl4000: Incorporate iio_device with PM runtime

 drivers/base/power/runtime.c | 18 ++++++++++++++++++
 drivers/iio/light/vcnl4000.c |  1 +
 include/linux/pm_runtime.h   |  2 ++
 3 files changed, 21 insertions(+)

-- 
2.30.2

[PATCH 1/2] PM: runtime: Synchronize PM runtime enable state with parent

[Mårten Lindahl]

A device that creates a child character device with cdev_device_add by
default create a PM sysfs group with power attributes for userspace
control. This means that the power attributes monitors the child device
only and thus does not reflect the parent device PM runtime behavior.

But as the PM runtime framework (rpm_suspend/rpm_resume) operates not
only on a single device that has been enabled for runtime PM, but also
on its parent, it should be possible to synchronize the child and the
parent so that the power attribute monitoring reflects the child and the
parent as one.

As an example, if an i2c_client device registers an iio_device, the
iio_device will create sysfs power/attribute nodes for userspace
control. But if the dev_pm_ops with resume/suspend callbacks is attached
to the struct i2c_driver.driver.pm, the PM runtime needs to be enabled
for the i2c_client device and not for the child iio_device.

In this case PM runtime can be enabled for the i2c_client device and
suspend/resume callbacks will be triggered, but the child sysfs power
attributes will be visible but marked as 'unsupported' and can not be
used for control or monitoring. This can be confusing as the sysfs
device node presents the i2c_client and the iio_device as one device.

Add a function to synchronize the runtime PM enable state of a device
with its parent. As there already exists a link from the child to its
parent and both are enabled, all sysfs control/monitoring can reflect
both devices, which from a userspace perspective makes more sense.

Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---
 drivers/base/power/runtime.c | 18 ++++++++++++++++++
 include/linux/pm_runtime.h   |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 997be3ac20a7..b202d513684a 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1499,6 +1499,24 @@ void pm_runtime_enable(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(pm_runtime_enable);
 
+/**
+ * pm_runtime_sync_parent_enable - Synchronize enable state.
+ * @dev: Device to handle.
+ *
+ * Synchronize the device enable state with its parent.
+ * NOTE: This function should only be used if the parent will never disable
+ * PM runtime (i.e. calling __pm_runtime_disable()) without telling its child.
+ */
+void pm_runtime_sync_parent_enable(struct device *dev)
+{
+	struct device *parent = dev->parent;
+
+	if (parent && !parent->power.ignore_children
+	    && pm_runtime_enabled(parent))
+		pm_runtime_enable(dev);
+}
+EXPORT_SYMBOL_GPL(pm_runtime_sync_parent_enable);
+
 static void pm_runtime_disable_action(void *data)
 {
 	pm_runtime_dont_use_autosuspend(data);
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 0a41b2dcccad..5054427adbd1 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -89,6 +89,7 @@ extern void pm_runtime_put_suppliers(struct device *dev);
 extern void pm_runtime_new_link(struct device *dev);
 extern void pm_runtime_drop_link(struct device_link *link);
 extern void pm_runtime_release_supplier(struct device_link *link);
+extern void pm_runtime_sync_parent_enable(struct device *dev);
 
 extern int devm_pm_runtime_enable(struct device *dev);
 
@@ -315,6 +316,7 @@ static inline void pm_runtime_put_suppliers(struct device *dev) {}
 static inline void pm_runtime_new_link(struct device *dev) {}
 static inline void pm_runtime_drop_link(struct device_link *link) {}
 static inline void pm_runtime_release_supplier(struct device_link *link) {}
+static inline void pm_runtime_sync_parent_enable(struct device *dev) {}
 
 #endif /* !CONFIG_PM */
 
-- 
2.30.2

[PATCH 2/2] iio: light: vcnl4000: Incorporate iio_device with PM runtime

[Mårten Lindahl]

When the vcnl4000 device registers the iio_device, the iio_device
creates basic sysfs power/attribute nodes, but PM runtime is only
enabled for the vcnl4000 device and not for the child iio_device.

A result of this is that the iio_device sysfs power attributes is marked
as 'unsupported' and can not be used for control or monitoring of the
vcnl4000 device.

To fix this the iio_device should be incorporated with the vcnl4000
device by enabling runtime PM for it.

Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---
 drivers/iio/light/vcnl4000.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index 3db4e26731bb..a904c7c99456 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -1077,6 +1077,7 @@ static int vcnl4000_probe(struct i2c_client *client,
 		goto fail_poweroff;
 
 	pm_runtime_enable(&client->dev);
+	pm_runtime_sync_parent_enable(&indio_dev->dev);
 	pm_runtime_set_autosuspend_delay(&client->dev, VCNL4000_SLEEP_DELAY_MS);
 	pm_runtime_use_autosuspend(&client->dev);
 
-- 
2.30.2

Re: [PATCH 0/2] Synchronize PM runtime enable state with parent

[Jonathan Cameron]

On Thu, 29 Sep 2022 16:46:16 +0200
Mårten Lindahl <marten.lindahl@axis.com> wrote:

> Hi!
> 
> A device that creates a child character device with cdev_device_add by
> default creates a PM sysfs group with power attributes for userspace
> control. This means that the power attributes monitors the child device
> only and thus does not reflect the parent device PM runtime behavior.
> 
> But as the PM runtime framework (rpm_suspend/rpm_resume) operates not
> only on a single device that has been enabled for runtime PM, but also
> on its parent, it should be possible to synchronize the child and the
> parent so that the power attribute monitoring reflects the child and
> the parent as one.
> 
> As an example, if an i2c_client device registers an iio_device, the
> iio_device will create sysfs power/attribute nodes for userspace
> control. But if the dev_pm_ops with resume/suspend callbacks is
> attached to the struct i2c_driver.driver.pm, the PM runtime needs to be
> enabled for the i2c_client device and not for the child iio_device.
> 
> In this case PM runtime can be enabled for the i2c_client device and
> suspend/resume callbacks will be triggered, but the child sysfs power
> attributes will be visible but marked as 'unsupported' and can not be
> used for control or monitoring. This can be confusing as the sysfs
> device node presents the i2c_client and the iio_device as one device.
> 
> A solution to this is to enable runtime PM not only for the i2c_device
> but for the iio_device child also. As there already exists a link from
> the child to its parent and both are enabled, all sysfs
> control/monitoring now reflects both devices, which from a userspace
> perspective makes more sense.
> 
> These patches [1] adds a function to the PM runtime framework to
> synchronize the PM runtime enable state of a device with its parent,
> and [2] applies it to an iio device, where the described issue exists.

Hi Mårten,

This seems sensible to me. With hindsight we should have probably
disabled the pm interface for IIO devices. It's a fun question on 
whether we could do so now without 'breaking' some user.  Obviously
the users wouldn't work today, for the reasons you point out, but
that doesn't mean some program isn't relying on the files being there.

Anyhow, I'm looking forward to input from others on this.

Jonathan


 
> 
> Kind regards
> Mårten Lindahl
> 
> Mårten Lindahl (2):
>   PM: runtime: Synchronize PM runtime enable state with parent
>   iio: light: vcnl4000: Incorporate iio_device with PM runtime
> 
>  drivers/base/power/runtime.c | 18 ++++++++++++++++++
>  drivers/iio/light/vcnl4000.c |  1 +
>  include/linux/pm_runtime.h   |  2 ++
>  3 files changed, 21 insertions(+)
> 

Re: [PATCH 1/2] PM: runtime: Synchronize PM runtime enable state with parent

[Rafael J. Wysocki]

On Thu, Sep 29, 2022 at 4:46 PM Mårten Lindahl <marten.lindahl@axis.com> wrote:
>
> A device that creates a child character device with cdev_device_add by
> default create a PM sysfs group with power attributes for userspace
> control. This means that the power attributes monitors the child device
> only and thus does not reflect the parent device PM runtime behavior.

It looks like device_set_pm_not_required() should be used on the child then.

> But as the PM runtime framework (rpm_suspend/rpm_resume) operates not
> only on a single device that has been enabled for runtime PM, but also
> on its parent, it should be possible to synchronize the child and the
> parent so that the power attribute monitoring reflects the child and the
> parent as one.
>
> As an example, if an i2c_client device registers an iio_device, the
> iio_device will create sysfs power/attribute nodes for userspace
> control. But if the dev_pm_ops with resume/suspend callbacks is attached
> to the struct i2c_driver.driver.pm, the PM runtime needs to be enabled
> for the i2c_client device and not for the child iio_device.
>
> In this case PM runtime can be enabled for the i2c_client device and
> suspend/resume callbacks will be triggered, but the child sysfs power
> attributes will be visible but marked as 'unsupported' and can not be
> used for control or monitoring. This can be confusing as the sysfs
> device node presents the i2c_client and the iio_device as one device.

I don't quite understand the last sentence.

They are separate struct device objects and so they each have a
directory in sysfs, right?

> Add a function to synchronize the runtime PM enable state of a device
> with its parent. As there already exists a link from the child to its
> parent and both are enabled, all sysfs control/monitoring can reflect
> both devices, which from a userspace perspective makes more sense.

Except that user space will be able to change "control" to "on" for
the parent alone AFAICS which still will be confusing.

For devices that are pure software constructs it only makes sense to
expose the PM-runtime interface for them if the plan is to indirectly
control the parent's runtime PM through them.

> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> ---
>  drivers/base/power/runtime.c | 18 ++++++++++++++++++
>  include/linux/pm_runtime.h   |  2 ++
>  2 files changed, 20 insertions(+)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 997be3ac20a7..b202d513684a 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1499,6 +1499,24 @@ void pm_runtime_enable(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(pm_runtime_enable);
>
> +/**
> + * pm_runtime_sync_parent_enable - Synchronize enable state.
> + * @dev: Device to handle.
> + *
> + * Synchronize the device enable state with its parent.
> + * NOTE: This function should only be used if the parent will never disable
> + * PM runtime (i.e. calling __pm_runtime_disable()) without telling its child.
> + */
> +void pm_runtime_sync_parent_enable(struct device *dev)
> +{
> +       struct device *parent = dev->parent;
> +
> +       if (parent && !parent->power.ignore_children
> +           && pm_runtime_enabled(parent))
> +               pm_runtime_enable(dev);
> +}
> +EXPORT_SYMBOL_GPL(pm_runtime_sync_parent_enable);
> +
>  static void pm_runtime_disable_action(void *data)
>  {
>         pm_runtime_dont_use_autosuspend(data);
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index 0a41b2dcccad..5054427adbd1 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -89,6 +89,7 @@ extern void pm_runtime_put_suppliers(struct device *dev);
>  extern void pm_runtime_new_link(struct device *dev);
>  extern void pm_runtime_drop_link(struct device_link *link);
>  extern void pm_runtime_release_supplier(struct device_link *link);
> +extern void pm_runtime_sync_parent_enable(struct device *dev);
>
>  extern int devm_pm_runtime_enable(struct device *dev);
>
> @@ -315,6 +316,7 @@ static inline void pm_runtime_put_suppliers(struct device *dev) {}
>  static inline void pm_runtime_new_link(struct device *dev) {}
>  static inline void pm_runtime_drop_link(struct device_link *link) {}
>  static inline void pm_runtime_release_supplier(struct device_link *link) {}
> +static inline void pm_runtime_sync_parent_enable(struct device *dev) {}
>
>  #endif /* !CONFIG_PM */
>
> --
> 2.30.2
>

Re: [PATCH 1/2] PM: runtime: Synchronize PM runtime enable state with parent

[Marten Lindahl]

On Tue, Oct 25, 2022 at 06:20:10PM +0200, Rafael J. Wysocki wrote:
> On Thu, Sep 29, 2022 at 4:46 PM Mårten Lindahl <marten.lindahl@axis.com> wrote:

Hi! Thanks for your feedback!

> >
> > A device that creates a child character device with cdev_device_add by
> > default create a PM sysfs group with power attributes for userspace
> > control. This means that the power attributes monitors the child device
> > only and thus does not reflect the parent device PM runtime behavior.
> 
> It looks like device_set_pm_not_required() should be used on the child then.
> 
> > But as the PM runtime framework (rpm_suspend/rpm_resume) operates not
> > only on a single device that has been enabled for runtime PM, but also
> > on its parent, it should be possible to synchronize the child and the
> > parent so that the power attribute monitoring reflects the child and the
> > parent as one.
> >
> > As an example, if an i2c_client device registers an iio_device, the
> > iio_device will create sysfs power/attribute nodes for userspace
> > control. But if the dev_pm_ops with resume/suspend callbacks is attached
> > to the struct i2c_driver.driver.pm, the PM runtime needs to be enabled
> > for the i2c_client device and not for the child iio_device.
> >
> > In this case PM runtime can be enabled for the i2c_client device and
> > suspend/resume callbacks will be triggered, but the child sysfs power
> > attributes will be visible but marked as 'unsupported' and can not be
> > used for control or monitoring. This can be confusing as the sysfs
> > device node presents the i2c_client and the iio_device as one device.
> 
> I don't quite understand the last sentence.
> 
> They are separate struct device objects and so they each have a
> directory in sysfs, right?
> 

Yes, they do have separate directories and if using device_set_pm_not_required
on the child it will make it clearer which device is PM runtime regulated, so
I guess that is what should be done.

I think it all depends on where in sysfs the user accesses the device from. My
point with these patches is that the iio_device may be perceived to be an
iio device that should be possible to manually power control, as the power
directory is visble. If looking at it from here:

~# ls /sys/bus/iio/devices/iio:device0/
in_illuminance_raw      in_proximity_raw        power
in_illuminance_scale    name                    subsystem
in_proximity_nearlevel  of_node                 uevent

my idea is to let this power directory inherity the parent power control. But
as you say, it is probably better to not create it at all, as the actual manual
power control can be done here:

~# ls /sys/devices/platform/soc/.../i2c-2/2-0060/
driver       modalias     of_node      subsystem
iio:device1  name         power        uevent

where it is more clear which device (the i2c parent) that can be power
controlled.

> > Add a function to synchronize the runtime PM enable state of a device
> > with its parent. As there already exists a link from the child to its
> > parent and both are enabled, all sysfs control/monitoring can reflect
> > both devices, which from a userspace perspective makes more sense.
> 
> Except that user space will be able to change "control" to "on" for
> the parent alone AFAICS which still will be confusing.

Yes, that is true.
> 
> For devices that are pure software constructs it only makes sense to
> expose the PM-runtime interface for them if the plan is to indirectly
> control the parent's runtime PM through them.

I will abandon this patchset and send a single patch for the iio device.

Kind regards
Mårten
> 
> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> > ---
> >  drivers/base/power/runtime.c | 18 ++++++++++++++++++
> >  include/linux/pm_runtime.h   |  2 ++
> >  2 files changed, 20 insertions(+)
> >
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index 997be3ac20a7..b202d513684a 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -1499,6 +1499,24 @@ void pm_runtime_enable(struct device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(pm_runtime_enable);
> >
> > +/**
> > + * pm_runtime_sync_parent_enable - Synchronize enable state.
> > + * @dev: Device to handle.
> > + *
> > + * Synchronize the device enable state with its parent.
> > + * NOTE: This function should only be used if the parent will never disable
> > + * PM runtime (i.e. calling __pm_runtime_disable()) without telling its child.
> > + */
> > +void pm_runtime_sync_parent_enable(struct device *dev)
> > +{
> > +       struct device *parent = dev->parent;
> > +
> > +       if (parent && !parent->power.ignore_children
> > +           && pm_runtime_enabled(parent))
> > +               pm_runtime_enable(dev);
> > +}
> > +EXPORT_SYMBOL_GPL(pm_runtime_sync_parent_enable);
> > +
> >  static void pm_runtime_disable_action(void *data)
> >  {
> >         pm_runtime_dont_use_autosuspend(data);
> > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> > index 0a41b2dcccad..5054427adbd1 100644
> > --- a/include/linux/pm_runtime.h
> > +++ b/include/linux/pm_runtime.h
> > @@ -89,6 +89,7 @@ extern void pm_runtime_put_suppliers(struct device *dev);
> >  extern void pm_runtime_new_link(struct device *dev);
> >  extern void pm_runtime_drop_link(struct device_link *link);
> >  extern void pm_runtime_release_supplier(struct device_link *link);
> > +extern void pm_runtime_sync_parent_enable(struct device *dev);
> >
> >  extern int devm_pm_runtime_enable(struct device *dev);
> >
> > @@ -315,6 +316,7 @@ static inline void pm_runtime_put_suppliers(struct device *dev) {}
> >  static inline void pm_runtime_new_link(struct device *dev) {}
> >  static inline void pm_runtime_drop_link(struct device_link *link) {}
> >  static inline void pm_runtime_release_supplier(struct device_link *link) {}
> > +static inline void pm_runtime_sync_parent_enable(struct device *dev) {}
> >
> >  #endif /* !CONFIG_PM */
> >
> > --
> > 2.30.2
> >

Re: [PATCH 1/2] PM: runtime: Synchronize PM runtime enable state with parent

[Jonathan Cameron]

On Mon, 31 Oct 2022 17:48:39 +0100
Marten Lindahl <martenli@axis.com> wrote:

> On Tue, Oct 25, 2022 at 06:20:10PM +0200, Rafael J. Wysocki wrote:
> > On Thu, Sep 29, 2022 at 4:46 PM Mårten Lindahl <marten.lindahl@axis.com> wrote:  
> 
> Hi! Thanks for your feedback!
> 
> > >
> > > A device that creates a child character device with cdev_device_add by
> > > default create a PM sysfs group with power attributes for userspace
> > > control. This means that the power attributes monitors the child device
> > > only and thus does not reflect the parent device PM runtime behavior.  
> > 
> > It looks like device_set_pm_not_required() should be used on the child then.
> >   
> > > But as the PM runtime framework (rpm_suspend/rpm_resume) operates not
> > > only on a single device that has been enabled for runtime PM, but also
> > > on its parent, it should be possible to synchronize the child and the
> > > parent so that the power attribute monitoring reflects the child and the
> > > parent as one.
> > >
> > > As an example, if an i2c_client device registers an iio_device, the
> > > iio_device will create sysfs power/attribute nodes for userspace
> > > control. But if the dev_pm_ops with resume/suspend callbacks is attached
> > > to the struct i2c_driver.driver.pm, the PM runtime needs to be enabled
> > > for the i2c_client device and not for the child iio_device.
> > >
> > > In this case PM runtime can be enabled for the i2c_client device and
> > > suspend/resume callbacks will be triggered, but the child sysfs power
> > > attributes will be visible but marked as 'unsupported' and can not be
> > > used for control or monitoring. This can be confusing as the sysfs
> > > device node presents the i2c_client and the iio_device as one device.  
> > 
> > I don't quite understand the last sentence.
> > 
> > They are separate struct device objects and so they each have a
> > directory in sysfs, right?
> >   
> 
> Yes, they do have separate directories and if using device_set_pm_not_required
> on the child it will make it clearer which device is PM runtime regulated, so
> I guess that is what should be done.
> 
> I think it all depends on where in sysfs the user accesses the device from. My
> point with these patches is that the iio_device may be perceived to be an
> iio device that should be possible to manually power control, as the power
> directory is visble. If looking at it from here:
> 
> ~# ls /sys/bus/iio/devices/iio:device0/
> in_illuminance_raw      in_proximity_raw        power
> in_illuminance_scale    name                    subsystem
> in_proximity_nearlevel  of_node                 uevent
> 
> my idea is to let this power directory inherity the parent power control. But
> as you say, it is probably better to not create it at all, as the actual manual
> power control can be done here:
> 
> ~# ls /sys/devices/platform/soc/.../i2c-2/2-0060/
> driver       modalias     of_node      subsystem
> iio:device1  name         power        uevent
> 
> where it is more clear which device (the i2c parent) that can be power
> controlled.
> 
> > > Add a function to synchronize the runtime PM enable state of a device
> > > with its parent. As there already exists a link from the child to its
> > > parent and both are enabled, all sysfs control/monitoring can reflect
> > > both devices, which from a userspace perspective makes more sense.  
> > 
> > Except that user space will be able to change "control" to "on" for
> > the parent alone AFAICS which still will be confusing.  
> 
> Yes, that is true.
> > 
> > For devices that are pure software constructs it only makes sense to
> > expose the PM-runtime interface for them if the plan is to indirectly
> > control the parent's runtime PM through them.  
> 
> I will abandon this patchset and send a single patch for the iio device.

I entirely agree with the statement that these are pointless and should not
be exposed.  However I don't want to see a per device tweak.  If we get
rid of these, we get rid of them for all of the iio:device0/ 
devices (and the various other types of device IIO uses).

The risk here is that, although pointless, some userspace is relying on the
ABI in sysfs.  Do people thing it's worth the gamble of getting rid
of this non functioning interface for the whole of IIO?

So far I think this is only been done for a few similar cases
and for new subsystems.

Jonathan

Re: [PATCH 1/2] PM: runtime: Synchronize PM runtime enable state with parent

[Rafael J. Wysocki]

On Sun, Nov 6, 2022 at 4:33 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 31 Oct 2022 17:48:39 +0100
> Marten Lindahl <martenli@axis.com> wrote:
>
> > On Tue, Oct 25, 2022 at 06:20:10PM +0200, Rafael J. Wysocki wrote:
> > > On Thu, Sep 29, 2022 at 4:46 PM Mårten Lindahl <marten.lindahl@axis.com> wrote:
> >
> > Hi! Thanks for your feedback!
> >
> > > >
> > > > A device that creates a child character device with cdev_device_add by
> > > > default create a PM sysfs group with power attributes for userspace
> > > > control. This means that the power attributes monitors the child device
> > > > only and thus does not reflect the parent device PM runtime behavior.
> > >
> > > It looks like device_set_pm_not_required() should be used on the child then.
> > >
> > > > But as the PM runtime framework (rpm_suspend/rpm_resume) operates not
> > > > only on a single device that has been enabled for runtime PM, but also
> > > > on its parent, it should be possible to synchronize the child and the
> > > > parent so that the power attribute monitoring reflects the child and the
> > > > parent as one.
> > > >
> > > > As an example, if an i2c_client device registers an iio_device, the
> > > > iio_device will create sysfs power/attribute nodes for userspace
> > > > control. But if the dev_pm_ops with resume/suspend callbacks is attached
> > > > to the struct i2c_driver.driver.pm, the PM runtime needs to be enabled
> > > > for the i2c_client device and not for the child iio_device.
> > > >
> > > > In this case PM runtime can be enabled for the i2c_client device and
> > > > suspend/resume callbacks will be triggered, but the child sysfs power
> > > > attributes will be visible but marked as 'unsupported' and can not be
> > > > used for control or monitoring. This can be confusing as the sysfs
> > > > device node presents the i2c_client and the iio_device as one device.
> > >
> > > I don't quite understand the last sentence.
> > >
> > > They are separate struct device objects and so they each have a
> > > directory in sysfs, right?
> > >
> >
> > Yes, they do have separate directories and if using device_set_pm_not_required
> > on the child it will make it clearer which device is PM runtime regulated, so
> > I guess that is what should be done.
> >
> > I think it all depends on where in sysfs the user accesses the device from. My
> > point with these patches is that the iio_device may be perceived to be an
> > iio device that should be possible to manually power control, as the power
> > directory is visble. If looking at it from here:
> >
> > ~# ls /sys/bus/iio/devices/iio:device0/
> > in_illuminance_raw      in_proximity_raw        power
> > in_illuminance_scale    name                    subsystem
> > in_proximity_nearlevel  of_node                 uevent
> >
> > my idea is to let this power directory inherity the parent power control. But
> > as you say, it is probably better to not create it at all, as the actual manual
> > power control can be done here:
> >
> > ~# ls /sys/devices/platform/soc/.../i2c-2/2-0060/
> > driver       modalias     of_node      subsystem
> > iio:device1  name         power        uevent
> >
> > where it is more clear which device (the i2c parent) that can be power
> > controlled.
> >
> > > > Add a function to synchronize the runtime PM enable state of a device
> > > > with its parent. As there already exists a link from the child to its
> > > > parent and both are enabled, all sysfs control/monitoring can reflect
> > > > both devices, which from a userspace perspective makes more sense.
> > >
> > > Except that user space will be able to change "control" to "on" for
> > > the parent alone AFAICS which still will be confusing.
> >
> > Yes, that is true.
> > >
> > > For devices that are pure software constructs it only makes sense to
> > > expose the PM-runtime interface for them if the plan is to indirectly
> > > control the parent's runtime PM through them.
> >
> > I will abandon this patchset and send a single patch for the iio device.
>
> I entirely agree with the statement that these are pointless and should not
> be exposed.  However I don't want to see a per device tweak.  If we get
> rid of these, we get rid of them for all of the iio:device0/
> devices (and the various other types of device IIO uses).
>
> The risk here is that, although pointless, some userspace is relying on the
> ABI in sysfs.  Do people thing it's worth the gamble of getting rid
> of this non functioning interface for the whole of IIO?

I think so.

It is better to avoid exposing garbage to user space even if the scope
of it is limited IMV.

> So far I think this is only been done for a few similar cases
> and for new subsystems.

Re: [PATCH 1/2] PM: runtime: Synchronize PM runtime enable state with parent

[Jonathan Cameron]

On Sun, 6 Nov 2022 18:16:10 +0100
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> On Sun, Nov 6, 2022 at 4:33 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Mon, 31 Oct 2022 17:48:39 +0100
> > Marten Lindahl <martenli@axis.com> wrote:
> >  
> > > On Tue, Oct 25, 2022 at 06:20:10PM +0200, Rafael J. Wysocki wrote:  
> > > > On Thu, Sep 29, 2022 at 4:46 PM Mårten Lindahl <marten.lindahl@axis.com> wrote:  
> > >
> > > Hi! Thanks for your feedback!
> > >  
> > > > >
> > > > > A device that creates a child character device with cdev_device_add by
> > > > > default create a PM sysfs group with power attributes for userspace
> > > > > control. This means that the power attributes monitors the child device
> > > > > only and thus does not reflect the parent device PM runtime behavior.  
> > > >
> > > > It looks like device_set_pm_not_required() should be used on the child then.
> > > >  
> > > > > But as the PM runtime framework (rpm_suspend/rpm_resume) operates not
> > > > > only on a single device that has been enabled for runtime PM, but also
> > > > > on its parent, it should be possible to synchronize the child and the
> > > > > parent so that the power attribute monitoring reflects the child and the
> > > > > parent as one.
> > > > >
> > > > > As an example, if an i2c_client device registers an iio_device, the
> > > > > iio_device will create sysfs power/attribute nodes for userspace
> > > > > control. But if the dev_pm_ops with resume/suspend callbacks is attached
> > > > > to the struct i2c_driver.driver.pm, the PM runtime needs to be enabled
> > > > > for the i2c_client device and not for the child iio_device.
> > > > >
> > > > > In this case PM runtime can be enabled for the i2c_client device and
> > > > > suspend/resume callbacks will be triggered, but the child sysfs power
> > > > > attributes will be visible but marked as 'unsupported' and can not be
> > > > > used for control or monitoring. This can be confusing as the sysfs
> > > > > device node presents the i2c_client and the iio_device as one device.  
> > > >
> > > > I don't quite understand the last sentence.
> > > >
> > > > They are separate struct device objects and so they each have a
> > > > directory in sysfs, right?
> > > >  
> > >
> > > Yes, they do have separate directories and if using device_set_pm_not_required
> > > on the child it will make it clearer which device is PM runtime regulated, so
> > > I guess that is what should be done.
> > >
> > > I think it all depends on where in sysfs the user accesses the device from. My
> > > point with these patches is that the iio_device may be perceived to be an
> > > iio device that should be possible to manually power control, as the power
> > > directory is visble. If looking at it from here:
> > >
> > > ~# ls /sys/bus/iio/devices/iio:device0/
> > > in_illuminance_raw      in_proximity_raw        power
> > > in_illuminance_scale    name                    subsystem
> > > in_proximity_nearlevel  of_node                 uevent
> > >
> > > my idea is to let this power directory inherity the parent power control. But
> > > as you say, it is probably better to not create it at all, as the actual manual
> > > power control can be done here:
> > >
> > > ~# ls /sys/devices/platform/soc/.../i2c-2/2-0060/
> > > driver       modalias     of_node      subsystem
> > > iio:device1  name         power        uevent
> > >
> > > where it is more clear which device (the i2c parent) that can be power
> > > controlled.
> > >  
> > > > > Add a function to synchronize the runtime PM enable state of a device
> > > > > with its parent. As there already exists a link from the child to its
> > > > > parent and both are enabled, all sysfs control/monitoring can reflect
> > > > > both devices, which from a userspace perspective makes more sense.  
> > > >
> > > > Except that user space will be able to change "control" to "on" for
> > > > the parent alone AFAICS which still will be confusing.  
> > >
> > > Yes, that is true.  
> > > >
> > > > For devices that are pure software constructs it only makes sense to
> > > > expose the PM-runtime interface for them if the plan is to indirectly
> > > > control the parent's runtime PM through them.  
> > >
> > > I will abandon this patchset and send a single patch for the iio device.  
> >
> > I entirely agree with the statement that these are pointless and should not
> > be exposed.  However I don't want to see a per device tweak.  If we get
> > rid of these, we get rid of them for all of the iio:device0/
> > devices (and the various other types of device IIO uses).
> >
> > The risk here is that, although pointless, some userspace is relying on the
> > ABI in sysfs.  Do people thing it's worth the gamble of getting rid
> > of this non functioning interface for the whole of IIO?  
> 
> I think so.
> 
> It is better to avoid exposing garbage to user space even if the scope
> of it is limited IMV.

Marten, do you want to spin a patch doing this in the iio core?

If not I'll do so sometime soon.  Given where we are in the cycle,
and the need to keep such a patch up for review for at least a few weeks,
we can look to get it into next early next cycle and hopefully any issues
will become apparent.

Jonathan

> 
> > So far I think this is only been done for a few similar cases
> > and for new subsystems.  

Re: [PATCH 1/2] PM: runtime: Synchronize PM runtime enable state with parent

[Marten Lindahl]

On Sat, Nov 12, 2022 at 04:33:58PM +0100, Jonathan Cameron wrote:
> On Sun, 6 Nov 2022 18:16:10 +0100
> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> 
> > On Sun, Nov 6, 2022 at 4:33 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Mon, 31 Oct 2022 17:48:39 +0100
> > > Marten Lindahl <martenli@axis.com> wrote:
> > >  
> > > > On Tue, Oct 25, 2022 at 06:20:10PM +0200, Rafael J. Wysocki wrote:  
> > > > > On Thu, Sep 29, 2022 at 4:46 PM Mårten Lindahl <marten.lindahl@axis.com> wrote:  
> > > >
> > > > Hi! Thanks for your feedback!
> > > >  
> > > > > >
> > > > > > A device that creates a child character device with cdev_device_add by
> > > > > > default create a PM sysfs group with power attributes for userspace
> > > > > > control. This means that the power attributes monitors the child device
> > > > > > only and thus does not reflect the parent device PM runtime behavior.  
> > > > >
> > > > > It looks like device_set_pm_not_required() should be used on the child then.
> > > > >  
> > > > > > But as the PM runtime framework (rpm_suspend/rpm_resume) operates not
> > > > > > only on a single device that has been enabled for runtime PM, but also
> > > > > > on its parent, it should be possible to synchronize the child and the
> > > > > > parent so that the power attribute monitoring reflects the child and the
> > > > > > parent as one.
> > > > > >
> > > > > > As an example, if an i2c_client device registers an iio_device, the
> > > > > > iio_device will create sysfs power/attribute nodes for userspace
> > > > > > control. But if the dev_pm_ops with resume/suspend callbacks is attached
> > > > > > to the struct i2c_driver.driver.pm, the PM runtime needs to be enabled
> > > > > > for the i2c_client device and not for the child iio_device.
> > > > > >
> > > > > > In this case PM runtime can be enabled for the i2c_client device and
> > > > > > suspend/resume callbacks will be triggered, but the child sysfs power
> > > > > > attributes will be visible but marked as 'unsupported' and can not be
> > > > > > used for control or monitoring. This can be confusing as the sysfs
> > > > > > device node presents the i2c_client and the iio_device as one device.  
> > > > >
> > > > > I don't quite understand the last sentence.
> > > > >
> > > > > They are separate struct device objects and so they each have a
> > > > > directory in sysfs, right?
> > > > >  
> > > >
> > > > Yes, they do have separate directories and if using device_set_pm_not_required
> > > > on the child it will make it clearer which device is PM runtime regulated, so
> > > > I guess that is what should be done.
> > > >
> > > > I think it all depends on where in sysfs the user accesses the device from. My
> > > > point with these patches is that the iio_device may be perceived to be an
> > > > iio device that should be possible to manually power control, as the power
> > > > directory is visble. If looking at it from here:
> > > >
> > > > ~# ls /sys/bus/iio/devices/iio:device0/
> > > > in_illuminance_raw      in_proximity_raw        power
> > > > in_illuminance_scale    name                    subsystem
> > > > in_proximity_nearlevel  of_node                 uevent
> > > >
> > > > my idea is to let this power directory inherity the parent power control. But
> > > > as you say, it is probably better to not create it at all, as the actual manual
> > > > power control can be done here:
> > > >
> > > > ~# ls /sys/devices/platform/soc/.../i2c-2/2-0060/
> > > > driver       modalias     of_node      subsystem
> > > > iio:device1  name         power        uevent
> > > >
> > > > where it is more clear which device (the i2c parent) that can be power
> > > > controlled.
> > > >  
> > > > > > Add a function to synchronize the runtime PM enable state of a device
> > > > > > with its parent. As there already exists a link from the child to its
> > > > > > parent and both are enabled, all sysfs control/monitoring can reflect
> > > > > > both devices, which from a userspace perspective makes more sense.  
> > > > >
> > > > > Except that user space will be able to change "control" to "on" for
> > > > > the parent alone AFAICS which still will be confusing.  
> > > >
> > > > Yes, that is true.  
> > > > >
> > > > > For devices that are pure software constructs it only makes sense to
> > > > > expose the PM-runtime interface for them if the plan is to indirectly
> > > > > control the parent's runtime PM through them.  
> > > >
> > > > I will abandon this patchset and send a single patch for the iio device.  
> > >
> > > I entirely agree with the statement that these are pointless and should not
> > > be exposed.  However I don't want to see a per device tweak.  If we get
> > > rid of these, we get rid of them for all of the iio:device0/
> > > devices (and the various other types of device IIO uses).
> > >
> > > The risk here is that, although pointless, some userspace is relying on the
> > > ABI in sysfs.  Do people thing it's worth the gamble of getting rid
> > > of this non functioning interface for the whole of IIO?  
> > 
> > I think so.
> > 
> > It is better to avoid exposing garbage to user space even if the scope
> > of it is limited IMV.
> 
> Marten, do you want to spin a patch doing this in the iio core?
> 
> If not I'll do so sometime soon.  Given where we are in the cycle,
> and the need to keep such a patch up for review for at least a few weeks,
> we can look to get it into next early next cycle and hopefully any issues
> will become apparent.

Please excuse me Jonathan, I missed this mail. I really do apologize.

Considering a patch for the discussed issue. I think I need to dig into
the core a bit more before I can do it. I only had the vcnl4000 driver
in focus, so if you have an idea of how it should be done in the core,
then I think it's better if you do it.

If yo ask me to look into it I will try to do it, but I'm afraid it will
take some time before I can send anything.

Kind regards
Mårten

> 
> Jonathan
> 
> > 
> > > So far I think this is only been done for a few similar cases
> > > and for new subsystems.  
>