[PATCHv2 0/3] watchdog: sama5d4: fix IRQ and timeout bugs, use platform_get_irq_optional

[PATCHv2 0/3] watchdog: sama5d4: fix IRQ and timeout bugs, use platform_get_irq_optional

[Rosen Penev]

This series fixes three issues in the sama5d4 watchdog driver:

Patch 1: Return IRQ_NONE when the status register indicates no watchdog
interrupt is pending, and pass the configured timeout (not the hardcoded
default) to sama5d4_wdt_init().

Patch 2: Replace irq_of_parse_and_map() with platform_get_irq_optional()
for simpler error handling and EPROBE_DEFER support.

Patch 3: Move platform_set_drvdata() before devm_request_irq() to prevent
a NULL pointer dereference in the interrupt handler if the shared System
IRQ fires immediately after registration.

v2: address sashiko review

Rosen Penev (3):
  watchdog: sama5d4: fix shared IRQ and hardcoded timeout issues
  watchdog: sama5d4: use platform_get_irq_optional()
  watchdog: sama5d4: fix NULL deref in irq handler

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

--
2.54.0

[PATCHv2 1/3] 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.

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 | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
index 704b786cc2ec..f74f1e8956b5 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.
@@ -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

[PATCHv2 2/3] 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 f74f1e8956b5..2536e2f2ab32 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

[PATCHv2 3/3] watchdog: sama5d4: fix NULL deref in irq handler

[Rosen Penev]

Move platform_set_drvdata() before devm_request_irq() so that the
interrupt handler can safely dereference the driver data via
platform_get_drvdata().  An interrupt on the shared System IRQ line
(SAM9X60/SAM9X7) could fire between request_irq and the late
platform_set_drvdata() call, causing the handler to dereference NULL.

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

diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
index 2536e2f2ab32..7cd6f35c8e21 100644
--- a/drivers/watchdog/sama5d4_wdt.c
+++ b/drivers/watchdog/sama5d4_wdt.c
@@ -268,6 +268,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
 						: AT91_WDT_WDDIS;
 
 	watchdog_set_drvdata(wdd, wdt);
+	platform_set_drvdata(pdev, wdt);
 
 	regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(regs))
@@ -319,8 +320,6 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	platform_set_drvdata(pdev, wdt);
-
 	dev_info(dev, "initialized (timeout = %d sec, nowayout = %d)\n",
 		 wdd->timeout, nowayout);
 
-- 
2.54.0

Re: [PATCHv2 2/3] 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] The interrupt handler relies on platform drvdata but is registered before drvdata is set, which can lead to a NULL pointer dereference.
--

commit 6a0bceec2bdd17bb8ca5d9b9518a193041169703
Author: Rosen Penev <rosenp@gmail.com>

watchdog: sama5d4: use platform_get_irq_optional()

This commit replaces irq_of_parse_and_map() with platform_get_irq_optional()
to avoid needing irq_dispose_mapping() on failure. It also adds handling for
-EPROBE_DEFER.

> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 13e72918338a2..e12499547d735 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c

[ ... ]

> @@ -286,8 +285,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 interrupt is registered
with IRQF_SHARED shortly after this, could the handler fire before
platform_set_drvdata() is called?

In sama5d4_wdt_probe(), the interrupt is registered like this:

	ret = devm_request_irq(dev, irq, sama5d4_wdt_irq_handler,
			       IRQF_SHARED | IRQF_IRQPOLL |
			       IRQF_NO_SUSPEND, pdev->name, pdev);

Because it is shared, another device on the same IRQ line could trigger
an interrupt immediately. The handler then retrieves the wdt pointer
using platform_get_drvdata():

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

However, platform_set_drvdata() is only called at the very end of
sama5d4_wdt_probe(). Would this result in a NULL pointer dereference
if the interrupt triggers early? Should platform_set_drvdata() be
moved before the interrupt is requested?

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

Re: [PATCHv2 3/3] watchdog: sama5d4: fix NULL deref in irq handler

[sashiko-bot]

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

Pre-existing issues:
- [High] The shared interrupt handler `sama5d4_wdt_irq_handler` unconditionally returns `IRQ_HANDLED`, defeating the generic IRQ subsystem's spurious interrupt detection and potentially leading to a system lockup.
--

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

watchdog: sama5d4: fix NULL deref in irq handler

This commit moves platform_set_drvdata() before devm_request_irq() to ensure
the interrupt handler can safely dereference the driver data. This prevents a
possible NULL dereference if an interrupt on the shared System IRQ line fires
early.

> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 704b786cc2ec6..dcf5b8ac9e0d2 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -268,6 +268,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
>  						: AT91_WDT_WDDIS;
>  
>  	watchdog_set_drvdata(wdd, wdt);
> +	platform_set_drvdata(pdev, wdt);

