RE: [PATCH] iio: light: adjd_s311: convert to device-managed functions

[Sa, Nuno]

> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Sunday, July 11, 2021 12:26 PM
> To: Alexandru Ardelean <aardelean@xxxxxxxxxxx>
> Cc: linux-iio <linux-iio@xxxxxxxxxxxxxxx>; Linux Kernel Mailing List
> <linux-kernel@xxxxxxxxxxxxxxx>; pmeerw@xxxxxxxxxx
> Subject: Re: [PATCH] iio: light: adjd_s311: convert to device-managed
> functions
> 
> On Mon, 5 Jul 2021 09:38:21 +0300
> Alexandru Ardelean <aardelean@xxxxxxxxxxx> wrote:
> 
> > On Sat, 3 Jul 2021 at 20:47, Jonathan Cameron <jic23@xxxxxxxxxx>
> wrote:
> > >
> > > On Mon, 28 Jun 2021 16:51:32 +0300
> > > Alexandru Ardelean <aardelean@xxxxxxxxxxx> wrote:
> > >
> > > > This one is a little easier to convert to device-managed, now with
> the
> > > > devm_krealloc() function.
> > > >
> > > > The other iio_triggered_buffer_setup() and iio_device_register()
> can be
> > > > converted to their devm_ variants. And devm_krealloc() can be
> used to
> > > > (re)alloc the buffer. When the driver unloads, this will also be
> free'd.
> > > >
> > > > Signed-off-by: Alexandru Ardelean <aardelean@xxxxxxxxxxx>
> > > > ---
> > > >  drivers/iio/light/adjd_s311.c | 34 +++++-----------------------------
> > > >  1 file changed, 5 insertions(+), 29 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/light/adjd_s311.c
> b/drivers/iio/light/adjd_s311.c
> > > > index 17dac8d0e11d..19d60d6986a1 100644
> > > > --- a/drivers/iio/light/adjd_s311.c
> > > > +++ b/drivers/iio/light/adjd_s311.c
> > > > @@ -230,8 +230,8 @@ static int
> adjd_s311_update_scan_mode(struct iio_dev *indio_dev,
> > > >  {
> > > >       struct adjd_s311_data *data = iio_priv(indio_dev);
> > > >
> > > > -     kfree(data->buffer);
> > > > -     data->buffer = kmalloc(indio_dev->scan_bytes,
> GFP_KERNEL);
> > > > +     data->buffer = devm_krealloc(indio_dev->dev.parent, data-
> >buffer,
> > > > +                                  indio_dev->scan_bytes, GFP_KERNEL);
> > > I got some complaints about exactly this trick in a review recently
> so I'll
> > > pass them on.
> > >
> > > Whilst devm_krealloc() usage like this won't lose the original
> reference, its
> > > not what people expect from a realloc() case, so to not confuse
> people it is
> > > better to do a dance where you use a local variable, then only set
> data->buffer
> > > to it once we know the realloc succeeded.
> > >
> > > That avoids this looking like the anti-pattern it would be if that
> were a normal
> > > realloc in which case you would just have leaked the original
> allocation.
> > >
> > > More interestingly, why are we bothering with resizing the buffer
> dependent on what
> > > is enabled?  Can't we just allocate a 128 byte buffer and not bother
> changing it
> > > as we really aren't wasting that much space?  Just embed it in the
> adjd_s311_data
> > > structure directly and don't worry about the allocations.  Will need
> to be
> > > aligned(8) though to avoid the push_to_buffer_with_timestamp()
> issue.
> > > Using something like
> > >
> > > struct {
> > >         s16 chans[4];
> > >         s64 ts __aligned(8); /* I hate x86 32 bit */
> >
> > do you want to me t also add this comment? :p
> > [just kidding]
> >
> > > } scan;
> > >
> > > Inside the priv structure should work nicely.
> >
> > i agree; will do it like this;
> > i hesitated a bit due to the inertia of converting things to devm_
> 
> A long discussion on rust usage in linux diverted into the issues around
> devm.
> I 'believe' that we are fine in IIO after some work Lars did a long time
> back
> to make us resilient to unbinds whilst the chardev was open, but
> probably
> worth keeping an eye on that discussion.
> 
> https://urldefense.com/v3/__https://lore.kernel.org/ksummit/CANiq
> 72nkNrekzbxMci6vW02w=Q2L-
> SVTk_U4KN_LT8u_b=YPgw@xxxxxxxxxxxxxx/T/*m6db86a574237c22a3
> 2ecf49b596b3c2917967c5e__;Iw!!A3Ni8CS0y2Y!oeM8GJzKVXb8mYa1m
> VJNw5fI2adsFk3FKkFzbnqyuDkUMKVTKQ3OoT0cnXP5rA$
> 
> I'm a tiny bit nervous that there might be races where we are doing
> the devm_realloc.
> I 'think' we are fine, but the 'think' and 'believe' in these statements
> expresses
> a slight lack of certainty!
> 
> Jonathan
> 

Hi,

It's the second thread where I see you mentioning this, so this I will take the
opportunity to also give a bit on though about this. I actually have in mind a RFC
(hopefully sending it out this week) for this as I think we might still have some 
issues with open chardevs and device unbinding.

What we have in [1] is not enough to make sure the whole thing is synchronized with
device unbinding... We still have the door open to races where we call 'iio_buffer_ready()'
or even 'rb->access->read()' after the device gets unbinded. Maybe we are lucky and
nothing bad really happens and we just error out in the next time 'read()' is done on
our fd. However, during the possible race,  I think it's very likely that we end up touching
the same data structures concurrently. On some devices, we surely
(in theory and if all the stars align) have a path where 'iio_buffer_flush_hwfifo()' might
be called with 'indio_dev->info' already set to NULL...

IMO, the only way to have this fully in sync is to use the 'info_exist_lock' as it's done
in [2]. I think [2] was actually "fixed" when Alex sent his patches for multi buffer support...
Naturally, for the read case, we need to make sure we are not going to sleep with the
mutex held so we might need an unlock -> lock dance which is not that nice. But I'm
not really seeing another way. We also need to look at other file operations and also
for the events case to see if this is also a thing.

Naturally, I might be missing some subtlety and that's why I had this planned as RFC.
But since is mentioned here, I thought I could bring this up as in the end I might not
even need to send the patches :)

[1]: https://elixir.bootlin.com/linux/latest/source/drivers/iio/industrialio-buffer.c#L117
[2]: https://elixir.bootlin.com/linux/latest/source/drivers/iio/industrialio-core.c#L1763

- Nuno Sá