linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] watchdog: iTCO_wdt: Leave running if the watchdog core ping thread is enabled
@ 2021-09-17 10:15 Mika Westerberg
  2021-09-17 14:31 ` Guenter Roeck
  0 siblings, 1 reply; 3+ messages in thread
From: Mika Westerberg @ 2021-09-17 10:15 UTC (permalink / raw)
  To: Wim Van Sebroeck, Guenter Roeck
  Cc: Malin Jonsson, john.jacques, linux-watchdog, Mika Westerberg

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).

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)) {
+		u16 val;
+
+		/* Bit 11: TCO Timer Halt -> 0 = The TCO timer is * enabled */
+		val = inw(TCO1_CNT(p));
+		if (!(val & BIT(11)))
+			set_bit(WDOG_HW_RUNNING, &p->wddev.status);
+
+		return true;
+	}
+
+	return false;
+}
+
 /*
  *	Kernel Interfaces
  */
@@ -572,15 +593,20 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 	watchdog_set_drvdata(&p->wddev, p);
 	platform_set_drvdata(pdev, p);
 
-	/* Make sure the watchdog is not running */
-	iTCO_wdt_stop(&p->wddev);
+	if (!iTCO_wdt_set_running(p)) {
+		/* Make sure the watchdog is not running */
+		iTCO_wdt_stop(&p->wddev);
 
-	/* Check that the heartbeat value is within it's range;
-	   if not reset to the default */
-	if (iTCO_wdt_set_timeout(&p->wddev, heartbeat)) {
-		iTCO_wdt_set_timeout(&p->wddev, WATCHDOG_TIMEOUT);
-		dev_info(dev, "timeout value out of range, using %d\n",
-			WATCHDOG_TIMEOUT);
+		/*
+		 * Check that the heartbeat value is within it's range;
+		 * if not reset to the default.
+		 */
+		if (iTCO_wdt_set_timeout(&p->wddev, heartbeat)) {
+			iTCO_wdt_set_timeout(&p->wddev, WATCHDOG_TIMEOUT);
+			dev_info(p->wddev.parent,
+				 "timeout value out of range, using %d\n",
+				 WATCHDOG_TIMEOUT);
+		}
 	}
 
 	watchdog_stop_on_reboot(&p->wddev);
-- 
2.33.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] watchdog: iTCO_wdt: Leave running if the watchdog core ping thread is enabled
  2021-09-17 10:15 [PATCH] watchdog: iTCO_wdt: Leave running if the watchdog core ping thread is enabled Mika Westerberg
@ 2021-09-17 14:31 ` Guenter Roeck
  2021-09-17 14:55   ` Mika Westerberg
  0 siblings, 1 reply; 3+ messages in thread
From: Guenter Roeck @ 2021-09-17 14:31 UTC (permalink / raw)
  To: Mika Westerberg, Wim Van Sebroeck
  Cc: Malin Jonsson, john.jacques, linux-watchdog

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.

> 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.

Guenter

> +		u16 val;
> +
> +		/* Bit 11: TCO Timer Halt -> 0 = The TCO timer is * enabled */
> +		val = inw(TCO1_CNT(p));
> +		if (!(val & BIT(11)))
> +			set_bit(WDOG_HW_RUNNING, &p->wddev.status);
> +
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
>   /*
>    *	Kernel Interfaces
>    */
> @@ -572,15 +593,20 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
>   	watchdog_set_drvdata(&p->wddev, p);
>   	platform_set_drvdata(pdev, p);
>   
> -	/* Make sure the watchdog is not running */
> -	iTCO_wdt_stop(&p->wddev);
> +	if (!iTCO_wdt_set_running(p)) {
> +		/* Make sure the watchdog is not running */
> +		iTCO_wdt_stop(&p->wddev);
>   
> -	/* Check that the heartbeat value is within it's range;
> -	   if not reset to the default */
> -	if (iTCO_wdt_set_timeout(&p->wddev, heartbeat)) {
> -		iTCO_wdt_set_timeout(&p->wddev, WATCHDOG_TIMEOUT);
> -		dev_info(dev, "timeout value out of range, using %d\n",
> -			WATCHDOG_TIMEOUT);
> +		/*
> +		 * Check that the heartbeat value is within it's range;
> +		 * if not reset to the default.
> +		 */
> +		if (iTCO_wdt_set_timeout(&p->wddev, heartbeat)) {
> +			iTCO_wdt_set_timeout(&p->wddev, WATCHDOG_TIMEOUT);
> +			dev_info(p->wddev.parent,
> +				 "timeout value out of range, using %d\n",
> +				 WATCHDOG_TIMEOUT);
> +		}
>   	}
>   
>   	watchdog_stop_on_reboot(&p->wddev);
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] watchdog: iTCO_wdt: Leave running if the watchdog core ping thread is enabled
  2021-09-17 14:31 ` Guenter Roeck
@ 2021-09-17 14:55   ` Mika Westerberg
  0 siblings, 0 replies; 3+ messages in thread
From: Mika Westerberg @ 2021-09-17 14:55 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Wim Van Sebroeck, Malin Jonsson, john.jacques, linux-watchdog

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.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-09-17 14:55 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-17 10:15 [PATCH] watchdog: iTCO_wdt: Leave running if the watchdog core ping thread is enabled Mika Westerberg
2021-09-17 14:31 ` Guenter Roeck
2021-09-17 14:55   ` Mika Westerberg

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).