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

Biju Das
2026-04-11T12:17:31Z

> -----Original Message-----
> From: Claudiu <claudiu.beznea@tuxon.dev>
> Sent: 11 April 2026 12:43
> Subject: [PATCH v4 01/17] dmaengine: sh: rz-dmac: Move interrupt request after everything is set up
>
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Once the interrupt is requested, the interrupt handler may run immediately.
> Since the IRQ handler can access channel->ch_base, which is initialized only after requesting the IRQ,
> this may lead to invalid memory access.
> Likewise, the IRQ thread may access uninitialized data (the ld_free, ld_queue, and ld_active lists),
> which may also lead to issues.
>
> Request the interrupts only after everything is set up. To keep the error path simpler, use
> dmam_alloc_coherent() instead of dma_alloc_coherent().
>
> Fixes: 5000d37042a6 ("dmaengine: sh: Add DMAC driver for RZ/G2L SoC")
> Cc: stable@vger.kernel.org
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>
> Changes in v4:
> - none, this patch is new
>
> drivers/dma/sh/rz-dmac.c | 88 +++++++++++++++-------------------------
> 1 file changed, 33 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c index 625ff29024de..9f206a33dcc6
> 100644
> --- a/drivers/dma/sh/rz-dmac.c
> +++ b/drivers/dma/sh/rz-dmac.c
> @@ -981,25 +981,6 @@ static int rz_dmac_chan_probe(struct rz_dmac *dmac,
> channel->index = index;
> channel->mid_rid = -EINVAL;
>
> - /* Request the channel interrupt. */
> - scnprintf(pdev_irqname, sizeof(pdev_irqname), "ch%u", index);
> - irq = platform_get_irq_byname(pdev, pdev_irqname);
> - if (irq < 0)
> - return irq;
> -
> - irqname = devm_kasprintf(dmac->dev, GFP_KERNEL, "%s:%u",
> - dev_name(dmac->dev), index);
> - if (!irqname)
> - return -ENOMEM;
> -
> - ret = devm_request_threaded_irq(dmac->dev, irq, rz_dmac_irq_handler,
> - rz_dmac_irq_handler_thread, 0,
> - irqname, channel);
> - if (ret) {
> - dev_err(dmac->dev, "failed to request IRQ %u (%d)\n", irq, ret);
> - return ret;
> - }
> -
> /* Set io base address for each channel */
> if (index < 8) {
> channel->ch_base = dmac->base + CHANNEL_0_7_OFFSET + @@ -1012,9 +993,9 @@ static int
> rz_dmac_chan_probe(struct rz_dmac *dmac,
> }
>
> /* Allocate descriptors */
> - lmdesc = dma_alloc_coherent(&pdev->dev,
> - sizeof(struct rz_lmdesc) * DMAC_NR_LMDESC,
> - &channel->lmdesc.base_dma, GFP_KERNEL);
> + lmdesc = dmam_alloc_coherent(&pdev->dev,
> + sizeof(struct rz_lmdesc) * DMAC_NR_LMDESC,
> + &channel->lmdesc.base_dma, GFP_KERNEL);
> if (!lmdesc) {
> dev_err(&pdev->dev, "Can't allocate memory (lmdesc)\n");
> return -ENOMEM;
> @@ -1030,7 +1011,24 @@ static int rz_dmac_chan_probe(struct rz_dmac *dmac,
> INIT_LIST_HEAD(&channel->ld_free);
> INIT_LIST_HEAD(&channel->ld_active);
>
> - return 0;
> + /* Request the channel interrupt. */
> + scnprintf(pdev_irqname, sizeof(pdev_irqname), "ch%u", index);
> + irq = platform_get_irq_byname(pdev, pdev_irqname);
> + if (irq < 0)
> + return irq;
> +
> + irqname = devm_kasprintf(dmac->dev, GFP_KERNEL, "%s:%u",
> + dev_name(dmac->dev), index);
> + if (!irqname)
> + return -ENOMEM;
> +
> + ret = devm_request_threaded_irq(dmac->dev, irq, rz_dmac_irq_handler,
> + rz_dmac_irq_handler_thread, 0,
> + irqname, channel);
> + if (ret)
> + dev_err(dmac->dev, "failed to request IRQ %u (%d)\n", irq, ret);

As per [1], it is redundant.

[1]
https://elixir.bootlin.com/linux/v7.0-rc7/source/kernel/irq/devres.c#L108

> +
> + return ret;
> }
>
> static void rz_dmac_put_device(void *_dev) @@ -1099,7 +1097,6 @@ static int rz_dmac_probe(struct
> platform_device *pdev)
> const char *irqname = "error";
> struct dma_device *engine;
> struct rz_dmac *dmac;
> - int channel_num;
> int ret;
> int irq;
> u8 i;
> @@ -1132,18 +1129,6 @@ static int rz_dmac_probe(struct platform_device *pdev)
> return PTR_ERR(dmac->ext_base);
> }
>
> - /* Register interrupt handler for error */
> - irq = platform_get_irq_byname_optional(pdev, irqname);
> - if (irq > 0) {
> - ret = devm_request_irq(&pdev->dev, irq, rz_dmac_irq_handler, 0,
> - irqname, NULL);
> - if (ret) {
> - dev_err(&pdev->dev, "failed to request IRQ %u (%d)\n",
> - irq, ret);
> - return ret;
> - }
> - }
> -
> /* Initialize the channels. */
> INIT_LIST_HEAD(&dmac->engine.channels);
>
> @@ -1169,6 +1154,18 @@ static int rz_dmac_probe(struct platform_device *pdev)
> goto err;
> }
>
> + /* Register interrupt handler for error */
> + irq = platform_get_irq_byname_optional(pdev, irqname);
> + if (irq > 0) {
> + ret = devm_request_irq(&pdev->dev, irq, rz_dmac_irq_handler, 0,
> + irqname, NULL);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to request IRQ %u (%d)\n",
> + irq, ret);

Same case here

Cheers,
Biju

> + goto err;
> + }
> + }
> +
> /* Register the DMAC as a DMA provider for DT. */
> ret = of_dma_controller_register(pdev->dev.of_node, rz_dmac_of_xlate,
> NULL);
> @@ -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:
> - channel_num = i ? i - 1 : 0;
> - for (i = 0; i < channel_num; i++) {
> - struct rz_dmac_chan *channel = &dmac->channels[i];
> -
> - dma_free_coherent(&pdev->dev,
> - sizeof(struct rz_lmdesc) * DMAC_NR_LMDESC,
> - channel->lmdesc.base,
> - channel->lmdesc.base_dma);
> - }
> -
> 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);
> - unsigned int i;
>
> dma_async_device_unregister(&dmac->engine);
> of_dma_controller_free(pdev->dev.of_node);
> - for (i = 0; i < dmac->n_channels; i++) {
> - struct rz_dmac_chan *channel = &dmac->channels[i];
> -
> - dma_free_coherent(&pdev->dev,
> - sizeof(struct rz_lmdesc) * DMAC_NR_LMDESC,
> - channel->lmdesc.base,
> - channel->lmdesc.base_dma);
> - }
> reset_control_assert(dmac->rstc);
> pm_runtime_put(&pdev->dev);
> pm_runtime_disable(&pdev->dev);
> --
> 2.43.0