rtc: ma35d1: fix unchecked IRQ error and IRQ-before-rtcdev ordering
Subsystems (2)
PATCH 1/1 rtc: ma35d1: fix unchecked IRQ error and IRQ-before-rtcdev ordering
platform_get_irq()'s return goes straight into rtc->irq_num and then devm_request_irq()'s unsigned int irq parameter with no check -- passing a negative error (like -EPROBE_DEFER) through that conversion turns it into a large positive number, so devm_request_irq() just fails with -EINVAL instead of deferring the probe.
Separately, the IRQ is requested before rtc->rtcdev is allocated and registered. Since devm teardown is LIFO, unbind or a later probe failure frees rtcdev before the IRQ, so an alarm firing in that window has ma35d1_rtc_interrupt() call rtc_update_irq() on freed memory.
Check irq_num for a negative return, and move the IRQ request to after the RTC device is fully registered.
--- drivers/rtc/rtc-ma35d1.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
drivers/rtc/rtc-ma35d1.c +7 -5
@@ -236,11 +236,8 @@ static int ma35d1_rtc_probe(struct platform_device *pdev) } rtc->irq_num =platform_get_irq (pdev,0 ); - - ret =devm_request_irq (&pdev->dev, rtc->irq_num, ma35d1_rtc_interrupt, -IRQF_NO_SUSPEND ,"ma35d1rtc" , rtc); -if (ret) -return dev_err_probe (&pdev->dev, ret,"Failed to request rtc irq\n" ); +if (rtc->irq_num <0 ) +return dev_err_probe (&pdev->dev, rtc->irq_num,"failed to get rtc irq\n" );platform_set_drvdata (pdev, rtc);
@@ -258,6 +255,11 @@ static int ma35d1_rtc_probe(struct platform_device *pdev)if (ret)return dev_err_probe (&pdev->dev, ret,"Failed to register rtc device\n" ); + ret =devm_request_irq (&pdev->dev, rtc->irq_num, ma35d1_rtc_interrupt, +IRQF_NO_SUSPEND ,"ma35d1rtc" , rtc); +if (ret) +return dev_err_probe (&pdev->dev, ret,"Failed to request rtc irq\n" ); +return 0 ; }
Signature · 1 line
2.25.1
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] rtc: devm_request_irq() called after devm_rtc_register_device() - [Low] rtc: redundant…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] rtc: devm_request_irq() called after devm_rtc_register_device()
- [Low] rtc: redundant dev_err_probe() on platform_get_irq() failure
--
commit 65db24f320f8394c839e4022831930f15fb56e95
Author: Cong Nguyen <[email protected]>
rtc: ma35d1: fix unchecked IRQ error and IRQ-before-rtcdev ordering
This commit checks the return value of platform_get_irq() to prevent passing a negative error code to devm_request_irq(). It also moves the IRQ request to after the RTC device is registered, intending to fix the teardown order.
Quoted 4 lines
drivers/rtc/rtc-ma35d1.c static int ma35d1_rtc_probe(struct platform_device *pdev) · line 240
Quoted 10 lines
drivers/rtc/rtc-ma35d1.c +2
-5@@ -236,11 +236,8 @@ static int ma35d1_rtc_probe(struct platform_device *pdev) } rtc->irq_num =platform_get_irq (pdev,0 ); - - ret =devm_request_irq (&pdev->dev, rtc->irq_num, ma35d1_rtc_interrupt, -IRQF_NO_SUSPEND ,"ma35d1rtc" , rtc); -if (ret) -return dev_err_probe (&pdev->dev, ret,"Failed to request rtc irq\n" ); +if (rtc->irq_num <0 ) +return dev_err_probe (&pdev->dev, rtc->irq_num,"failed to get rtc irq\n" );
[Severity: Low]
Does this cause a redundant error message to be printed? platform_get_irq() already calls dev_err_probe() internally when it fails. Returning the error code directly without another dev_err_probe() might be preferred.
[ ... ]
drivers/rtc/rtc-ma35d1.c static int ma35d1_rtc_probe(struct platform_device *pdev) · line 264
Quoted 11 lines
drivers/rtc/rtc-ma35d1.c +5
-0@@ -258,6 +255,11 @@ static int ma35d1_rtc_probe(struct platform_device *pdev)if (ret)return dev_err_probe (&pdev->dev, ret,"Failed to register rtc device\n" ); + ret =devm_request_irq (&pdev->dev, rtc->irq_num, ma35d1_rtc_interrupt, +IRQF_NO_SUSPEND ,"ma35d1rtc" , rtc); +if (ret) +return dev_err_probe (&pdev->dev, ret,"Failed to request rtc irq\n" ); +return 0 ; }
[Severity: High]
Does placing devm_request_irq() after devm_rtc_register_device() introduce a race condition?
Since devres teardown is last-in, first-out, during an unbind or a probe failure, the IRQ would be freed before the RTC device is unregistered. This leaves the device exposed to userspace without an active interrupt handler, potentially leading to missed alarms or unhandled interrupt storms.
Would it be better to place devm_request_irq() between devm_rtc_allocate_device() and devm_rtc_register_device() in ma35d1_rtc_probe()?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1