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

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.

> +		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/[email protected]?part=1