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