diff mbox series

[4/5] iio: light: lm3533-als: remove explicit parent assignment

Message ID 20200522082208.383631-4-alexandru.ardelean@analog.com (mailing list archive)
State New, archived
Headers show
Series [1/5] iio: core: pass parent device as parameter during allocation | expand

Commit Message

Alexandru Ardelean May 22, 2020, 8:22 a.m. UTC
This assignment is the more peculiar of the bunch as it assigns the parent
of the platform-device's device (i.e. pdev->dev.parent) as the IIO device's
parent.

It's unclear whether this is intentional or not.
Hence it is in it's own patch.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/light/lm3533-als.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Johan Hovold May 29, 2020, 10:16 a.m. UTC | #1
On Fri, May 22, 2020 at 11:22:07AM +0300, Alexandru Ardelean wrote:
> This assignment is the more peculiar of the bunch as it assigns the parent
> of the platform-device's device (i.e. pdev->dev.parent) as the IIO device's
> parent.
>
> It's unclear whether this is intentional or not.
> Hence it is in it's own patch.

Yeah, we have a few mfd drivers whose child drivers registers their
class devices directly under the parent mfd device rather than the
corresponding child platform device.

Since it's done consistently I think you need to update them all if you
really want to change this. 

And it may not be worth it since at least in theory someone could now be
relying on this topology.

> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/iio/light/lm3533-als.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> index bc196c212881..0f380ec8d30c 100644
> --- a/drivers/iio/light/lm3533-als.c
> +++ b/drivers/iio/light/lm3533-als.c
> @@ -852,7 +852,6 @@ static int lm3533_als_probe(struct platform_device *pdev)
>  	indio_dev->channels = lm3533_als_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(lm3533_als_channels);
>  	indio_dev->name = dev_name(&pdev->dev);
> -	indio_dev->dev.parent = pdev->dev.parent;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
>  	als = iio_priv(indio_dev);

Johan
Johan Hovold May 29, 2020, 1:45 p.m. UTC | #2
[ Trimming CC to something more reasonable... ]

On Fri, May 29, 2020 at 11:08:38AM +0000, Ardelean, Alexandru wrote:
> On Fri, 2020-05-29 at 12:16 +0200, Johan Hovold wrote:
> > On Fri, May 22, 2020 at 11:22:07AM +0300, Alexandru Ardelean wrote:
> > > This assignment is the more peculiar of the bunch as it assigns the parent
> > > of the platform-device's device (i.e. pdev->dev.parent) as the IIO device's
> > > parent.
> > > 
> > > It's unclear whether this is intentional or not.
> > > Hence it is in it's own patch.
> > 
> > Yeah, we have a few mfd drivers whose child drivers registers their
> > class devices directly under the parent mfd device rather than the
> > corresponding child platform device.
> > 
> > Since it's done consistently I think you need to update them all if you
> > really want to change this. 
> > 
> > And it may not be worth it since at least in theory someone could now be
> > relying on this topology.
> 
> Thanks for the feedback.
> I guess, it could make sense to do here:
>       devm_iio_device_alloc(pdev->dev.parent, ...)
> 
> Currently it's:
>       devm_iio_device_alloc(&pdev->dev, ...)
> 
> That would make it slightly more consistent.  i.e. the life-time of
> the object would be attached to the parent of the platform device,
> versus the platform-device.

Not really. If you unbind the iio driver, the iio device gets
deregistered (as it should be) and there's no need to keep it around any
more.

You'd essentially just leak resources every time you rebind the driver
(e.g. during development).

And in fact, you could also introduce a use-after-free depending on if
the parent mfd driver use devres to deregister its children.

> Currently, as it is, the allocation [of the IIO device] is tied the
> platform-device, and the IIO registration to the parent (of the
> platform-device).

Not quite; the iio device still gets deregistered when the platform
device is unbound.

> I'm not super-familiar with the internals here, but does this sound a
> bit wrong?

It's not a common pattern but not necessarily wrong per se.

> Is there a chance where the IIO device could be de-allocated, while
> registered?

No, the device-managed iio device object is freed when the platform
device is unbound and specifically after the iio device has been
deregistered.

> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > ---
> > >  drivers/iio/light/lm3533-als.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> > > index bc196c212881..0f380ec8d30c 100644
> > > --- a/drivers/iio/light/lm3533-als.c
> > > +++ b/drivers/iio/light/lm3533-als.c
> > > @@ -852,7 +852,6 @@ static int lm3533_als_probe(struct platform_device
> > > *pdev)
> > >  	indio_dev->channels = lm3533_als_channels;
> > >  	indio_dev->num_channels = ARRAY_SIZE(lm3533_als_channels);
> > >  	indio_dev->name = dev_name(&pdev->dev);
> > > -	indio_dev->dev.parent = pdev->dev.parent;
> > >  	indio_dev->modes = INDIO_DIRECT_MODE;
> > >  
> > >  	als = iio_priv(indio_dev);

