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

On 7/22/26 09:46, [email protected] 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 <[email protected]>
> 
> 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