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@xxxxxxxxxxxx>
> > Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> > ---
> > 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.
