Re: [PATCH 3/9] watchdog: intel-mid_wdt: Remove redundant dev_err()

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

Pre-existing issues:
- [High] The driver advertises the WDIOF_SETTIMEOUT capability but lacks the required .set_timeout callback, causing timeout updates to silently fail.
- [High] The shared IRQ handler mid_wdt_irq unconditionally panics the system, violating shared IRQ semantics and risking spurious panics.
--

--- Patch [3]: [PATCH 3/9] watchdog: intel-mid_wdt: Remove redundant dev_err() ---
commit f0c7169449cddf6ac1b4686957e0ef5aa4340ca5
Author: Pan Chuang <[email protected]>

watchdog: intel-mid_wdt: Remove redundant dev_err()

This commit removes a redundant dev_err() call after devm_request_irq().
The core function now automatically logs detailed error messages on failure,
making the driver-specific logging unnecessary.

> diff --git a/drivers/watchdog/intel-mid_wdt.c b/drivers/watchdog/intel-mid_wdt.c
> index 756d262dc5808..b1e1b77ed1905 100644
> --- a/drivers/watchdog/intel-mid_wdt.c
> +++ b/drivers/watchdog/intel-mid_wdt.c
> @@ -165,10 +165,8 @@ static int mid_wdt_probe(struct platform_device *pdev)
>  	ret = devm_request_irq(dev, pdata->irq, mid_wdt_irq,
>  			       IRQF_SHARED | IRQF_NO_SUSPEND, "watchdog",
>  			       wdt_dev);

[Severity: High]
This is a pre-existing issue, but does this interrupt handler violate
shared IRQ semantics?

The interrupt is requested here with IRQF_SHARED, which explicitly allows
other devices to share the line.

However, looking at the handler:

drivers/watchdog/intel-mid_wdt.c:mid_wdt_irq()
{
	panic("Kernel Watchdog");

	/* This code should not be reached */
	return IRQ_HANDLED;
}

Since it unconditionally calls panic() without verifying that the watchdog
device actually triggered the interrupt, couldn't any other device sharing
this IRQ line cause a spurious kernel panic?

[Severity: High]
This is also a pre-existing issue, but does the driver support updating
the timeout properly?

The driver advertises the ability to change the timeout to userspace:

drivers/watchdog/intel-mid_wdt.c:mid_wdt_info
	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,

But the operations struct is missing the .set_timeout callback:

drivers/watchdog/intel-mid_wdt.c:mid_wdt_ops
static const struct watchdog_ops mid_wdt_ops = {
	.owner = THIS_MODULE,
	.start = wdt_start,
	.stop = wdt_stop,
	.ping = wdt_ping,
};

When a userspace daemon attempts to change the timeout, the core watchdog
subsystem will call the .ping callback instead of programming the hardware.

Does this result in the hardware continuing to use the default timeout while
userspace believes a longer timeout has been set, potentially causing
unexpected watchdog resets?

> -	if (ret) {
> -		dev_err(dev, "error requesting warning irq %d\n", pdata->irq);
> +	if (ret)
>  		return ret;
> -	}
>  
>  	/*
>  	 * The firmware followed by U-Boot leaves the watchdog running

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