[PATCHv3 0/6] ata: pata_mpc52xx: fix cleanup ordering and modernize

[PATCHv3 0/6] 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.

v3: extra sashiko fix and platform_get_irq moved.
v2: add extra patch from sashiko review.

Rosen Penev (6):
  ata: pata_mpc52xx: fix NULL pointer dereference in IRQ handler
  ata: pata_mpc52xx: synchronize with task IRQ before resetting DMA
    state
  ata: pata_mpc52xx: reset cached DMA direction on resume
  ata: pata_mpc52xx: remove redundant irq_dispose_mapping calls
  ata: pata_mpc52xx: convert to platform_get_irq()
  ata: pata_mpc52xx: convert to full devm resource management

 drivers/ata/pata_mpc52xx.c | 101 ++++++++++++-------------------------
 1 file changed, 33 insertions(+), 68 deletions(-)

--
2.54.0

[PATCHv3 1/6] ata: pata_mpc52xx: fix NULL pointer dereference in IRQ handler

[Rosen Penev]

Assign priv->dmatsk before request_irq() to ensure the IRQ handler
does not dereference a NULL pointer. The handler accesses priv->dmatsk
via bcom_retrieve_buffer().

Assisted-by: opencode:big-pickle
Fixes: 6b61e69e7bc1 ("powerpc/mpc5200: Add MDMA/UDMA support to MPC5200 ATA driver")
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/pata_mpc52xx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
index 210a63283f62..fe445d6aaff6 100644
--- a/drivers/ata/pata_mpc52xx.c
+++ b/drivers/ata/pata_mpc52xx.c
@@ -766,6 +766,8 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 		goto err1;
 	}
 
+	priv->dmatsk = dmatsk;
+
 	task_irq = bcom_get_task_irq(dmatsk);
 	rv = devm_request_irq(&op->dev, task_irq, &mpc52xx_ata_task_irq, 0,
 				"ATA task", priv);
@@ -773,7 +775,6 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 		dev_err(&op->dev, "error requesting DMA IRQ\n");
 		goto err2;
 	}
-	priv->dmatsk = dmatsk;
 
 	/* Init the hw */
 	rv = mpc52xx_ata_hw_init(priv);
-- 
2.54.0

[PATCHv3 2/6] ata: pata_mpc52xx: synchronize with task IRQ before resetting DMA state

[Rosen Penev]

mpc52xx_bmdma_stop() calls bcom_disable() to stop the BestComm task,
but an already-pending task completion IRQ can still fire and execute
mpc52xx_ata_task_irq() concurrently. The handler iterates the BD ring
via bcom_buffer_done()/bcom_retrieve_buffer() and writes to
priv->waiting_for_dma, while bmdma_stop resets the BD ring with
bcom_ata_reset_bd() and clears waiting_for_dma -- all without
synchronization.

Add a task_irq field to priv, store it during probe, and call
synchronize_irq() after bcom_disable() and before bcom_ata_reset_bd()
to ensure the handler has completed and cannot race with the BD ring
reset or the waiting_for_dma clear.

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

diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
index fe445d6aaff6..72a2b677bc90 100644
--- a/drivers/ata/pata_mpc52xx.c
+++ b/drivers/ata/pata_mpc52xx.c
@@ -58,6 +58,7 @@ struct mpc52xx_ata_priv {
 
 	/* DMA */
 	struct bcom_task		*dmatsk;
+	int				task_irq;
 	const struct udmaspec		*udmaspec;
 	const struct mdmaspec		*mdmaspec;
 	int 				mpc52xx_ata_dma_last_write;
@@ -571,6 +572,7 @@ mpc52xx_bmdma_stop(struct ata_queued_cmd *qc)
 	struct mpc52xx_ata_priv *priv = ap->host->private_data;
 
 	bcom_disable(priv->dmatsk);
+	synchronize_irq(priv->task_irq);
 	bcom_ata_reset_bd(priv->dmatsk);
 	priv->waiting_for_dma = 0;
 
@@ -769,6 +771,7 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 	priv->dmatsk = dmatsk;
 
 	task_irq = bcom_get_task_irq(dmatsk);
+	priv->task_irq = task_irq;
 	rv = devm_request_irq(&op->dev, task_irq, &mpc52xx_ata_task_irq, 0,
 				"ATA task", priv);
 	if (rv) {
-- 
2.54.0

[PATCHv3 3/6] ata: pata_mpc52xx: reset cached DMA direction on resume

[Rosen Penev]

mpc52xx_ata_hw_init() is called from both probe and resume, but only
probe initializes priv->mpc52xx_ata_dma_last_write to -1. On resume
the hardware FIFO state is lost, yet the cached direction survives.
If the first post-resume DMA uses the same direction as before suspend,
mpc52xx_bmdma_setup() skips reconfiguring the FIFO (fifo_control,
fifo_alarm) because the cache matches, leaving the FIFO in its default
reset state.

Move the initialization of mpc52xx_ata_dma_last_write into
mpc52xx_ata_hw_init() so it is reset on both probe and resume.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/pata_mpc52xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
index 72a2b677bc90..b2e2f83aa9ca 100644
--- a/drivers/ata/pata_mpc52xx.c
+++ b/drivers/ata/pata_mpc52xx.c
@@ -375,6 +375,7 @@ mpc52xx_ata_hw_init(struct mpc52xx_ata_priv *priv)
 
 	/* Init timings to PIO0 */
 	memset(priv->timings, 0x00, 2*sizeof(struct mpc52xx_ata_timings));
+	priv->mpc52xx_ata_dma_last_write = -1;
 
 	mpc52xx_ata_compute_pio_timings(priv, 0, 0);
 	mpc52xx_ata_compute_pio_timings(priv, 1, 0);
@@ -750,7 +751,6 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 	priv->ata_regs_pa = res_mem.start;
 	priv->ata_irq = ata_irq;
 	priv->csel = -1;
-	priv->mpc52xx_ata_dma_last_write = -1;
 
 	if (ipb_freq/1000000 == 66) {
 		priv->mdmaspec = mdmaspec66;
-- 
2.54.0

[PATCHv3 4/6] 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 | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
index b2e2f83aa9ca..4645de4cc257 100644
--- a/drivers/ata/pata_mpc52xx.c
+++ b/drivers/ata/pata_mpc52xx.c
@@ -797,7 +797,6 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 	return 0;
 
  err2:
-	irq_dispose_mapping(task_irq);
 	bcom_ata_release(dmatsk);
  err1:
 	irq_dispose_mapping(ata_irq);
@@ -808,14 +807,11 @@ static void mpc52xx_ata_remove(struct platform_device *op)
 {
 	struct ata_host *host = platform_get_drvdata(op);
 	struct mpc52xx_ata_priv *priv = host->private_data;
-	int task_irq;
 
 	/* Deregister the ATA interface */
 	ata_platform_remove_one(op);
 
 	/* Clean up DMA */
-	task_irq = bcom_get_task_irq(priv->dmatsk);
-	irq_dispose_mapping(task_irq);
 	bcom_ata_release(priv->dmatsk);
 	irq_dispose_mapping(priv->ata_irq);
 }
-- 
2.54.0

[PATCHv3 5/6] 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.

Move platform_get_irq to after devm_kzalloc, as requested.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>

ata: pata_mpc52xx: move jiowserhfiohoqwe

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/pata_mpc52xx.c | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
index 4645de4cc257..238b39af38db 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>
 
@@ -676,7 +675,6 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 {
 	unsigned int ipb_freq;
 	struct resource res_mem;
-	int ata_irq = 0;
 	struct mpc52xx_ata __iomem *ata_regs;
 	struct mpc52xx_ata_priv *priv = NULL;
 	int rv, task_irq;
@@ -733,25 +731,20 @@ 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;
-	}
-
 	/* 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;
 	priv->ata_regs_pa = res_mem.start;
-	priv->ata_irq = ata_irq;
 	priv->csel = -1;
 
+	priv->ata_irq = platform_get_irq(op, 0);
+	if (priv->ata_irq < 0)
+		return priv->ata_irq;
+
 	if (ipb_freq/1000000 == 66) {
 		priv->mdmaspec = mdmaspec66;
 		priv->udmaspec = udmaspec66;
@@ -764,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;
 	}
 
 	priv->dmatsk = dmatsk;
@@ -798,8 +790,6 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 
  err2:
 	bcom_ata_release(dmatsk);
- err1:
-	irq_dispose_mapping(ata_irq);
 	return rv;
 }
 
@@ -813,7 +803,6 @@ static void mpc52xx_ata_remove(struct platform_device *op)
 
 	/* Clean up DMA */
 	bcom_ata_release(priv->dmatsk);
-	irq_dispose_mapping(priv->ata_irq);
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.54.0

[PATCHv3 6/6] ata: pata_mpc52xx: convert to full devm resource management

[Rosen Penev]

Replace the open-coded of_address_to_resource()/devm_request_mem_region()/
devm_ioremap() sequence with devm_platform_get_and_ioremap_resource(),
and switch irq_of_parse_and_map() to platform_get_irq().

Use devm_add_action_or_reset() to manage the BestComm DMA task
(bcom_task) lifetime, and switch the task IRQ to devm_request_irq().
With both resources under devm, the LIFO teardown is free_irq before
bcom_ata_release, which is the correct ordering.

Remove the mpc52xx_ata_remove() wrapper and point .remove directly
at ata_platform_remove_one.  All error-path gotos and manual cleanup
are gone; any failure after devm_add_action_or_reset simply returns
and devm handles the unwind.

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

diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
index 238b39af38db..2472a2e2fbf0 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,10 +670,15 @@ static int mpc52xx_ata_init_one(struct device *dev,
 /* OF Platform driver                                                       */
 /* ======================================================================== */
 
+static void devm_bcom_ata_release(void *data)
+{
+	bcom_ata_release(data);
+}
+
 static int mpc52xx_ata_probe(struct platform_device *op)
 {
 	unsigned int ipb_freq;
-	struct resource res_mem;
+	struct resource *res_mem;
 	struct mpc52xx_ata __iomem *ata_regs;
 	struct mpc52xx_ata_priv *priv = NULL;
 	int rv, task_irq;
@@ -690,25 +694,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
@@ -738,7 +726,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->csel = -1;
 
 	priv->ata_irq = platform_get_irq(op, 0);
@@ -762,47 +750,35 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 
 	priv->dmatsk = dmatsk;
 
+	rv = devm_add_action_or_reset(&op->dev, devm_bcom_ata_release, dmatsk);
+	if (rv)
+		return rv;
+
 	task_irq = bcom_get_task_irq(dmatsk);
 	priv->task_irq = task_irq;
 	rv = devm_request_irq(&op->dev, task_irq, &mpc52xx_ata_task_irq, 0,
-				"ATA task", priv);
+			      "ATA task", priv);
 	if (rv) {
 		dev_err(&op->dev, "error requesting DMA IRQ\n");
-		goto err2;
+		return rv;
 	}
 
 	/* Init the hw */
 	rv = mpc52xx_ata_hw_init(priv);
 	if (rv) {
 		dev_err(&op->dev, "error initializing hardware\n");
-		goto err2;
+		return rv;
 	}
 
 	/* 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");
-		goto err2;
+		return rv;
 	}
 
 	return 0;
-
- err2:
-	bcom_ata_release(dmatsk);
-	return rv;
-}
-
-static void mpc52xx_ata_remove(struct platform_device *op)
-{
-	struct ata_host *host = platform_get_drvdata(op);
-	struct mpc52xx_ata_priv *priv = host->private_data;
-
-	/* Deregister the ATA interface */
-	ata_platform_remove_one(op);
-
-	/* Clean up DMA */
-	bcom_ata_release(priv->dmatsk);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -843,7 +819,7 @@ static const struct of_device_id mpc52xx_ata_of_match[] = {
 
 static struct platform_driver mpc52xx_ata_of_platform_driver = {
 	.probe		= mpc52xx_ata_probe,
-	.remove		= mpc52xx_ata_remove,
+	.remove		= ata_platform_remove_one,
 #ifdef CONFIG_PM_SLEEP
 	.suspend	= mpc52xx_ata_suspend,
 	.resume		= mpc52xx_ata_resume,
-- 
2.54.0

Re: [PATCHv3 4/6] ata: pata_mpc52xx: remove redundant irq_dispose_mapping calls

[sashiko-bot]

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

Pre-existing issues:
- [High] Calling `irq_dispose_mapping()` on devres-managed IRQs destroys the IRQ descriptor before `devm_free_irq()` can execute, leading to a memory leak of `struct irqaction` and bypassing interrupt synchronization.
--

commit 22d637bd9ec3129e235484c8f8f7b01af8844e9d
Author: Rosen Penev <rosenp@gmail.com>

ata: pata_mpc52xx: remove redundant irq_dispose_mapping calls

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.

> diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
> index 210a63283f62b..408b1cb75d1e7 100644
> --- a/drivers/ata/pata_mpc52xx.c
> +++ b/drivers/ata/pata_mpc52xx.c
> @@ -793,7 +793,6 @@ static int mpc52xx_ata_probe(struct platform_device *op)
>  	return 0;
>  
>   err2:
> -	irq_dispose_mapping(task_irq);
>  	bcom_ata_release(dmatsk);
>   err1:
>  	irq_dispose_mapping(ata_irq);

[Severity: High]
This is a pre-existing issue, but does calling irq_dispose_mapping() on these
IRQs bypass the devres cleanup mechanisms?

Because task_irq and ata_irq are managed via devm_request_irq(), their 
free_irq() calls are deferred to the devres cleanup phase.

If irq_dispose_mapping() (called explicitly for ata_irq, and implicitly via
bcom_ata_release() for task_irq) destroys the IRQ descriptor before devres
cleanup runs, the subsequent devm_irq_release() -> free_irq() will fail to
find the descriptor when irq_to_desc() returns NULL.

This causes free_irq() to return immediately without cleaning up, which
leaks the struct irqaction memory. 

More importantly, it skips the call to __synchronize_irq(). Could this allow
the interrupt handler to continue running concurrently while devres tears
down the driver's mapped registers, posing a use-after-free risk?

> @@ -804,14 +803,11 @@ static void mpc52xx_ata_remove(struct platform_device *op)
>  {
>  	struct ata_host *host = platform_get_drvdata(op);
>  	struct mpc52xx_ata_priv *priv = host->private_data;
[ ... ]
>  	/* Clean up DMA */
> -	task_irq = bcom_get_task_irq(priv->dmatsk);
> -	irq_dispose_mapping(task_irq);
>  	bcom_ata_release(priv->dmatsk);
>  	irq_dispose_mapping(priv->ata_irq);
>  }

[Severity: High]
This is also a pre-existing issue, but the same devres synchronization concern
applies here in mpc52xx_ata_remove() when disposing the mappings prior to the
device devres teardown.

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