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

[Marten Lindahl <martenli@axis.com>]

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.  
>