Re: [PATCH v5 10/16] iio: buffer-dmaengine: add devm_iio_dmaengine_buffer_setup_ext2()

[David Lechner <dlechner@baylibre.com>]

On 11/24/24 11:16 AM, Jonathan Cameron wrote:
> On Fri, 15 Nov 2024 14:18:49 -0600
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> Add a new devm_iio_dmaengine_buffer_setup_ext2() function to handle
>> cases where the DMA channel is managed by the caller rather than being
>> requested and released by the iio_dmaengine module.
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
> Fresh read and I'm wondering if the lifetimes in here can be managed
> more simply either by pushing the DMA channel get down, or dragging
> the release up.   Basically I'd like to see them at the same level
> of nesting in the code.  If it ends up being necessary to duplicate
> more code that is fine if it makes this easier to reason about.
> 

One option could be instead of introducing a 2nd function, change
the existing iio_dmaengine_buffer_setup_ext() to use the signature
of the proposed devm_iio_dmaengine_buffer_setup_ext2(). There are
only two users of these functions. So we could move the dma chan
request/release out to the drivers for those.

Otherwise, we can't completly get rid of the owns_chan bit.

> Jonathan
> 
>> ---
>>
>> v5 changes: none
>>
>> v4 changes:
>> * This replaces "iio: buffer-dmaengine: generalize requesting DMA channel"
>> ---
>>  drivers/iio/buffer/industrialio-buffer-dmaengine.c | 107 +++++++++++++++------
>>  include/linux/iio/buffer-dmaengine.h               |   5 +
>>  2 files changed, 81 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
>> index 054af21dfa65..602cb2e147a6 100644
>> --- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
>> +++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
>> @@ -33,6 +33,7 @@ struct dmaengine_buffer {
>>  	struct iio_dma_buffer_queue queue;
>>  
>>  	struct dma_chan *chan;
>> +	bool owns_chan;
>>  	struct list_head active;
>>  
>>  	size_t align;
>> @@ -216,28 +217,23 @@ static const struct iio_dev_attr *iio_dmaengine_buffer_attrs[] = {
>>   * Once done using the buffer iio_dmaengine_buffer_free() should be used to
>>   * release it.
>>   */
>> -static struct iio_buffer *iio_dmaengine_buffer_alloc(struct device *dev,
>> -	const char *channel)
>> +static struct iio_buffer *iio_dmaengine_buffer_alloc(struct dma_chan *chan,
>> +						     bool owns_chan)
>>  {
>>  	struct dmaengine_buffer *dmaengine_buffer;
>>  	unsigned int width, src_width, dest_width;
>>  	struct dma_slave_caps caps;
>> -	struct dma_chan *chan;
>>  	int ret;
>>  
>>  	dmaengine_buffer = kzalloc(sizeof(*dmaengine_buffer), GFP_KERNEL);
>> -	if (!dmaengine_buffer)
>> -		return ERR_PTR(-ENOMEM);
>> -
>> -	chan = dma_request_chan(dev, channel);
>> -	if (IS_ERR(chan)) {
>> -		ret = PTR_ERR(chan);
>> -		goto err_free;
>> +	if (!dmaengine_buffer) {
>> +		ret = -ENOMEM;
>> +		goto err_release;
>>  	}
>>  
>>  	ret = dma_get_slave_caps(chan, &caps);
>>  	if (ret < 0)
>> -		goto err_release;
>> +		goto err_free;
>>  
>>  	/* Needs to be aligned to the maximum of the minimums */
>>  	if (caps.src_addr_widths)
>> @@ -252,6 +248,7 @@ static struct iio_buffer *iio_dmaengine_buffer_alloc(struct device *dev,
>>  
>>  	INIT_LIST_HEAD(&dmaengine_buffer->active);
>>  	dmaengine_buffer->chan = chan;
>> +	dmaengine_buffer->owns_chan = owns_chan;
>>  	dmaengine_buffer->align = width;
>>  	dmaengine_buffer->max_size = dma_get_max_seg_size(chan->device->dev);
>>  
>> @@ -263,10 +260,12 @@ static struct iio_buffer *iio_dmaengine_buffer_alloc(struct device *dev,
>>  
>>  	return &dmaengine_buffer->queue.buffer;
>>  
>> -err_release:
>> -	dma_release_channel(chan);
>>  err_free:
>>  	kfree(dmaengine_buffer);
>> +err_release:
> This ends up oddly miss balanced.  If you get an error here, why
> not just do the release at the caller instead?

Yes, we can do that.

> 
>> +	if (owns_chan)
>> +		dma_release_channel(chan);
>> +
>>  	return ERR_PTR(ret);
>>  }
>>  
>> @@ -282,12 +281,38 @@ void iio_dmaengine_buffer_free(struct iio_buffer *buffer)
>>  		iio_buffer_to_dmaengine_buffer(buffer);
>>  
>>  	iio_dma_buffer_exit(&dmaengine_buffer->queue);
>> -	dma_release_channel(dmaengine_buffer->chan);
>> -
>>  	iio_buffer_put(buffer);
>> +
>> +	if (dmaengine_buffer->owns_chan)
>> +		dma_release_channel(dmaengine_buffer->chan);
>>  }
>>  EXPORT_SYMBOL_NS_GPL(iio_dmaengine_buffer_free, IIO_DMAENGINE_BUFFER);
>>  
>> +static struct iio_buffer
>> +*__iio_dmaengine_buffer_setup_ext(struct iio_dev *indio_dev,
>> +				  struct dma_chan *chan, bool owns_chan,
>> +				  enum iio_buffer_direction dir)
>> +{
>> +	struct iio_buffer *buffer;
>> +	int ret;
>> +
>> +	buffer = iio_dmaengine_buffer_alloc(chan, owns_chan);
>> +	if (IS_ERR(buffer))
>> +		return ERR_CAST(buffer);
>> +
>> +	indio_dev->modes |= INDIO_BUFFER_HARDWARE;
>> +
>> +	buffer->direction = dir;
>> +
>> +	ret = iio_device_attach_buffer(indio_dev, buffer);
>> +	if (ret) {
>> +		iio_dmaengine_buffer_free(buffer);
> Note that if you did push the free out to the caller for owns_chan
> as suggested above this would need to never free the buffer as
> that would be done if necessary at the caller for this...

We can't push iio_dmaengine_buffer_free() out here since it is balancing
the call to iio_dmaengine_buffer_alloc() which is called just above in
this function.

But we do have a problem then with the caller not knowing if the channel
was released or not on error. So perhaps we get rid of this helper
function and duplicate some code as you suggested.

> 
>> +		return ERR_PTR(ret);
>> +	}
>> +
>> +	return buffer;
>> +}
>> +
>>  /**
>>   * iio_dmaengine_buffer_setup_ext() - Setup a DMA buffer for an IIO device
>>   * @dev: DMA channel consumer device
>> @@ -308,24 +333,13 @@ struct iio_buffer *iio_dmaengine_buffer_setup_ext(struct device *dev,
>>  						  const char *channel,
>>  						  enum iio_buffer_direction dir)
>>  {
>> -	struct iio_buffer *buffer;
>> -	int ret;
>> -
>> -	buffer = iio_dmaengine_buffer_alloc(dev, channel);
>> -	if (IS_ERR(buffer))
>> -		return ERR_CAST(buffer);
>> -
>> -	indio_dev->modes |= INDIO_BUFFER_HARDWARE;
>> -
>> -	buffer->direction = dir;
>> +	struct dma_chan *chan;
>>  
>> -	ret = iio_device_attach_buffer(indio_dev, buffer);
>> -	if (ret) {
>> -		iio_dmaengine_buffer_free(buffer);
>> -		return ERR_PTR(ret);
>> -	}
>> +	chan = dma_request_chan(dev, channel);
>> +	if (IS_ERR(chan))
>> +		return ERR_CAST(chan);
>>  
>> -	return buffer;
>> +	return __iio_dmaengine_buffer_setup_ext(indio_dev, chan, true, dir);
> As above, why not just free the channel here if this fails? Thus matching
> the dma_request_chan just above.

This one is manageable.

> 
> 
>>  }
>>  EXPORT_SYMBOL_NS_GPL(iio_dmaengine_buffer_setup_ext, IIO_DMAENGINE_BUFFER);
>>