[PATCH] watchdog: atcwdt200: fix return value when watchdog is enabled

[PATCH] watchdog: atcwdt200: fix return value when watchdog is enabled

[kr494167]

From: Surendra Singh Chouhan <kr494167@gmail.com>

atcwdt_get_int_timer_type() returned TMR_UNKNOWN (enum value 3) when
CTRL_WDT_EN was set in the control register.

Because atcwdt_probe() checks "ret = atcwdt_get_int_timer_type(drv_data);
if (ret) return ret;", returning TMR_UNKNOWN caused probe() to return 3
instead of a valid negative error code.

Fix this by setting drv_data->int_timer_type to TMR_UNKNOWN and returning
-EBUSY via dev_err_probe() when the watchdog is already enabled.

Fixes: e4e0848ad046 ("watchdog: atcwdt200: Add driver for Andes ATCWDT200")
Signed-off-by: Surendra Singh Chouhan <kr494167@gmail.com>
---
 drivers/watchdog/atcwdt200_wdt.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/atcwdt200_wdt.c b/drivers/watchdog/atcwdt200_wdt.c
index 8e3b18aea368..b91c748b3603 100644
--- a/drivers/watchdog/atcwdt200_wdt.c
+++ b/drivers/watchdog/atcwdt200_wdt.c
@@ -274,7 +274,9 @@ static int atcwdt_get_int_timer_type(struct atcwdt_drv *drv_data)
 	regmap_read(drv_data->regmap, REG_CTRL, &val);
 	if (val & CTRL_WDT_EN) {
 		spin_unlock(&drv_data->lock);
-		return TMR_UNKNOWN;
+		drv_data->int_timer_type = TMR_UNKNOWN;
+		return dev_err_probe(dev, -EBUSY,
+				     "Watchdog is enabled, cannot detect timer type\n");
 	}
 
 	/*
-- 
2.55.0

Re: [PATCH] watchdog: atcwdt200: fix return value when watchdog is enabled

[sashiko-bot]

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

New issues:
- [Low] Setting `drv_data->int_timer_type` is useless dead code because the function immediately returns a fatal error.
- [Low] Kernel-doc comment for `atcwdt_get_int_timer_type()` contradicts the new code.

Pre-existing issues:
- [High] Failing driver probe when the hardware watchdog is already running orphans the timer and guarantees an unhandled system reset.
- [High] The watchdog restart handler acquires a standard spinlock in atomic context, causing a lockdep splat and sleep-in-atomic bug on PREEMPT_RT.
--

--- Patch [1]: [PATCH] watchdog: atcwdt200: fix return value when watchdog is enabled ---
commit 8139a405984f14ed6f8cead4736a58db3d02c322
Author: Surendra Singh Chouhan <kr494167@gmail.com>

watchdog: atcwdt200: fix return value when watchdog is enabled

This commit modifies atcwdt_get_int_timer_type() to return -EBUSY via
dev_err_probe() when the watchdog is already enabled, fixing an issue
where probe() incorrectly returned a positive value on failure.

> diff --git a/drivers/watchdog/atcwdt200_wdt.c b/drivers/watchdog/atcwdt200_wdt.c
> --- a/drivers/watchdog/atcwdt200_wdt.c
> +++ b/drivers/watchdog/atcwdt200_wdt.c
> @@ -274,7 +274,9 @@ static int atcwdt_get_int_timer_type(struct atcwdt_drv *drv_data)
>  	regmap_read(drv_data->regmap, REG_CTRL, &val);
>  	if (val & CTRL_WDT_EN) {
>  		spin_unlock(&drv_data->lock);
> -		return TMR_UNKNOWN;
> +		drv_data->int_timer_type = TMR_UNKNOWN;

[Severity: Low]
Is there a need to set drv_data->int_timer_type here? Since the function
immediately returns a fatal error, the driver probe will fail and free the
structure. This appears to be dead code.

> +		return dev_err_probe(dev, -EBUSY,
> +				     "Watchdog is enabled, cannot detect timer type\n");

[Severity: High]
This is a pre-existing issue, but does failing the driver probe when the
hardware watchdog is running orphan the timer?

If a bootloader leaves the watchdog enabled, refusing to bind the driver here
guarantees an unhandled system reset. Should the driver adopt the running
watchdog instead and set WDOG_HW_RUNNING?

>  	}
>  
>  	/*

[Severity: Low]
The kernel-doc comment for atcwdt_get_int_timer_type() currently states:

 * Note: This function must only be called when the ATCWDT200 watchdog is
 * disabled. If the watchdog is enabled, this function returns TMR_UNKNOWN.

Since this patch changes the return value to -EBUSY, should the documentation
be updated to match the new behavior?

[Severity: High]
This is a pre-existing issue, but can the lock used in atcwdt_restart() cause
a sleep-in-atomic bug on PREEMPT_RT kernels?

The watchdog restart handler acquires a standard spinlock:

drivers/watchdog/atcwdt200_wdt.c:atcwdt_restart() {
    ...
    spin_lock(&drv_data->lock);
    ...
}

Since restart handlers are invoked in atomic context with local interrupts
and preemption disabled, acquiring a standard spinlock_t blocks and can sleep
on PREEMPT_RT. Should this lock be converted to a raw_spinlock_t?

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

Re: [PATCH] watchdog: atcwdt200: fix return value when watchdog is enabled

[Guenter Roeck]

On 7/22/26 09:46, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> 
> New issues:
> - [Low] Setting `drv_data->int_timer_type` is useless dead code because the function immediately returns a fatal error.
> - [Low] Kernel-doc comment for `atcwdt_get_int_timer_type()` contradicts the new code.
> 
> Pre-existing issues:
> - [High] Failing driver probe when the hardware watchdog is already running orphans the timer and guarantees an unhandled system reset.
> - [High] The watchdog restart handler acquires a standard spinlock in atomic context, causing a lockdep splat and sleep-in-atomic bug on PREEMPT_RT.
> --
> 
> --- Patch [1]: [PATCH] watchdog: atcwdt200: fix return value when watchdog is enabled ---
> commit 8139a405984f14ed6f8cead4736a58db3d02c322
> Author: Surendra Singh Chouhan <kr494167@gmail.com>
> 
> watchdog: atcwdt200: fix return value when watchdog is enabled
> 
> This commit modifies atcwdt_get_int_timer_type() to return -EBUSY via
> dev_err_probe() when the watchdog is already enabled, fixing an issue
> where probe() incorrectly returned a positive value on failure.
> 
>> diff --git a/drivers/watchdog/atcwdt200_wdt.c b/drivers/watchdog/atcwdt200_wdt.c
>> --- a/drivers/watchdog/atcwdt200_wdt.c
>> +++ b/drivers/watchdog/atcwdt200_wdt.c
>> @@ -274,7 +274,9 @@ static int atcwdt_get_int_timer_type(struct atcwdt_drv *drv_data)
>>   	regmap_read(drv_data->regmap, REG_CTRL, &val);
>>   	if (val & CTRL_WDT_EN) {
>>   		spin_unlock(&drv_data->lock);
>> -		return TMR_UNKNOWN;
>> +		drv_data->int_timer_type = TMR_UNKNOWN;
> 
> [Severity: Low]
> Is there a need to set drv_data->int_timer_type here? Since the function
> immediately returns a fatal error, the driver probe will fail and free the
> structure. This appears to be dead code.
> 

It is. Code further down does the same, but that is not excuse for doing the same.
Actually, TMR_UNKNOWN itself is pointless since the driver does not really
use it, but that is yet another pre-existing issue.

>> +		return dev_err_probe(dev, -EBUSY,
>> +				     "Watchdog is enabled, cannot detect timer type\n");
> 
> [Severity: High]
> This is a pre-existing issue, but does failing the driver probe when the
> hardware watchdog is running orphan the timer?
> 
> If a bootloader leaves the watchdog enabled, refusing to bind the driver here
> guarantees an unhandled system reset. Should the driver adopt the running
> watchdog instead and set WDOG_HW_RUNNING?
> 

Yes, that is correct, but it appears that there is no (known) means to
determine the timer type if the watchdog is running, meaning such a system
is actually not currently supported.

Maybe the driver could stop the watchdog if it is running, retrieve the
timer type, and restart the watchdog. However, that would have to be a
separate patch, and it is questionable if such a system actually exists.
So it is ok to ignore that problem until someone actually observes it.

>>   	}
>>   
>>   	/*
> 
> [Severity: Low]
> The kernel-doc comment for atcwdt_get_int_timer_type() currently states:
> 
>   * Note: This function must only be called when the ATCWDT200 watchdog is
>   * disabled. If the watchdog is enabled, this function returns TMR_UNKNOWN.
> 
> Since this patch changes the return value to -EBUSY, should the documentation
> be updated to match the new behavior?
> 
Yes, definitely.

> [Severity: High]
> This is a pre-existing issue, but can the lock used in atcwdt_restart() cause
> a sleep-in-atomic bug on PREEMPT_RT kernels?
> 
> The watchdog restart handler acquires a standard spinlock:
> 
> drivers/watchdog/atcwdt200_wdt.c:atcwdt_restart() {
>      ...
>      spin_lock(&drv_data->lock);
>      ...
> }
> 
> Since restart handlers are invoked in atomic context with local interrupts
> and preemption disabled, acquiring a standard spinlock_t blocks and can sleep
> on PREEMPT_RT. Should this lock be converted to a raw_spinlock_t?
> 
That would have to be another separate patch.

Thanks,
Guenter