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

[Ardelean, Alexandru]

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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > > ---
> > > > >  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