[PATCH v2 0/2] dmaengine: dw-axi-dmac: clean up DMAC enable and PM

[PATCH v2 0/2] dmaengine: dw-axi-dmac: clean up DMAC enable and PM

[tze.yee.ng]

From: Tze Yee Ng <tze.yee.ng@altera.com>

The DesignWare AXI DMAC driver enables the controller in axi_dma_resume(),
which is invoked from the runtime PM resume path and from probe. Calling
axi_dma_enable() again at the start of every block transfer is redundant
on the normal path.

That extra call had also masked a gap in system-sleep power management:
with only runtime PM callbacks registered, a channel could remain allocated
across suspend/resume while the runtime usage count stayed non-zero and
axi_dma_runtime_resume() was not run, leaving DMAC_CFG and clocks out of
sync with software state. Removing the per-transfer enable without fixing
PM would make that scenario more visible.

This series drops the redundant enable and adds the missing system-sleep
and channel-allocation PM handling called out during review.

Patch 1 removes axi_dma_enable() from axi_chan_block_xfer_start().

Patch 2 (follow-up to review feedback from Sashiko Watanabe):

 - Add SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
   pm_runtime_force_resume) so system suspend/resume reuses the existing
   axi_dma_suspend() and axi_dma_resume() paths even when the runtime
   usage count is non-zero.

 - Replace pm_runtime_get() with pm_runtime_resume_and_get() in
   dma_chan_alloc_chan_resources(), with pm_runtime_put() on error paths,
   so clocks are enabled before a client can submit a transfer immediately
   after allocation.

Changes in v2:
- Add Patch 2 as a follow-up to review feedback from Sashiko Watanabe.
- No changes to Patch 1.

Niravkumar L Rabara (1):
  dmaengine: dw-axi-dmac: drop redundant DMAC enable in block start

Tze Yee Ng (1):
  dmaengine: dw-axi-dmac: fix PM for system sleep and channel alloc

 drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

-- 
2.43.7

[PATCH v2 1/2] dmaengine: dw-axi-dmac: drop redundant DMAC enable in block start

[tze.yee.ng]

From: Niravkumar L Rabara <niravkumarlaxmidas.rabara@altera.com>

axi_chan_block_xfer_start() runs after the controller is already enabled,
so calling axi_dma_enable() again is unnecessary. Remove the redundant
enable call to keep the transfer start path clean and avoid repeated no-op
programming.

Signed-off-by: Niravkumar L Rabara <niravkumarlaxmidas.rabara@altera.com>
Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
---
 drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
index 4d53f077e9d2..f7a50f470461 100644
--- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
+++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
@@ -437,8 +437,6 @@ static void axi_chan_block_xfer_start(struct axi_dma_chan *chan,
 		return;
 	}
 
-	axi_dma_enable(chan->chip);
-
 	config.dst_multblk_type = DWAXIDMAC_MBLK_TYPE_LL;
 	config.src_multblk_type = DWAXIDMAC_MBLK_TYPE_LL;
 	config.tt_fc = DWAXIDMAC_TT_FC_MEM_TO_MEM_DMAC;
-- 
2.43.7

Re: [PATCH v2 1/2] dmaengine: dw-axi-dmac: drop redundant DMAC enable in block start

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent transfers can race on global APB registers due to lack of a global lock in `dw_axi_dma_set_byte_halfword`.
--

commit b6ec2512e872c5f388866aa7fdbd27c4bf168a25
Author: Niravkumar L Rabara <niravkumarlaxmidas.rabara@altera.com>

dmaengine: dw-axi-dmac: drop redundant DMAC enable in block start

This commit removes an unnecessary call to axi_dma_enable() in
axi_chan_block_xfer_start(), as the controller is already enabled.
This avoids repeated no-op programming and keeps the transfer start
path clean.

> diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> index 4d53f077e9d25..f7a50f470461c 100644
> --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> @@ -437,8 +437,6 @@ static void axi_chan_block_xfer_start(struct axi_dma_chan *chan,
>  		return;
>  	}
>  
> -	axi_dma_enable(chan->chip);
> -
>  	config.dst_multblk_type = DWAXIDMAC_MBLK_TYPE_LL;
>  	config.src_multblk_type = DWAXIDMAC_MBLK_TYPE_LL;
>  	config.tt_fc = DWAXIDMAC_TT_FC_MEM_TO_MEM_DMAC;

[Severity: High]
This is a pre-existing issue, but can concurrent transfers race on the
global APB registers during this setup phase?

When this function executes for the DMA_MEM_TO_DEV direction, it calls
dw_axi_dma_set_byte_halfword(). That function performs read-modify-write
operations on the global DMAC_APB_BYTE_WR_CH_EN or 
DMAC_APB_HALFWORD_WR_CH_EN registers, which contain configuration bits
for all channels.

Since the locking in these paths typically relies on the per-channel
chan->vc.lock, concurrent calls on different channels could interleave.
Would this lead to one channel's configuration bit being silently
overwritten and lost, potentially resulting in incorrectly configured
transfers or memory corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1779688569.git.tze.yee.ng@altera.com?part=1

