[PATCH 0/2] watchdog: sama5d4: fix IRQ and timeout bugs, use platform_get_irq_optional

[PATCH 0/2] watchdog: sama5d4: fix IRQ and timeout bugs, use platform_get_irq_optional

[Rosen Penev]

This series fixes three pre-existing issues in the sama5d4 watchdog
driver (unsafe IRQF_SHARED, incorrect IRQ_HANDLED return, ignored
device-tree timeout), then simplifies interrupt acquisition by
switching from irq_of_parse_and_map() to platform_get_irq_optional().

Rosen Penev (2):
  watchdog: sama5d4: fix shared IRQ and hardcoded timeout issues
  watchdog: sama5d4: use platform_get_irq_optional()

 drivers/watchdog/sama5d4_wdt.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

-- 
2.54.0

[PATCH 1/2] watchdog: sama5d4: fix shared IRQ and hardcoded timeout issues

[Rosen Penev]

Fix three pre-existing issues in the sama5d4 watchdog driver:

1. Unsafe IRQF_SHARED | IRQF_NO_SUSPEND combination: The watchdog
   interrupt is a dedicated peripheral line, not shared with other
   devices.  Drop IRQF_SHARED to avoid the documented unsafe interaction
   where IRQF_NO_SUSPEND keeps the line enabled during suspend while
   other sharing devices may be powered down.

2. Unconditional IRQ_HANDLED on shared line: The handler returned
   IRQ_HANDLED even when the status register indicated no watchdog
   interrupt was pending.  Return IRQ_NONE in that case so the kernel
   can properly detect spurious interrupts on the line.

3. Hardcoded 16-second timeout: sama5d4_wdt_init() unconditionally
   used WDT_DEFAULT_TIMEOUT (16s) for the hardware timeout, ignoring
   any timeout configured via device tree (watchdog_init_timeout) or
   userspace.  Pass wdd->timeout to sama5d4_wdt_init() so the
   configured timeout is honored during probe and resume.

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

diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
index 704b786cc2ec..b7a8cfed335d 100644
--- a/drivers/watchdog/sama5d4_wdt.c
+++ b/drivers/watchdog/sama5d4_wdt.c
@@ -169,11 +169,12 @@ static irqreturn_t sama5d4_wdt_irq_handler(int irq, void *dev_id)
 	else
 		reg = wdt_read(wdt, AT91_WDT_SR);
 
-	if (reg) {
-		pr_crit("Atmel Watchdog Software Reset\n");
-		emergency_restart();
-		pr_crit("Reboot didn't succeed\n");
-	}
+	if (!reg)
+		return IRQ_NONE;
+
+	pr_crit("Atmel Watchdog Software Reset\n");
+	emergency_restart();
+	pr_crit("Reboot didn't succeed\n");
 
 	return IRQ_HANDLED;
 }
@@ -197,11 +198,11 @@ static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt)
 	return 0;
 }
 
