Re: [PATCH 2/9] watchdog: cadence_wdt: Remove redundant dev_err()

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

Pre-existing issues:
- [High] Incorrect handling of platform_get_irq() return value leads to silently broken watchdogs, ignored probe deferrals, and spurious boot errors.
- [High] The driver advertises a hardcoded max_timeout of 516 seconds, which causes silent truncation and premature system reboots on platforms with slower clocks.
- [High] cdns_wdt_settimeout() unconditionally enables the hardware watchdog, which can cause spurious system resets.
--

--- Patch [2]: [PATCH 2/9] watchdog: cadence_wdt: Remove redundant dev_err() ---
commit 8e2910f9fc6c98ef77eadf7acc598ee3e8c4f9f7
Author: Pan Chuang <[email protected]>

watchdog: cadence_wdt: Remove redundant dev_err()

This commit removes a redundant dev_err() call after devm_request_irq() 
because devm_request_irq() now automatically logs detailed error messages 
on failure.

> diff --git a/drivers/watchdog/cadence_wdt.c b/drivers/watchdog/cadence_wdt.c
> index 23d41043863f6..ad46b33ce5f2e 100644
> --- a/drivers/watchdog/cadence_wdt.c
> +++ b/drivers/watchdog/cadence_wdt.c
> @@ -312,12 +312,8 @@ static int cdns_wdt_probe(struct platform_device *pdev)

[Severity: High]
This is a pre-existing issue, but is the driver advertising a maximum timeout
that exceeds hardware capabilities?

In cdns_wdt_probe(), the max_timeout is hardcoded:

	cdns_wdt_device->max_timeout = CDNS_WDT_MAX_TIMEOUT;

If this is 516 seconds, but the hardware uses a slower clock (e.g., 75MHz
with a 512 prescaler), a requested timeout of 516 seconds calculates to
18454 in cdns_wdt_start(), which is silently clamped:

	count = (wdd->timeout * (clock_f / wdt->prescaler)) /
		 CDNS_WDT_COUNTER_VALUE_DIVISOR + 1;

	if (count > CDNS_WDT_COUNTER_MAX)
		count = CDNS_WDT_COUNTER_MAX;

Since the hardware maximum is only 4095 (12-bit), could this result in an
actual timeout of ~114.5 seconds, causing premature system reboots before
the watchdog daemon expects to ping it?

>  	if (!wdt->rst && irq >= 0) {

[Severity: High]
This is a pre-existing issue, but does this correctly handle cases where the
IRQ is deferred or omitted?

Looking at cdns_wdt_probe(), platform_get_irq() is called unconditionally
before this block:

	wdt->rst = of_property_read_bool(dev->of_node, "reset-on-timeout");
	irq = platform_get_irq(pdev, 0);

If reset-on-timeout is specified, the IRQ is unneeded, but an error is still
logged to dmesg. If reset-on-timeout is false and platform_get_irq() returns
-EPROBE_DEFER, does this silently ignore the error since irq >= 0 fails,
completing probe without an IRQ and leaving the watchdog broken?

>  		ret = devm_request_irq(dev, irq, cdns_wdt_irq_handler, 0,
>  				       pdev->name, pdev);
> -		if (ret) {
> -			dev_err(dev,
> -				"cannot register interrupt handler err=%d\n",
> -				ret);
> +		if (ret)
>  			return ret;
> -		}
>  	}
>  
>  	/* Initialize the members of cdns_wdt structure */

[Severity: High]
This is a pre-existing issue, but could configuring the timeout unexpectedly
start the hardware watchdog?

When the timeout is configured via WDIOC_SETTIMEOUT, the core calls
cdns_wdt_settimeout(), which directly calls cdns_wdt_start():

static int cdns_wdt_settimeout(struct watchdog_device *wdd,
			       unsigned int new_time)
{
	wdd->timeout = new_time;

	return cdns_wdt_start(wdd);
}

Inside cdns_wdt_start(), the timer is fully enabled:

	data = CDNS_WDT_ZMR_WDEN_MASK | CDNS_WDT_ZMR_RSTLEN_16 |
	       CDNS_WDT_ZMR_ZKEY_VAL;

If userspace had previously stopped the watchdog, won't this start the
hardware timer without the watchdog core knowing it is active? Could this
lead to spurious system resets since the core won't send keepalive pings for
a stopped device?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2