[PATCH v2 2/2] dmaengine: dw-axi-dmac: fix PM for system sleep and channel alloc

[tze.yee.ng]

From: Tze Yee Ng <tze.yee.ng@altera.com>

The driver only had runtime PM callbacks. If a channel stayed allocated
across system suspend/resume, the runtime usage count could remain
non-zero while hardware state (DMAC_CFG, clocks) was lost, and
axi_dma_runtime_resume() would not run to restore it.

Add system-sleep PM ops that use pm_runtime_force_suspend() and
pm_runtime_force_resume() so suspend/resume reuses the existing
axi_dma_suspend() and axi_dma_resume() paths.

Replace pm_runtime_get() with pm_runtime_resume_and_get() in
dma_chan_alloc_chan_resources() so clocks are enabled before a client
can immediately submit a transfer and touch MMIO.

Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
---
 drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
index f7a50f470461..bcefaff03b5c 100644
--- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
+++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
@@ -516,11 +516,17 @@ static void dw_axi_dma_synchronize(struct dma_chan *dchan)
 static int dma_chan_alloc_chan_resources(struct dma_chan *dchan)
 {
 	struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(chan->chip->dev);
+	if (ret < 0)
+		return ret;
 
 	/* ASSERT: channel is idle */
 	if (axi_chan_is_hw_enable(chan)) {
 		dev_err(chan2dev(chan), "%s is non-idle!\n",
 			axi_chan_name(chan));
+		pm_runtime_put(chan->chip->dev);
 		return -EBUSY;
 	}
 
@@ -531,12 +537,11 @@ static int dma_chan_alloc_chan_resources(struct dma_chan *dchan)
 					  64, 0);
 	if (!chan->desc_pool) {
 		dev_err(chan2dev(chan), "No memory for descriptors\n");
+		pm_runtime_put(chan->chip->dev);
 		return -ENOMEM;
 	}
 	dev_vdbg(dchan2dev(dchan), "%s: allocating\n", axi_chan_name(chan));
 
-	pm_runtime_get(chan->chip->dev);
-
 	return 0;
 }
 
@@ -1663,6 +1668,8 @@ static void dw_remove(struct platform_device *pdev)
 }
 
 static const struct dev_pm_ops dw_axi_dma_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
 	SET_RUNTIME_PM_OPS(axi_dma_runtime_suspend, axi_dma_runtime_resume, NULL)
 };
 
-- 
2.43.7

Re: [PATCH v2 2/2] dmaengine: dw-axi-dmac: fix PM for system sleep and channel alloc

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:

Pre-existing issues:
- [High] Hardware is forcefully powered down in `dw_remove()` while the devm-managed DMA engine is still registered and actively exposing channels to clients.
- [High] `dw_remove()` leaks clock references if the device is in an active Runtime PM state.
- [High] `axi_dma_resume()` leaks the `cfgr_clk` reference if `core_clk` fails to enable.
- [High] The runtime suspend callback `axi_dma_suspend()` does not call `synchronize_irq()` before disabling hardware clocks.
- [High] The shared interrupt handler `dw_axi_dma_interrupt()` accesses hardware registers without ensuring the device is runtime active.
- [High] An asynchronous `pm_runtime_put()` is used before jumping to an error handling path that immediately calls `pm_runtime_disable()`, causing a race condition in `dw_probe()`.
- [High] The function `axi_req_irqs()` writes to the fixed-size `chip->irq` array without validating that `irq_count` does not exceed the array bounds.
- [Low] The `dw_axi_dma_pm_ops` structure is assigned unconditionally to the driver's `.pm` pointer without using the `pm_ptr()` wrapper macro.
--

commit 80df1f731d96db5786b01419f199171e1d6714a5
Author: Tze Yee Ng <tze.yee.ng@altera.com>

dmaengine: dw-axi-dmac: fix PM for system sleep and channel alloc

The driver only had runtime PM callbacks. If a channel stayed allocated
across system suspend/resume, the runtime usage count could remain
non-zero while hardware state (DMAC_CFG, clocks) was lost, and
axi_dma_runtime_resume() would not run to restore it. This commit adds
system-sleep PM ops to reuse suspend/resume paths and replaces
pm_runtime_get() with pm_runtime_resume_and_get().

> diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> index f7a50f470461c..bcefaff03b5c6 100644
> --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c

[ ... ]

> @@ -1663,6 +1668,8 @@ static void dw_remove(struct platform_device *pdev)
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does dw_remove() forcefully
power down hardware while the devm-managed DMA engine is still registered and
actively exposing channels to clients?

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:dw_remove() {
	...
	axi_dma_disable(chip);
	pm_runtime_disable(chip->dev);
	axi_dma_suspend(chip);
	...
}

The driver uses dmaenginem_async_device_register() to register the DMA engine,
relying on devres for unregistration, which executes strictly after the
remove() function returns.