-static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
+static int sama5d4_wdt_init(struct sama5d4_wdt *wdt, unsigned int timeout)
 {
 	u32 reg, val;
 
-	val = WDT_SEC2TICKS(WDT_DEFAULT_TIMEOUT);
+	val = WDT_SEC2TICKS(timeout);
 	/*
 	 * When booting and resuming, the bootloader may have changed the
 	 * watchdog configuration.
@@ -289,8 +290,8 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
 
 	if (wdt->need_irq) {
 		ret = devm_request_irq(dev, irq, sama5d4_wdt_irq_handler,
-				       IRQF_SHARED | IRQF_IRQPOLL |
-				       IRQF_NO_SUSPEND, pdev->name, pdev);
+				       IRQF_IRQPOLL | IRQF_NO_SUSPEND,
+				       pdev->name, pdev);
 		if (ret) {
 			dev_err(dev, "cannot register interrupt handler\n");
 			return ret;
@@ -305,7 +306,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
 		set_bit(WDOG_HW_RUNNING, &wdd->status);
 	}
 
-	ret = sama5d4_wdt_init(wdt);
+	ret = sama5d4_wdt_init(wdt, wdd->timeout);
 	if (ret)
 		return ret;
 
@@ -358,7 +359,7 @@ static int sama5d4_wdt_resume_early(struct device *dev)
 	 * This should only be done when the registers are lost on suspend but
 	 * there is no way to get this information right now.
 	 */
-	sama5d4_wdt_init(wdt);
+	sama5d4_wdt_init(wdt, wdt->wdd.timeout);
 
 	if (watchdog_active(&wdt->wdd))
 		sama5d4_wdt_start(&wdt->wdd);
-- 
2.54.0

[PATCH 2/2] watchdog: sama5d4: use platform_get_irq_optional()

[Rosen Penev]

irq_of_parse_and_map() requires irq_dispose_mapping() on failure. Don't
bother with it as platform_get_irq_optional() doesn't need it.

Also handle EPROBE_DEFER.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/watchdog/sama5d4_wdt.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
index b7a8cfed335d..030029d50257 100644
--- a/drivers/watchdog/sama5d4_wdt.c
+++ b/drivers/watchdog/sama5d4_wdt.c
@@ -11,7 +11,6 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
-#include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/reboot.h>
 #include <linux/watchdog.h>
@@ -245,7 +244,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
 	struct watchdog_device *wdd;
 	struct sama5d4_wdt *wdt;
 	void __iomem *regs;
-	u32 irq = 0;
+	int irq = 0;
 	u32 reg;
 	int ret;
 
@@ -281,8 +280,11 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
 		return ret;
 
 	if (wdt->need_irq) {
-		irq = irq_of_parse_and_map(dev->of_node, 0);
-		if (!irq) {
+		irq = platform_get_irq_optional(pdev, 0);
+		if (irq == -EPROBE_DEFER)
+			return irq;
+
+		if (irq < 0) {
 			dev_warn(dev, "failed to get IRQ from DT\n");
 			wdt->need_irq = false;
 		}
-- 
2.54.0

Re: [PATCH 2/2] watchdog: sama5d4: use platform_get_irq_optional()

[sashiko-bot]

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

Pre-existing issues:
- [High] Race condition leading to NULL pointer dereference if the watchdog interrupt fires before probe completes.
--

commit 81aa11d20f236c5efcd53479d7c7d1601cbc92d0
Author: Rosen Penev <rosenp@gmail.com>
watchdog: sama5d4: use platform_get_irq_optional()

irq_of_parse_and_map() requires irq_dispose_mapping() on failure. Don't
bother with it as platform_get_irq_optional() doesn't need it.

Also handle EPROBE_DEFER.

> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index b7b5b24afb34b..d218fc04b770c 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c

[ ... ]

> @@ -287,8 +286,11 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	if (wdt->need_irq) {
> -		irq = irq_of_parse_and_map(dev->of_node, 0);
> -		if (!irq) {
> +		irq = platform_get_irq_optional(pdev, 0);
> +		if (irq == -EPROBE_DEFER)
> +			return irq;
> +
> +		if (irq < 0) {
>  			dev_warn(dev, "failed to get IRQ from DT\n");
>  			wdt->need_irq = false;
>  		}

[Severity: High]
This is a pre-existing issue, but since the IRQ handling is being updated
here, could this lead to a NULL pointer dereference if the interrupt fires
early?

Further down in sama5d4_wdt_probe(), the interrupt is registered and
unmasked, passing pdev as the dev_id context:

drivers/watchdog/sama5d4_wdt.c:sama5d4_wdt_probe() {
        ...
        ret = devm_request_irq(dev, irq, sama5d4_wdt_irq_handler,
                               IRQF_IRQPOLL | IRQF_NO_SUSPEND,
                               pdev->name, pdev);
        ...
        ret = sama5d4_wdt_init(wdt, wdd->timeout);
        ...
}

The sama5d4_wdt_init() function enables the hardware interrupts, creating
a race window because platform_set_drvdata(pdev, wdt) is not called until
the very end of the probe function.

If an interrupt fires before platform_set_drvdata() completes (for example,
if left pending by the bootloader), the interrupt handler will retrieve a NULL
pointer:

drivers/watchdog/sama5d4_wdt.c:sama5d4_wdt_irq_handler() {
        struct sama5d4_wdt *wdt = platform_get_drvdata(dev_id);
        ...
        if (wdt->sam9x60_support)
        ...
}

Should platform_set_drvdata() be moved up before the interrupt is
requested, or should devm_request_irq() be passed the wdt pointer directly
instead of pdev?

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

Re: [PATCH 1/2] watchdog: sama5d4: fix shared IRQ and hardcoded timeout issues

[sashiko-bot]

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

New issues:
- [High] Dropping IRQF_SHARED causes a probe regression on SAM9X60 and SAM9X7 platforms where the watchdog IRQ is genuinely shared.

Pre-existing issues:
- [High] A race condition during probe can cause a NULL pointer dereference in the IRQ handler because the IRQ is requested before the platform drvdata is set.
- [High] The driver incorrectly uses the SAMA5D4 watchdog disable bit (`AT91_WDT_WDDIS`) instead of the SAM9X60 watchdog disable bit (`AT91_SAM9X60_WDDIS`) when checking if the hardware is running.
--

commit 10beb96fcac3b8fc975881a372fcfe158d7e1630
Author: Rosen Penev <rosenp@gmail.com>

watchdog: sama5d4: fix shared IRQ and hardcoded timeout issues

This commit modifies the sama5d4 watchdog driver to drop the shared IRQ
flag, handle spurious interrupts appropriately by returning IRQ_NONE, and
use the configured timeout during initialization instead of a hardcoded
16-second default.

> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c

[ ... ]

>  	if (wdt->need_irq) {
>  		ret = devm_request_irq(dev, irq, sama5d4_wdt_irq_handler,
> -				       IRQF_SHARED | IRQF_IRQPOLL |
> -				       IRQF_NO_SUSPEND, pdev->name, pdev);
> +				       IRQF_IRQPOLL | IRQF_NO_SUSPEND,
> +				       pdev->name, pdev);

[Severity: High]
Will dropping IRQF_SHARED cause probe failures on SAM9X60 and SAM9X7
platforms?  On these devices, the watchdog uses the System IRQ, which is
shared with other critical peripherals like the RTC, RTT, PMC, and PIT.  If
another driver has already requested the shared line, won't
devm_request_irq() fail with -EBUSY and disable the watchdog entirely?

[Severity: High]
This is a pre-existing issue, but can this sequence lead to a NULL pointer
dereference?

The IRQ is requested and enabled here, but platform_set_drvdata() isn't
called until the end of sama5d4_wdt_probe().  If an interrupt fires
immediately (for instance, from a shared device like the RTC on SAM9X60),
sama5d4_wdt_irq_handler() will execute:

sama5d4_wdt_irq_handler() {
        struct sama5d4_wdt *wdt = platform_get_drvdata(dev_id);
        u32 reg;

        if (wdt->sam9x60_support)
        ...
}

Since platform_get_drvdata() returns NULL before the driver data is set,
won't this crash when attempting to access wdt->sam9x60_support?

>  		if (ret) {
>  			dev_err(dev, "cannot register interrupt handler\n");
>  			return ret;

[ ... ]

>  	reg = wdt_read(wdt, AT91_WDT_MR);
>  	if (!(reg & AT91_WDT_WDDIS)) {
>  		wdt->mr &= ~AT91_WDT_WDDIS;
>  		set_bit(WDOG_HW_RUNNING, &wdd->status);
>  	}
>  
> -	ret = sama5d4_wdt_init(wdt);
> +	ret = sama5d4_wdt_init(wdt, wdd->timeout);

[Severity: High]
This is a pre-existing issue, but does this logic correctly identify if the
hardware is running on SAM9X60 platforms?

The driver checks AT91_WDT_WDDIS (which is bit 15) unconditionally here and
in the wdt_enabled macro.  However, SAM9X60 uses AT91_SAM9X60_WDDIS 
(which is bit 12).

Since the SAM9X60 initial wdt->mr only has bit 12 set, won't this check
always evaluate to true, bypassing the safe watchdog initialization block
inside sama5d4_wdt_init() and potentially leaving the system exposed to
spurious resets?

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