[PATCH] iio: light: vcnl4000: Don't create sysfs PM nodes for child

[PATCH] iio: light: vcnl4000: Don't create sysfs PM nodes for child

[Mårten Lindahl]

PM runtime is enabled for the parent i2c device, but it is disabled for
the iio child device and remains so in this driver. But as the child
sysfs PM directory is created by default by iio_device_register =>
cdev_device_add => dpm_sysfs_add it doesn't bring any value exposing it
since it won't give any usable data for the user.

Tell dpm_sysfs to not create the PM directory for the iio device.

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 f6c83ecaad8b..400dc1155559 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -1072,6 +1072,7 @@ static int vcnl4000_probe(struct i2c_client *client,
 	if (ret < 0)
 		goto fail_poweroff;
 
+	device_set_pm_not_required(&indio_dev->dev);
 	ret = iio_device_register(indio_dev);
 	if (ret < 0)
 		goto fail_poweroff;
-- 
2.30.2

Re: [PATCH] iio: light: vcnl4000: Don't create sysfs PM nodes for child

[Jonathan Cameron]

On Tue, 1 Nov 2022 13:21:51 +0100
Mårten Lindahl <marten.lindahl@axis.com> wrote:

> PM runtime is enabled for the parent i2c device, but it is disabled for
> the iio child device and remains so in this driver. But as the child
> sysfs PM directory is created by default by iio_device_register =>
> cdev_device_add => dpm_sysfs_add it doesn't bring any value exposing it
> since it won't give any usable data for the user.
> 
> Tell dpm_sysfs to not create the PM directory for the iio device.
> 
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>

Ah this one..  +CC Sudeep who I am sure has considered such changes in other
similar cases (he did the original ones that mean this infastructure exists).

So, the problem here is that it drops an ABI that has existed in IIO for
a long time. I fully admit the ABI is confusing and pointless.  What
I'm not sure on is that we are entirely safe to remove it.

If we are going to do this, it shouldn't be on a per driver basis, but rather
a global change for all IIO drivers with the proviso that we are relying on
the 'if no one notices an ABI change, it is fine' exception to never changing
userspace ABI.

Until now this has more or less only been used for new subsystems or where
there were warnings generated due to spurious interface usage in normal flows.

In IIO, nothing should touch these and I 'think' they don't do anything if
anyone pokes them.

Jonathan


> ---
>  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 f6c83ecaad8b..400dc1155559 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -1072,6 +1072,7 @@ static int vcnl4000_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		goto fail_poweroff;
>  
> +	device_set_pm_not_required(&indio_dev->dev);
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0)
>  		goto fail_poweroff;

Re: [PATCH] iio: light: vcnl4000: Don't create sysfs PM nodes for child

[Marten Lindahl]

On Sun, Nov 06, 2022 at 04:23:07PM +0100, Jonathan Cameron wrote:
> On Tue, 1 Nov 2022 13:21:51 +0100
> Mårten Lindahl <marten.lindahl@axis.com> wrote:
> 
> > PM runtime is enabled for the parent i2c device, but it is disabled for
> > the iio child device and remains so in this driver. But as the child
> > sysfs PM directory is created by default by iio_device_register =>
> > cdev_device_add => dpm_sysfs_add it doesn't bring any value exposing it
> > since it won't give any usable data for the user.
> > 
> > Tell dpm_sysfs to not create the PM directory for the iio device.
> > 
> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> 
> Ah this one..  +CC Sudeep who I am sure has considered such changes in other
> similar cases (he did the original ones that mean this infastructure exists).
> 
> So, the problem here is that it drops an ABI that has existed in IIO for
> a long time. I fully admit the ABI is confusing and pointless.  What
> I'm not sure on is that we are entirely safe to remove it.
> 
> If we are going to do this, it shouldn't be on a per driver basis, but rather
> a global change for all IIO drivers with the proviso that we are relying on
> the 'if no one notices an ABI change, it is fine' exception to never changing
> userspace ABI.
> 
> Until now this has more or less only been used for new subsystems or where
> there were warnings generated due to spurious interface usage in normal flows.
> 
> In IIO, nothing should touch these and I 'think' they don't do anything if
> anyone pokes them.
> 
> Jonathan

Hi Jonathan!

Thanks. I'll drop this change then.

