[PATCH] rtc: mpc5121: convert from irq_of_parse_and_map to platform_get_irq

[PATCH] rtc: mpc5121: convert from irq_of_parse_and_map to platform_get_irq

[Rosen Penev]

irq_of_parse_and_map() is an OF-specific low-level function that
requires the caller to pair it with irq_dispose_mapping(). Mixing
it with devm_request_irq() creates a lifecycle hazard — the probe
error path and remove both dispose the mapping while the managed
free_irq is still pending, leading to a dangling descriptor use in
free_irq().

Switch to platform_get_irq(), which is the standard interface for
platform devices. It returns a negative errno on failure, integrates
with platform resource lookup, and eliminates the need for explicit
irq_dispose_mapping() calls since the IRQ domain core manages the
mapping lifecycle. This lets us drop the error labels and simplify
both probe and remove.

Remove these irq variables from the private struct. They were
originally used to free the irq in _remove. devm has replaced that.

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

m
---
 drivers/rtc/rtc-mpc5121.c | 59 ++++++++++++++-------------------------
 1 file changed, 21 insertions(+), 38 deletions(-)

diff --git a/drivers/rtc/rtc-mpc5121.c b/drivers/rtc/rtc-mpc5121.c
index b90f8337a7e6..964d9594b1d7 100644
--- a/drivers/rtc/rtc-mpc5121.c
+++ b/drivers/rtc/rtc-mpc5121.c
@@ -11,7 +11,6 @@
 #include <linux/module.h>
 #include <linux/rtc.h>
 #include <linux/of.h>
-#include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/io.h>
 #include <linux/slab.h>
@@ -72,8 +71,6 @@ struct mpc5121_rtc_regs {
 };
 
 struct mpc5121_rtc_data {
-	unsigned irq;
-	unsigned irq_periodic;
 	struct mpc5121_rtc_regs __iomem *regs;
 	struct rtc_device *rtc;
 	struct rtc_wkalrm wkalarm;
@@ -292,6 +289,16 @@ static int mpc5121_rtc_probe(struct platform_device *op)
 {
 	struct mpc5121_rtc_data *rtc;
 	int err = 0;
+	int irq_periodic;
+	int irq;
+
+	irq = platform_get_irq(op, 1);
+	if (irq < 0)
+		return irq;
+
+	irq_periodic = platform_get_irq(op, 0);
+	if (irq_periodic < 0)
+		return irq_periodic;
 
 	rtc = devm_kzalloc(&op->dev, sizeof(*rtc), GFP_KERNEL);
 	if (!rtc)
@@ -307,30 +314,20 @@ static int mpc5121_rtc_probe(struct platform_device *op)
 
 	platform_set_drvdata(op, rtc);
 
-	rtc->irq = irq_of_parse_and_map(op->dev.of_node, 1);
-	err = devm_request_irq(&op->dev, rtc->irq, mpc5121_rtc_handler, 0,
+	err = devm_request_irq(&op->dev, irq, mpc5121_rtc_handler, 0,
 			       "mpc5121-rtc", &op->dev);
-	if (err) {
-		dev_err(&op->dev, "%s: could not request irq: %i\n",
-							__func__, rtc->irq);
-		goto out_dispose;
-	}
+	if (err)
+		return err;
 
-	rtc->irq_periodic = irq_of_parse_and_map(op->dev.of_node, 0);
-	err = devm_request_irq(&op->dev, rtc->irq_periodic,
-			       mpc5121_rtc_handler_upd, 0, "mpc5121-rtc_upd",
-			       &op->dev);
-	if (err) {
-		dev_err(&op->dev, "%s: could not request irq: %i\n",
-						__func__, rtc->irq_periodic);
-		goto out_dispose2;
-	}
+	err = devm_request_irq(&op->dev, irq_periodic,
+			       mpc5121_rtc_handler_upd, 0,
+			       "mpc5121-rtc_upd", &op->dev);
+	if (err)
+		return err;
 
 	rtc->rtc = devm_rtc_allocate_device(&op->dev);
-	if (IS_ERR(rtc->rtc)) {
-		err = PTR_ERR(rtc->rtc);
-		goto out_dispose2;
-	}
+	if (IS_ERR(rtc->rtc))
+		return PTR_ERR(rtc->rtc);
 
 	rtc->rtc->ops = &mpc5200_rtc_ops;
 	set_bit(RTC_FEATURE_ALARM_RES_MINUTE, rtc->rtc->features);
@@ -356,18 +353,7 @@ static int mpc5121_rtc_probe(struct platform_device *op)
 		rtc->rtc->range_max = U32_MAX;
 	}
 
-	err = devm_rtc_register_device(rtc->rtc);
-	if (err)
-		goto out_dispose2;
-
-	return 0;
-
-out_dispose2:
-	irq_dispose_mapping(rtc->irq_periodic);
-out_dispose:
-	irq_dispose_mapping(rtc->irq);
-
-	return err;
+	return devm_rtc_register_device(rtc->rtc);
 }
 
 static void mpc5121_rtc_remove(struct platform_device *op)
@@ -378,9 +364,6 @@ static void mpc5121_rtc_remove(struct platform_device *op)
 	/* disable interrupt, so there are no nasty surprises */
 	out_8(&regs->alm_enable, 0);
 	out_8(&regs->int_enable, in_8(&regs->int_enable) & ~0x1);
-
-	irq_dispose_mapping(rtc->irq);
-	irq_dispose_mapping(rtc->irq_periodic);
 }
 
 #ifdef CONFIG_OF
-- 
2.55.0

Re: [PATCH] rtc: mpc5121: convert from irq_of_parse_and_map to platform_get_irq

[sashiko-bot]

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

Pre-existing issues:
- [High] Devm LIFO ordering causes a potential Use-After-Free of `rtc->rtc` during probe failure or device removal.
--

--- Patch [1]: [PATCH] rtc: mpc5121: convert from irq_of_parse_and_map to platform_get_irq ---
[Note: The standard review format has been altered to a free-form summary due to recitation restrictions. No code diffs are quoted.]

commit 67266c7661895f337b8df1626c97434560227e94
Author: Rosen Penev <rosenp@gmail.com>

rtc: mpc5121: convert from irq_of_parse_and_map to platform_get_irq

This commit converts the driver to use platform_get_irq instead of
irq_of_parse_and_map, removing the need for explicit irq_dispose_mapping
calls during teardown.

[Severity: High]
This is a pre-existing issue, but does the ordering of devres allocations
create a use-after-free hazard during teardown?

In mpc5121_rtc_probe(), devm_request_irq() is called before
devm_rtc_allocate_device(). Because the devres framework tears down resources
in LIFO order, during a probe failure or device removal, the RTC device
structure will be freed before the IRQ is unregistered and synchronized.

While mpc5121_rtc_remove() disables hardware interrupts, it does not wait for
currently executing handlers to finish. If an in-flight interrupt on another
CPU executes mpc5121_rtc_handler() during this teardown window, it will
access the already freed rtc->rtc pointer.

Could the IRQ requests be moved after the RTC device allocation to ensure the
IRQ handler is properly unregistered and synchronized before the RTC device
is freed?

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