[PATCH 0/4] dmaengine: dw-edma: Fix probe paths and register races

[PATCH 0/4] dmaengine: dw-edma: Fix probe paths and register races

[Koichiro Den]

Hi,

This series fixes pre-existing dw-edma issues flagged by Sashiko in:
https://lore.kernel.org/dmaengine/20260521063115.2842238-1-den@valinux.co.jp/

Note: Patch 4 was based on a patch Frank posted in January:
      https://lore.kernel.org/dmaengine/20260109-edma_ll-v2-1-5c0b27b2c664@nxp.com/
      Since it has not been merged, I included it here. Frank, please let me
      know if you prefer a different handling.

Best regards,
Koichiro


Frank Li (1):
  dmaengine: dw-edma: Add spinlock to protect DONE_INT_MASK and
    ABORT_INT_MASK

Koichiro Den (3):
  dmaengine: dw-edma-pcie: Free IRQ vectors on probe failures
  dmaengine: dw-edma-pcie: Reject devices without driver data
  dmaengine: dw-edma: Initialize IRQ data before requesting IRQs

 drivers/dma/dw-edma/dw-edma-core.c    |  3 +-
 drivers/dma/dw-edma/dw-edma-core.h    |  2 +-
 drivers/dma/dw-edma/dw-edma-pcie.c    | 42 +++++++++++++++++++--------
 drivers/dma/dw-edma/dw-edma-v0-core.c |  6 ++++
 4 files changed, 39 insertions(+), 14 deletions(-)

-- 
2.51.0

[PATCH 1/4] dmaengine: dw-edma-pcie: Free IRQ vectors on probe failures

[Koichiro Den]

dw_edma_pcie_probe() leaks IRQ vectors by returning without calling
pci_free_irq_vectors() in error paths after pci_alloc_irq_vectors()
succeeds.

Route the post-allocation failures through a common cleanup path so the
vectors are released before probe returns.

Fixes: 41aaff2a2ac0 ("dmaengine: Add Synopsys eDMA IP PCIe glue-logic")
Cc: stable@vger.kernel.org
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/dma/dw-edma/dw-edma-pcie.c | 39 +++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
index 0b30ce138503..87c31d01fb10 100644
--- a/drivers/dma/dw-edma/dw-edma-pcie.c
+++ b/drivers/dma/dw-edma/dw-edma-pcie.c
@@ -410,8 +410,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
 	chip->ll_rd_cnt = vsec_data->rd_ch_cnt;
 
 	chip->reg_base = pcim_iomap_table(pdev)[vsec_data->rg.bar];
