Re: [PATCH 5/9] watchdog: marvell_gti: Remove redundant dev_err_probe()

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

Pre-existing issues:
- [High] Double disable and unprepare of the clock due to a redundant devres action.
- [Medium] The watchdog device is exposed to userspace before its IRQ is fully requested, creating a race condition.
- [Medium] Pretimeout value is not updated when the pretimeout interrupt is disabled, leaving a stale state.
--

--- Patch [5]: [PATCH 5/9] watchdog: marvell_gti: Remove redundant dev_err_probe() ---
commit 2a9a85c950abb5fda8867255ce0f94190626fc24
Author: Pan Chuang <[email protected]>

watchdog: marvell_gti: Remove redundant dev_err_probe()
    
Since commit 55b48e23f5c4 ("genirq/devres: Add error handling in
devm_request_*_irq()"), devm_request_irq() automatically logs detailed error
messages on failure. Remove the now-redundant driver-specific dev_err_probe()
call.

> diff --git a/drivers/watchdog/marvell_gti_wdt.c b/drivers/watchdog/marvell_gti_wdt.c
> index 298089d45ab89..31167034963c1 100644
> --- a/drivers/watchdog/marvell_gti_wdt.c
> +++ b/drivers/watchdog/marvell_gti_wdt.c
> @@ -321,7 +321,7 @@ static int gti_wdt_probe(struct platform_device *pdev)
>  	err = devm_request_irq(dev, irq, gti_wdt_interrupt, 0,
>  			       pdev->name, &priv->wdev);

[Severity: Medium]
This is a pre-existing issue, but is there a race condition here if the device
is exposed to userspace before its IRQ is fully requested?

In drivers/watchdog/marvell_gti_wdt.c:gti_wdt_probe(), the watchdog device is
registered before requesting the interrupt:

	err = devm_watchdog_register_device(dev, wdog_dev);
	if (err)
		return err;

	irq = platform_get_irq(pdev, 0);
	if (irq < 0)
		return irq;

	err = devm_request_irq(dev, irq, gti_wdt_interrupt, 0,
			       pdev->name, &priv->wdev);

If userspace opens /dev/watchdog and starts the watchdog immediately after
devm_watchdog_register_device() completes, but before devm_request_irq()
registers the handler, could this lead to unhandled hardware interrupts or
spurious interrupt warnings?

Should device registration be the final step in probe() to guarantee all
hardware resources are fully prepared before userspace can interact with them?

>  	if (err)
> -		return dev_err_probe(dev, err, "Failed to register interrupt handler\n");
> +		return err;
>  
>  	dev_info(dev, "Watchdog enabled (timeout=%d sec)\n", wdog_dev->timeout);
>  	return 0;

[Severity: High]
This is a pre-existing issue, but does this code cause a double disable and
unprepare of the clock?

Looking at drivers/watchdog/marvell_gti_wdt.c:gti_wdt_get_cntfrq():

	priv->sclk = devm_clk_get_enabled(&pdev->dev, NULL);
	if (IS_ERR(priv->sclk))
		return PTR_ERR(priv->sclk);

	err = devm_add_action_or_reset(&pdev->dev,
				       gti_clk_disable_unprepare, priv->sclk);

Since devm_clk_get_enabled() automatically adds a devres action to disable
and unprepare the clock on teardown, does registering a redundant devres action
(gti_clk_disable_unprepare) for the same clock cause clk_disable_unprepare()
to be called twice on a single reference during driver unbind or probe error
paths?

[Severity: Medium]
This is a pre-existing issue, but does the pretimeout value remain stale when
the pretimeout interrupt is disabled?

Looking at drivers/watchdog/marvell_gti_wdt.c:gti_wdt_set_pretimeout():

	if (!timeout) {
		/* Disable Interrupt */
		writeq(GTI_CWD_INT_ENA_CLR_VAL(priv->wdt_timer_idx),
		       priv->base + GTI_CWD_INT_ENA_CLR);
		return 0;
	}

When timeout is 0, the code disables the pretimeout in hardware and returns 0.

However, because the driver implements the set_pretimeout callback, does the
watchdog core framework delegate software state tracking to it?

Since the driver fails to update wdev->pretimeout = 0, would userspace queries
via WDIOC_GETPRETIMEOUT return the old, non-zero value, falsely indicating
pretimeout is still active?

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