lkml.org 
[lkml]   [2020]   [Jun]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 4/5] iio: light: lm3533-als: remove explicit parent assignment
Date
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
\
 
 \ /
  Last update: 2020-06-02 16:33    [W:0.086 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site