[PATCH 0/4] ata: pata_mpc52xx: fix cleanup ordering and modernize

[PATCH 0/4] ata: pata_mpc52xx: fix cleanup ordering and modernize

[Rosen Penev]

Fix several lifecycle bugs in the MPC52xx ATA driver:

 - The BestComm task IRQ was managed by devm, creating a use-after-free
   and teardown inversion.  Switch to non-devm request_irq/free_irq so
   the interrupt is unregistered before the task is freed.

 - The error and remove paths called irq_dispose_mapping() explicitly,
   duplicating the one in bcom_task_free().  Remove the redundant calls.

 - Replace irq_of_parse_and_map() with platform_get_irq() for proper
   error handling and integration with the platform device model.

 - Replace the open-coded of_address_to_resource()/devm_request_mem_region()
   /devm_ioremap() sequence with the standard helper.

Rosen Penev (4):
  ata: pata_mpc52xx: switch to non-devm request_irq for proper ordering
  ata: pata_mpc52xx: remove redundant irq_dispose_mapping calls
  ata: pata_mpc52xx: convert to platform_get_irq()
  ata: pata_mpc52xx: use devm_platform_get_and_ioremap_resource

 drivers/ata/pata_mpc52xx.c | 67 ++++++++++++--------------------------
 1 file changed, 21 insertions(+), 46 deletions(-)

-- 
2.54.0

[PATCH 1/4] ata: pata_mpc52xx: switch to non-devm request_irq for proper ordering

[Rosen Penev]

The BestComm task IRQ handler accesses priv->dmatsk, which is freed
by bcom_ata_release().  With devm_request_irq(), the handler remains
registered until devres unwinds after the error/remove function
returns, creating a window where the handler can access freed memory.

Switch to request_irq() / free_irq() so that the interrupt is
unregistered before bcom_ata_release() tears down the task.

Assisted-by: opencode:big-pickle
Fixes: d01159dffa15 ("drivers/ata/pata_mpc52xx.c: clean up error handling code")
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/pata_mpc52xx.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
index 210a63283f62..9af0aa89a5c6 100644
--- a/drivers/ata/pata_mpc52xx.c
+++ b/drivers/ata/pata_mpc52xx.c
@@ -767,11 +767,11 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 	}
 
 	task_irq = bcom_get_task_irq(dmatsk);
-	rv = devm_request_irq(&op->dev, task_irq, &mpc52xx_ata_task_irq, 0,
-				"ATA task", priv);
+	rv = request_irq(task_irq, &mpc52xx_ata_task_irq, 0,
+			  "ATA task", priv);
 	if (rv) {
 		dev_err(&op->dev, "error requesting DMA IRQ\n");
-		goto err2;
+		goto err_free_task;
 	}
 	priv->dmatsk = dmatsk;
 
@@ -779,7 +779,7 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 	rv = mpc52xx_ata_hw_init(priv);
 	if (rv) {
 		dev_err(&op->dev, "error initializing hardware\n");
-		goto err2;
+		goto err_free_irq;
 	}
 
 	/* Register ourselves to libata */
@@ -787,13 +787,15 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 				  mwdma_mask, udma_mask);
 	if (rv) {
 		dev_err(&op->dev, "error registering with ATA layer\n");
-		goto err2;
+		goto err_free_irq;
 	}
 
 	return 0;
 
- err2:
+ err_free_irq:
+	free_irq(task_irq, priv);
 	irq_dispose_mapping(task_irq);
+ err_free_task:
 	bcom_ata_release(dmatsk);
  err1:
 	irq_dispose_mapping(ata_irq);
@@ -811,6 +813,7 @@ static void mpc52xx_ata_remove(struct platform_device *op)
 
 	/* Clean up DMA */
 	task_irq = bcom_get_task_irq(priv->dmatsk);
+	free_irq(task_irq, priv);
 	irq_dispose_mapping(task_irq);
 	bcom_ata_release(priv->dmatsk);
 	irq_dispose_mapping(priv->ata_irq);
