Re: [PATCH] watchdog: iTCO_wdt: Leave running if the watchdog core ping thread is enabled

[Mika Westerberg <mika.westerberg@linux.intel.com>]

On Fri, Sep 17, 2021 at 07:31:17AM -0700, Guenter Roeck wrote:
> On 9/17/21 3:15 AM, Mika Westerberg wrote:
> > The watchdog core can handle pinging of the watchdog before userspace
> > gets control so we can take advantage of that in iTCO_wdt. This also
> > allows users to disable the watchdog core ping thread by passing
> > watchdog.handle_boot_enabled=0 in the kernel command line if needed.
> > 
> > To avoid any unexpected resets we keep the existing functionality of
> > stopping the watchdog on probe if the watchdog core ping thread is not
> > enabled in the Kconfig (CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED=n).
> > 
> 
> CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED is enabled by default, and it should be
> enabled for all normal use cases, so this is a bit misleading.

OK.

> > Cc: Malin Jonsson <malin.jonsson@ericsson.com>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >   drivers/watchdog/iTCO_wdt.c | 42 ++++++++++++++++++++++++++++++-------
> >   1 file changed, 34 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> > index 643c6c2d0b72..234494c03df3 100644
> > --- a/drivers/watchdog/iTCO_wdt.c
> > +++ b/drivers/watchdog/iTCO_wdt.c
> > @@ -430,6 +430,27 @@ static unsigned int iTCO_wdt_get_timeleft(struct watchdog_device *wd_dev)
> >   	return time_left;
> >   }
> > +static bool iTCO_wdt_set_running(struct iTCO_wdt_private *p)
> > +{
> > +	/*
> > +	 * If the watchdog core is enabled to handle pinging the
> > +	 * watchdog before userspace takes control we can leave the
> > +	 * hardware as is.
> > +	 */
> > +	if (IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED)) {
> 
> This is neither necessary nor appropriate. Just set the flag. The core
> won't handle boot enabled if CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED=n
> even if WDOG_HW_RUNNING is set.
> 
> CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED is not a driver configuration option.
> It is a core option, and its description says:
> 
> " ... is to ping watchdog devices that were enabled before the driver has
>  been loaded until control is taken over from userspace using the
>  /dev/watchdog file."
> 
> This is not what is implemented here. Yes, there is a driver using
> the option, but that hardware does not support the ability to detect
> if the watchdog is running. That is not the case here.
> 
> If you want to have the ability to both keep the watchdog running or
> to stop it at boot, you'll need to add a module option.

Okay, I guess if CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED is set by default
there's minimal risk of breaking existing users if we simply set the
flag. I would rather not add new module parameters if possible.

Will do these changes in the next version.