Re: [RFC PATCH v3 3/5] iio: buffer: Extend DMAengine buffer interfaces to take extra sysfs attributes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Critical] Unconditional assignment of `buffer->attrs` overwrites the default DMAengine buffer attributes, removing `length_align_bytes` from sysfs and breaking the userspace ABI.

Pre-existing issues:
- [High] The ad4691 driver manually overwrites the IIO buffer's `attrs` field after setup, clobbering the generic DMAengine buffer attributes.
--

commit e15805cbfeb85d6ab2a3f8819279738d4402da62
Author: Marcelo Schmitt <marcelo.schmitt@analog.com>

iio: buffer: Extend DMAengine buffer interfaces to take extra sysfs attributes

This patch extends the IIO DMAengine buffer setup interfaces to accept an
additional parameter for extra sysfs attributes. It updates all existing
callers to pass NULL for this new parameter.

> diff --git a/drivers/iio/adc/ad4691.c b/drivers/iio/adc/ad4691.c
> --- a/drivers/iio/adc/ad4691.c
> +++ b/drivers/iio/adc/ad4691.c
> @@ -1980,7 +1980,8 @@ static int ad4691_setup_offload(struct iio_dev *indio_dev,
>  		indio_dev->setup_ops = &ad4691_cnv_burst_offload_buffer_setup_ops;
>  
>  	ret = devm_iio_dmaengine_buffer_setup_with_handle(dev, indio_dev, rx_dma,
> -							  IIO_BUFFER_DIRECTION_IN);
> +							  IIO_BUFFER_DIRECTION_IN,
> +							  NULL);

[Severity: High]
This is a pre-existing issue, but does manually assigning the buffer attributes
further down in this function clobber the generic DMAengine buffer attributes?

Following this setup call in ad4691_setup_offload(), the driver manually
assigns:

indio_dev->buffer->attrs = ad4691_buffer_attrs;

Could this overwrite the default DMAengine buffer attributes like
length_align_bytes? Since the new API allows injecting custom attributes,
should this driver be updated to pass ad4691_buffer_attrs into
devm_iio_dmaengine_buffer_setup_with_handle() rather than passing NULL and
manually overwriting it afterwards?

>  	if (ret)
>  		return ret;
>  

[ ... ]

> diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> --- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> +++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> @@ -289,7 +289,8 @@ EXPORT_SYMBOL_NS_GPL(iio_dmaengine_buffer_teardown, "IIO_DMAENGINE_BUFFER");
>  static struct iio_buffer
>  *__iio_dmaengine_buffer_setup_ext(struct iio_dev *indio_dev,
>  				  struct dma_chan *chan,
> -				  enum iio_buffer_direction dir)
> +				  enum iio_buffer_direction dir,
> +				  const struct iio_dev_attr **buffer_attrs)
>  {
>  	struct iio_buffer *buffer;
>  	int ret;
> @@ -301,6 +302,7 @@ static struct iio_buffer
>  	indio_dev->modes |= INDIO_BUFFER_HARDWARE;
>  
>  	buffer->direction = dir;
> +	buffer->attrs = buffer_attrs;

[Severity: Critical]
Does unconditionally overwriting buffer->attrs here remove the default
DMAengine buffer attributes?

Since iio_dmaengine_buffer_alloc() sets up the default attributes array
(which exposes length_align_bytes to userspace), and all current callers
updated in this patch pass NULL for buffer_attrs, it appears this will
silently delete length_align_bytes from sysfs for all IIO DMAengine buffers.

Could this break the userspace ABI, as applications might rely on
length_align_bytes to determine the required memory alignment for DMA
buffers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1781661028.git.marcelo.schmitt@analog.com?part=3