Re: [PATCH v6 09/18] dmaengine: sh: rz-dmac: Use virt-dma APIs for channel descriptor processing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing cleanup of the active descriptor (`channel->desc`) in `rz_dmac_free_chan_resources` causes permanent memory leaks and channel breakage.
- [High] Premature tasklet synchronization in `rz_dmac_device_synchronize` violates DMA synchronization guarantees by allowing callbacks to execute after the function returns.
--

commit d09437d0a9b5a01f2fe83f477e60abbcf577d287
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

dmaengine: sh: rz-dmac: Use virt-dma APIs for channel descriptor processing

This commit updates the driver to fully rely on the virt-dma APIs for
descriptor management. It replaces custom active and queue lists with a
single channel->desc pointer to track the current descriptor and relies
on virt-dma to handle the rest.

> diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
> index 217657513fa76..1f884ec101f80 100644
> --- a/drivers/dma/sh/rz-dmac.c
> +++ b/drivers/dma/sh/rz-dmac.c

[ ... ]

> @@ -494,8 +485,6 @@ static void rz_dmac_free_chan_resources(struct dma_chan *chan)
>  	rz_lmdesc_setup(channel, channel->lmdesc.base);
>  
>  	rz_dmac_disable_hw(channel);
> -	list_splice_tail_init(&channel->ld_active, &channel->ld_free);
> -	list_splice_tail_init(&channel->ld_queue, &channel->ld_free);
>  
>  	if (channel->mid_rid >= 0) {
>  		clear_bit(channel->mid_rid, dmac->modules);
> @@ -504,13 +493,19 @@ static void rz_dmac_free_chan_resources(struct dma_chan *chan)
>  
>  	spin_unlock_irqrestore(&channel->vc.lock, flags);
>  
> +	vchan_free_chan_resources(&channel->vc);
> +
> +	spin_lock_irqsave(&channel->vc.lock, flags);
> +
>  	list_for_each_entry_safe(desc, _desc, &channel->ld_free, node) {
> +		list_del(&desc->node);
>  		kfree(desc);
>  		channel->descs_allocated--;
>  	}
>  
>  	INIT_LIST_HEAD(&channel->ld_free);
> -	vchan_free_chan_resources(&channel->vc);
> +
> +	spin_unlock_irqrestore(&channel->vc.lock, flags);
>  }

[Severity: High]
Does this code leak the active descriptor? If channel->desc is currently set,
vchan_free_chan_resources only clears virt-dma's internal lists, leaving
the memory allocated for channel->desc dangling.

Could this also cause the channel to be permanently broken for future clients?

If channel->desc remains non-NULL, a subsequent request for the channel
might cause rz_dmac_issue_pending to see channel->desc != NULL and
silently refuse to start new transfers. Unlike rz_dmac_terminate_all,
this function does not appear to terminate and clear channel->desc.

[ ... ]

> @@ -692,6 +681,8 @@ static void rz_dmac_device_synchronize(struct dma_chan *chan)
>  	u32 chstat;
>  	int ret;
>  
> +	vchan_synchronize(&channel->vc);
> +
>  	ret = read_poll_timeout(rz_dmac_ch_readl, chstat, !(chstat & CHSTAT_EN),
>  				100, 100000, false, channel, CHSTAT, 1);
>  	if (ret < 0)

[Severity: High]
Does calling vchan_synchronize here allow client completion callbacks to
run after this function returns?

If the hardware is still actively transferring, vchan_synchronize will
return immediately. Then, the polling loop waits for the hardware to stop.
Once the hardware finishes, it raises an interrupt, and the IRQ handler
calls vchan_cookie_complete, which schedules the virt-dma tasklet again.

This seems like it could expose clients to use-after-free bugs if they rely
on the DMA synchronization guarantee to safely free DMA buffers after
calling dmaengine_synchronize. 

Should vchan_synchronize be called after the hardware polling loop finishes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526084710.3491480-1-claudiu.beznea@kernel.org?part=9