[PATCH 0/2] dmaengine: zynqmp_dma: Add per-channel reset support

[PATCH 0/2] dmaengine: zynqmp_dma: Add per-channel reset support

[Golla Nagendra]

This series adds per-channel reset support to the ZynqMP DMA driver using the generic
reset framework, along with the corresponding dt-bindings update.

Patch 1 adds the optional 'resets' property to the ZynqMP DMA dt-binding.

Patch 2 adds reset control handling in the channel probe path to assert
and deassert the channel reset during initialization.

Golla Nagendra (1):
  dmaengine: zynqmp_dma: Add per-channel reset support

Jay Buddhabhatti (1):
  dt-bindings: dma: xilinx: Add optional resets property for ZDMA

 .../devicetree/bindings/dma/xilinx/xlnx,zynqmp-dma-1.0.yaml | 3 +++
 drivers/dma/xilinx/zynqmp_dma.c                             | 6 ++++++
 2 files changed, 9 insertions(+)

-- 
2.43.0

[PATCH 1/2] dt-bindings: dma: xilinx: Add optional resets property for ZDMA

[Golla Nagendra]

From: Jay Buddhabhatti <jay.buddhabhatti@amd.com>

Newer SoCs such as Versal Gen2 and Versal‑Net expose a reset line
for ZDMA. Older SoCs do not have this provision. Add an optional
resets property to describe this reset.

Signed-off-by: Jay Buddhabhatti <jay.buddhabhatti@amd.com>
Co-developed-by: Golla Nagendra <nagendra.golla@amd.com>
Signed-off-by: Golla Nagendra <nagendra.golla@amd.com>
---
 .../devicetree/bindings/dma/xilinx/xlnx,zynqmp-dma-1.0.yaml    | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dma-1.0.yaml b/Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dma-1.0.yaml
index 2da86037ad79..dff16763e11b 100644
--- a/Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dma-1.0.yaml
+++ b/Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dma-1.0.yaml
@@ -56,6 +56,9 @@ properties:
   iommus:
     maxItems: 1
 
+  resets:
+    maxItems: 1
+
   power-domains:
     maxItems: 1
 
-- 
2.43.0

[PATCH 2/2] dmaengine: zynqmp_dma: Add per-channel reset support

[Golla Nagendra]

Versal Gen 2 and Versal Net SoCs expose a dedicated reset line per
ZDMA channel, replacing the earlier approach where a single reset
was shared across all channels. Add reset handling in the channel
probe path using device_reset_optional() to trigger a reset pulse
on the channel during initialization.

Platforms without per-channel reset continue to work unaffected
since device_reset_optional() returns 0 when no reset is specified.

Signed-off-by: Golla Nagendra <nagendra.golla@amd.com>
---
 drivers/dma/xilinx/zynqmp_dma.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c
index f6a812e49ddc..51c831760372 100644
--- a/drivers/dma/xilinx/zynqmp_dma.c
+++ b/drivers/dma/xilinx/zynqmp_dma.c
@@ -18,6 +18,7 @@
 #include <linux/clk.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include <linux/pm_runtime.h>
+#include <linux/reset.h>
 
 #include "../dmaengine.h"
 
