Re: [RESEND PATCH] watchdog: sbsa_gwdt: add force_enable module parameter

On 8/3/26 00:18, Zexin Wang wrote:
> The sbsa_gwdt driver only manages and registers the watchdog timer if it
> was already enabled by a previous boot stage (such as firmware). If the
> watchdog was left disabled prior to entering the kernel, the driver
> will not actively enable it during the probe phase.
> 
> Introduce a new module parameter 'force_enable' to allow overriding this
> behavior. When 'force_enable' is set, the driver will forcibly start
> the watchdog and register it as running during the probe phase, even if
> it was disabled in the previous boot stage.
> 

That describes what is done. It does not describe why it is needed. If the
watchdog is left disabled by firmware, there is nothing that can be done
if the system hangs before the watchdog module is loaded. Enabling it when
it is loaded will in most cases not buy much additional protection, because
the userspace watchdog daemon is loaded shortly thereafter. In many cases,
the userspace daemon will be the entity loading the watchdog driver.

So, _why_ is this needed ? Why can;t the firmware enable the watchdog on
systems needing such protection ?

> Signed-off-by: Zexin Wang <[email protected]>
> ---
>   drivers/watchdog/sbsa_gwdt.c | 23 +++++++++++++++++++++--
>   1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
> index e04d42cc7774..4b49e1aeef84 100644
> --- a/drivers/watchdog/sbsa_gwdt.c
> +++ b/drivers/watchdog/sbsa_gwdt.c
> @@ -122,6 +122,11 @@ MODULE_PARM_DESC(nowayout,
>   		 "Watchdog cannot be stopped once started (default="
>   		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>   
> +static bool force_enable;
> +module_param(force_enable, bool, 0);
> +MODULE_PARM_DESC(force_enable,
> +		 "Force enable watchdog on probe (default=0)");
> +

This enables watchdog while booting. There is nothing "force" about it,
unless everything the kernel is directed to do is done by force, and
everything needs to be named "force <something>".

Please drop "force". Name the parameter "early_enable" and describe it
similar to other drivers (such as omap, s32g) with the same functionality.

Thanks,
Guenter

>   /*
>    * Arm Base System Architecture 1.0 introduces watchdog v1 which
>    * increases the length watchdog offset register to 48 bits.
> @@ -296,6 +301,7 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
>   	struct sbsa_gwdt *gwdt;
>   	int ret, irq;
>   	u32 status;
> +	bool force_action;
>   
>   	gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL);
>   	if (!gwdt)
> @@ -386,14 +392,27 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
>   	 */
>   	sbsa_gwdt_set_timeout(wdd, wdd->timeout);
>   
> +	force_action = force_enable && !(status & SBSA_GWDT_WCS_EN);
> +	if (force_action) {
> +		sbsa_gwdt_start(wdd);
> +		set_bit(WDOG_HW_RUNNING, &wdd->status);
> +		dev_info(dev, "Watchdog force enabled.\n");
> +	}
> +
>   	watchdog_stop_on_reboot(wdd);
>   	ret = devm_watchdog_register_device(dev, wdd);
> -	if (ret)
> +	if (ret) {
> +		dev_err(dev, "Failed to register watchdog: %d\n", ret);
> +		if (force_action) {
> +			clear_bit(WDOG_HW_RUNNING, &wdd->status);
> +			sbsa_gwdt_stop(wdd);
> +		}
>   		return ret;
> +	}
>   
>   	dev_info(dev, "Initialized with %ds timeout @ %u Hz, action=%d.%s\n",
>   		 wdd->timeout, gwdt->clk, action,
> -		 status & SBSA_GWDT_WCS_EN ? " [enabled]" : "");
> +		 watchdog_hw_running(wdd) ? " [enabled]" : "");
>   
>   	return 0;
>   }