Message-ID: <172837007815.3337.5869213289160447430@njaxe.localdomain>
Date: Tue, 08 Oct 2024 08:47:58 +0200
From: Matteo Martelli <matteomartelli3@...il.com>
To: Alisa-Dariana Roman <alisa.roman@...log.com>, Christian Eggers <ceggers@...i.de>, Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>, Nuno Sá <noname.nuno@...il.com>, Paul Cercueil <paul@...pouillou.net>, Peter Rosin <peda@...ntia.se>, Sebastian Reichel <sre@...nel.org>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, linux-mips@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v2 5/7] iio: inkern: copy/release available info from producer

Quoting Nuno Sá (2024-10-07 17:15:13)
> On Mon, 2024-10-07 at 10:37 +0200, Matteo Martelli wrote:
> > Consumers need to call the read_avail_release_resource after reading the
> > available info. To call the release with info_exists locked, copy the
> > available info from the producer and immediately call its release
> > callback. With this change, users of iio_read_avail_channel_raw() and
> > iio_read_avail_channel_attribute() must free the copied avail info after
> > calling them.
> > 
> > Signed-off-by: Matteo Martelli <matteomartelli3@...il.com>
> > ---
> >  drivers/iio/inkern.c         | 64 +++++++++++++++++++++++++++++++++-----------
> >  include/linux/iio/consumer.h |  4 +--
> >  2 files changed, 50 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > index
> > 7f325b3ed08fae6674245312cf8f57bb151006c0..cc65ef79451e5aa2cea447e168007a447ffc0d91
> > 100644
> > --- a/drivers/iio/inkern.c
> > +++ b/drivers/iio/inkern.c
> > @@ -760,9 +760,25 @@ static int iio_channel_read_avail(struct iio_channel *chan,
> >       if (!iio_channel_has_available(chan->channel, info))
> >               return -EINVAL;
> >  
> > -     if (iio_info->read_avail)
> > -             return iio_info->read_avail(chan->indio_dev, chan->channel,
> > -                                         vals, type, length, info);
> > +     if (iio_info->read_avail) {
> > +             const int *vals_tmp;
> > +             int ret;
> > +
> > +             ret = iio_info->read_avail(chan->indio_dev, chan->channel,
> > +                                        &vals_tmp, type, length, info);
> > +             if (ret < 0)
> > +                     return ret;
> > +
> > +             *vals = kmemdup_array(vals_tmp, *length, sizeof(int), GFP_KERNEL);
> > +             if (!*vals)
> > +                     return -ENOMEM;
> > +
> 
> Not a big deal but I would likely prefer to avoid yet another copy. If I'm
> understanding things correctly, I would rather create an inkern wrapper API like 
> iio_channel_read_avail_release_resource() - maybe something with a smaller name :).
> Hence, the lifetime of the data would be only controlled by the producer of it. It
> would also produce a smaller diff (I think). I just find it a bit confusing that we
> duplicate the data in here and the producer also duplicates it on the ->read_avail()
> call. Another advantage I see is that often the available data is indeed const in
> which case no kmemdup_array() is needed at all.


If I understand correctly your suggestion you would leave the inkern
iio_channel_read_avail() untouched, then add a new inkern wrapper, something
like iio_channel_read_avail_release_resource(), that would call the producer's
read_avail_release_resource(). The consumer would invoke this new wrapper in its
own read_avail_release_resource() avoiding the additional copy. The call stack
would look something like the following:

iio_read_channel_info_avail() {
    consumer->read_avail() {
        iio_read_avail_channel_raw() {
            iio_channel_read_avail() {
                producer->read_avail() {
                    kmemdup_array();
                }
            }
        }
    }

    iio_format_list();

    consumer->read_avail_release_resource() {
        iio_read_avail_channel_release_resource() {
            producer->read_avail_release_resource() {
                kfree();
            }
        }
    }
}


I was going with the simpler solution you described, but my concern with it was
that the info_exists_lock mutex would be unlocked between a iio_channel_read_avail()
call and its corresponding iio_channel_read_avail_release_resource() call.
To my understanding, this could potentially allow for the device to be
unregistered between the two calls and result in a memleak of the avail buffer
allocated by the producer.

However, I have been trying to reproduce a similar case by adding a delay
between the consumer->read_avail() and the
consumer->read_avail_release_resources(), and by unbinding the driver during
that delay, thus with the info_exists_lock mutex unlocked. In this case the
driver is not unregistered until the iio_read_channel_info_avail() function
completes, likely because of some other lock on the sysfs file after the call of
cdev_device_del() in iio_device_unregister().

Are there are other cases in which the device could be unregistered between the
two calls? If the info_exists_lock mutex is not necessary for this read_avail()
flow then I could switch it to the simpler solution without the additional consumer
copy, but at that point I would question why the info_exists_lock mutex is being
locked in iio_read_avail_channel_raw().

For some additional context see also my previous conversation with Jonathan on
the subject [1]. I followed Jonathan's suggestion to keep the implementation
simple by letting the consumer to always copy the producer buffer, but I could
also consider different solutions.

Regarding the release function names being too long, I totally agree and I would also
shorten the iio_info read_avail_release_resource() callback if that remains
clear: something like read_avail_release_res() or just read_avail_release()?

Link: https://lore.kernel.org/linux-iio/20240810105411.705cb225@jic23-huawei/ [1]

> 
> - Nuno Sá
> 
> 

Thanks,
Matteo Martelli

Powered by blists - more mailing lists