-- 
2.54.0

Re: [PATCH 1/4] ata: pata_mpc52xx: switch to non-devm request_irq for proper ordering

[sashiko-bot]

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

Pre-existing issues:
- [High] The IRQ handler `mpc52xx_ata_task_irq` is registered and unmasked before the driver's private state (`priv->dmatsk`) is fully initialized, creating a window where an immediate interrupt could trigger a NULL pointer dereference.
--

commit 51800f0b7ed178011074a74c7015353a1624d9fd
Author: Rosen Penev <rosenp@gmail.com>

ata: pata_mpc52xx: switch to non-devm request_irq for proper ordering

This patch replaces devm_request_irq with request_irq for the BestComm
task IRQ to ensure the interrupt is unregistered before bcom_ata_release()
tears down the task. This fixes a race condition during device removal or
error paths where the handler could access freed memory.

> diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
> index 210a63283f62b..9af0aa89a5c66 100644
> --- a/drivers/ata/pata_mpc52xx.c
> +++ b/drivers/ata/pata_mpc52xx.c
> @@ -767,11 +767,11 @@ static int mpc52xx_ata_probe(struct platform_device *op)
>  	}
>  
>  	task_irq = bcom_get_task_irq(dmatsk);
> -	rv = devm_request_irq(&op->dev, task_irq, &mpc52xx_ata_task_irq, 0,
> -				"ATA task", priv);
> +	rv = request_irq(task_irq, &mpc52xx_ata_task_irq, 0,
> +			  "ATA task", priv);
>  	if (rv) {
>  		dev_err(&op->dev, "error requesting DMA IRQ\n");
> -		goto err2;
> +		goto err_free_task;
>  	}
>  	priv->dmatsk = dmatsk;

[Severity: High]
This is a pre-existing issue, but could this ordering cause a NULL pointer
dereference if an interrupt fires immediately after registration?

In mpc52xx_ata_probe(), the IRQ is requested and enabled before the DMA
task pointer is assigned to the driver's private state. Since priv is
allocated with devm_kzalloc(), priv->dmatsk is NULL during the
request_irq() call.

If a pending or spurious interrupt fires before the priv->dmatsk assignment
completes, the handler is invoked and dereferences the uninitialized pointer:

mpc52xx_ata_task_irq()
    while (bcom_buffer_done(priv->dmatsk))
        bcom_retrieve_buffer(priv->dmatsk, NULL, NULL);

Should priv->dmatsk be assigned to the private structure before registering
the interrupt handler to avoid this window?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608214718.49503-1-rosenp@gmail.com?part=1

Re: [PATCH 1/4] ata: pata_mpc52xx: switch to non-devm request_irq for proper ordering

[Niklas Cassel]

