Re: [PATCH v2] watchdog: sbsa_gwdt: add early_enable module parameter

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

New issues:
- [Low] The newly added module parameter `early_enable` is incorrectly indented in the documentation, which will cause it to render as part of the description for the `nowayout` parameter.

Pre-existing issues:
- [High] If the `action=1` (two-stage mode) parameter is requested but the WS0 interrupt is missing, the driver falls back to single-stage mode but fails to recalculate the WS0 race workaround, leaving the device vulnerable to race-induced panics/resets.
--

--- Patch [1]: [PATCH v2] watchdog: sbsa_gwdt: add early_enable module parameter ---
commit 53d84fbbff2306e50b5e3a377d06b179b96d99f7
Author: Zexin Wang <[email protected]>

watchdog: sbsa_gwdt: add early_enable module parameter

This commit adds an early_enable module parameter to the sbsa_gwdt driver.
It allows the watchdog to be re-enabled immediately during probe to prevent
a protection gap between firmware handover and userspace daemon startup.

> diff --git a/Documentation/watchdog/watchdog-parameters.rst b/Documentation/watchdog/watchdog-parameters.rst
> --- a/Documentation/watchdog/watchdog-parameters.rst
> +++ b/Documentation/watchdog/watchdog-parameters.rst
> @@ -515,6 +515,8 @@ sbsa_gwdt:
>      nowayout:
>  	Watchdog cannot be stopped once started
>  	(default=kernel config parameter)
> +	early_enable:
> +	Watchdog is started on module insertion (default=0)

[Severity: Low]
This isn't a bug, but will this indentation cause early_enable to render
nested under the nowayout parameter description in the generated Sphinx
documentation? It looks like it uses a tab instead of being aligned with
the other parameter names.

> diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
> --- a/drivers/watchdog/sbsa_gwdt.c
> +++ b/drivers/watchdog/sbsa_gwdt.c
[ ... ]
> @@ -386,14 +392,27 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
>  	 */
>  	sbsa_gwdt_set_timeout(wdd, wdd->timeout);

[Severity: High]
This is a pre-existing issue, but does the driver leave the device vulnerable
to race-induced panics if it falls back to single-stage mode?

Looking at sbsa_gwdt_probe(), the workaround condition for 
need_ws0_race_workaround is evaluated early based on the initial action state
during sbsa_gwdt_get_version(wdd). If the WS0 interrupt is missing, the driver
falls back by setting action to 0:

sbsa_gwdt_probe() {
    ...
    if (action) {
        irq = platform_get_irq(pdev, 0);
        if (irq < 0) {
            action = 0;
            dev_warn(dev, "unable to get ws0 interrupt.\n");
        }
    ...
}

However, it doesn't recalculate need_ws0_race_workaround after this fallback.
If a system boots with action=1 but fails to find the IRQ, could the
watchdog core schedule keepalives that align with the hardware's WS0
assertion, triggering the known race condition where the ping is ignored?

> +	early_action = early_enable && !(status & SBSA_GWDT_WCS_EN);
> +	if (early_action) {
> +		sbsa_gwdt_start(wdd);
> +		set_bit(WDOG_HW_RUNNING, &wdd->status);
> +		dev_info(dev, "Watchdog early enabled.\n");
> +	}
> +
>  	watchdog_stop_on_reboot(wdd);

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