[Severity: High]
This is a pre-existing issue, but does sama5d4_wdt_irq_handler() defeat the
generic IRQ subsystem's spurious interrupt detection by unconditionally
returning IRQ_HANDLED?

The driver requests a shared interrupt line using the IRQF_SHARED flag. If
the status register check indicates the watchdog did not generate the
interrupt, the handler bypasses the emergency restart logic but still
returns IRQ_HANDLED:

drivers/watchdog/sama5d4_wdt.c:sama5d4_wdt_irq_handler() {
	...
	if (reg) {
		pr_crit("Atmel Watchdog Software Reset\n");
		emergency_restart();
		pr_crit("Reboot didn't succeed\n");
	}

	return IRQ_HANDLED;
}

Should this return IRQ_NONE when it did not service the interrupt to avoid
falsely claiming unhandled interrupts?

>  
>  	regs = devm_platform_ioremap_resource(pdev, 0);

[ ... ]

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

Re: [PATCHv2 1/3] 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:

Pre-existing issues:
- [High] Race Condition / NULL Pointer Dereference in IRQ Handler
- [High] Broken hardware running detection silently disables SAM9X60 watchdog
- [Medium] Missing Implementation of Claimed Fix
--

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

watchdog: sama5d4: fix shared IRQ and hardcoded timeout issues

This commit aims to fix three issues in the sama5d4 watchdog driver: an
unsafe IRQF_SHARED combination, unconditional IRQ_HANDLED returns on shared
lines, and a hardcoded timeout. It modifies the IRQ handler to return
IRQ_NONE for spurious interrupts and passes the configured timeout to the
init function.

>     1. Unsafe IRQF_SHARED | IRQF_NO_SUSPEND combination: The watchdog
>        interrupt is a dedicated peripheral line, not shared with other
>        devices.

[Severity: Medium]
This is a pre-existing issue, but the commit message states this
combination is being removed, yet the patch does not appear to change the
devm_request_irq() flags in the driver.

Was the code change for this specific item accidentally omitted from the
patch?

> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 13e72918338a2..c7d6f877537c3 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c

[ ... ]

> @@ -311,7 +312,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
>  		set_bit(WDOG_HW_RUNNING, &wdd->status);
>  	}

[Severity: High]
This is a pre-existing issue, but does this hardware running check use the
correct bit for SAM9X60 devices?

In sama5d4_wdt_probe(), the code hardcodes AT91_WDT_WDDIS when reading the
status:

drivers/watchdog/sama5d4_wdt.c:sama5d4_wdt_probe() {
	...
	reg = wdt_read(wdt, AT91_WDT_MR);
	if (!(reg & AT91_WDT_WDDIS)) {
		wdt->mr &= ~AT91_WDT_WDDIS;
		set_bit(WDOG_HW_RUNNING, &wdd->status);
	}
	...
}

For SAM9X60, shouldn't this check AT91_SAM9X60_WDDIS instead?

Additionally, the wdt_enabled macro is hardcoded to check AT91_WDT_WDDIS
against wdt->mr. Since wdt->mr is initialized with AT91_SAM9X60_WDDIS for
SAM9X60 devices, the macro evaluates to true.

When wdt_enabled evaluates to true in sama5d4_wdt_init(), the block intended
to preserve the running state is skipped. The driver then writes wdt->mr
(which has the disable bit set) to the hardware. 

Does this silently disable the watchdog on SAM9X60 while leaving the core
believing it is running?

> -	ret = sama5d4_wdt_init(wdt);
> +	ret = sama5d4_wdt_init(wdt, wdd->timeout);
>  	if (ret)
>  		return ret;

[Severity: High]
This is also a pre-existing issue, but is there a race condition between
devm_request_irq() and platform_set_drvdata() in this probe function?

Earlier in sama5d4_wdt_probe(), devm_request_irq() is called with
IRQF_SHARED to register sama5d4_wdt_irq_handler():

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

If the line is shared and active, or if a spurious interrupt fires before
probe finishes, sama5d4_wdt_irq_handler() will be invoked:

drivers/watchdog/sama5d4_wdt.c:sama5d4_wdt_irq_handler() {
	struct sama5d4_wdt *wdt = platform_get_drvdata(dev_id);
	u32 reg;

	if (wdt->sam9x60_support)
	...
}

However, platform_set_drvdata(pdev, wdt) is not called until the very
end of sama5d4_wdt_probe(). If the interrupt handler fires before this,
platform_get_drvdata() will return NULL, causing an immediate NULL pointer
dereference when accessing wdt->sam9x60_support.

Could platform_set_drvdata() be moved earlier in the probe sequence,
or devm_request_irq() moved later?

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