During the window between this hardware shutdown and the delayed devres
unregistration, the DMA engine is still fully registered.  Could a client
that allocates a channel or submits a transfer during this time access
powered-off hardware, causing faults or synchronous aborts?


[Severity: High]
This is a pre-existing issue, but does dw_remove() leak clock references if
the device is in an active Runtime PM state?

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:dw_remove() {
	...
	clk_prepare_enable(chip->cfgr_clk);
	clk_prepare_enable(chip->core_clk);
	...
	pm_runtime_disable(chip->dev);
	axi_dma_suspend(chip);
	...
}

If the device is runtime-active when dw_remove() is invoked, its clocks are
already enabled.  dw_remove() unconditionally calls clk_prepare_enable() for
both clocks, then disables Runtime PM via pm_runtime_disable(), and finally
calls axi_dma_suspend() to disable the clocks.

Because pm_runtime_disable() prevents any further PM transitions, could the
runtime suspend callback be prevented from running and dropping the remaining
usage count, resulting in a permanent clock leak on unbind?

>  
>  static const struct dev_pm_ops dw_axi_dma_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +				pm_runtime_force_resume)
>  	SET_RUNTIME_PM_OPS(axi_dma_runtime_suspend, axi_dma_runtime_resume, NULL)
>  };

[Severity: Low]
This is a pre-existing issue, but is the dw_axi_dma_pm_ops structure assigned
unconditionally to the driver's .pm pointer without using the pm_ptr()
wrapper macro?

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:
static struct platform_driver dw_driver = {
	...
	.driver = {
		...
		.pm = &dw_axi_dma_pm_ops,
	},
};

Could this produce dead code or compiler warnings when PM support is disabled?


[Severity: High]
This isn't a bug introduced by this patch, but does axi_dma_resume() leak
the cfgr_clk reference if core_clk fails to enable?

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:axi_dma_resume() {
	ret = clk_prepare_enable(chip->cfgr_clk);
	if (ret < 0)
		return ret;

	ret = clk_prepare_enable(chip->core_clk);
	if (ret < 0)
		return ret;
	...
}

If clk_prepare_enable(chip->core_clk) fails, the function immediately returns
the error code.  Should it call clk_disable_unprepare(chip->cfgr_clk) to
clean up the successfully enabled clock?


[Severity: High]
This is a pre-existing issue, but does the runtime suspend callback
axi_dma_suspend() fail to call synchronize_irq() before disabling hardware
clocks?

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:axi_dma_suspend() {
	axi_dma_irq_disable(chip);
	axi_dma_disable(chip);

	clk_disable_unprepare(chip->core_clk);
	clk_disable_unprepare(chip->cfgr_clk);
	...
}

The driver requests its interrupt with the IRQF_SHARED flag.  Could failing to
call synchronize_irq() prior to powering down hardware introduce a race
condition where the interrupt handler might execute mid-flight on another CPU,
leading to invalid register accesses and bus faults?


[Severity: High]
This isn't a bug introduced by this patch, but does the shared interrupt
handler dw_axi_dma_interrupt() access hardware registers without ensuring
the device is runtime active?

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:dw_axi_dma_interrupt() {
	...
	axi_dma_irq_disable(chip);
	...
}

Because the driver uses IRQF_SHARED, its interrupt handler can be triggered
when another device on the same shared line asserts an interrupt.

Should the handler check pm_runtime_get_if_active() before accessing MMIO
registers to avoid a precise external abort or system crash if the DMA
controller is runtime-suspended and its clocks are disabled?


[Severity: High]
This is a pre-existing issue, but is an asynchronous pm_runtime_put() used
before jumping to an error handling path that immediately calls
pm_runtime_disable(), causing a race condition in dw_probe()?

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:dw_probe() {
	...
	pm_runtime_put(chip->dev);

	ret = dmaenginem_async_device_register(&dw->dma);
	if (ret)
		goto err_pm_disable;
	...
err_pm_disable:
	pm_runtime_disable(chip->dev);
	return ret;
}

If dmaenginem_async_device_register() fails, pm_runtime_disable() cancels
the pending async idle/suspend work.  Does this permanently leave the hardware
in an incorrect active power state and leak the clock resources?

Should a synchronous pm_runtime_put_sync() be used instead?


[Severity: High]
This isn't a bug introduced by this patch, but does the function
axi_req_irqs() write to the fixed-size chip->irq array without validating
that irq_count does not exceed the array bounds?

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:axi_req_irqs() {
	int irq_count = platform_irq_count(pdev);
	int ret;

	for (int i = 0; i < irq_count; i++) {
		chip->irq[i] = platform_get_irq(pdev, i);
		...
	}
	...
}

The chip->irq array is statically allocated with a size of DMAC_MAX_CHANNELS
(32).  If a misconfigured device tree specifies more than 32 interrupts, could
this loop write beyond the array boundaries, corrupting adjacent members in
struct axi_dma_chip and leading to arbitrary memory corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1779688569.git.tze.yee.ng@altera.com?part=2