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.
- Nuno Sá