-	if (!chip->reg_base)
-		return -ENOMEM;
+	if (!chip->reg_base) {
+		err = -ENOMEM;
+		goto err_free_irq_vectors;
+	}
 
 	for (i = 0; i < chip->ll_wr_cnt && !non_ll; i++) {
 		struct dw_edma_region *ll_region = &chip->ll_region_wr[i];
@@ -420,8 +422,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
 		struct dw_edma_block *dt_block = &vsec_data->dt_wr[i];
 
 		ll_region->vaddr.io = pcim_iomap_table(pdev)[ll_block->bar];
-		if (!ll_region->vaddr.io)
-			return -ENOMEM;
+		if (!ll_region->vaddr.io) {
+			err = -ENOMEM;
+			goto err_free_irq_vectors;
+		}
 
 		ll_region->vaddr.io += ll_block->off;
 		ll_region->paddr = dw_edma_get_phys_addr(pdev, vsec_data,
@@ -430,8 +434,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
 		ll_region->sz = ll_block->sz;
 
 		dt_region->vaddr.io = pcim_iomap_table(pdev)[dt_block->bar];
-		if (!dt_region->vaddr.io)
-			return -ENOMEM;
+		if (!dt_region->vaddr.io) {
+			err = -ENOMEM;
+			goto err_free_irq_vectors;
+		}
 
 		dt_region->vaddr.io += dt_block->off;
 		dt_region->paddr = dw_edma_get_phys_addr(pdev, vsec_data,
@@ -447,8 +453,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
 		struct dw_edma_block *dt_block = &vsec_data->dt_rd[i];
 
 		ll_region->vaddr.io = pcim_iomap_table(pdev)[ll_block->bar];
-		if (!ll_region->vaddr.io)
-			return -ENOMEM;
+		if (!ll_region->vaddr.io) {
+			err = -ENOMEM;
+			goto err_free_irq_vectors;
+		}
 
 		ll_region->vaddr.io += ll_block->off;
 		ll_region->paddr = dw_edma_get_phys_addr(pdev, vsec_data,
@@ -457,8 +465,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
 		ll_region->sz = ll_block->sz;
 
 		dt_region->vaddr.io = pcim_iomap_table(pdev)[dt_block->bar];
-		if (!dt_region->vaddr.io)
-			return -ENOMEM;
+		if (!dt_region->vaddr.io) {
+			err = -ENOMEM;
+			goto err_free_irq_vectors;
+		}
 
 		dt_region->vaddr.io += dt_block->off;
 		dt_region->paddr = dw_edma_get_phys_addr(pdev, vsec_data,
@@ -513,20 +523,25 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
 	/* Validating if PCI interrupts were enabled */
 	if (!pci_dev_msi_enabled(pdev)) {
 		pci_err(pdev, "enable interrupt failed\n");
-		return -EPERM;
+		err = -EPERM;
+		goto err_free_irq_vectors;
 	}
 
 	/* Starting eDMA driver */
 	err = dw_edma_probe(chip);
 	if (err) {
 		pci_err(pdev, "eDMA probe failed\n");
-		return err;
+		goto err_free_irq_vectors;
 	}
 
 	/* Saving data structure reference */
 	pci_set_drvdata(pdev, chip);
 
 	return 0;
+
+err_free_irq_vectors:
+	pci_free_irq_vectors(pdev);
+	return err;
 }
 
 static void dw_edma_pcie_remove(struct pci_dev *pdev)
-- 
2.51.0

Re: [PATCH 1/4] dmaengine: dw-edma-pcie: Free IRQ vectors on probe failures

[Frank Li]

On Thu, May 21, 2026 at 11:21:50PM +0900, Koichiro Den wrote:
> dw_edma_pcie_probe() leaks IRQ vectors by returning without calling
> pci_free_irq_vectors() in error paths after pci_alloc_irq_vectors()
> succeeds.

I remember pcim_enable_device() already auto manage irqs.

Frank

>
> Route the post-allocation failures through a common cleanup path so the
> vectors are released before probe returns.
>
> Fixes: 41aaff2a2ac0 ("dmaengine: Add Synopsys eDMA IP PCIe glue-logic")
> Cc: stable@vger.kernel.org
> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> ---
>  drivers/dma/dw-edma/dw-edma-pcie.c | 39 +++++++++++++++++++++---------
>  1 file changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
> index 0b30ce138503..87c31d01fb10 100644
> --- a/drivers/dma/dw-edma/dw-edma-pcie.c
> +++ b/drivers/dma/dw-edma/dw-edma-pcie.c
> @@ -410,8 +410,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
>  	chip->ll_rd_cnt = vsec_data->rd_ch_cnt;
>
>  	chip->reg_base = pcim_iomap_table(pdev)[vsec_data->rg.bar];
> -	if (!chip->reg_base)
> -		return -ENOMEM;
> +	if (!chip->reg_base) {
> +		err = -ENOMEM;
> +		goto err_free_irq_vectors;
> +	}
>
>  	for (i = 0; i < chip->ll_wr_cnt && !non_ll; i++) {
>  		struct dw_edma_region *ll_region = &chip->ll_region_wr[i];
> @@ -420,8 +422,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
>  		struct dw_edma_block *dt_block = &vsec_data->dt_wr[i];
>
>  		ll_region->vaddr.io = pcim_iomap_table(pdev)[ll_block->bar];
> -		if (!ll_region->vaddr.io)
> -			return -ENOMEM;
> +		if (!ll_region->vaddr.io) {
> +			err = -ENOMEM;
> +			goto err_free_irq_vectors;
> +		}
>
>  		ll_region->vaddr.io += ll_block->off;
>  		ll_region->paddr = dw_edma_get_phys_addr(pdev, vsec_data,
> @@ -430,8 +434,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
>  		ll_region->sz = ll_block->sz;
>
>  		dt_region->vaddr.io = pcim_iomap_table(pdev)[dt_block->bar];
> -		if (!dt_region->vaddr.io)
> -			return -ENOMEM;
> +		if (!dt_region->vaddr.io) {
> +			err = -ENOMEM;
> +			goto err_free_irq_vectors;
> +		}
>
>  		dt_region->vaddr.io += dt_block->off;
>  		dt_region->paddr = dw_edma_get_phys_addr(pdev, vsec_data,
> @@ -447,8 +453,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
>  		struct dw_edma_block *dt_block = &vsec_data->dt_rd[i];
>
>  		ll_region->vaddr.io = pcim_iomap_table(pdev)[ll_block->bar];
> -		if (!ll_region->vaddr.io)
> -			return -ENOMEM;
> +		if (!ll_region->vaddr.io) {
> +			err = -ENOMEM;
> +			goto err_free_irq_vectors;
> +		}
>
>  		ll_region->vaddr.io += ll_block->off;
>  		ll_region->paddr = dw_edma_get_phys_addr(pdev, vsec_data,
> @@ -457,8 +465,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
>  		ll_region->sz = ll_block->sz;
>
>  		dt_region->vaddr.io = pcim_iomap_table(pdev)[dt_block->bar];
> -		if (!dt_region->vaddr.io)
> -			return -ENOMEM;
> +		if (!dt_region->vaddr.io) {
> +			err = -ENOMEM;
> +			goto err_free_irq_vectors;
> +		}
>
>  		dt_region->vaddr.io += dt_block->off;
>  		dt_region->paddr = dw_edma_get_phys_addr(pdev, vsec_data,
> @@ -513,20 +523,25 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
>  	/* Validating if PCI interrupts were enabled */
>  	if (!pci_dev_msi_enabled(pdev)) {
>  		pci_err(pdev, "enable interrupt failed\n");
> -		return -EPERM;
> +		err = -EPERM;
> +		goto err_free_irq_vectors;
>  	}
>
>  	/* Starting eDMA driver */
>  	err = dw_edma_probe(chip);
>  	if (err) {
>  		pci_err(pdev, "eDMA probe failed\n");
> -		return err;
> +		goto err_free_irq_vectors;
>  	}
>
>  	/* Saving data structure reference */
>  	pci_set_drvdata(pdev, chip);
>
>  	return 0;
> +
> +err_free_irq_vectors:
> +	pci_free_irq_vectors(pdev);
> +	return err;
>  }
>
>  static void dw_edma_pcie_remove(struct pci_dev *pdev)
> --
> 2.51.0
>

Re: [PATCH 1/4] dmaengine: dw-edma-pcie: Free IRQ vectors on probe failures

[Koichiro Den]

On Thu, May 21, 2026 at 10:39:09AM -0400, Frank Li wrote:
> On Thu, May 21, 2026 at 11:21:50PM +0900, Koichiro Den wrote:
> > dw_edma_pcie_probe() leaks IRQ vectors by returning without calling
> > pci_free_irq_vectors() in error paths after pci_alloc_irq_vectors()
> > succeeds.
> 
> I remember pcim_enable_device() already auto manage irqs.

You are right, pcim_enable_device() already manages the IRQ vectors.

Thanks for pointing it out. I'll drop patch 1 if I need to respin, or ask the
maintainers to disregard it when applying.

Best regards,
Koichiro

> 
> Frank
> 
> >
> > Route the post-allocation failures through a common cleanup path so the
> > vectors are released before probe returns.
> >
> > Fixes: 41aaff2a2ac0 ("dmaengine: Add Synopsys eDMA IP PCIe glue-logic")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Koichiro Den <den@valinux.co.jp>
> > ---
> >  drivers/dma/dw-edma/dw-edma-pcie.c | 39 +++++++++++++++++++++---------
> >  1 file changed, 27 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
> > index 0b30ce138503..87c31d01fb10 100644
> > --- a/drivers/dma/dw-edma/dw-edma-pcie.c
> > +++ b/drivers/dma/dw-edma/dw-edma-pcie.c
> > @@ -410,8 +410,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
> >  	chip->ll_rd_cnt = vsec_data->rd_ch_cnt;
> >
> >  	chip->reg_base = pcim_iomap_table(pdev)[vsec_data->rg.bar];
> > -	if (!chip->reg_base)
> > -		return -ENOMEM;
> > +	if (!chip->reg_base) {
> > +		err = -ENOMEM;
> > +		goto err_free_irq_vectors;
> > +	}
> >
> >  	for (i = 0; i < chip->ll_wr_cnt && !non_ll; i++) {
> >  		struct dw_edma_region *ll_region = &chip->ll_region_wr[i];
> > @@ -420,8 +422,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
> >  		struct dw_edma_block *dt_block = &vsec_data->dt_wr[i];
> >
> >  		ll_region->vaddr.io = pcim_iomap_table(pdev)[ll_block->bar];
> > -		if (!ll_region->vaddr.io)
> > -			return -ENOMEM;
> > +		if (!ll_region->vaddr.io) {
> > +			err = -ENOMEM;
> > +			goto err_free_irq_vectors;
> > +		}
> >
> >  		ll_region->vaddr.io += ll_block->off;
> >  		ll_region->paddr = dw_edma_get_phys_addr(pdev, vsec_data,
> > @@ -430,8 +434,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
> >  		ll_region->sz = ll_block->sz;
> >
> >  		dt_region->vaddr.io = pcim_iomap_table(pdev)[dt_block->bar];
> > -		if (!dt_region->vaddr.io)
> > -			return -ENOMEM;
> > +		if (!dt_region->vaddr.io) {
> > +			err = -ENOMEM;
> > +			goto err_free_irq_vectors;
> > +		}
> >
> >  		dt_region->vaddr.io += dt_block->off;
> >  		dt_region->paddr = dw_edma_get_phys_addr(pdev, vsec_data,
> > @@ -447,8 +453,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
> >  		struct dw_edma_block *dt_block = &vsec_data->dt_rd[i];
> >
> >  		ll_region->vaddr.io = pcim_iomap_table(pdev)[ll_block->bar];
> > -		if (!ll_region->vaddr.io)
> > -			return -ENOMEM;
> > +		if (!ll_region->vaddr.io) {
> > +			err = -ENOMEM;
> > +			goto err_free_irq_vectors;
> > +		}
> >
> >  		ll_region->vaddr.io += ll_block->off;
> >  		ll_region->paddr = dw_edma_get_phys_addr(pdev, vsec_data,
> > @@ -457,8 +465,10 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
> >  		ll_region->sz = ll_block->sz;
> >
> >  		dt_region->vaddr.io = pcim_iomap_table(pdev)[dt_block->bar];
> > -		if (!dt_region->vaddr.io)
> > -			return -ENOMEM;
> > +		if (!dt_region->vaddr.io) {
> > +			err = -ENOMEM;
> > +			goto err_free_irq_vectors;
> > +		}
> >
> >  		dt_region->vaddr.io += dt_block->off;
> >  		dt_region->paddr = dw_edma_get_phys_addr(pdev, vsec_data,
> > @@ -513,20 +523,25 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
> >  	/* Validating if PCI interrupts were enabled */
> >  	if (!pci_dev_msi_enabled(pdev)) {
> >  		pci_err(pdev, "enable interrupt failed\n");
> > -		return -EPERM;
> > +		err = -EPERM;
> > +		goto err_free_irq_vectors;
> >  	}
> >
> >  	/* Starting eDMA driver */
> >  	err = dw_edma_probe(chip);
> >  	if (err) {
> >  		pci_err(pdev, "eDMA probe failed\n");
> > -		return err;
> > +		goto err_free_irq_vectors;
> >  	}
> >
> >  	/* Saving data structure reference */
> >  	pci_set_drvdata(pdev, chip);
> >
> >  	return 0;
> > +
> > +err_free_irq_vectors:
> > +	pci_free_irq_vectors(pdev);
> > +	return err;
> >  }
> >
> >  static void dw_edma_pcie_remove(struct pci_dev *pdev)
> > --
> > 2.51.0
> >

[PATCH 2/4] dmaengine: dw-edma-pcie: Reject devices without driver data

[Koichiro Den]

dw_edma_pcie_probe() treats the PCI device ID driver_data as the
template for the controller layout and copies it unconditionally. A
device bound dynamically via sysfs can match the driver without that
data, which leads to a NULL pointer dereference.

Reject such matches before enabling the device.

Fixes: 41aaff2a2ac0 ("dmaengine: Add Synopsys eDMA IP PCIe glue-logic")
Cc: stable@vger.kernel.org
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/dma/dw-edma/dw-edma-pcie.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
index 87c31d01fb10..c2024fa824e0 100644
--- a/drivers/dma/dw-edma/dw-edma-pcie.c
+++ b/drivers/dma/dw-edma/dw-edma-pcie.c
@@ -314,6 +314,9 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
 	int i, mask;
 	bool non_ll = false;
 
+	if (!pdata)
+		return -ENODEV;
+
 	struct dw_edma_pcie_data *vsec_data __free(kfree) =
 		kmalloc_obj(*vsec_data);
 	if (!vsec_data)
-- 
2.51.0

Re: [PATCH 2/4] dmaengine: dw-edma-pcie: Reject devices without driver data

[Frank Li]

On Thu, May 21, 2026 at 11:21:51PM +0900, Koichiro Den wrote:
> dw_edma_pcie_probe() treats the PCI device ID driver_data as the
> template for the controller layout and copies it unconditionally. A
> device bound dynamically via sysfs can match the driver without that
> data, which leads to a NULL pointer dereference.
>
> Reject such matches before enabling the device.
>
> Fixes: 41aaff2a2ac0 ("dmaengine: Add Synopsys eDMA IP PCIe glue-logic")
> Cc: stable@vger.kernel.org
> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/dma/dw-edma/dw-edma-pcie.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
> index 87c31d01fb10..c2024fa824e0 100644
> --- a/drivers/dma/dw-edma/dw-edma-pcie.c
> +++ b/drivers/dma/dw-edma/dw-edma-pcie.c
> @@ -314,6 +314,9 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
>  	int i, mask;
>  	bool non_ll = false;
>
> +	if (!pdata)
> +		return -ENODEV;
> +
>  	struct dw_edma_pcie_data *vsec_data __free(kfree) =
>  		kmalloc_obj(*vsec_data);
>  	if (!vsec_data)
> --
> 2.51.0
>

Re: [PATCH 2/4] dmaengine: dw-edma-pcie: Reject devices without driver data

[sashiko-bot]

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

New issues:
- [Medium] Mixing goto-based error handling with scope-based cleanup (__free) in the same function.

Pre-existing issues:
- [High] Missing bounds check for the hardware-provided BAR index when accessing the `pcim_iomap_table()` array.
--

commit 451669b3f83e6d7ef131e591fd15b9e144ab9005
Author: Koichiro Den <den@valinux.co.jp>
Subject: dmaengine: dw-edma-pcie: Reject devices without driver data

This commit adds a check to reject devices without driver data during
dw_edma_pcie_probe. This prevents a NULL pointer dereference when a device
bound dynamically via sysfs matches the driver without that data.

> diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
> index 87c31d01fb102..c2024fa824e03 100644
> --- a/drivers/dma/dw-edma/dw-edma-pcie.c
> +++ b/drivers/dma/dw-edma/dw-edma-pcie.c
> @@ -314,6 +314,9 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
>  	int i, mask;
>  	bool non_ll = false;
>  
> +	if (!pdata)
> +		return -ENODEV;
> +
>  	struct dw_edma_pcie_data *vsec_data __free(kfree) =
>  		kmalloc_obj(*vsec_data);

[Severity: Medium]
The kernel's cleanup subsystem guidelines state that goto-based error
handling and scope-based cleanup should not be mixed in the same function.

Since dw_edma_pcie_probe() uses goto-based cleanup labels on failure paths
like goto err_free_irq_vectors, could mixing them with the __free attribute
here lead to confusing ownership semantics or resource tracking bugs?

Would it be better to either convert all cleanups in this function to
scope-based guards, or avoid using the __free attribute entirely?

[ ... ]

>  	chip->ll_wr_cnt = vsec_data->wr_ch_cnt;
>  	chip->ll_rd_cnt = vsec_data->rd_ch_cnt;
>  
>  	chip->reg_base = pcim_iomap_table(pdev)[vsec_data->rg.bar];

[Severity: High]
This is a pre-existing issue, but does this code properly validate the
hardware-provided BAR index before using it as an array index?

The value for vsec_data->rg.bar is extracted from the PCIe VSEC
capabilities, which can yield values up to 7. However,
pcim_iomap_table(pdev) returns a pointer to an array of size
PCI_STD_NUM_BARS, which is 6.

If a misconfigured or malicious device provides a BAR index of 6 or 7,
could accessing this array result in an out-of-bounds memory read and
crash the kernel when the resulting invalid pointer is dereferenced?

>  	if (!chip->reg_base) {
>  		err = -ENOMEM;
>  		goto err_free_irq_vectors;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521142153.2957432-1-den@valinux.co.jp?part=2

Re: [PATCH 2/4] dmaengine: dw-edma-pcie: Reject devices without driver data

[Koichiro Den]

On Thu, May 21, 2026 at 03:15:03PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Medium] Mixing goto-based error handling with scope-based cleanup (__free) in the same function.

This should be gone after dropping patch 1.

See:
https://lore.kernel.org/dmaengine/xcrbweipase44isv6lfrgqos4mrfhc7o3v4f3lzlovekvsubim@yoz4kfxhka33/

> 
> Pre-existing issues:
> - [High] Missing bounds check for the hardware-provided BAR index when accessing the `pcim_iomap_table()` array.

I do not think the exact OOB concern (See (A) below) is correct since commit
f09d3937d400 ("PCI: Fix wrong length of devres array") (v6.15+). But I agree
that the VSEC-provided BAR fields might be worth validating against
PCI_STD_NUM_BARS.

> --
> 
> commit 451669b3f83e6d7ef131e591fd15b9e144ab9005
> Author: Koichiro Den <den@valinux.co.jp>
> Subject: dmaengine: dw-edma-pcie: Reject devices without driver data
> 
> This commit adds a check to reject devices without driver data during
> dw_edma_pcie_probe. This prevents a NULL pointer dereference when a device
> bound dynamically via sysfs matches the driver without that data.
> 
> > diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
> > index 87c31d01fb102..c2024fa824e03 100644
> > --- a/drivers/dma/dw-edma/dw-edma-pcie.c
> > +++ b/drivers/dma/dw-edma/dw-edma-pcie.c
> > @@ -314,6 +314,9 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
> >  	int i, mask;
> >  	bool non_ll = false;
> >  
> > +	if (!pdata)
> > +		return -ENODEV;
> > +
> >  	struct dw_edma_pcie_data *vsec_data __free(kfree) =
> >  		kmalloc_obj(*vsec_data);
> 
> [Severity: Medium]
> The kernel's cleanup subsystem guidelines state that goto-based error
> handling and scope-based cleanup should not be mixed in the same function.
> 
> Since dw_edma_pcie_probe() uses goto-based cleanup labels on failure paths
> like goto err_free_irq_vectors, could mixing them with the __free attribute
> here lead to confusing ownership semantics or resource tracking bugs?
> 
> Would it be better to either convert all cleanups in this function to
> scope-based guards, or avoid using the __free attribute entirely?
> 
> [ ... ]
> 
> >  	chip->ll_wr_cnt = vsec_data->wr_ch_cnt;
> >  	chip->ll_rd_cnt = vsec_data->rd_ch_cnt;
> >  
> >  	chip->reg_base = pcim_iomap_table(pdev)[vsec_data->rg.bar];
> 
> [Severity: High]
> This is a pre-existing issue, but does this code properly validate the
> hardware-provided BAR index before using it as an array index?
> 
> The value for vsec_data->rg.bar is extracted from the PCIe VSEC
> capabilities, which can yield values up to 7. However,
> pcim_iomap_table(pdev) returns a pointer to an array of size
> PCI_STD_NUM_BARS, which is 6.
  ^^^^^^^^^^^^^^^^
          '--- (A)
> 
> If a misconfigured or malicious device provides a BAR index of 6 or 7,
> could accessing this array result in an out-of-bounds memory read and
                                          ^^^^^^^^^^^^^^^^^^^^^^^^^
                                                      '--- (A)

Best regards,
Koichiro

> crash the kernel when the resulting invalid pointer is dereferenced?
> 
> >  	if (!chip->reg_base) {
> >  		err = -ENOMEM;
> >  		goto err_free_irq_vectors;
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260521142153.2957432-1-den@valinux.co.jp?part=2

[PATCH 3/4] dmaengine: dw-edma: Initialize IRQ data before requesting IRQs

[Koichiro Den]

dw_edma_irq_request() passes struct dw_edma_irq to request_irq()
before dw_edma_channel_setup() fills the back pointer. A shared
interrupt can therefore enter the handler with dw_irq->dw still NULL,
leading to a NULL pointer dereference.

Set the back pointer before installing each handler.

Fixes: e63d79d1ffcd ("dmaengine: Add Synopsys eDMA IP core driver")
Cc: stable@vger.kernel.org
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/dma/dw-edma/dw-edma-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
index c2feb3adc79f..d221e3efcb36 100644
--- a/drivers/dma/dw-edma/dw-edma-core.c
+++ b/drivers/dma/dw-edma/dw-edma-core.c
@@ -929,7 +929,6 @@ static int dw_edma_channel_setup(struct dw_edma *dw, u32 wr_alloc, u32 rd_alloc)
 		else
 			irq->rd_mask |= BIT(chan->id);
 
-		irq->dw = dw;
 		memcpy(&chan->msi, &irq->msi, sizeof(chan->msi));
 
 		dev_vdbg(dev, "MSI:\t\tChannel %s[%u] addr=0x%.8x%.8x, data=0x%.8x\n",
@@ -1018,6 +1017,7 @@ static int dw_edma_irq_request(struct dw_edma *dw,
 	if (chip->nr_irqs == 1) {
 		/* Common IRQ shared among all channels */
 		irq = chip->ops->irq_vector(dev, 0);
+		dw->irq[0].dw = dw;
 		err = request_irq(irq, dw_edma_interrupt_common,
 				  IRQF_SHARED, dw->name, &dw->irq[0]);
 		if (err) {
@@ -1043,6 +1043,7 @@ static int dw_edma_irq_request(struct dw_edma *dw,
 
 		for (i = 0; i < (*wr_alloc + *rd_alloc); i++) {
 			irq = chip->ops->irq_vector(dev, i);
+			dw->irq[i].dw = dw;
 			err = request_irq(irq,
 					  i < *wr_alloc ?
 						dw_edma_interrupt_write :
-- 
2.51.0

Re: [PATCH 3/4] dmaengine: dw-edma: Initialize IRQ data before requesting IRQs

[Frank Li]

On Thu, May 21, 2026 at 11:21:52PM +0900, Koichiro Den wrote:
> dw_edma_irq_request() passes struct dw_edma_irq to request_irq()
> before dw_edma_channel_setup() fills the back pointer. A shared
> interrupt can therefore enter the handler with dw_irq->dw still NULL,
> leading to a NULL pointer dereference.
>
> Set the back pointer before installing each handler.
>
> Fixes: e63d79d1ffcd ("dmaengine: Add Synopsys eDMA IP core driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/dma/dw-edma/dw-edma-core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index c2feb3adc79f..d221e3efcb36 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -929,7 +929,6 @@ static int dw_edma_channel_setup(struct dw_edma *dw, u32 wr_alloc, u32 rd_alloc)
>  		else
>  			irq->rd_mask |= BIT(chan->id);
>
> -		irq->dw = dw;
>  		memcpy(&chan->msi, &irq->msi, sizeof(chan->msi));
>
>  		dev_vdbg(dev, "MSI:\t\tChannel %s[%u] addr=0x%.8x%.8x, data=0x%.8x\n",
> @@ -1018,6 +1017,7 @@ static int dw_edma_irq_request(struct dw_edma *dw,
>  	if (chip->nr_irqs == 1) {
>  		/* Common IRQ shared among all channels */
>  		irq = chip->ops->irq_vector(dev, 0);
> +		dw->irq[0].dw = dw;
>  		err = request_irq(irq, dw_edma_interrupt_common,
>  				  IRQF_SHARED, dw->name, &dw->irq[0]);
>  		if (err) {
> @@ -1043,6 +1043,7 @@ static int dw_edma_irq_request(struct dw_edma *dw,
>
>  		for (i = 0; i < (*wr_alloc + *rd_alloc); i++) {
>  			irq = chip->ops->irq_vector(dev, i);
> +			dw->irq[i].dw = dw;
>  			err = request_irq(irq,
>  					  i < *wr_alloc ?
>  						dw_edma_interrupt_write :
> --
> 2.51.0
>

Re: [PATCH 3/4] dmaengine: dw-edma: Initialize IRQ data before requesting IRQs

[Koichiro Den]

On Thu, May 21, 2026 at 10:44:47AM -0400, Frank Li wrote:
> On Thu, May 21, 2026 at 11:21:52PM +0900, Koichiro Den wrote:
> > dw_edma_irq_request() passes struct dw_edma_irq to request_irq()
> > before dw_edma_channel_setup() fills the back pointer. A shared
> > interrupt can therefore enter the handler with dw_irq->dw still NULL,
> > leading to a NULL pointer dereference.
> >
> > Set the back pointer before installing each handler.
> >
> > Fixes: e63d79d1ffcd ("dmaengine: Add Synopsys eDMA IP core driver")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Koichiro Den <den@valinux.co.jp>
> > ---
> 
> Reviewed-by: Frank Li <Frank.Li@nxp.com>

Hi Frank,

Thanks for reviewing.

After Sashiko raised another point about patch 3, I looked into the init
ordering again. I now think patch 3 is half-baked fix, and probably not
needed in this small-fixes series.

The two Sashiko comments were about the IRQ handler being registered before
dw_edma_channel_setup():

  https://lore.kernel.org/dmaengine/20260521072453.E5AD21F00A3C@smtp.kernel.org/
  https://lore.kernel.org/dmaengine/20260521155834.D8DFF1F00A3C@smtp.kernel.org/

For current upstream, I don't think a shared IRQ alone can reach
dw_edma_done_interrupt() or dw_edma_abort_interrupt(). dw_edma_core_off() runs
before dw_edma_irq_request(), and the handler still needs DONE/ABORT status bits
before it dispatches to those callbacks.

So patch 3 only fixes part of a defensive ordering concern, and the later
Sashiko comment shows that moving irq->dw alone would be incomplete anyway.
If we want to harden this path, I think the cleaner change would be to split and
reorder the setup flow like this:

  Before:
    (1). dw_edma_irq_request()
      (1-a). allocate/populate dw->irq[] and cache MSI messages
      (1-b). request_irq()
    (2). dw_edma_channel_setup()
      (2-a). initialize channels, including vchan_init() and
             dw_edma_core_ch_config()
      (2-b). register the DMA device with dma_async_device_register()

  After:
    (1-a) -> (2-a) -> (1-b) -> (2-b)

But that is more of a cleanup/hardening change than a small pre-existing fix.
(If preferred, I can send a separate patch for that.)

So my conclusion is that only patches 2 and 4 are really needed in this series.
Patch 3 should be dropped. If a respin is needed for any resons, I will send v2
with dropping Patch 1 and 3.

Best regards,
Koichiro

> 
> >  drivers/dma/dw-edma/dw-edma-core.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> > index c2feb3adc79f..d221e3efcb36 100644
> > --- a/drivers/dma/dw-edma/dw-edma-core.c
> > +++ b/drivers/dma/dw-edma/dw-edma-core.c
> > @@ -929,7 +929,6 @@ static int dw_edma_channel_setup(struct dw_edma *dw, u32 wr_alloc, u32 rd_alloc)
> >  		else
> >  			irq->rd_mask |= BIT(chan->id);
> >
> > -		irq->dw = dw;
> >  		memcpy(&chan->msi, &irq->msi, sizeof(chan->msi));
> >
> >  		dev_vdbg(dev, "MSI:\t\tChannel %s[%u] addr=0x%.8x%.8x, data=0x%.8x\n",
> > @@ -1018,6 +1017,7 @@ static int dw_edma_irq_request(struct dw_edma *dw,
> >  	if (chip->nr_irqs == 1) {
> >  		/* Common IRQ shared among all channels */
> >  		irq = chip->ops->irq_vector(dev, 0);
> > +		dw->irq[0].dw = dw;
> >  		err = request_irq(irq, dw_edma_interrupt_common,
> >  				  IRQF_SHARED, dw->name, &dw->irq[0]);
> >  		if (err) {
> > @@ -1043,6 +1043,7 @@ static int dw_edma_irq_request(struct dw_edma *dw,
> >
> >  		for (i = 0; i < (*wr_alloc + *rd_alloc); i++) {
> >  			irq = chip->ops->irq_vector(dev, i);
> > +			dw->irq[i].dw = dw;
> >  			err = request_irq(irq,
> >  					  i < *wr_alloc ?
> >  						dw_edma_interrupt_write :
> > --
> > 2.51.0
> >

Re: [PATCH 3/4] dmaengine: dw-edma: Initialize IRQ data before requesting IRQs

[sashiko-bot]

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

Pre-existing issues:
- [High] A race condition exists where shared interrupt handlers can access uninitialized channel structures (like spinlocks and list heads), leading to a kernel crash. The patch fixes one early-IRQ NULL dereference but leaves this secondary crash exposed.
--

commit f0655e3acfc0eee2ccdf6dbee38ba6bad6f04ae5
Author: Koichiro Den <den@valinux.co.jp>

dmaengine: dw-edma: Initialize IRQ data before requesting IRQs

This commit fixes a NULL pointer dereference caused by passing an
uninitialized pointer to request_irq(). It resolves the issue by setting
the back pointer for each IRQ before the handler is installed.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index c2feb3adc79fa..d221e3efcb36b 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -929,7 +929,6 @@ static int dw_edma_channel_setup(struct dw_edma *dw, u32 wr_alloc, u32 rd_alloc)
>  		else
>  			irq->rd_mask |= BIT(chan->id);
>  
> -		irq->dw = dw;

[Severity: High]
This isn't a bug introduced by this patch, but does this sequence still leave
a race condition?

Since dw_edma_irq_request() already registered the shared IRQs before
dw_edma_channel_setup() is called, setting irq->wr_mask or irq->rd_mask
makes the channel visible to the interrupt handler.

If a shared interrupt fires immediately after the mask is updated but
before vchan_init() is called later in this loop:

		if (chan->dir == EDMA_DIR_WRITE)
			irq->wr_mask |= BIT(chan->id);
		...
		vchan_init(&chan->vc, dma);

Could the handler process the channel and call dw_edma_done_interrupt()
or dw_edma_abort_interrupt()?

If so, these functions would attempt to lock the uninitialized
chan->vc.lock and call vchan_next_desc() on the zeroed desc_issued list
head, which may lead to a crash.

>  		memcpy(&chan->msi, &irq->msi, sizeof(chan->msi));
>  
>  		dev_vdbg(dev, "MSI:\t\tChannel %s[%u] addr=0x%.8x%.8x, data=0x%.8x\n",
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521142153.2957432-1-den@valinux.co.jp?part=3

[PATCH 4/4] dmaengine: dw-edma: Add spinlock to protect DONE_INT_MASK and ABORT_INT_MASK

[Koichiro Den]

From: Frank Li <Frank.Li@nxp.com>

The DONE_INT_MASK and ABORT_INT_MASK registers are shared by all DMA
channels, and modifying them requires a read-modify-write sequence.
Because this operation is not atomic, concurrent calls to
dw_edma_v0_core_start() can introduce race conditions if two channels
update these registers simultaneously.

Add a spinlock to serialize access to these registers and prevent race
conditions.

Fixes: 7e4b8a4fbe2c ("dmaengine: Add Synopsys eDMA IP version 0 support")
Cc: stable@vger.kernel.org
Signed-off-by: Frank Li <Frank.Li@nxp.com>
[den: update dw_edma.lock comment]
Link: https://lore.kernel.org/dmaengine/20260109-edma_ll-v2-1-5c0b27b2c664@nxp.com/
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/dma/dw-edma/dw-edma-core.h    | 2 +-
 drivers/dma/dw-edma/dw-edma-v0-core.c | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/dw-edma/dw-edma-core.h b/drivers/dma/dw-edma/dw-edma-core.h
index 902574b1ba86..6474cacf7195 100644
--- a/drivers/dma/dw-edma/dw-edma-core.h
+++ b/drivers/dma/dw-edma/dw-edma-core.h
@@ -109,7 +109,7 @@ struct dw_edma {
 
 	struct dw_edma_chan		*chan;
 
-	raw_spinlock_t			lock;		/* Only for legacy */
+	raw_spinlock_t			lock;		/* Protect v0 shared registers */
 
 	struct dw_edma_chip             *chip;
 
diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
index 69e8279adec8..cfdd6463252e 100644
--- a/drivers/dma/dw-edma/dw-edma-v0-core.c
+++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
@@ -364,6 +364,7 @@ static void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
 {
 	struct dw_edma_chan *chan = chunk->chan;
 	struct dw_edma *dw = chan->dw;
+	unsigned long flags;
 	u32 tmp;
 
 	dw_edma_v0_core_write_chunk(chunk);
@@ -408,6 +409,8 @@ static void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
 			}
 		}
 		/* Interrupt unmask - done, abort */
+		raw_spin_lock_irqsave(&dw->lock, flags);
+
 		tmp = GET_RW_32(dw, chan->dir, int_mask);
 		tmp &= ~FIELD_PREP(EDMA_V0_DONE_INT_MASK, BIT(chan->id));
 		tmp &= ~FIELD_PREP(EDMA_V0_ABORT_INT_MASK, BIT(chan->id));
@@ -416,6 +419,9 @@ static void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
 		tmp = GET_RW_32(dw, chan->dir, linked_list_err_en);
 		tmp |= FIELD_PREP(EDMA_V0_LINKED_LIST_ERR_MASK, BIT(chan->id));
 		SET_RW_32(dw, chan->dir, linked_list_err_en, tmp);
+
+		raw_spin_unlock_irqrestore(&dw->lock, flags);
+
 		/* Channel control */
 		SET_CH_32(dw, chan->dir, chan->id, ch_control1,
 			  (DW_EDMA_V0_CCS | DW_EDMA_V0_LLE));
-- 
2.51.0

Re: [PATCH 4/4] dmaengine: dw-edma: Add spinlock to protect DONE_INT_MASK and ABORT_INT_MASK

[sashiko-bot]

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

Pre-existing issues:
- [High] The teardown sequence in `dw_edma_remove` incorrectly powers off the hardware and frees interrupts before unregistering the DMA device, exposing a race window.
--

commit 3dac310797da8c464a27d4656c26826f3d2a7401
Author: Frank Li <Frank.Li@nxp.com>

dmaengine: dw-edma: Add spinlock to protect DONE_INT_MASK and ABORT_INT_MASK

This commit adds a spinlock to dw_edma_v0_core_start() to serialize access
to the shared DONE_INT_MASK and ABORT_INT_MASK registers. This prevents
race conditions from concurrent updates across multiple channels.

Link: https://lore.kernel.org/dmaengine/20260109-edma_ll-v2-1-5c0b27b2c664@nxp.com/

> diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> index 69e8279adec81..cfdd6463252e6 100644
> --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
[ ... ]
> @@ -408,6 +409,8 @@ static void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
>  			}
>  		}
>  		/* Interrupt unmask - done, abort */
> +		raw_spin_lock_irqsave(&dw->lock, flags);
> +

[Severity: High]
This is a pre-existing issue, but I noticed a potential race window related to
how dw_edma_remove() interacts with this code.

In dw_edma_remove(), the driver teardown sequence calls dw_edma_core_off() to
disable the engine and mask interrupts, then frees the IRQs, and only
subsequently calls dma_async_device_unregister().

Since the DMA device remains registered in the dmaengine framework during this
window, can concurrent DMA clients still issue new transfers?

If a client calls dmaengine_submit() and dma_async_issue_pending(), the
execution traces down to dw_edma_v0_core_start().

Could this write to the engine_en register and unmask interrupts here,
unexpectedly re-enabling the hardware? Since the IRQs have already been freed,
would this result in the transfer completion never being signaled, leading to
a hung task?

Should the teardown sequence call dma_async_device_unregister() first to
safely quiesce the subsystem and prevent new requests before stopping the
hardware and freeing IRQs?

>  		tmp = GET_RW_32(dw, chan->dir, int_mask);
>  		tmp &= ~FIELD_PREP(EDMA_V0_DONE_INT_MASK, BIT(chan->id));
>  		tmp &= ~FIELD_PREP(EDMA_V0_ABORT_INT_MASK, BIT(chan->id));
> @@ -416,6 +419,9 @@ static void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
>  		tmp = GET_RW_32(dw, chan->dir, linked_list_err_en);
>  		tmp |= FIELD_PREP(EDMA_V0_LINKED_LIST_ERR_MASK, BIT(chan->id));
>  		SET_RW_32(dw, chan->dir, linked_list_err_en, tmp);
> +
> +		raw_spin_unlock_irqrestore(&dw->lock, flags);
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521142153.2957432-1-den@valinux.co.jp?part=4

Re: [PATCH 4/4] dmaengine: dw-edma: Add spinlock to protect DONE_INT_MASK and ABORT_INT_MASK

[Koichiro Den]

On Thu, May 21, 2026 at 04:56:49PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] The teardown sequence in `dw_edma_remove` incorrectly powers off the hardware and frees interrupts before unregistering the DMA device, exposing a race window.

I looked into this a bit. I agree the race described here can happen in the
current code, but I think the problem is broader than the ordering between
free_irq() and dma_async_device_unregister().

While trying to reproduce the window with the following debug delay:

  @@ -1177,6 +1178,8 @@ int dw_edma_remove(struct dw_edma_chip *chip)
                  free_irq(chip->ops->irq_vector(dev, i), &dw->irq[i]);
          dw_edma_emul_irq_free(dw);
   
  +       msleep(10000);
  +
          /* Deregister eDMA device */
          dma_async_device_unregister(&dw->dma);
          list_for_each_entry_safe(chan, _chan, &dw->dma.channels,

I hit a more fundamental problem: unsupported in-use unbind can lead to a UAF
and kernel panic. I suspect that the teardown ordering issue Sashiko pointed
out is part of that larger in-use unbind problem.

A better fix may be to support in-use unbind in a similar manner to e.g. commit
bf453a0a18b2 ("dmaengine: ioat: Support in-use unbind"). I'm not sure whether we
want to do that now, so I would like to hear maintainers' thoughts.

If we do, I think the fix should include at least:

  (a). Stop using devm for dma_device backing objects and add a device_release
       callback, so the objects stay alive until the last dmaengine reference
       is dropped.

  (b). Add a removing flag and serialize it with the transfer start path.
       For example, dw_edma_device_issue_pending() should check the flag and call
       dw_edma_start_transfer() under chan->vc.lock, while the remove path
       should set the flag under the same lock at the start of dw_edma_remove().

> --
> 
> commit 3dac310797da8c464a27d4656c26826f3d2a7401
> Author: Frank Li <Frank.Li@nxp.com>
> 
> dmaengine: dw-edma: Add spinlock to protect DONE_INT_MASK and ABORT_INT_MASK
> 
> This commit adds a spinlock to dw_edma_v0_core_start() to serialize access
> to the shared DONE_INT_MASK and ABORT_INT_MASK registers. This prevents
> race conditions from concurrent updates across multiple channels.
> 
> Link: https://lore.kernel.org/dmaengine/20260109-edma_ll-v2-1-5c0b27b2c664@nxp.com/
> 
> > diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> > index 69e8279adec81..cfdd6463252e6 100644
> > --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> > +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
> [ ... ]
> > @@ -408,6 +409,8 @@ static void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
> >  			}
> >  		}
> >  		/* Interrupt unmask - done, abort */
> > +		raw_spin_lock_irqsave(&dw->lock, flags);
> > +
> 
> [Severity: High]
> This is a pre-existing issue, but I noticed a potential race window related to
> how dw_edma_remove() interacts with this code.
> 
> In dw_edma_remove(), the driver teardown sequence calls dw_edma_core_off() to
> disable the engine and mask interrupts, then frees the IRQs, and only
> subsequently calls dma_async_device_unregister().
> 
> Since the DMA device remains registered in the dmaengine framework during this
> window, can concurrent DMA clients still issue new transfers?

Yes, they can in the current code. I think (b) would close that path.

> 
> If a client calls dmaengine_submit() and dma_async_issue_pending(), the
> execution traces down to dw_edma_v0_core_start().
> 
> Could this write to the engine_en register and unmask interrupts here,
> unexpectedly re-enabling the hardware?

Yes. That is the path I think should be blocked by serializing the removing flag
with the transfer start path, by using chan->vc.lock.

> Since the IRQs have already been freed,
> would this result in the transfer completion never being signaled, leading to
> a hung task?

Yes, that looks possible if a transfer is started after IRQs are freed. And I
think (b) would prevent that new-start case. (An already in-flight transfer is a
separate case.)

> 
> Should the teardown sequence call dma_async_device_unregister() first to
> safely quiesce the subsystem and prevent new requests before stopping the
> hardware and freeing IRQs?

I do not think unregister-before-free_irq can be the real fix by itself. It only
narrows the window. It does not protect clients that already hold DMA channels,
and those clients can still reach the driver callbacks after unregister.

If I'm missing something, please let me know.

Best regards,
Koichiro

> 
> >  		tmp = GET_RW_32(dw, chan->dir, int_mask);
> >  		tmp &= ~FIELD_PREP(EDMA_V0_DONE_INT_MASK, BIT(chan->id));
> >  		tmp &= ~FIELD_PREP(EDMA_V0_ABORT_INT_MASK, BIT(chan->id));
> > @@ -416,6 +419,9 @@ static void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
> >  		tmp = GET_RW_32(dw, chan->dir, linked_list_err_en);
> >  		tmp |= FIELD_PREP(EDMA_V0_LINKED_LIST_ERR_MASK, BIT(chan->id));
> >  		SET_RW_32(dw, chan->dir, linked_list_err_en, tmp);
> > +
> > +		raw_spin_unlock_irqrestore(&dw->lock, flags);
> > +
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260521142153.2957432-1-den@valinux.co.jp?part=4

Re: [PATCH 0/4] dmaengine: dw-edma: Fix probe paths and register races

[Koichiro Den]

On Thu, May 21, 2026 at 11:21:49PM +0900, Koichiro Den wrote:
> Hi,
> 
> This series fixes pre-existing dw-edma issues flagged by Sashiko in:
> https://lore.kernel.org/dmaengine/20260521063115.2842238-1-den@valinux.co.jp/
> 
> Note: Patch 4 was based on a patch Frank posted in January:
>       https://lore.kernel.org/dmaengine/20260109-edma_ll-v2-1-5c0b27b2c664@nxp.com/
>       Since it has not been merged, I included it here. Frank, please let me
>       know if you prefer a different handling.
> 
> Best regards,
> Koichiro
> 
> 
> Frank Li (1):
>   dmaengine: dw-edma: Add spinlock to protect DONE_INT_MASK and
>     ABORT_INT_MASK
> 
> Koichiro Den (3):
>   dmaengine: dw-edma-pcie: Free IRQ vectors on probe failures
>   dmaengine: dw-edma-pcie: Reject devices without driver data
>   dmaengine: dw-edma: Initialize IRQ data before requesting IRQs

Frank, thank you for reviewing.

Mani, Vinod, if there are no objections, could you please consider applying only
patches 2 and 4 from this series?

- Patch 1 should be dropped. As Frank pointed out, pcim_enable_device() already
  manages IRQ vectors (i.e. sort of false-positive from Sashiko).

- Patch 2 still looks valid to me. After dropping patch 1, the new issue
  reported by Sashiko no longer applies, and I think the remaining concern is a
  false positive.

- Patch 3 should be dropped. I rechecked the initialization path and I no longer
  think this patch is needed. See:
  https://lore.kernel.org/dmaengine/kjslqii4bs3g4pi22mxh72hxnlm7nkesdd3va6zi5fhmjamerw@j7lbrlq5oszd/

- Patch 4 still looks valid to me as an independent fix. Sashiko's feedback
  against patch 4 also revealed a broader in-use unbind issue, but Frank's
  original path fixes a real race issue on its own. I am not sure whether we
  should add in-use unbind support right now. See:
  https://lore.kernel.org/dmaengine/ne76elxedfnngi7dilpyvpzwm7tghyj6kpg4ninwxecxsajkkx@zkarppyurl2s/

Best regards,
Koichiro

> 
>  drivers/dma/dw-edma/dw-edma-core.c    |  3 +-
>  drivers/dma/dw-edma/dw-edma-core.h    |  2 +-
>  drivers/dma/dw-edma/dw-edma-pcie.c    | 42 +++++++++++++++++++--------
>  drivers/dma/dw-edma/dw-edma-v0-core.c |  6 ++++
>  4 files changed, 39 insertions(+), 14 deletions(-)
> 
> -- 
> 2.51.0
> 
> 

Re: (subset) [PATCH 0/4] dmaengine: dw-edma: Fix probe paths and register races

[Vinod Koul]

On Thu, 21 May 2026 23:21:49 +0900, Koichiro Den wrote:
> This series fixes pre-existing dw-edma issues flagged by Sashiko in:
> https://lore.kernel.org/dmaengine/20260521063115.2842238-1-den@valinux.co.jp/
> 
> Note: Patch 4 was based on a patch Frank posted in January:
>       https://lore.kernel.org/dmaengine/20260109-edma_ll-v2-1-5c0b27b2c664@nxp.com/
>       Since it has not been merged, I included it here. Frank, please let me
>       know if you prefer a different handling.
> 
> [...]

Applied, thanks!

[2/4] dmaengine: dw-edma-pcie: Reject devices without driver data
      commit: 11d7cfe0c119691b2dafbb699bbca90258c678aa
[4/4] dmaengine: dw-edma: Add spinlock to protect DONE_INT_MASK and ABORT_INT_MASK
      commit: 8ffba0171c6bbce5f093c6dba5a02c0805b31203

Best regards,
-- 
~Vinod