dmaengine: ppc4xx: convert irq_of_parse_and_map() to platform_get_irq()

PATCH Superseded Rosen Penev 2 participants 2 messages Sep 11 dmaengine

Share & tools

Rosen Penev

author

2026-09-11 22:12 UTC #0 lore raw

PATCH v2 1/1 dmaengine: ppc4xx: convert irq_of_parse_and_map() to platform_get_irq()

Replace irq_of_parse_and_map() with platform_get_irq(), which is the preferred way to obtain IRQ resources from platform devices. This eliminates the corresponding irq_dispose_mapping() calls.

Use the return code from platform_get_irq() instead of just -ENXIO.

Fix a latent bug in the err_req2 error path: the error IRQ was not freed when a subsequent step (I2O setup) failed.

The struct device_node *np declaration is moved to the scope where it is still needed (I2O register lookup).

Assisted-by LLM

Signed-off-by Rosen Penev <[email protected]>

---
v2: fix missing free_irq in remove and fix description

 drivers/dma/ppc4xx/adma.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

drivers/dma/ppc4xx/adma.c +14 ~~-20~~

index 76b9eaa05657..1bdc781efbb5 100644
@@ -3865,28 +3865,25 @@ static int ppc440spe_adma_setup_irqs(struct ppc440spe_adma_device *adev,
 				     int *initcode)
 {
 	struct platform_device *ofdev;
-	struct device_node *np;
 	int ret;
 
 	ofdev = container_of(adev->dev, struct platform_device, dev);
-	np = ofdev->dev.of_node;
 	if (adev->id != PPC440SPE_XOR_ID) {
-		adev->err_irq = irq_of_parse_and_map(np, 1);
-		if (!adev->err_irq) {
+		adev->err_irq = platform_get_irq(ofdev, 1);
+		if (adev->err_irq < 0) {
 			dev_warn(adev->dev, "no err irq resource?\n");
 			*initcode = PPC_ADMA_INIT_IRQ2;
-			adev->err_irq = -ENXIO;
 		} else
 			atomic_inc(&ppc440spe_adma_err_irq_ref);
 	} else {
 		adev->err_irq = -ENXIO;
 	}
 
-	adev->irq = irq_of_parse_and_map(np, 0);
-	if (!adev->irq) {
+	adev->irq = platform_get_irq(ofdev, 0);
+	if (adev->irq < 0) {
 		dev_err(adev->dev, "no irq resource\n");
 		*initcode = PPC_ADMA_INIT_IRQ1;
-		ret = -ENXIO;
+		ret = adev->irq;
 		goto err_irq_map;
 	}
 	dev_dbg(adev->dev, "irq %d, err irq %d\n",

@@ -3899,7 +3896,7 @@ static int ppc440spe_adma_setup_irqs(struct ppc440spe_adma_device *adev,
 			adev->irq);
 		*initcode = PPC_ADMA_INIT_IRQ1;
 		ret = -EIO;
-		goto err_req1;
+		goto err_irq_map;
 	}
 
 	/* only DMA engines have a separate error IRQ

@@ -3917,7 +3914,7 @@ static int ppc440spe_adma_setup_irqs(struct ppc440spe_adma_device *adev,
 				adev->err_irq);
 			*initcode = PPC_ADMA_INIT_IRQ2;
 			ret = -EIO;
-			goto err_req2;
+			goto err_req1;
 		}
 	}

@@ -3927,6 +3924,7 @@ static int ppc440spe_adma_setup_irqs(struct ppc440spe_adma_device *adev,
 			    XOR_IE_ICIE_BIT | XOR_IE_RPTIE_BIT,
 			    &adev->xor_reg->ier);
 	} else {
+		struct device_node *np;
 		u32 mask, enable;
 
 		np = of_find_compatible_node(NULL, NULL, "ibm,i2o-440spe");

@@ -3956,14 +3954,13 @@ static int ppc440spe_adma_setup_irqs(struct ppc440spe_adma_device *adev,
 	return 0;
 
 err_req2:
-	free_irq(adev->irq, chan);
+	if (adev->err_irq > 0)
+		free_irq(adev->err_irq, chan);
 err_req1:
-	irq_dispose_mapping(adev->irq);
+	free_irq(adev->irq, chan);
 err_irq_map:
-	if (adev->err_irq > 0) {
-		if (atomic_dec_and_test(&ppc440spe_adma_err_irq_ref))
-			irq_dispose_mapping(adev->err_irq);
-	}
+	if (adev->err_irq > 0)
+		atomic_dec(&ppc440spe_adma_err_irq_ref);
 	return ret;
 }

@@ -3987,13 +3984,10 @@ static void ppc440spe_adma_release_irqs(struct ppc440spe_adma_device *adev,
 		iowrite32(mask, &adev->i2o_reg->iopim);
 	}
 	free_irq(adev->irq, chan);
-	irq_dispose_mapping(adev->irq);
 	if (adev->err_irq > 0) {
 		free_irq(adev->err_irq, chan);
-		if (atomic_dec_and_test(&ppc440spe_adma_err_irq_ref)) {
-			irq_dispose_mapping(adev->err_irq);
+		if (atomic_dec_and_test(&ppc440spe_adma_err_irq_ref))
 			iounmap(adev->i2o_reg);
-		}
 	}
 }

Signature · 1 line

2.55.0

[email protected]

AI review

2026-09-11 22:26 UTC #1 lore raw

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

New issues:
- [Medium] Using platform_get_irq() for an optional error IRQ causes spurious error logs and improper deferred probe handling.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but there is a lockless traversal of chan->chain in hardirq context while it is concurrently mutated in process/softirq context.
- [High] This problem wasn't introduced by this patch, but the global list ppc440spe_adma_chan_list is modified and traversed without any synchronization. --

commit 905b47acf0bc2689f9a6c8d641b95aa9d8afae93
Author: Rosen Penev <[email protected]>

dmaengine: ppc4xx: convert irq_of_parse_and_map() to platform_get_irq()

Replace irq_of_parse_and_map() with platform_get_irq(), which is the preferred way to obtain IRQ resources from platform devices. This eliminates the corresponding irq_dispose_mapping() calls.

Quoted 6 lines

drivers/dma/ppc4xx/adma.c +0 -0

index 279a431ccae38..f159c887c6a60 100644
@@ -3865,28 +3865,25 @@ static int ppc440spe_adma_setup_irqs(struct ppc440spe_adma_device *adev,
 				     int *initcode)

drivers/dma/ppc4xx/adma.c static int ppc440spe_adma_setup_irqs(struct ppc440spe_adma_device *adev, · line 3873

Quoted 10 lines

drivers/dma/ppc4xx/adma.c +2 -2

 {
 	struct platform_device *ofdev;
 	int ret;
 
 	ofdev = container_of(adev->dev, struct platform_device, dev);
 	if (adev->id != PPC440SPE_XOR_ID) {
-		adev->err_irq = irq_of_parse_and_map(np, 1);
-		if (!adev->err_irq) {
+		adev->err_irq = platform_get_irq(ofdev, 1);
+		if (adev->err_irq < 0) {

[Severity: Medium]
Will this log a spurious error message during normal probe of devices without an error IRQ?

platform_get_irq() calls dev_err_probe() when the IRQ is missing, which will print an error before the driver's own warning below.

Also, if the IRQ controller is not yet ready and platform_get_irq() returns -EPROBE_DEFER, will this error be swallowed, preventing proper probe deferral?

Should platform_get_irq_optional() be used instead?

drivers/dma/ppc4xx/adma.c static int ppc440spe_adma_setup_irqs(struct ppc440spe_adma_device *adev, · line 3880

Quoted 8 lines

drivers/dma/ppc4xx/adma.c +0 -1

 			dev_warn(adev->dev, "no err irq resource?\n");
 			*initcode = PPC_ADMA_INIT_IRQ2;
-			adev->err_irq = -ENXIO;
 		} else
 			atomic_inc(&ppc440spe_adma_err_irq_ref);
 	} else {
 		adev->err_irq = -ENXIO;
 	}

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does ppc440spe_adma_device_clear_eot_status() traverse the channel chain locklessly in hardirq context while it can be concurrently modified?

drivers/dma/ppc4xx/adma.c:ppc440spe_adma_device_clear_eot_status() {
    ...
				/*
				 * Update the status of corresponding
				 * descriptor.
				 */
				list_for_each_entry(iter, &chan->chain,
				    chain_node) {
					if (iter->phys == phys)
						break;
				}
    ...
}

This is called from the hardirq handler, while the list is modified in process or softirq context with only spin_lock_bh() held, which does not disable hardirqs:

drivers/dma/ppc4xx/adma.c:ppc440spe_adma_tx_submit() {
    ...
	spin_lock_bh(&chan->lock);
    ...
	if (unlikely(list_empty(&chan->chain))) {
		/* first peer */
		list_splice_init(&sw_desc->group_list, &chan->chain);
    ...
}

Can this lead to list corruption or crashes when the hardirq preempts the list modification?

[Severity: High]
This is a pre-existing issue, but is the global ppc440spe_adma_chan_list modified and traversed without any synchronization?

drivers/dma/ppc4xx/adma.c:ppc440spe_adma_probe() {
    ...
	ref = kmalloc_obj(*ref);
	if (ref) {
		ref->chan = &chan->common;
		INIT_LIST_HEAD(&ref->node);
		list_add_tail(&ref->node, &ppc440spe_adma_chan_list);
	}
    ...
}

This list is traversed locklessly by external callers:

drivers/dma/ppc4xx/adma.c:ppc440spe_async_tx_find_best_channel() {
    ...
	list_for_each_entry(ref, &ppc440spe_adma_chan_list, node) {
		if (dma_has_cap(cap, ref->chan->device->cap_mask)) {
    ...
}

Could this cause a kernel panic if ppc440spe_async_tx_find_best_channel() is called concurrently with device probe or remove?

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1