@@ -916,6 +917,11 @@ static int zynqmp_dma_chan_probe(struct zynqmp_dma_device *zdev,
 	if (IS_ERR(chan->regs))
 		return PTR_ERR(chan->regs);
 
+	err = device_reset_optional(&pdev->dev);
+	if (err)
+		return dev_err_probe(&pdev->dev, err,
+				     "failed to reset channel\n");
+
 	chan->bus_width = ZYNQMP_DMA_BUS_WIDTH_64;
 	chan->dst_burst_len = ZYNQMP_DMA_MAX_DST_BURST_LEN;
 	chan->src_burst_len = ZYNQMP_DMA_MAX_SRC_BURST_LEN;
-- 
2.43.0

Re: [PATCH 1/2] dt-bindings: dma: xilinx: Add optional resets property for ZDMA

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The `resets` property is added globally without conditional constraints, allowing it to pass validation on older SoCs that do not support it in hardware.
--

commit 832c482914e7639cb1a410d061bf9d631b41ae68
Author: Jay Buddhabhatti <jay.buddhabhatti@amd.com>

dt-bindings: dma: xilinx: Add optional resets property for ZDMA

This commit adds an optional resets property for ZDMA on newer SoCs like
Versal Gen2 and Versal-Net. Older SoCs do not have a reset line provision.

> diff --git a/Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dma-1.0.yaml b/Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dma-1.0.yaml
> index 2da86037ad79e..dff16763e11b9 100644
> --- a/Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dma-1.0.yaml
> +++ b/Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dma-1.0.yaml
> @@ -56,6 +56,9 @@ properties:
>    iommus:
>      maxItems: 1
>  
> +  resets:
> +    maxItems: 1
> +

[Severity: Medium]
The commit message notes that older SoCs do not support this reset line.
Since the resets property is added to the top-level properties block, will
this allow device trees for older hardware to erroneously define a reset line
and silently pass validation?

Should this be guarded by an allOf conditional block restricting the resets
property to the newer hardware compatible strings?

>    power-domains:
>      maxItems: 1

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525105042.2249542-1-nagendra.golla@amd.com?part=1

Re: [PATCH 2/2] dmaengine: zynqmp_dma: Add per-channel reset support

[sashiko-bot]

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

Pre-existing issues:
- [High] The probe error path leaks a runtime PM usage counter reference, permanently breaking runtime PM and leaving hardware clocks enabled.
- [High] Dropping the spinlock inside `list_for_each_entry_safe` allows concurrent modification of the list, leading to memory corruption.
- [High] Hardware registers are accessed in the IRQ handler without verifying if the device is active, risking a kernel panic on spurious interrupts.
--

commit b2d2a678f48614c7188f8d81cbc70143b2bdfc0f
Author: Golla Nagendra <nagendra.golla@amd.com>
Subject: dmaengine: zynqmp_dma: Add per-channel reset support

This commit introduces per-channel reset support for Versal Gen 2 and
Versal Net SoCs. It adds reset handling in the channel probe path using
device_reset_optional() to trigger a reset pulse during initialization.
Platforms without per-channel reset are unaffected.

> diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c
> index f6a812e49ddca..51c831760372c 100644
> --- a/drivers/dma/xilinx/zynqmp_dma.c
> +++ b/drivers/dma/xilinx/zynqmp_dma.c
[ ... ]
> @@ -916,6 +917,11 @@ static int zynqmp_dma_chan_probe(struct zynqmp_dma_device *zdev,
>  	if (IS_ERR(chan->regs))
>  		return PTR_ERR(chan->regs);
>  
> +	err = device_reset_optional(&pdev->dev);
> +	if (err)
> +		return dev_err_probe(&pdev->dev, err,
> +				     "failed to reset channel\n");
> +

[Severity: High]
This isn't a bug introduced by this patch, but does this error path leak a
runtime PM usage counter reference?

In zynqmp_dma_probe(), pm_runtime_resume_and_get() is called to increment
the runtime PM usage counter and enable the device clocks. If a subsequent
initialization step fails, such as the newly added device_reset_optional()
returning -EPROBE_DEFER, the code jumps to the err_disable_pm label:

zynqmp_dma_probe()
    ...
	ret = pm_runtime_resume_and_get(zdev->dev);
    ...
	ret = zynqmp_dma_chan_probe(zdev, pdev);
	if (ret) {
		dev_err_probe(&pdev->dev, ret, "Probing channel failed\n");
		goto err_disable_pm;
	}
    ...
err_disable_pm:
	if (!pm_runtime_enabled(zdev->dev))
		zynqmp_dma_runtime_suspend(zdev->dev);
	pm_runtime_disable(zdev->dev);
	return ret;

Since the device is still runtime PM enabled, it skips
zynqmp_dma_runtime_suspend() and fails to drop the usage counter with
pm_runtime_put_noidle().

Because -EPROBE_DEFER is a normal occurrence during boot, this path can
be hit multiple times. Will this permanently leave the usage counter above
zero, preventing the device from ever entering runtime suspend and leaving
its clocks enabled indefinitely?

>  	chan->bus_width = ZYNQMP_DMA_BUS_WIDTH_64;
>  	chan->dst_burst_len = ZYNQMP_DMA_MAX_DST_BURST_LEN;

[Severity: High]
This is a pre-existing issue, but does dropping the spinlock inside
list_for_each_entry_safe() in zynqmp_dma_chan_desc_cleanup() allow concurrent
modification of the list, leading to memory corruption?

When zynqmp_dma_chan_desc_cleanup() temporarily drops the lock to invoke
the DMA callback:

zynqmp_dma_chan_desc_cleanup()
    ...
	list_for_each_entry_safe(desc, next, &chan->done_list, node) {
        ...
		if (dmaengine_desc_callback_valid(&cb)) {
			spin_unlock_irqrestore(&chan->lock, irqflags);
			dmaengine_desc_callback_invoke(&cb, NULL);
			spin_lock_irqsave(&chan->lock, irqflags);
		}

A client driver can concurrently invoke dmaengine_terminate_sync(), which
calls zynqmp_dma_device_terminate_all() and executes
zynqmp_dma_free_descriptors(). This moves all remaining elements, including
the prefetched next pointer, from done_list to free_list.

When zynqmp_dma_chan_desc_cleanup() reacquires the lock, won't it continue
iterating using the corrupted next pointer that now resides in free_list,
eventually interpreting the list head &chan->free_list as a struct
zynqmp_dma_desc_sw and resulting in out-of-bounds memory accesses?

[Severity: High]
This is a pre-existing issue, but are hardware registers accessed in the
IRQ handler without verifying if the device is active, risking a kernel
panic on spurious interrupts?

The IRQ handler zynqmp_dma_irq_handler() directly reads the ISR and IMR
hardware registers:

zynqmp_dma_irq_handler()
    ...
	isr = readl(chan->regs + ZYNQMP_DMA_ISR);
	imr = readl(chan->regs + ZYNQMP_DMA_IMR);

According to the Power Management Subsystem guidelines, IRQ handlers must
use pm_runtime_get_if_active() before accessing hardware registers.

Although this IRQ is not shared, spurious interrupts can still occur. If a
spurious interrupt fires while the device is in a suspended state with its
clocks disabled via zynqmp_dma_runtime_suspend(), could this unprotected
register read lead to a synchronous external abort and crash the kernel?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525105042.2249542-1-nagendra.golla@amd.com?part=2

Re: [PATCH 1/2] dt-bindings: dma: xilinx: Add optional resets property for ZDMA

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1887 bytes --]

On Mon, May 25, 2026 at 11:00:25AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The `resets` property is added globally without conditional constraints, allowing it to pass validation on older SoCs that do not support it in hardware.
> --
> 
> commit 832c482914e7639cb1a410d061bf9d631b41ae68
> Author: Jay Buddhabhatti <jay.buddhabhatti@amd.com>
> 
> dt-bindings: dma: xilinx: Add optional resets property for ZDMA
> 
> This commit adds an optional resets property for ZDMA on newer SoCs like
> Versal Gen2 and Versal-Net. Older SoCs do not have a reset line provision.
> 
> > diff --git a/Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dma-1.0.yaml b/Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dma-1.0.yaml
> > index 2da86037ad79e..dff16763e11b9 100644
> > --- a/Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dma-1.0.yaml
> > +++ b/Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dma-1.0.yaml
> > @@ -56,6 +56,9 @@ properties:
> >    iommus:
> >      maxItems: 1
> >  
> > +  resets:
> > +    maxItems: 1
> > +
> 
> [Severity: Medium]
> The commit message notes that older SoCs do not support this reset line.
> Since the resets property is added to the top-level properties block, will
> this allow device trees for older hardware to erroneously define a reset line
> and silently pass validation?
> 
> Should this be guarded by an allOf conditional block restricting the resets
> property to the newer hardware compatible strings?

Additionally, should it be required on the new platforms?


Either way,
pw-bot: changes-requested


> 
> >    power-domains:
> >      maxItems: 1
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260525105042.2249542-1-nagendra.golla@amd.com?part=1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH 1/2] dt-bindings: dma: xilinx: Add optional resets property for ZDMA

[Golla, Nagendra]

AMD General

>-----Original Message-----
>From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
>Sent: Monday, May 25, 2026 4:30 PM
>To: Golla, Nagendra <Nagendra.Golla@amd.com>
>Cc: devicetree@vger.kernel.org; dmaengine@vger.kernel.org;
>robh@kernel.org; vkoul@kernel.org; Frank.Li@kernel.org;
>conor+dt@kernel.org
>Subject: Re: [PATCH 1/2] dt-bindings: dma: xilinx: Add optional resets property
>for ZDMA
>
>Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
>to consider:
>- [Medium] The `resets` property is added globally without conditional
>constraints, allowing it to pass validation on older SoCs that do not support it
>in hardware.
>--
>
>commit 832c482914e7639cb1a410d061bf9d631b41ae68
>Author: Jay Buddhabhatti <jay.buddhabhatti@amd.com>
>
>dt-bindings: dma: xilinx: Add optional resets property for ZDMA
>
>This commit adds an optional resets property for ZDMA on newer SoCs like
>Versal Gen2 and Versal-Net. Older SoCs do not have a reset line provision.
>
>> diff --git
>> a/Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dma-1.0.yam
>> l
>> b/Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dma-1.0.yam
>> l index 2da86037ad79e..dff16763e11b9 100644
>> ---
>> a/Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dma-1.0.yam
>> l
>> +++ b/Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dma-1.0
>> +++ .yaml
>> @@ -56,6 +56,9 @@ properties:
>>    iommus:
>>      maxItems: 1
>>
>> +  resets:
>> +    maxItems: 1
>> +
>
>[Severity: Medium]
>The commit message notes that older SoCs do not support this reset line.
>Since the resets property is added to the top-level properties block, will this
>allow device trees for older hardware to erroneously define a reset line and
>silently pass validation?
>
>Should this be guarded by an allOf conditional block restricting the resets
>property to the newer hardware compatible strings?

Hi,

Tying the resets property to a specific compatible string is not feasible here. Versal Net reuses the older "xlnx,zynqmp-dma-1.0" compatible
while Versal Gen 2 uses the new "amd,versal2-dma-1.0" — both platforms require per-channel reset support. An allOf conditional block
would not be able to cover both without also matching the older SoCs that share the same compatible.

The property is intentionally optional. On the driver side, device_reset_optional() returns 0 when no reset is specified in the DT, so there is
no adverse effect if someone erroneously adds it on hardware that lacks reset support — the property is simply ignored.

Thanks,
Nagendra
>
>>    power-domains:
>>      maxItems: 1
>
>--
>Sashiko AI review · https://sashiko.dev/#/patchset/20260525105042.2249542-
>1-nagendra.golla@amd.com?part=1

RE: [PATCH 1/2] dt-bindings: dma: xilinx: Add optional resets property for ZDMA

[Golla, Nagendra]

AMD General

>-----Original Message-----
>From: Conor Dooley <conor@kernel.org>
>Sent: Monday, May 25, 2026 10:33 PM
>To: sashiko-reviews@lists.linux.dev
>Cc: Golla, Nagendra <Nagendra.Golla@amd.com>; devicetree@vger.kernel.org;
>dmaengine@vger.kernel.org; robh@kernel.org; vkoul@kernel.org;
>Frank.Li@kernel.org; conor+dt@kernel.org
>Subject: Re: [PATCH 1/2] dt-bindings: dma: xilinx: Add optional resets property
>for ZDMA
>
>On Mon, May 25, 2026 at 11:00:25AM +0000, sashiko-bot@kernel.org wrote:
>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
>to consider:
>> - [Medium] The `resets` property is added globally without conditional
>constraints, allowing it to pass validation on older SoCs that do not support it in
>hardware.
>> --
>>
>> commit 832c482914e7639cb1a410d061bf9d631b41ae68
>> Author: Jay Buddhabhatti <jay.buddhabhatti@amd.com>
>>
>> dt-bindings: dma: xilinx: Add optional resets property for ZDMA
>>
>> This commit adds an optional resets property for ZDMA on newer SoCs
>> like Versal Gen2 and Versal-Net. Older SoCs do not have a reset line provision.
>>
>> > diff --git
>> > a/Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dma-1.0.y
>> > aml
>> > b/Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dma-1.0.y
>> > aml index 2da86037ad79e..dff16763e11b9 100644
>> > ---
>> > a/Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dma-1.0.y
>> > aml
>> > +++ b/Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-dma-1
>> > +++ .0.yaml
>> > @@ -56,6 +56,9 @@ properties:
>> >    iommus:
>> >      maxItems: 1
>> >
>> > +  resets:
>> > +    maxItems: 1
>> > +
>>
>> [Severity: Medium]
>> The commit message notes that older SoCs do not support this reset line.
>> Since the resets property is added to the top-level properties block,
>> will this allow device trees for older hardware to erroneously define
>> a reset line and silently pass validation?
>>
>> Should this be guarded by an allOf conditional block restricting the
>> resets property to the newer hardware compatible strings?
>
>Additionally, should it be required on the new platforms?

Hi,

Currently, the resets property is optional for both Versal Net and Versal Gen 2. If future platforms add per-channel reset support, we
can add an allOf conditional at that point to make the resets property required for those specific compatible strings.

Thanks,
Nagendra

>
>
>Either way,
>pw-bot: changes-requested
>
>
>>
>> >    power-domains:
>> >      maxItems: 1
>>
>> --
>> Sashiko AI review *
>> https://sashiko.dev/#/patchset/20260525105042.2249542-1-
>nagendra.golla
>> @amd.com?part=1

Re: [PATCH 2/2] dmaengine: zynqmp_dma: Add per-channel reset support

[Golla, Nagendra]

On 5/25/2026 5:03 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] The probe error path leaks a runtime PM usage counter reference, permanently breaking runtime PM and leaving hardware clocks enabled.
> - [High] Dropping the spinlock inside `list_for_each_entry_safe` allows concurrent modification of the list, leading to memory corruption.
> - [High] Hardware registers are accessed in the IRQ handler without verifying if the device is active, risking a kernel panic on spurious interrupts.
> --
> 
> commit b2d2a678f48614c7188f8d81cbc70143b2bdfc0f
> Author: Golla Nagendra <nagendra.golla@amd.com>
> Subject: dmaengine: zynqmp_dma: Add per-channel reset support
> 
> This commit introduces per-channel reset support for Versal Gen 2 and
> Versal Net SoCs. It adds reset handling in the channel probe path using
> device_reset_optional() to trigger a reset pulse during initialization.
> Platforms without per-channel reset are unaffected.
> 
>> diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c
>> index f6a812e49ddca..51c831760372c 100644
>> --- a/drivers/dma/xilinx/zynqmp_dma.c
>> +++ b/drivers/dma/xilinx/zynqmp_dma.c
> [ ... ]
>> @@ -916,6 +917,11 @@ static int zynqmp_dma_chan_probe(struct zynqmp_dma_device *zdev,
>>   	if (IS_ERR(chan->regs))
>>   		return PTR_ERR(chan->regs);
>>   
>> +	err = device_reset_optional(&pdev->dev);
>> +	if (err)
>> +		return dev_err_probe(&pdev->dev, err,
>> +				     "failed to reset channel\n");
>> +
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does this error path leak a
> runtime PM usage counter reference?
> 
> In zynqmp_dma_probe(), pm_runtime_resume_and_get() is called to increment
> the runtime PM usage counter and enable the device clocks. If a subsequent
> initialization step fails, such as the newly added device_reset_optional()
> returning -EPROBE_DEFER, the code jumps to the err_disable_pm label:
> 
> zynqmp_dma_probe()
>      ...
> 	ret = pm_runtime_resume_and_get(zdev->dev);
>      ...
> 	ret = zynqmp_dma_chan_probe(zdev, pdev);
> 	if (ret) {
> 		dev_err_probe(&pdev->dev, ret, "Probing channel failed\n");
> 		goto err_disable_pm;
> 	}
>      ...
> err_disable_pm:
> 	if (!pm_runtime_enabled(zdev->dev))
> 		zynqmp_dma_runtime_suspend(zdev->dev);
> 	pm_runtime_disable(zdev->dev);
> 	return ret;
> 
> Since the device is still runtime PM enabled, it skips
> zynqmp_dma_runtime_suspend() and fails to drop the usage counter with
> pm_runtime_put_noidle().
> 
> Because -EPROBE_DEFER is a normal occurrence during boot, this path can
> be hit multiple times. Will this permanently leave the usage counter above
> zero, preventing the device from ever entering runtime suspend and leaving
> its clocks enabled indefinitely?
> 
>>   	chan->bus_width = ZYNQMP_DMA_BUS_WIDTH_64;
>>   	chan->dst_burst_len = ZYNQMP_DMA_MAX_DST_BURST_LEN;
> 
> [Severity: High]
> This is a pre-existing issue, but does dropping the spinlock inside
> list_for_each_entry_safe() in zynqmp_dma_chan_desc_cleanup() allow concurrent
> modification of the list, leading to memory corruption?
> 
> When zynqmp_dma_chan_desc_cleanup() temporarily drops the lock to invoke
> the DMA callback:
> 
> zynqmp_dma_chan_desc_cleanup()
>      ...
> 	list_for_each_entry_safe(desc, next, &chan->done_list, node) {
>          ...
> 		if (dmaengine_desc_callback_valid(&cb)) {
> 			spin_unlock_irqrestore(&chan->lock, irqflags);
> 			dmaengine_desc_callback_invoke(&cb, NULL);
> 			spin_lock_irqsave(&chan->lock, irqflags);
> 		}
> 
> A client driver can concurrently invoke dmaengine_terminate_sync(), which
> calls zynqmp_dma_device_terminate_all() and executes
> zynqmp_dma_free_descriptors(). This moves all remaining elements, including
> the prefetched next pointer, from done_list to free_list.
> 
> When zynqmp_dma_chan_desc_cleanup() reacquires the lock, won't it continue
> iterating using the corrupted next pointer that now resides in free_list,
> eventually interpreting the list head &chan->free_list as a struct
> zynqmp_dma_desc_sw and resulting in out-of-bounds memory accesses?
> 
> [Severity: High]
> This is a pre-existing issue, but are hardware registers accessed in the
> IRQ handler without verifying if the device is active, risking a kernel
> panic on spurious interrupts?

Hi,

Thanks for pointing this out.After reviewing the runtime PM guidelines 
and the driver behavior, I believe this is not a strict requirement for 
all IRQ handlers. The PM documentation does not mandate that 
pm_runtime_get_if_active() must be used before every register access in 
an ISR. It is mainly intended for cases where an interrupt can occur 
while the device might be runtime-suspended, such as shared IRQ or 
spurious interrupt scenarios.

In the case of the zynqmp DMA driver, the interrupt is generated only 
when the hardware is active during a transfer, and the driver ensures 
the device remains runtime-active while DMA operations are in progress. 
Because of this, the ISR is not expected to run when the device is 
suspended, and accessing registers directly should be safe.

So, in this context, adding pm_runtime_get_if_active() in the IRQ 
handler may not be necessary. That said, please let me know  if you see 
a scenario where the interrupt could be triggered while the device is 
runtime-suspended, and I can revisit accordingly.

Thanks,
Nagendra


> 
> The IRQ handler zynqmp_dma_irq_handler() directly reads the ISR and IMR
> hardware registers:
> 
> zynqmp_dma_irq_handler()
>      ...
> 	isr = readl(chan->regs + ZYNQMP_DMA_ISR);
> 	imr = readl(chan->regs + ZYNQMP_DMA_IMR);
> 
> According to the Power Management Subsystem guidelines, IRQ handlers must
> use pm_runtime_get_if_active() before accessing hardware registers.
> 
> Although this IRQ is not shared, spurious interrupts can still occur. If a
> spurious interrupt fires while the device is in a suspended state with its
> clocks disabled via zynqmp_dma_runtime_suspend(), could this unprotected
> register read lead to a synchronous external abort and crash the kernel?
> 

Re: [PATCH 2/2] dmaengine: zynqmp_dma: Add per-channel reset support

[Vinod Koul]

On 15-06-26, 14:59, Golla, Nagendra wrote:
> 
> 
> On 5/25/2026 5:03 PM, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> > 
> > Pre-existing issues:
> > - [High] The probe error path leaks a runtime PM usage counter reference, permanently breaking runtime PM and leaving hardware clocks enabled.
> > - [High] Dropping the spinlock inside `list_for_each_entry_safe` allows concurrent modification of the list, leading to memory corruption.
> > - [High] Hardware registers are accessed in the IRQ handler without verifying if the device is active, risking a kernel panic on spurious interrupts.
> > --
> > 
> > commit b2d2a678f48614c7188f8d81cbc70143b2bdfc0f
> > Author: Golla Nagendra <nagendra.golla@amd.com>
> > Subject: dmaengine: zynqmp_dma: Add per-channel reset support
> > 
> > This commit introduces per-channel reset support for Versal Gen 2 and
> > Versal Net SoCs. It adds reset handling in the channel probe path using
> > device_reset_optional() to trigger a reset pulse during initialization.
> > Platforms without per-channel reset are unaffected.
> > 
> > > diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c
> > > index f6a812e49ddca..51c831760372c 100644
> > > --- a/drivers/dma/xilinx/zynqmp_dma.c
> > > +++ b/drivers/dma/xilinx/zynqmp_dma.c
> > [ ... ]
> > > @@ -916,6 +917,11 @@ static int zynqmp_dma_chan_probe(struct zynqmp_dma_device *zdev,
> > >   	if (IS_ERR(chan->regs))
> > >   		return PTR_ERR(chan->regs);
> > > +	err = device_reset_optional(&pdev->dev);
> > > +	if (err)
> > > +		return dev_err_probe(&pdev->dev, err,
> > > +				     "failed to reset channel\n");
> > > +
> > 
> > [Severity: High]
> > This isn't a bug introduced by this patch, but does this error path leak a
> > runtime PM usage counter reference?
> > 
> > In zynqmp_dma_probe(), pm_runtime_resume_and_get() is called to increment
> > the runtime PM usage counter and enable the device clocks. If a subsequent
> > initialization step fails, such as the newly added device_reset_optional()
> > returning -EPROBE_DEFER, the code jumps to the err_disable_pm label:
> > 
> > zynqmp_dma_probe()
> >      ...
> > 	ret = pm_runtime_resume_and_get(zdev->dev);
> >      ...
> > 	ret = zynqmp_dma_chan_probe(zdev, pdev);
> > 	if (ret) {
> > 		dev_err_probe(&pdev->dev, ret, "Probing channel failed\n");
> > 		goto err_disable_pm;
> > 	}
> >      ...
> > err_disable_pm:
> > 	if (!pm_runtime_enabled(zdev->dev))
> > 		zynqmp_dma_runtime_suspend(zdev->dev);
> > 	pm_runtime_disable(zdev->dev);
> > 	return ret;
> > 
> > Since the device is still runtime PM enabled, it skips
> > zynqmp_dma_runtime_suspend() and fails to drop the usage counter with
> > pm_runtime_put_noidle().
> > 
> > Because -EPROBE_DEFER is a normal occurrence during boot, this path can
> > be hit multiple times. Will this permanently leave the usage counter above
> > zero, preventing the device from ever entering runtime suspend and leaving
> > its clocks enabled indefinitely?
> > 
> > >   	chan->bus_width = ZYNQMP_DMA_BUS_WIDTH_64;
> > >   	chan->dst_burst_len = ZYNQMP_DMA_MAX_DST_BURST_LEN;
> > 
> > [Severity: High]
> > This is a pre-existing issue, but does dropping the spinlock inside
> > list_for_each_entry_safe() in zynqmp_dma_chan_desc_cleanup() allow concurrent
> > modification of the list, leading to memory corruption?
> > 
> > When zynqmp_dma_chan_desc_cleanup() temporarily drops the lock to invoke
> > the DMA callback:
> > 
> > zynqmp_dma_chan_desc_cleanup()
> >      ...
> > 	list_for_each_entry_safe(desc, next, &chan->done_list, node) {
> >          ...
> > 		if (dmaengine_desc_callback_valid(&cb)) {
> > 			spin_unlock_irqrestore(&chan->lock, irqflags);
> > 			dmaengine_desc_callback_invoke(&cb, NULL);
> > 			spin_lock_irqsave(&chan->lock, irqflags);
> > 		}
> > 
> > A client driver can concurrently invoke dmaengine_terminate_sync(), which
> > calls zynqmp_dma_device_terminate_all() and executes
> > zynqmp_dma_free_descriptors(). This moves all remaining elements, including
> > the prefetched next pointer, from done_list to free_list.
> > 
> > When zynqmp_dma_chan_desc_cleanup() reacquires the lock, won't it continue
> > iterating using the corrupted next pointer that now resides in free_list,
> > eventually interpreting the list head &chan->free_list as a struct
> > zynqmp_dma_desc_sw and resulting in out-of-bounds memory accesses?
> > 
> > [Severity: High]
> > This is a pre-existing issue, but are hardware registers accessed in the
> > IRQ handler without verifying if the device is active, risking a kernel
> > panic on spurious interrupts?
> 
> Hi,
> 
> Thanks for pointing this out.After reviewing the runtime PM guidelines and
> the driver behavior, I believe this is not a strict requirement for all IRQ
> handlers. The PM documentation does not mandate that
> pm_runtime_get_if_active() must be used before every register access in an
> ISR. It is mainly intended for cases where an interrupt can occur while the
> device might be runtime-suspended, such as shared IRQ or spurious interrupt
> scenarios.
> 
> In the case of the zynqmp DMA driver, the interrupt is generated only when
> the hardware is active during a transfer, and the driver ensures the device
> remains runtime-active while DMA operations are in progress. Because of
> this, the ISR is not expected to run when the device is suspended, and
> accessing registers directly should be safe.
> 
> So, in this context, adding pm_runtime_get_if_active() in the IRQ handler
> may not be necessary. That said, please let me know  if you see a scenario
> where the interrupt could be triggered while the device is
> runtime-suspended, and I can revisit accordingly.

We can have a spurious interrupt. if you have an isr registered, your
cide should be able to handle the invocation... I think it is indeed
pragmatic to handle this

-- 
~Vinod