On Mon, Jun 08, 2026 at 02:47:15PM -0700, Rosen Penev wrote:
> The BestComm task IRQ handler accesses priv->dmatsk, which is freed
> by bcom_ata_release().  With devm_request_irq(), the handler remains
> registered until devres unwinds after the error/remove function
> returns, creating a window where the handler can access freed memory.
> 
> Switch to request_irq() / free_irq() so that the interrupt is
> unregistered before bcom_ata_release() tears down the task.
> 
> Assisted-by: opencode:big-pickle
> Fixes: d01159dffa15 ("drivers/ata/pata_mpc52xx.c: clean up error handling code")
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/ata/pata_mpc52xx.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
> index 210a63283f62..9af0aa89a5c6 100644
> --- a/drivers/ata/pata_mpc52xx.c
> +++ b/drivers/ata/pata_mpc52xx.c
> @@ -767,11 +767,11 @@ static int mpc52xx_ata_probe(struct platform_device *op)
>  	}
>  
>  	task_irq = bcom_get_task_irq(dmatsk);
> -	rv = devm_request_irq(&op->dev, task_irq, &mpc52xx_ata_task_irq, 0,
> -				"ATA task", priv);
> +	rv = request_irq(task_irq, &mpc52xx_ata_task_irq, 0,
> +			  "ATA task", priv);
>  	if (rv) {
>  		dev_err(&op->dev, "error requesting DMA IRQ\n");
> -		goto err2;
> +		goto err_free_task;
>  	}
>  	priv->dmatsk = dmatsk;
>  
> @@ -779,7 +779,7 @@ static int mpc52xx_ata_probe(struct platform_device *op)
>  	rv = mpc52xx_ata_hw_init(priv);
>  	if (rv) {
>  		dev_err(&op->dev, "error initializing hardware\n");
> -		goto err2;
> +		goto err_free_irq;
>  	}
>  
>  	/* Register ourselves to libata */
> @@ -787,13 +787,15 @@ static int mpc52xx_ata_probe(struct platform_device *op)
>  				  mwdma_mask, udma_mask);
>  	if (rv) {
>  		dev_err(&op->dev, "error registering with ATA layer\n");
> -		goto err2;
> +		goto err_free_irq;
>  	}
>  
>  	return 0;
>  
> - err2:
> + err_free_irq:
> +	free_irq(task_irq, priv);
>  	irq_dispose_mapping(task_irq);
> + err_free_task:
>  	bcom_ata_release(dmatsk);
>   err1:
>  	irq_dispose_mapping(ata_irq);
> @@ -811,6 +813,7 @@ static void mpc52xx_ata_remove(struct platform_device *op)
>  
>  	/* Clean up DMA */
>  	task_irq = bcom_get_task_irq(priv->dmatsk);
> +	free_irq(task_irq, priv);
>  	irq_dispose_mapping(task_irq);
>  	bcom_ata_release(priv->dmatsk);
>  	irq_dispose_mapping(priv->ata_irq);
> -- 
> 2.54.0
> 

+ Andy

Andy, since you gave a talk about this recently.
Surely there must be a better way than to migrate away from the device
managed APIs.

Do you have some suggestion?


Kind regards,
Niklas

Re: [PATCH 1/4] ata: pata_mpc52xx: switch to non-devm request_irq for proper ordering

[Andy Shevchenko]

On Tue, Jun 09, 2026 at 09:20:35AM +0200, Niklas Cassel wrote:
> On Mon, Jun 08, 2026 at 02:47:15PM -0700, Rosen Penev wrote:
> > The BestComm task IRQ handler accesses priv->dmatsk, which is freed
> > by bcom_ata_release().  With devm_request_irq(), the handler remains
> > registered until devres unwinds after the error/remove function
> > returns, creating a window where the handler can access freed memory.
> > 
> > Switch to request_irq() / free_irq() so that the interrupt is
> > unregistered before bcom_ata_release() tears down the task.

...

> >  	task_irq = bcom_get_task_irq(dmatsk);
> > -	rv = devm_request_irq(&op->dev, task_irq, &mpc52xx_ata_task_irq, 0,
> > -				"ATA task", priv);
> > +	rv = request_irq(task_irq, &mpc52xx_ata_task_irq, 0,
> > +			  "ATA task", priv);
> >  	if (rv) {

> >  		dev_err(&op->dev, "error requesting DMA IRQ\n");

Side note: In case we leave devm_request_irq(), this message is a dup
and may be removed.

> > -		goto err2;
> > +		goto err_free_task;

Now, devm_*() should be never followed by goto. If we want to keep devm
approach, the original code has more issues.

> >  	}

...

> Andy, since you gave a talk about this recently.
> Surely there must be a better way than to migrate away from the device
> managed APIs.

Thank for Cc'ing me.

> Do you have some suggestion?

This patch either way (are we going to devm fully or rollback to non-devm)
is half-baked.

What I may suggest here is to extend this patch to cover the second
problematic place (*) and make it as a fix for backporting.

After that one may carefully go and finish converting probe to devm_*().