Johan
Jonathan Cameron May 31, 2020, 2:54 p.m. UTC | #3
On Fri, 29 May 2020 15:45:33 +0200
Johan Hovold <johan@kernel.org> wrote:

> [ Trimming CC to something more reasonable... ]
> 
> On Fri, May 29, 2020 at 11:08:38AM +0000, Ardelean, Alexandru wrote:
> > On Fri, 2020-05-29 at 12:16 +0200, Johan Hovold wrote:  
> > > On Fri, May 22, 2020 at 11:22:07AM +0300, Alexandru Ardelean wrote:  
> > > > This assignment is the more peculiar of the bunch as it assigns the parent
> > > > of the platform-device's device (i.e. pdev->dev.parent) as the IIO device's
> > > > parent.
> > > > 
> > > > It's unclear whether this is intentional or not.
> > > > Hence it is in it's own patch.  
> > > 
> > > Yeah, we have a few mfd drivers whose child drivers registers their
> > > class devices directly under the parent mfd device rather than the
> > > corresponding child platform device.
> > > 
> > > Since it's done consistently I think you need to update them all if you
> > > really want to change this. 
> > > 
> > > And it may not be worth it since at least in theory someone could now be
> > > relying on this topology.  
> > 
> > Thanks for the feedback.
> > I guess, it could make sense to do here:
> >       devm_iio_device_alloc(pdev->dev.parent, ...)
> > 
> > Currently it's:
> >       devm_iio_device_alloc(&pdev->dev, ...)
> > 
> > That would make it slightly more consistent.  i.e. the life-time of
> > the object would be attached to the parent of the platform device,
> > versus the platform-device.  
> 
> Not really. If you unbind the iio driver, the iio device gets
> deregistered (as it should be) and there's no need to keep it around any
> more.
> 
> You'd essentially just leak resources every time you rebind the driver
> (e.g. during development).
> 
> And in fact, you could also introduce a use-after-free depending on if
> the parent mfd driver use devres to deregister its children.
> 
> > Currently, as it is, the allocation [of the IIO device] is tied the
> > platform-device, and the IIO registration to the parent (of the
> > platform-device).  
> 
> Not quite; the iio device still gets deregistered when the platform
> device is unbound.
> 
> > I'm not super-familiar with the internals here, but does this sound a
> > bit wrong?  
> 
> It's not a common pattern but not necessarily wrong per se.
> 
> > Is there a chance where the IIO device could be de-allocated, while
> > registered?  
> 
> No, the device-managed iio device object is freed when the platform
> device is unbound and specifically after the iio device has been
> deregistered.

I had a feeling we might have a few cases like this hiding in IIO.

For these I'd just leave the parent being manually set.
It doesn't do any harm and the facility existing is useful for messing
around with topology.

We may however want to wrap it up in a utility function on the
basis that we may want to change the visibility and location
of the IIO device down the line.

iio_device_set_parent() perhaps with docs to specify that it must
be called between allocation and registration + is meant to allow
cases where we want a different parent than the device used for
managed allocations etc.

Jonathan


