[PATCH] rtc: mt7622: allocate the RTC before requesting its IRQ

[PATCH] rtc: mt7622: allocate the RTC before requesting its IRQ

[Myeonghun Pak]

The alarm handler passes hw->rtc to rtc_update_irq(). Currently, the
managed IRQ is requested before the RTC is allocated. On detach, devres
unregisters the RTC and drops its allocation reference before freeing the
IRQ. If no other references remain, the RTC can be freed while the alarm
handler is still registered or running.

Allocate the RTC before requesting the IRQ, and initialize the hardware
and RTC operations before installing the handler. Register the RTC last,
once these dependencies are ready. Managed cleanup then unregisters the
RTC, frees the IRQ and waits for running handlers, and finally drops the
RTC allocation reference. The final irqwork cancellation therefore runs
after the IRQ producer has stopped.

This issue was identified as part of our ongoing research while
reviewing kernel code.

Fixes: ba5d018135ef ("rtc: mediatek: add driver for RTC on MT7622 SoC")
Assisted-by: LLM
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>

---
Compile-tested with Clang, W=1. No hardware runtime testing.

Based on mainline 0fcb05b72344f40806cbe810de8872f1698bf053.

diff --git a/drivers/rtc/rtc-mt7622.c b/drivers/rtc/rtc-mt7622.c
index 9e17dd09d567..c86c8cf3f2d7 100644
--- a/drivers/rtc/rtc-mt7622.c
+++ b/drivers/rtc/rtc-mt7622.c
@@ -324,11 +324,20 @@
 	if (ret)
 		return ret;
 
+	hw->rtc = devm_rtc_allocate_device(&pdev->dev);
+	if (IS_ERR(hw->rtc)) {
+		ret = PTR_ERR(hw->rtc);
+		goto err;
+	}
+
 	hw->irq = platform_get_irq(pdev, 0);
 	if (hw->irq < 0) {
 		ret = hw->irq;
 		goto err;
 	}
+
+	mtk_rtc_hw_init(hw);
+	hw->rtc->ops = &mtk_rtc_ops;
 
 	ret = devm_request_irq(&pdev->dev, hw->irq, mtk_rtc_alarmirq,
 			       0, dev_name(&pdev->dev), hw);
@@ -337,14 +346,10 @@
 		goto err;
 	}
 
-	mtk_rtc_hw_init(hw);
-
 	device_init_wakeup(&pdev->dev, true);
 
-	hw->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
-					   &mtk_rtc_ops, THIS_MODULE);
-	if (IS_ERR(hw->rtc)) {
-		ret = PTR_ERR(hw->rtc);
+	ret = devm_rtc_register_device(hw->rtc);
+	if (ret) {
 		dev_err(&pdev->dev, "Unable to register device\n");
 		goto err;
 	}

Re: [PATCH] rtc: mt7622: allocate the RTC before requesting its IRQ

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] rtc: mt7622: clk disabled in mtk_rtc_remove() before devres teardown
--

commit aaf543ae0f6d2f3fb8f602ad5f4977facde6059e
Author: Myeonghun Pak <mhun512@gmail.com>

rtc: mt7622: allocate the RTC before requesting its IRQ

This patch reorders RTC allocation, hardware initialization, IRQ request,
and RTC registration in mtk_rtc_probe() so that devres unregisters the
RTC and frees its IRQ before dropping the final RTC device reference.

> diff --git a/drivers/rtc/rtc-mt7622.c b/drivers/rtc/rtc-mt7622.c
> index 9e17dd09d5673..c86c8cf3f2d77 100644
> --- a/drivers/rtc/rtc-mt7622.c
> +++ b/drivers/rtc/rtc-mt7622.c
> @@ -324,12 +324,21 @@ static int mtk_rtc_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	hw->rtc = devm_rtc_allocate_device(&pdev->dev);
> +	if (IS_ERR(hw->rtc)) {
> +		ret = PTR_ERR(hw->rtc);
> +		goto err;
> +	}
> +
>  	hw->irq = platform_get_irq(pdev, 0);
>  	if (hw->irq < 0) {
>  		ret = hw->irq;
>  		goto err;
>  	}
>  
> +	mtk_rtc_hw_init(hw);
> +	hw->rtc->ops = &mtk_rtc_ops;
> +
>  	ret = devm_request_irq(&pdev->dev, hw->irq, mtk_rtc_alarmirq,
>  			       0, dev_name(&pdev->dev), hw);
>  	if (ret) {
> @@ -337,14 +346,10 @@ static int mtk_rtc_probe(struct platform_device *pdev)
>  		goto err;
>  	}
>  
> -	mtk_rtc_hw_init(hw);
> -
>  	device_init_wakeup(&pdev->dev, true);
>  
> -	hw->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
> -					   &mtk_rtc_ops, THIS_MODULE);
> -	if (IS_ERR(hw->rtc)) {
> -		ret = PTR_ERR(hw->rtc);
> +	ret = devm_rtc_register_device(hw->rtc);
> +	if (ret) {
>  		dev_err(&pdev->dev, "Unable to register device\n");
>  		goto err;
>  	}

[Severity: Medium]
While this fixes the devres ordering between hw->rtc and hw->irq, what
about hw->clk during detach?

In mtk_rtc_probe(), hw->clk is enabled manually before the devm calls:

drivers/rtc/rtc-mt7622.c:mtk_rtc_probe() {
	...
	ret = clk_prepare_enable(hw->clk);
	if (ret)
		return ret;

	hw->rtc = devm_rtc_allocate_device(&pdev->dev);
	...
}

And it is disabled in mtk_rtc_remove():

drivers/rtc/rtc-mt7622.c:mtk_rtc_remove() {
	struct mtk_rtc *hw = platform_get_drvdata(pdev);

	clk_disable_unprepare(hw->clk);
}

During driver unbind or module unload, __device_release_driver() calls
device_remove() -> mtk_rtc_remove() before device_unbind_cleanup() calls
devres_release_all().

Because hw->rtc and hw->irq are cleaned up by devres,
clk_disable_unprepare(hw->clk) runs while hw->rtc is still registered and
mtk_rtc_alarmirq() is still active. If an alarm interrupt fires or an
mtk_rtc_ops callback runs in that window, mtk_rtc_alarmirq() or the RTC
operations will access hw->base MMIO registers after hw->clk has already
been disabled:

drivers/rtc/rtc-mt7622.c:mtk_rtc_alarmirq() {
	struct mtk_rtc *hw = (struct mtk_rtc *)id;
	u32 irq_sta;

	irq_sta = mtk_r32(hw, MTK_RTC_INT);
	...
}

Should mtk_rtc_probe() use devm_clk_get_enabled() (and drop
mtk_rtc_remove()) so that hw->clk stays enabled until after devres
unregisters hw->rtc and frees hw->irq?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179229507333.3965554.17726204422948048488.rtc-mt7622-irq-init-order@gmail.com?part=1