Re: [PATCH v5 01/17] dmaengine: sh: rz-dmac: Move interrupt request after everything is set up

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Asynchronous pm_runtime_put() used immediately before pm_runtime_disable() leaves hardware in incorrect state.
- [High] Hardware is placed in reset and powered down while devm-managed interrupts are still active, risking a kernel crash.
- [High] Missing tasklet_kill() during probe error and removal paths leads to a Use-After-Free.
--

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

dmaengine: sh: rz-dmac: Move interrupt request after everything is set up

This patch fixes potential invalid memory access during probe by moving the
interrupt request to the end of the channel initialization, ensuring all
data structures are fully set up before the interrupt handler can run. It
also switches to dmam_alloc_coherent() to simplify the error and removal
paths.

> diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
> index 625ff29024de0..9f206a33dcc6b 100644
> --- a/drivers/dma/sh/rz-dmac.c
> +++ b/drivers/dma/sh/rz-dmac.c

[ ... ]

> @@ -1210,16 +1207,6 @@ static int rz_dmac_probe(struct platform_device *pdev)
>  dma_register_err:
>  	of_dma_controller_free(pdev->dev.of_node);
>  err:
>  	reset_control_assert(dmac->rstc);
>  err_pm_runtime_put:
>  	pm_runtime_put(&pdev->dev);
> @@ -1232,18 +1219,9 @@ static int rz_dmac_probe(struct platform_device *pdev)
>  static void rz_dmac_remove(struct platform_device *pdev)
>  {
>  	struct rz_dmac *dmac = platform_get_drvdata(pdev);
>  
>  	dma_async_device_unregister(&dmac->engine);
>  	of_dma_controller_free(pdev->dev.of_node);
>  	reset_control_assert(dmac->rstc);
>  	pm_runtime_put(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  }

While reviewing these changes, I noticed a few pre-existing regressions that
remain after this patch.

Does calling pm_runtime_put() asynchronously right before pm_runtime_disable()
leave the hardware active?

pm_runtime_put() queues an idle request, but the subsequent
pm_runtime_disable() cancels pending async operations via
__pm_runtime_barrier(). Should this use pm_runtime_put_sync() instead to
ensure the device actually suspends and doesn't leak power resources?

Also, the hardware reset is asserted (reset_control_assert) and PM is
disabled before returning. Since the interrupts are registered with
devm_request_irq() and devm_request_threaded_irq(), they will be freed by
devres only after returning.

If a spurious or delayed interrupt fires during this window, could the IRQ
handler access hardware registers while the IP block is in reset and unclocked,
leading to a crash?

Finally, vchan_init() initializes a tasklet (channel->vc.task) for each
channel. Since the channels array is allocated with devm_kcalloc(), it is
automatically freed by devres on removal or probe failure.

However, there is no explicit tasklet_kill() during removal or probe error
handling. Although devres frees the IRQ, free_irq() only syncs the handler
and thread, not pending tasklets. If the tasklet is scheduled before the IRQ
is freed, could it remain on the system softirq queue and execute after
devres frees the channel memory, causing a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512121219.216159-1-claudiu.beznea.uj@bp.renesas.com?part=1