*) https://elixir.bootlin.com/linux/v7.1-rc7/source/drivers/ata/pata_mpc52xx.c#L733
The call of irq_of_parse_and_map() should be moved after devm_kzalloc().
So, this fix should do what it does + the above.

P.S.
Someone may tide up this driver a lot, but this is out of the scope of
this patch. FWIW, the brief list of potential refactoring patches
based on probe (in unsorted order):
- use devm_platform_ioremap_resource()
- moving towards device_property_read_*()
- using dev_err_probe()
- use temporary variable to hold struct device pointer
- use fwnode_irq_get()
- apply multipliers from units.h and time.h (like HZ_PER_MHZ)
- moving to a new struct dev_pm_ops and pm_sleep_ptr() macro

-- 
With Best Regards,
Andy Shevchenko

[PATCH 2/4] ata: pata_mpc52xx: remove redundant irq_dispose_mapping calls

[Rosen Penev]

bcom_ata_release() calls bcom_task_free(), which already calls
irq_dispose_mapping(tsk->irq).  The explicit irq_dispose_mapping()
calls in the probe error path and remove function are redundant
and cause a double-free of the same mapping.

Remove them.

Assisted-by: opencode:big-pickle
Fixes: d01159dffa15 ("drivers/ata/pata_mpc52xx.c: clean up error handling code")
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/pata_mpc52xx.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
index 9af0aa89a5c6..1246b705a411 100644
--- a/drivers/ata/pata_mpc52xx.c
+++ b/drivers/ata/pata_mpc52xx.c
@@ -794,7 +794,6 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 
  err_free_irq:
 	free_irq(task_irq, priv);
-	irq_dispose_mapping(task_irq);
  err_free_task:
 	bcom_ata_release(dmatsk);
  err1:
@@ -814,7 +813,6 @@ static void mpc52xx_ata_remove(struct platform_device *op)
 	/* Clean up DMA */
 	task_irq = bcom_get_task_irq(priv->dmatsk);
 	free_irq(task_irq, priv);
-	irq_dispose_mapping(task_irq);
 	bcom_ata_release(priv->dmatsk);
 	irq_dispose_mapping(priv->ata_irq);
 }
-- 
2.54.0

[PATCH 3/4] ata: pata_mpc52xx: convert to platform_get_irq()

[Rosen Penev]

Replace irq_of_parse_and_map() with platform_get_irq(), which returns
a negative errno on failure and integrates with the platform device
model. Since platform_get_irq() does not create a separately managed
mapping, the corresponding irq_dispose_mapping() calls in the probe
error path and remove function are no longer needed.

Remove most gotos from probe. Simpler to return directly.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/pata_mpc52xx.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
index 1246b705a411..d27390eda98e 100644
--- a/drivers/ata/pata_mpc52xx.c
+++ b/drivers/ata/pata_mpc52xx.c
@@ -21,7 +21,6 @@
 #include <linux/libata.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
-#include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/types.h>
 
@@ -730,18 +729,14 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 	if ((prop) && (proplen >= 4))
 		udma_mask = ATA_UDMA2 & ((1 << (*prop + 1)) - 1);
 
-	ata_irq = irq_of_parse_and_map(op->dev.of_node, 0);
-	if (!ata_irq) {
-		dev_err(&op->dev, "error mapping irq\n");
-		return -EINVAL;
-	}
+	ata_irq = platform_get_irq(op, 0);
+	if (ata_irq < 0)
+		return ata_irq;
 
 	/* Prepare our private structure */
 	priv = devm_kzalloc(&op->dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv) {
-		rv = -ENOMEM;
-		goto err1;
-	}
+	if (!priv)
+		return -ENOMEM;
 
 	priv->ipb_period = 1000000000 / (ipb_freq / 1000);
 	priv->ata_regs = ata_regs;
@@ -762,8 +757,7 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 	dmatsk = bcom_ata_init(MAX_DMA_BUFFERS, MAX_DMA_BUFFER_SIZE);
 	if (!dmatsk) {
 		dev_err(&op->dev, "bestcomm initialization failed\n");
-		rv = -ENOMEM;
-		goto err1;
+		return -ENOMEM;
 	}
 
 	task_irq = bcom_get_task_irq(dmatsk);