Re: [PATCHv2 0/3] watchdog: sama5d4: fix IRQ and timeout bugs, use platform_get_irq_optional

[Nicolas Ferre]

On 08/06/2026 at 13:09, Rosen Penev wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> This series fixes three issues in the sama5d4 watchdog driver:
> 
> Patch 1: Return IRQ_NONE when the status register indicates no watchdog
> interrupt is pending, and pass the configured timeout (not the hardcoded
> default) to sama5d4_wdt_init().
> 
> Patch 2: Replace irq_of_parse_and_map() with platform_get_irq_optional()
> for simpler error handling and EPROBE_DEFER support.
> 
> Patch 3: Move platform_set_drvdata() before devm_request_irq() to prevent
> a NULL pointer dereference in the interrupt handler if the shared System
> IRQ fires immediately after registration.
> 
> v2: address sashiko review

Rosen,

I'd like to hear how these issues where found and how the patches were 
tested.

Best regards,
   Nicolas

> Rosen Penev (3):
>    watchdog: sama5d4: fix shared IRQ and hardcoded timeout issues
>    watchdog: sama5d4: use platform_get_irq_optional()
>    watchdog: sama5d4: fix NULL deref in irq handler
> 
>   drivers/watchdog/sama5d4_wdt.c | 32 +++++++++++++++++---------------
>   1 file changed, 17 insertions(+), 15 deletions(-)
> 
> --
> 2.54.0
> 

Re: [PATCHv2 0/3] watchdog: sama5d4: fix IRQ and timeout bugs, use platform_get_irq_optional

[Rosen Penev]

On Mon, Jun 8, 2026 at 1:38 PM Nicolas Ferre
<nicolas.ferre@microchip.com> wrote:
>
> On 08/06/2026 at 13:09, Rosen Penev wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > This series fixes three issues in the sama5d4 watchdog driver:
> >
> > Patch 1: Return IRQ_NONE when the status register indicates no watchdog
> > interrupt is pending, and pass the configured timeout (not the hardcoded
> > default) to sama5d4_wdt_init().
> >
> > Patch 2: Replace irq_of_parse_and_map() with platform_get_irq_optional()
> > for simpler error handling and EPROBE_DEFER support.
> >
> > Patch 3: Move platform_set_drvdata() before devm_request_irq() to prevent
> > a NULL pointer dereference in the interrupt handler if the shared System
> > IRQ fires immediately after registration.
> >
> > v2: address sashiko review
>
> Rosen,
>
> I'd like to hear how these issues where found and how the patches were
> tested.
They were not. Maintainer has explicitly asked me to fix issues
discovered by https://sashiko.dev

I originally only had patch 2, which is true of multiple places.
>
> Best regards,
>    Nicolas
>
> > Rosen Penev (3):
> >    watchdog: sama5d4: fix shared IRQ and hardcoded timeout issues
> >    watchdog: sama5d4: use platform_get_irq_optional()
> >    watchdog: sama5d4: fix NULL deref in irq handler
> >
> >   drivers/watchdog/sama5d4_wdt.c | 32 +++++++++++++++++---------------
> >   1 file changed, 17 insertions(+), 15 deletions(-)
> >
> > --
> > 2.54.0
> >
>

Re: [PATCHv2 1/3] watchdog: sama5d4: fix shared IRQ and hardcoded timeout issues

[Guenter Roeck]

On 6/8/26 13:09, Rosen Penev wrote:
> 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.
> 

Not addressed in this patch, and can not be addressed without testing
on real hardware. We'll have to leave that alone.

> 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.
> 
While I think this is a real problem, changing the code would require
testing on real hardware. We'll have to leave this alone.

Please resubmit, only fixing issue 2). Please list the other problems as
explicitly not fixed in the summary e-mail.

This also applies to the new problem regarding AT91_WDT_WDDIS vs.
AT91_SAM9X60_WDDIS. If that is a real problem, it can only be fixed
by someone with access to hardware and with chip knowledge.

Besides that, I would suggest to re-order the patches.
Patch 3 (the null pointer fix) should be first, followed by this patch.
Patch 2 should be last.

Thanks,
Guenter

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

[Guenter Roeck]

On 6/8/26 13:09, Rosen Penev wrote:
> 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.
> 

That is misleading. irq_of_parse_and_map() did not return errors,
so this is not an "also" but made necessary by the use of
platform_get_irq_optional(). That means that replacing the
call with platform_get_irq_optional() is not a 1:1 replacement,
making its use risky without extensive testing.

A much safer solution is to add a call to devm_add_action_or_reset()
after successful irq_of_parse_and_map() and to call irq_dispose_mapping()
from its callback function.

Thanks,
Guenter