> 
> > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > > ---
> > > >  drivers/iio/light/lm3533-als.c | 1 -
> > > >  1 file changed, 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> > > > index bc196c212881..0f380ec8d30c 100644
> > > > --- a/drivers/iio/light/lm3533-als.c
> > > > +++ b/drivers/iio/light/lm3533-als.c
> > > > @@ -852,7 +852,6 @@ static int lm3533_als_probe(struct platform_device
> > > > *pdev)
> > > >  	indio_dev->channels = lm3533_als_channels;
> > > >  	indio_dev->num_channels = ARRAY_SIZE(lm3533_als_channels);
> > > >  	indio_dev->name = dev_name(&pdev->dev);
> > > > -	indio_dev->dev.parent = pdev->dev.parent;
> > > >  	indio_dev->modes = INDIO_DIRECT_MODE;
> > > >  
> > > >  	als = iio_priv(indio_dev);  
> 
> Johan
Alexandru Ardelean June 2, 2020, 2:32 p.m. UTC | #4
On Sun, 2020-05-31 at 15:54 +0100, Jonathan Cameron wrote:
> [External]
> 
> On Fri, 29 May 2020 15:45:33 +0200
> Johan Hovold <johan@kernel.org> wrote:
> 
> > [ Trimming CC to something more reasonable... ]
> > 
> > On Fri, May 29, 2020 at 11:08:38AM +0000, Ardelean, Alexandru wrote:
> > > On Fri, 2020-05-29 at 12:16 +0200, Johan Hovold wrote:  
> > > > On Fri, May 22, 2020 at 11:22:07AM +0300, Alexandru Ardelean
> > > > wrote:  
> > > > > This assignment is the more peculiar of the bunch as it assigns
> > > > > the parent
> > > > > of the platform-device's device (i.e. pdev->dev.parent) as the
> > > > > IIO device's
> > > > > parent.
> > > > > 
> > > > > It's unclear whether this is intentional or not.
> > > > > Hence it is in it's own patch.  
> > > > 
> > > > Yeah, we have a few mfd drivers whose child drivers registers their
> > > > class devices directly under the parent mfd device rather than the
> > > > corresponding child platform device.
> > > > 
> > > > Since it's done consistently I think you need to update them all if
> > > > you
> > > > really want to change this. 
> > > > 
> > > > And it may not be worth it since at least in theory someone could
> > > > now be
> > > > relying on this topology.  
> > > 
> > > Thanks for the feedback.
> > > I guess, it could make sense to do here:
> > >       devm_iio_device_alloc(pdev->dev.parent, ...)
> > > 
> > > Currently it's:
> > >       devm_iio_device_alloc(&pdev->dev, ...)
> > > 
> > > That would make it slightly more consistent.  i.e. the life-time of
> > > the object would be attached to the parent of the platform device,
> > > versus the platform-device.  
> > 
> > Not really. If you unbind the iio driver, the iio device gets
> > deregistered (as it should be) and there's no need to keep it around
> > any
> > more.
> > 
> > You'd essentially just leak resources every time you rebind the driver
> > (e.g. during development).
> > 
> > And in fact, you could also introduce a use-after-free depending on if
> > the parent mfd driver use devres to deregister its children.
> > 
> > > Currently, as it is, the allocation [of the IIO device] is tied the
> > > platform-device, and the IIO registration to the parent (of the
> > > platform-device).  
> > 
> > Not quite; the iio device still gets deregistered when the platform
> > device is unbound.
> > 
> > > I'm not super-familiar with the internals here, but does this sound a
> > > bit wrong?  
> > 
> > It's not a common pattern but not necessarily wrong per se.
> > 
> > > Is there a chance where the IIO device could be de-allocated, while
> > > registered?  
> > 
> > No, the device-managed iio device object is freed when the platform
> > device is unbound and specifically after the iio device has been
> > deregistered.
> 
> I had a feeling we might have a few cases like this hiding in IIO.
> 
> For these I'd just leave the parent being manually set.
> It doesn't do any harm and the facility existing is useful for messing
> around with topology.
> 
> We may however want to wrap it up in a utility function on the
> basis that we may want to change the visibility and location
> of the IIO device down the line.
> 
> iio_device_set_parent() perhaps with docs to specify that it must
> be called between allocation and registration + is meant to allow
> cases where we want a different parent than the device used for
> managed allocations etc.
> 

Works for me.
If no objections, I'll include in the next re-spin.


> Jonathan
> 
> 
> > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > > > ---
> > > > >  drivers/iio/light/lm3533-als.c | 1 -
> > > > >  1 file changed, 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/iio/light/lm3533-als.c
> > > > > b/drivers/iio/light/lm3533-als.c
> > > > > index bc196c212881..0f380ec8d30c 100644
> > > > > --- a/drivers/iio/light/lm3533-als.c
> > > > > +++ b/drivers/iio/light/lm3533-als.c
> > > > > @@ -852,7 +852,6 @@ static int lm3533_als_probe(struct
> > > > > platform_device
> > > > > *pdev)
> > > > >  	indio_dev->channels = lm3533_als_channels;
> > > > >  	indio_dev->num_channels = ARRAY_SIZE(lm3533_als_channels);
> > > > >  	indio_dev->name = dev_name(&pdev->dev);
> > > > > -	indio_dev->dev.parent = pdev->dev.parent;
> > > > >  	indio_dev->modes = INDIO_DIRECT_MODE;
> > > > >  
> > > > >  	als = iio_priv(indio_dev);  
> > 
> > Johan
diff mbox series

Patch

diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
index bc196c212881..0f380ec8d30c 100644
--- a/drivers/iio/light/lm3533-als.c
+++ b/drivers/iio/light/lm3533-als.c
@@ -852,7 +852,6 @@  static int lm3533_als_probe(struct platform_device *pdev)
 	indio_dev->channels = lm3533_als_channels;
 	indio_dev->num_channels = ARRAY_SIZE(lm3533_als_channels);
 	indio_dev->name = dev_name(&pdev->dev);
-	indio_dev->dev.parent = pdev->dev.parent;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	als = iio_priv(indio_dev);