Re: [PATCH v2 1/5] media: iris: Add dma sync calls for input and output buffers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Calling `dma_sync_single_*` on an IOVA managed by videobuf2-dma-contig violates the DMA API and can cause severe memory corruption for non-contiguous DMA-BUF imports.
- [High] The DMA synchronization logic for output buffers is fundamentally broken: it synchronizes the CPU cache before the hardware writes, uses the wrong API direction, specifies a zero size, and omits the necessary sync upon completion.
--

--- Patch [1]: [PATCH v2 1/5] media: iris: Add dma sync calls for input and output buffers ---
commit ee7c5670aeeb217b4b298594d79282fac01b4135
Author: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>

media: iris: Add dma sync calls for input and output buffers

This commit adds DMA synchronization calls in iris_queue_buffer() to fix
data corruption and hardware faults. It uses dma_sync_single_for_device for
input buffers and dma_sync_single_for_cpu for output buffers.

> diff --git a/drivers/media/platform/qcom/iris/iris_buffer.c b/drivers/media/platform/qcom/iris/iris_buffer.c
> index eb8de60c1177f..c7a664426187d 100644
> --- a/drivers/media/platform/qcom/iris/iris_buffer.c
> +++ b/drivers/media/platform/qcom/iris/iris_buffer.c
> @@ -585,6 +585,13 @@ int iris_queue_buffer(struct iris_inst *inst, struct iris_buffer *buf)
>  	const struct iris_hfi_session_ops *hfi_ops = inst->hfi_session_ops;
>  	int ret;
>  
> +	if (buf->type == BUF_INPUT)
> +		dma_sync_single_for_device(inst->core->dev, buf->device_addr,
> +					   buf->data_size, DMA_TO_DEVICE);

[Severity: Critical]
Does this violate the DMA API by passing a videobuf2-managed IOVA to
dma_sync_single_for_device()?

For imported scatter-gather DMA-BUFs, the IOVA might be contiguous but backed
by physically discontiguous pages. Calling a single-mapped sync on the IOVA 
translates the start IOVA to a single physical address and performs cache 
maintenance on a linear physical address range. Can this corrupt arbitrary 
physical memory while leaving the actual backing pages unsynchronized?

> +	else if (buf->type == BUF_OUTPUT)
> +		dma_sync_single_for_cpu(inst->core->dev, buf->device_addr,
> +					buf->data_size, DMA_FROM_DEVICE);
> +

[Severity: High]
Is the synchronization direction correct for a queued capture buffer?

This calls dma_sync_single_for_cpu() before the hardware writes to the 
buffer, claiming CPU ownership instead of giving it to the device with 
dma_sync_single_for_device(..., DMA_FROM_DEVICE). 

Additionally, buf->data_size for a queued empty capture buffer is typically
zero. Does this result in syncing zero bytes?

Finally, is there a missing dma_sync_single_for_cpu() in the completion path
(such as iris_vb2_buffer_done()) after the hardware finishes writing and 
buf->data_size is populated? Without it, the CPU might read speculative or 
stale data.

>  	ret = hfi_ops->session_queue_buf(inst, buf);
>  	if (ret)
>  		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818-iris-fixes-dma-pseq-fint-v2-0-8dc8c4b3880e@oss.qualcomm.com?part=1