Kind regards
Mårten
> 
> 
> > ---
> >  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 f6c83ecaad8b..400dc1155559 100644
> > --- a/drivers/iio/light/vcnl4000.c
> > +++ b/drivers/iio/light/vcnl4000.c
> > @@ -1072,6 +1072,7 @@ static int vcnl4000_probe(struct i2c_client *client,
> >  	if (ret < 0)
> >  		goto fail_poweroff;
> >  
> > +	device_set_pm_not_required(&indio_dev->dev);
> >  	ret = iio_device_register(indio_dev);
> >  	if (ret < 0)
> >  		goto fail_poweroff;
> 

Re: [PATCH] iio: light: vcnl4000: Don't create sysfs PM nodes for child

[Sudeep Holla]

On Sun, Nov 06, 2022 at 03:23:07PM +0000, Jonathan Cameron wrote:
> On Tue, 1 Nov 2022 13:21:51 +0100
> Mårten Lindahl <marten.lindahl@axis.com> wrote:
> 
> > PM runtime is enabled for the parent i2c device, but it is disabled for
> > the iio child device and remains so in this driver. But as the child
> > sysfs PM directory is created by default by iio_device_register =>
> > cdev_device_add => dpm_sysfs_add it doesn't bring any value exposing it
> > since it won't give any usable data for the user.
> > 
> > Tell dpm_sysfs to not create the PM directory for the iio device.
> > 
> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> 
> Ah this one..  +CC Sudeep who I am sure has considered such changes in other
> similar cases (he did the original ones that mean this infastructure exists).
>

Yes it was added for cache devices which are child devices of CPU devices via
Commit 85945c28b5a8 ("PM / core: Add support to skip power management in device/driver model")

Since CPU and its children are power managed quite differently(via cpuidle
or cpu hotplug), the new API(device_set_pm_not_required) fits the usecase
there well. I am not sure about this IIO usecase.

-- 
Regards,
Sudeep

Re: [PATCH] iio: light: vcnl4000: Don't create sysfs PM nodes for child

[Jonathan Cameron]

On Mon, 7 Nov 2022 10:36:36 +0000
Sudeep Holla <sudeep.holla@arm.com> wrote:

> On Sun, Nov 06, 2022 at 03:23:07PM +0000, Jonathan Cameron wrote:
> > On Tue, 1 Nov 2022 13:21:51 +0100
> > Mårten Lindahl <marten.lindahl@axis.com> wrote:
> >   
> > > PM runtime is enabled for the parent i2c device, but it is disabled for
> > > the iio child device and remains so in this driver. But as the child
> > > sysfs PM directory is created by default by iio_device_register =>
> > > cdev_device_add => dpm_sysfs_add it doesn't bring any value exposing it
> > > since it won't give any usable data for the user.
> > > 
> > > Tell dpm_sysfs to not create the PM directory for the iio device.
> > > 
> > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>  
> > 
> > Ah this one..  +CC Sudeep who I am sure has considered such changes in other
> > similar cases (he did the original ones that mean this infastructure exists).
> >  
> 
> Yes it was added for cache devices which are child devices of CPU devices via
> Commit 85945c28b5a8 ("PM / core: Add support to skip power management in device/driver model")
> 
> Since CPU and its children are power managed quite differently(via cpuidle
> or cpu hotplug), the new API(device_set_pm_not_required) fits the usecase
> there well. I am not sure about this IIO usecase.
> 

These are effectively pseudo devices for purposes of the driver model, with parents
as the actual physical devices. As such all the PM infrastructure is associate
with the appropriate bus specific device rather than the struct iio_dev->dev.

So I'm fairly sure we should just not expose the sysfs attributes.
This is similar to CXL for which they are only exposed for the PCI devices, not
the bunch of subdevices created.

Jonathan

Re: [PATCH] iio: light: vcnl4000: Don't create sysfs PM nodes for child

[Sudeep Holla]

On Mon, Nov 07, 2022 at 05:08:57PM +0000, Jonathan Cameron wrote:
> On Mon, 7 Nov 2022 10:36:36 +0000
> Sudeep Holla <sudeep.holla@arm.com> wrote:
> 
> > On Sun, Nov 06, 2022 at 03:23:07PM +0000, Jonathan Cameron wrote:
> > > On Tue, 1 Nov 2022 13:21:51 +0100
> > > Mårten Lindahl <marten.lindahl@axis.com> wrote:
> > >   
> > > > PM runtime is enabled for the parent i2c device, but it is disabled for
> > > > the iio child device and remains so in this driver. But as the child
> > > > sysfs PM directory is created by default by iio_device_register =>
> > > > cdev_device_add => dpm_sysfs_add it doesn't bring any value exposing it
> > > > since it won't give any usable data for the user.
> > > > 
> > > > Tell dpm_sysfs to not create the PM directory for the iio device.
> > > > 
> > > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>  
> > > 
> > > Ah this one..  +CC Sudeep who I am sure has considered such changes in other
> > > similar cases (he did the original ones that mean this infastructure exists).
> > >  
> > 
> > Yes it was added for cache devices which are child devices of CPU devices via
> > Commit 85945c28b5a8 ("PM / core: Add support to skip power management in device/driver model")
> > 
> > Since CPU and its children are power managed quite differently(via cpuidle
> > or cpu hotplug), the new API(device_set_pm_not_required) fits the usecase
> > there well. I am not sure about this IIO usecase.
> > 
> 
> These are effectively pseudo devices for purposes of the driver model, with parents
> as the actual physical devices. As such all the PM infrastructure is associate
> with the appropriate bus specific device rather than the struct iio_dev->dev.
>

Ah if there are psuedo devices or companion like devices, then yes I agree
worth not exposing the sysfs.

> So I'm fairly sure we should just not expose the sysfs attributes.

Agreed if it is psuedo device like you mention above.

> This is similar to CXL for which they are only exposed for the PCI devices, not
> the bunch of subdevices created.
> 

OK

-- 
Regards,
Sudeep