@@ -796,8 +790,6 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 	free_irq(task_irq, priv);
  err_free_task:
 	bcom_ata_release(dmatsk);
- err1:
-	irq_dispose_mapping(ata_irq);
 	return rv;
 }
 
@@ -814,7 +806,6 @@ static void mpc52xx_ata_remove(struct platform_device *op)
 	task_irq = bcom_get_task_irq(priv->dmatsk);
 	free_irq(task_irq, priv);
 	bcom_ata_release(priv->dmatsk);
-	irq_dispose_mapping(priv->ata_irq);
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.54.0

[PATCH 4/4] ata: pata_mpc52xx: use devm_platform_get_and_ioremap_resource

[Rosen Penev]

Replace the open-coded sequence of of_address_to_resource(),
devm_request_mem_region(), and devm_ioremap() with the standard
devm_platform_get_and_ioremap_resource() helper.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/pata_mpc52xx.c | 29 ++++++-----------------------
 1 file changed, 6 insertions(+), 23 deletions(-)

diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
index d27390eda98e..3c41a7b860bd 100644
--- a/drivers/ata/pata_mpc52xx.c
+++ b/drivers/ata/pata_mpc52xx.c
@@ -20,7 +20,6 @@
 #include <linux/delay.h>
 #include <linux/libata.h>
 #include <linux/of.h>
-#include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/types.h>
 
@@ -671,7 +670,7 @@ static int mpc52xx_ata_init_one(struct device *dev,
 static int mpc52xx_ata_probe(struct platform_device *op)
 {
 	unsigned int ipb_freq;
-	struct resource res_mem;
+	struct resource *res_mem;
 	int ata_irq = 0;
 	struct mpc52xx_ata __iomem *ata_regs;
 	struct mpc52xx_ata_priv *priv = NULL;
@@ -688,25 +687,9 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 		return -ENODEV;
 	}
 
-	/* Get device base address from device tree, request the region
-	 * and ioremap it. */
-	rv = of_address_to_resource(op->dev.of_node, 0, &res_mem);
-	if (rv) {
-		dev_err(&op->dev, "could not determine device base address\n");
-		return rv;
-	}
-
-	if (!devm_request_mem_region(&op->dev, res_mem.start,
-				     sizeof(*ata_regs), DRV_NAME)) {
-		dev_err(&op->dev, "error requesting register region\n");
-		return -EBUSY;
-	}
-
-	ata_regs = devm_ioremap(&op->dev, res_mem.start, sizeof(*ata_regs));
-	if (!ata_regs) {
-		dev_err(&op->dev, "error mapping device registers\n");
-		return -ENOMEM;
-	}
+	ata_regs = devm_platform_get_and_ioremap_resource(op, 0, &res_mem);
+	if (IS_ERR(ata_regs))
+		return PTR_ERR(ata_regs);
 
 	/*
 	 * By default, all DMA modes are disabled for the MPC5200.  Some
@@ -740,7 +723,7 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 
 	priv->ipb_period = 1000000000 / (ipb_freq / 1000);
 	priv->ata_regs = ata_regs;
-	priv->ata_regs_pa = res_mem.start;
+	priv->ata_regs_pa = res_mem->start;
 	priv->ata_irq = ata_irq;
 	priv->csel = -1;
 	priv->mpc52xx_ata_dma_last_write = -1;
@@ -777,7 +760,7 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 	}
 
 	/* Register ourselves to libata */
-	rv = mpc52xx_ata_init_one(&op->dev, priv, res_mem.start,
+	rv = mpc52xx_ata_init_one(&op->dev, priv, res_mem->start,
 				  mwdma_mask, udma_mask);
 	if (rv) {
 		dev_err(&op->dev, "error registering with ATA layer\n");
-- 
2.54.0