On 10/25/24 8:24 AM, Nuno Sá wrote: > I still need to look better at this but I do have one though already :) > > On Wed, 2024-10-23 at 15:59 -0500, David Lechner 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@xxxxxxxxxxxx> >> --- >> >> v4 changes: >> * This replaces "iio: buffer-dmaengine: generalize requesting DMA channel" >> --- ... >> @@ -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); > > Not sure if I agree much with this owns_chan flag. The way I see it, we should always > handover the lifetime of the DMA channel to the IIO DMA framework. Note that even the > device you pass in for both requesting the channel of the spi_offload and for > setting up the DMA buffer is the same (and i suspect it will always be) so I would > not go with the trouble. And with this assumption we could simplify a bit more the > spi implementation. I tried something like this in v3 but Jonathan didn't seem to like it. https://lore.kernel.org/all/20240727144303.4a8604cb@jic23-huawei/ > > And not even related but I even suspect the current implementation could be > problematic. Basically I'm suspecting that the lifetime of the DMA channel should be > attached to the lifetime of the iio_buffer. IOW, we should only release the channel > in iio_dmaengine_buffer_release() - in which case the current implementation with the > spi_offload would also be buggy. The buffer can outlive the iio device driver that created the buffer? > > But bah, the second point is completely theoretical and likely very hard to reproduce > in real life (if reproducible at all - for now it's only something I suspect) > > - Nuno Sá > >
