[PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Pratyush Anand]

Currently only WOR is used to program both first and second stage which
provided very limited range of timeout.

This patch uses WCV as well to achieve higher range of timeout. This patch
programs max_timeout as 255, but that can be increased further as well.

Following testing shows that we can happily achieve 40 second default timeout.

 # modprobe sbsa_gwdt action=1
 [  131.187562] sbsa-gwdt sbsa-gwdt.0: Initialized with 40s timeout @ 250000000 Hz, action=1.
 # cd /sys/class/watchdog/watchdog0/
 # cat state
 inactive
 # cat /dev/watchdog0
 cat: /dev/watchdog0: Invalid argument
 [  161.710593] watchdog: watchdog0: watchdog did not stop!
 # cat state
 active
 # cat timeout
 40
 # cat timeleft
 38
 # cat timeleft
 25
 # cat /dev/watchdog0
 cat: /dev/watchdog0: Invalid argument
 [  184.931030] watchdog: watchdog0: watchdog did not stop!
 # cat timeleft
 37
 # cat timeleft
 21
 ...
 ...
 # cat timeleft
 1

panic() is called upon timeout of 40s. See timestamp of last kick (cat) and
next panic() message.

 [  224.939065] Kernel panic - not syncing: SBSA Watchdog timeout

Signed-off-by: Pratyush Anand <panand@redhat.com>
---
 drivers/watchdog/sbsa_gwdt.c | 83 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 62 insertions(+), 21 deletions(-)

diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
index ad383f6f15fc..529dd5e99fcd 100644
--- a/drivers/watchdog/sbsa_gwdt.c
+++ b/drivers/watchdog/sbsa_gwdt.c
@@ -35,17 +35,23 @@
  *
  * SBSA GWDT:
  * if action is 1 (the two stages mode):
- * |--------WOR-------WS0--------WOR-------WS1
+ * |--------WCV-------WS0--------WCV-------WS1
  * |----timeout-----(panic)----timeout-----reset
  *
  * if action is 0 (the single stage mode):
- * |------WOR-----WS0(ignored)-----WOR------WS1
+ * |------WCV-----WS0(ignored)-----WOR------WS1
  * |--------------timeout-------------------reset
  *
- * Note: Since this watchdog timer has two stages, and each stage is determined
- * by WOR, in the single stage mode, the timeout is (WOR * 2); in the two
- * stages mode, the timeout is WOR. The maximum timeout in the two stages mode
- * is half of that in the single stage mode.
+ * Note: This watchdog timer has two stages. If action is 0, first stage is
+ * determined by directly programming WCV and second by WOR. When first
+ * timeout is reached, WS0 is triggered and WCV is reloaded with value in
+ * WOR. WS0 interrupt will be ignored, then the second watch period starts;
+ * when second timeout is reached, then WS1 is triggered, system resets. WCV
+ * and WOR are programmed in such a way that total time corresponding to
+ * WCV+WOR becomes equivalent to user programmed "timeout".
+ * If action is 1, then we expect to call panic() at user programmed
+ * "timeout". Therefore, we program both first and second stage using WCV
+ * only.
  *
  */
 
@@ -95,7 +101,17 @@ struct sbsa_gwdt {
 	void __iomem		*control_base;
 };
 
-#define DEFAULT_TIMEOUT		10 /* seconds */
+/*
+ * Max Timeout Can be in days, but 255 seconds seems reasonable for all use
+ * cases
+ */
+#define MAX_TIMEOUT		255
+
+/* Default timeout is 40 seconds, which is the 1st + 2nd watch periods when
+ * action is 0. When action is 1 then both 1st and 2nd watch periods will
+ * be of 40 seconds.
+ */
+#define DEFAULT_TIMEOUT		40 /* seconds */
 
 static unsigned int timeout;
 module_param(timeout, uint, 0);
@@ -127,20 +143,21 @@ static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd,
 				 unsigned int timeout)
 {
 	struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
+	u64 timeout_1, timeout_2;
 
 	wdd->timeout = timeout;
 
 	if (action)
-		writel(gwdt->clk * timeout,
-		       gwdt->control_base + SBSA_GWDT_WOR);
+		timeout_1 = (u64)gwdt->clk * timeout;
 	else
-		/*
-		 * In the single stage mode, The first signal (WS0) is ignored,
-		 * the timeout is (WOR * 2), so the WOR should be configured
-		 * to half value of timeout.
-		 */
-		writel(gwdt->clk / 2 * timeout,
-		       gwdt->control_base + SBSA_GWDT_WOR);
+		timeout_1 = (u64)gwdt->clk * (timeout - wdd->min_timeout);
+
+	/* when action=1, timeout_2 will be overwritten in ISR */
+	timeout_2 = (u64)gwdt->clk * wdd->min_timeout;
+
+	writel(timeout_2, gwdt->control_base + SBSA_GWDT_WOR);
+	writeq(timeout_1 + arch_counter_get_cntvct(),
+		gwdt->control_base + SBSA_GWDT_WCV);
 
 	return 0;
 }
@@ -172,12 +189,17 @@ static int sbsa_gwdt_keepalive(struct watchdog_device *wdd)
 	struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
 
 	/*
-	 * Writing WRR for an explicit watchdog refresh.
-	 * You can write anyting (like 0).
+	 * play safe: program WOR with max value so that we have sufficient
+	 * time to overwrite them after explicit refresh
 	 */
+	writel(U32_MAX, gwdt->control_base + SBSA_GWDT_WOR);
+	/*
+	* Writing WRR for an explicit watchdog refresh.
+	* You can write anyting (like 0).
+	*/
 	writel(0, gwdt->refresh_base + SBSA_GWDT_WRR);
 
-	return 0;
+	return sbsa_gwdt_set_timeout(wdd, wdd->timeout);;
 }
 
 static unsigned int sbsa_gwdt_status(struct watchdog_device *wdd)
@@ -193,10 +215,15 @@ static int sbsa_gwdt_start(struct watchdog_device *wdd)
 {
 	struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
 
+	/*
+	 * play safe: program WOR with max value so that we have sufficient
+	 * time to overwrite them after explicit refresh
+	 */
+	writel(U32_MAX, gwdt->control_base + SBSA_GWDT_WOR);
 	/* writing WCS will cause an explicit watchdog refresh */
 	writel(SBSA_GWDT_WCS_EN, gwdt->control_base + SBSA_GWDT_WCS);
 
-	return 0;
+	return sbsa_gwdt_set_timeout(wdd, wdd->timeout);;
 }
 
 static int sbsa_gwdt_stop(struct watchdog_device *wdd)
@@ -211,6 +238,20 @@ static int sbsa_gwdt_stop(struct watchdog_device *wdd)
 
 static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
 {
+	struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
+	struct watchdog_device *wdd = &gwdt->wdd;
+	u64 timeout_2 = (u64)gwdt->clk * wdd->timeout;
+
+	/*
+	 * Since we can not trust system at this moment, therefore re-write
+	 * WCV only if wdd->timeout <= MAX_TIMEOUT to avoid a corner
+	 * scenario when we might have corrupted wdd->timeout values at
+	 * this point.
+	 */
+	if (wdd->timeout <= MAX_TIMEOUT)
+		writeq(timeout_2 + arch_counter_get_cntvct(),
+			gwdt->control_base + SBSA_GWDT_WCV);
+
 	panic(WATCHDOG_NAME " timeout");
 
 	return IRQ_HANDLED;
@@ -273,7 +314,7 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
 	wdd->info = &sbsa_gwdt_info;
 	wdd->ops = &sbsa_gwdt_ops;
 	wdd->min_timeout = 1;
-	wdd->max_timeout = U32_MAX / gwdt->clk;
+	wdd->max_timeout = MAX_TIMEOUT;
 	wdd->timeout = DEFAULT_TIMEOUT;
 	watchdog_set_drvdata(wdd, gwdt);
 	watchdog_set_nowayout(wdd, nowayout);
-- 
2.5.5

Re: [PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Timur Tabi]

Pratyush Anand wrote:
> + * Note: This watchdog timer has two stages. If action is 0, first stage is
> + * determined by directly programming WCV and second by WOR. When first
> + * timeout is reached, WS0 is triggered and WCV is reloaded with value in
> + * WOR. WS0 interrupt will be ignored, then the second watch period starts;
> + * when second timeout is reached, then WS1 is triggered, system resets. WCV
> + * and WOR are programmed in such a way that total time corresponding to
> + * WCV+WOR becomes equivalent to user programmed "timeout".
> + * If action is 1, then we expect to call panic() at user programmed
> + * "timeout". Therefore, we program both first and second stage using WCV
> + * only.

So I'm not sure I understand how this works yet, but there was an 
earlier version of Fu's driver that did something similar.  It depended 
on being able to reprogram the hardware during the WS0 interrupt, and 
that was rejected by the community.

How is what you are doing different?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Pratyush Anand]

On 03/05/2016:07:12:04 AM, Timur Tabi wrote:
> Pratyush Anand wrote:
> >+ * Note: This watchdog timer has two stages. If action is 0, first stage is
> >+ * determined by directly programming WCV and second by WOR. When first
> >+ * timeout is reached, WS0 is triggered and WCV is reloaded with value in
> >+ * WOR. WS0 interrupt will be ignored, then the second watch period starts;
> >+ * when second timeout is reached, then WS1 is triggered, system resets. WCV
> >+ * and WOR are programmed in such a way that total time corresponding to
> >+ * WCV+WOR becomes equivalent to user programmed "timeout".
> >+ * If action is 1, then we expect to call panic() at user programmed
> >+ * "timeout". Therefore, we program both first and second stage using WCV
> >+ * only.
> 
> So I'm not sure I understand how this works yet, but there was an earlier
> version of Fu's driver that did something similar.  It depended on being
> able to reprogram the hardware during the WS0 interrupt, and that was
> rejected by the community.
> 
> How is what you are doing different?

* Following was the comment for Fu Wei's primitive version of patch [1], because
* of which community rejected it.

> The triggering of the hardware reset should never depend on an interrupt being
> handled properly.  You should always program WCV correctly in advance.

Now, there are couple of things different:

(1) There is an important difference in upstreamed version than the version [1]
which was rejected on above ground. In upstreamed version,  there would be no
interrupt handler when we are in normal mode ie action=0.  So, there is no
possibility of doing any thing in ISR for all normal usage of this timer. In
this mode WCV is always programmed well in advance now.

(2)action=1 mechanism was introduced to implement a dump saving mechanism if
watchdog timeout expires before next kick. So, the current upstream version
calls panic() in ISR. When action=1, then we do write WCV now in ISR, but there
too some precaution have been taken. 

When action=1, and we land into isr handler sbsa_gwdt_interrupt() we can not
trust watchdog data structure any more. That might have been corrupted.
(i) So it might happen that gwdt or wdd pointers have a corrupted value and as
soon as we access gwdt->wdd or wdd->timeout, kernel panics. *No harm*, just
panic() is called a bit early, which dump saving mechanism would be able to
find. So, in fact it will give an extra information to dump saving mechanism
that watchdog structure was corrupted as well.
(ii) Another case, It might happen that wdd->timeout has been corrupted with
large values. This patch does a protection while programming WCV in ISR. It
checks wdd->timeout against MAX_TIMEOUT value and reprograms WCV only when
wdd->timeout is lesser than MAX_TIMEOUT. So, here too, there would be watchdog
reset for sure if dump saving mechanism hangs.

~Pratyush

[1] https://lists.linaro.org/pipermail/linaro-acpi/2015-June/004956.html

Re: [PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Guenter Roeck]

On 05/03/2016 01:20 AM, Pratyush Anand wrote:
> Currently only WOR is used to program both first and second stage which
> provided very limited range of timeout.
>
> This patch uses WCV as well to achieve higher range of timeout. This patch
> programs max_timeout as 255, but that can be increased further as well.
>
> Following testing shows that we can happily achieve 40 second default timeout.
>
>   # modprobe sbsa_gwdt action=1
>   [  131.187562] sbsa-gwdt sbsa-gwdt.0: Initialized with 40s timeout @ 250000000 Hz, action=1.
>   # cd /sys/class/watchdog/watchdog0/
>   # cat state
>   inactive
>   # cat /dev/watchdog0
>   cat: /dev/watchdog0: Invalid argument
>   [  161.710593] watchdog: watchdog0: watchdog did not stop!
>   # cat state
>   active
>   # cat timeout
>   40
>   # cat timeleft
>   38
>   # cat timeleft
>   25
>   # cat /dev/watchdog0
>   cat: /dev/watchdog0: Invalid argument
>   [  184.931030] watchdog: watchdog0: watchdog did not stop!
>   # cat timeleft
>   37
>   # cat timeleft
>   21
>   ...
>   ...
>   # cat timeleft
>   1
>
> panic() is called upon timeout of 40s. See timestamp of last kick (cat) and
> next panic() message.
>
>   [  224.939065] Kernel panic - not syncing: SBSA Watchdog timeout
>
> Signed-off-by: Pratyush Anand <panand@redhat.com>

You could also use the new infrastructure (specify max_hw_heartbeat_ms instead
of max_timeout), and not depend on the correct implementation of WCV.

Overall this adds a lot of complexity for something that could by now easily
be handled by the infrastructure. Is this really necessary ?

Guenter

> ---
>   drivers/watchdog/sbsa_gwdt.c | 83 +++++++++++++++++++++++++++++++++-----------
>   1 file changed, 62 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
> index ad383f6f15fc..529dd5e99fcd 100644
> --- a/drivers/watchdog/sbsa_gwdt.c
> +++ b/drivers/watchdog/sbsa_gwdt.c
> @@ -35,17 +35,23 @@
>    *
>    * SBSA GWDT:
>    * if action is 1 (the two stages mode):
> - * |--------WOR-------WS0--------WOR-------WS1
> + * |--------WCV-------WS0--------WCV-------WS1
>    * |----timeout-----(panic)----timeout-----reset
>    *
>    * if action is 0 (the single stage mode):
> - * |------WOR-----WS0(ignored)-----WOR------WS1
> + * |------WCV-----WS0(ignored)-----WOR------WS1
>    * |--------------timeout-------------------reset
>    *
> - * Note: Since this watchdog timer has two stages, and each stage is determined
> - * by WOR, in the single stage mode, the timeout is (WOR * 2); in the two
> - * stages mode, the timeout is WOR. The maximum timeout in the two stages mode
> - * is half of that in the single stage mode.
> + * Note: This watchdog timer has two stages. If action is 0, first stage is
> + * determined by directly programming WCV and second by WOR. When first
> + * timeout is reached, WS0 is triggered and WCV is reloaded with value in
> + * WOR. WS0 interrupt will be ignored, then the second watch period starts;
> + * when second timeout is reached, then WS1 is triggered, system resets. WCV
> + * and WOR are programmed in such a way that total time corresponding to
> + * WCV+WOR becomes equivalent to user programmed "timeout".
> + * If action is 1, then we expect to call panic() at user programmed
> + * "timeout". Therefore, we program both first and second stage using WCV
> + * only.
>    *
>    */
>
> @@ -95,7 +101,17 @@ struct sbsa_gwdt {
>   	void __iomem		*control_base;
>   };
>
> -#define DEFAULT_TIMEOUT		10 /* seconds */
> +/*
> + * Max Timeout Can be in days, but 255 seconds seems reasonable for all use
> + * cases
> + */
> +#define MAX_TIMEOUT		255

We don't usually define such arbitrary limits.

> +
> +/* Default timeout is 40 seconds, which is the 1st + 2nd watch periods when
> + * action is 0. When action is 1 then both 1st and 2nd watch periods will
> + * be of 40 seconds.
> + */
> +#define DEFAULT_TIMEOUT		40 /* seconds */
>
>   static unsigned int timeout;
>   module_param(timeout, uint, 0);
> @@ -127,20 +143,21 @@ static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd,
>   				 unsigned int timeout)
>   {
>   	struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
> +	u64 timeout_1, timeout_2;
>
>   	wdd->timeout = timeout;
>
>   	if (action)
> -		writel(gwdt->clk * timeout,
> -		       gwdt->control_base + SBSA_GWDT_WOR);
> +		timeout_1 = (u64)gwdt->clk * timeout;
>   	else
> -		/*
> -		 * In the single stage mode, The first signal (WS0) is ignored,
> -		 * the timeout is (WOR * 2), so the WOR should be configured
> -		 * to half value of timeout.
> -		 */
> -		writel(gwdt->clk / 2 * timeout,
> -		       gwdt->control_base + SBSA_GWDT_WOR);
> +		timeout_1 = (u64)gwdt->clk * (timeout - wdd->min_timeout);
> +
> +	/* when action=1, timeout_2 will be overwritten in ISR */
> +	timeout_2 = (u64)gwdt->clk * wdd->min_timeout;
> +
Why min_timeout ? Also, where is it overwritten in the interrupt handler,
and to which value ?

> +	writel(timeout_2, gwdt->control_base + SBSA_GWDT_WOR);
> +	writeq(timeout_1 + arch_counter_get_cntvct(),
> +		gwdt->control_base + SBSA_GWDT_WCV);
>
>   	return 0;
>   }
> @@ -172,12 +189,17 @@ static int sbsa_gwdt_keepalive(struct watchdog_device *wdd)
>   	struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
>
>   	/*
> -	 * Writing WRR for an explicit watchdog refresh.
> -	 * You can write anyting (like 0).
> +	 * play safe: program WOR with max value so that we have sufficient
> +	 * time to overwrite them after explicit refresh
>   	 */
> +	writel(U32_MAX, gwdt->control_base + SBSA_GWDT_WOR);
> +	/*
> +	* Writing WRR for an explicit watchdog refresh.
> +	* You can write anyting (like 0).
> +	*/

Please stick with standard multi-line comments.

>   	writel(0, gwdt->refresh_base + SBSA_GWDT_WRR);
>
> -	return 0;
> +	return sbsa_gwdt_set_timeout(wdd, wdd->timeout);;
>   }
>
>   static unsigned int sbsa_gwdt_status(struct watchdog_device *wdd)
> @@ -193,10 +215,15 @@ static int sbsa_gwdt_start(struct watchdog_device *wdd)
>   {
>   	struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
>
> +	/*
> +	 * play safe: program WOR with max value so that we have sufficient
> +	 * time to overwrite them after explicit refresh
> +	 */
> +	writel(U32_MAX, gwdt->control_base + SBSA_GWDT_WOR);
>   	/* writing WCS will cause an explicit watchdog refresh */
>   	writel(SBSA_GWDT_WCS_EN, gwdt->control_base + SBSA_GWDT_WCS);
>
> -	return 0;
> +	return sbsa_gwdt_set_timeout(wdd, wdd->timeout);;
>   }
>
>   static int sbsa_gwdt_stop(struct watchdog_device *wdd)
> @@ -211,6 +238,20 @@ static int sbsa_gwdt_stop(struct watchdog_device *wdd)
>
>   static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
>   {
> +	struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
> +	struct watchdog_device *wdd = &gwdt->wdd;
> +	u64 timeout_2 = (u64)gwdt->clk * wdd->timeout;
> +
> +	/*
> +	 * Since we can not trust system at this moment, therefore re-write
> +	 * WCV only if wdd->timeout <= MAX_TIMEOUT to avoid a corner
> +	 * scenario when we might have corrupted wdd->timeout values at
> +	 * this point.
> +	 */

This is quite vague. What is this corner scenario where wdd->timeout
would be corrupted ? How can wdd->timeout ever be larger than MAX_TIMEOUT ?

> +	if (wdd->timeout <= MAX_TIMEOUT)
> +		writeq(timeout_2 + arch_counter_get_cntvct(),
> +			gwdt->control_base + SBSA_GWDT_WCV);
> +
>   	panic(WATCHDOG_NAME " timeout");
>
>   	return IRQ_HANDLED;
> @@ -273,7 +314,7 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
>   	wdd->info = &sbsa_gwdt_info;
>   	wdd->ops = &sbsa_gwdt_ops;
>   	wdd->min_timeout = 1;
> -	wdd->max_timeout = U32_MAX / gwdt->clk;
> +	wdd->max_timeout = MAX_TIMEOUT;
>   	wdd->timeout = DEFAULT_TIMEOUT;
>   	watchdog_set_drvdata(wdd, gwdt);
>   	watchdog_set_nowayout(wdd, nowayout);
>

Re: [PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Guenter Roeck]

On 05/03/2016 06:24 AM, Pratyush Anand wrote:
> On 03/05/2016:07:12:04 AM, Timur Tabi wrote:
>> Pratyush Anand wrote:
>>> + * Note: This watchdog timer has two stages. If action is 0, first stage is
>>> + * determined by directly programming WCV and second by WOR. When first
>>> + * timeout is reached, WS0 is triggered and WCV is reloaded with value in
>>> + * WOR. WS0 interrupt will be ignored, then the second watch period starts;
>>> + * when second timeout is reached, then WS1 is triggered, system resets. WCV
>>> + * and WOR are programmed in such a way that total time corresponding to
>>> + * WCV+WOR becomes equivalent to user programmed "timeout".
>>> + * If action is 1, then we expect to call panic() at user programmed
>>> + * "timeout". Therefore, we program both first and second stage using WCV
>>> + * only.
>>
>> So I'm not sure I understand how this works yet, but there was an earlier
>> version of Fu's driver that did something similar.  It depended on being
>> able to reprogram the hardware during the WS0 interrupt, and that was
>> rejected by the community.
>>
>> How is what you are doing different?
>
> * Following was the comment for Fu Wei's primitive version of patch [1], because
> * of which community rejected it.
>
>> The triggering of the hardware reset should never depend on an interrupt being
>> handled properly.  You should always program WCV correctly in advance.
>
> Now, there are couple of things different:
>
> (1) There is an important difference in upstreamed version than the version [1]
> which was rejected on above ground. In upstreamed version,  there would be no
> interrupt handler when we are in normal mode ie action=0.  So, there is no
> possibility of doing any thing in ISR for all normal usage of this timer. In
> this mode WCV is always programmed well in advance now.
>
> (2)action=1 mechanism was introduced to implement a dump saving mechanism if
> watchdog timeout expires before next kick. So, the current upstream version
> calls panic() in ISR. When action=1, then we do write WCV now in ISR, but there
> too some precaution have been taken.
>
> When action=1, and we land into isr handler sbsa_gwdt_interrupt() we can not
> trust watchdog data structure any more. That might have been corrupted.

Why ?

> (i) So it might happen that gwdt or wdd pointers have a corrupted value and as
> soon as we access gwdt->wdd or wdd->timeout, kernel panics. *No harm*, just
> panic() is called a bit early, which dump saving mechanism would be able to
> find. So, in fact it will give an extra information to dump saving mechanism
> that watchdog structure was corrupted as well.
> (ii) Another case, It might happen that wdd->timeout has been corrupted with
> large values. This patch does a protection while programming WCV in ISR. It

How would wdd->timeout be corrupted ?

If what you are say is correct, the interrupt handler can not be trusted, period,
and should be disabled entirely.

Guenter

> checks wdd->timeout against MAX_TIMEOUT value and reprograms WCV only when
> wdd->timeout is lesser than MAX_TIMEOUT. So, here too, there would be watchdog
> reset for sure if dump saving mechanism hangs.
>
> ~Pratyush
>
> [1] https://lists.linaro.org/pipermail/linaro-acpi/2015-June/004956.html
>

Re: [PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Pratyush Anand]

Hi Guenter,

On 03/05/2016:06:47:12 AM, Guenter Roeck wrote:
> On 05/03/2016 06:24 AM, Pratyush Anand wrote:
> >On 03/05/2016:07:12:04 AM, Timur Tabi wrote:
> >>Pratyush Anand wrote:
> >>>+ * Note: This watchdog timer has two stages. If action is 0, first stage is
> >>>+ * determined by directly programming WCV and second by WOR. When first
> >>>+ * timeout is reached, WS0 is triggered and WCV is reloaded with value in
> >>>+ * WOR. WS0 interrupt will be ignored, then the second watch period starts;
> >>>+ * when second timeout is reached, then WS1 is triggered, system resets. WCV
> >>>+ * and WOR are programmed in such a way that total time corresponding to
> >>>+ * WCV+WOR becomes equivalent to user programmed "timeout".
> >>>+ * If action is 1, then we expect to call panic() at user programmed
> >>>+ * "timeout". Therefore, we program both first and second stage using WCV
> >>>+ * only.
> >>
> >>So I'm not sure I understand how this works yet, but there was an earlier
> >>version of Fu's driver that did something similar.  It depended on being
> >>able to reprogram the hardware during the WS0 interrupt, and that was
> >>rejected by the community.
> >>
> >>How is what you are doing different?
> >
> >* Following was the comment for Fu Wei's primitive version of patch [1], because
> >* of which community rejected it.
> >
> >>The triggering of the hardware reset should never depend on an interrupt being
> >>handled properly.  You should always program WCV correctly in advance.
> >
> >Now, there are couple of things different:
> >
> >(1) There is an important difference in upstreamed version than the version [1]
> >which was rejected on above ground. In upstreamed version,  there would be no
> >interrupt handler when we are in normal mode ie action=0.  So, there is no
> >possibility of doing any thing in ISR for all normal usage of this timer. In
> >this mode WCV is always programmed well in advance now.
> >
> >(2)action=1 mechanism was introduced to implement a dump saving mechanism if
> >watchdog timeout expires before next kick. So, the current upstream version
> >calls panic() in ISR. When action=1, then we do write WCV now in ISR, but there
> >too some precaution have been taken.
> >
> >When action=1, and we land into isr handler sbsa_gwdt_interrupt() we can not
> >trust watchdog data structure any more. That might have been corrupted.
> 
> Why ?
> 
> >(i) So it might happen that gwdt or wdd pointers have a corrupted value and as
> >soon as we access gwdt->wdd or wdd->timeout, kernel panics. *No harm*, just
> >panic() is called a bit early, which dump saving mechanism would be able to
> >find. So, in fact it will give an extra information to dump saving mechanism
> >that watchdog structure was corrupted as well.
> >(ii) Another case, It might happen that wdd->timeout has been corrupted with
> >large values. This patch does a protection while programming WCV in ISR. It
> 
> How would wdd->timeout be corrupted ?

Most likely it will not be corrupted. But, if ISR has been called it means
something went wrong, and watchdog was not kicked for the time programmed as
"timeout". So, probably we should be extra careful.

Anyway, there is no side effect of checking wdd->timeout against MAX_TIMEOUT.

Moreover, I was unable to envisage any regression with this patch other than
that it introduces a bit complexity. But, then it gives us an advantage of
having any timeout value.

> If what you are say is correct, the interrupt handler can not be trusted, period,
> and should be disabled entirely.

Yes, When system is hang, then probably we can not trust even interrupt handler.
We can not be 100% sure that handler will be called, but that is the case with
existing upstream code as well. In such situation upstream code will also
misbehave. There is little we can do for such cases.

If one wants to be fully assured for a hardware watchdog reset then the choice
is action=0. Even after current patch, action=0 will ensure that there would be
a hardware reset upon timeout.

~Pratyush

Re: [PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Pratyush Anand]

Hi Guenter,

On 03/05/2016:06:29:39 AM, Guenter Roeck wrote:
> On 05/03/2016 01:20 AM, Pratyush Anand wrote:
> >Currently only WOR is used to program both first and second stage which
> >provided very limited range of timeout.
> >
> >This patch uses WCV as well to achieve higher range of timeout. This patch
> >programs max_timeout as 255, but that can be increased further as well.
> >
> >Following testing shows that we can happily achieve 40 second default timeout.
> >
> >  # modprobe sbsa_gwdt action=1
> >  [  131.187562] sbsa-gwdt sbsa-gwdt.0: Initialized with 40s timeout @ 250000000 Hz, action=1.
> >  # cd /sys/class/watchdog/watchdog0/
> >  # cat state
> >  inactive
> >  # cat /dev/watchdog0
> >  cat: /dev/watchdog0: Invalid argument
> >  [  161.710593] watchdog: watchdog0: watchdog did not stop!
> >  # cat state
> >  active
> >  # cat timeout
> >  40
> >  # cat timeleft
> >  38
> >  # cat timeleft
> >  25
> >  # cat /dev/watchdog0
> >  cat: /dev/watchdog0: Invalid argument
> >  [  184.931030] watchdog: watchdog0: watchdog did not stop!
> >  # cat timeleft
> >  37
> >  # cat timeleft
> >  21
> >  ...
> >  ...
> >  # cat timeleft
> >  1
> >
> >panic() is called upon timeout of 40s. See timestamp of last kick (cat) and
> >next panic() message.
> >
> >  [  224.939065] Kernel panic - not syncing: SBSA Watchdog timeout
> >
> >Signed-off-by: Pratyush Anand <panand@redhat.com>
> 
> You could also use the new infrastructure (specify max_hw_heartbeat_ms instead
> of max_timeout), and not depend on the correct implementation of WCV.

Thanks for pointing to max_hw_heartbeat_ms. Just gone through it. Certainly it
would be helpful, and some part of this patch will go away. 

In fact after supporting max_hw_heartbeat_ms, there should be no change for
action=0 functionally. However, we would still need some changes for action=1.

When action=1, isr is called, which calls panic(). Calling panic() will further
trigger a dump saving mechanism, which can cause to execute a secondary kernel.
Now, it might happen that with the limited timeout (max_hw_heartbeat_ms)
programmed in first kernel, we land into a reset before secondary kernel could
start kicking it again or would complete dump save. 
So, in my opinion:
(1) We should use max_hw_heartbeat_ms.
(2) Then we should overwrite WCV in ISR so that it ensures a timeout of user
programmed "timeout" value for hardware reset.

~Pratyush

Re: [PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Guenter Roeck]

On 05/03/2016 07:17 AM, Pratyush Anand wrote:
> Hi Guenter,
>
> On 03/05/2016:06:47:12 AM, Guenter Roeck wrote:
>> On 05/03/2016 06:24 AM, Pratyush Anand wrote:
>>> On 03/05/2016:07:12:04 AM, Timur Tabi wrote:
>>>> Pratyush Anand wrote:
>>>>> + * Note: This watchdog timer has two stages. If action is 0, first stage is
>>>>> + * determined by directly programming WCV and second by WOR. When first
>>>>> + * timeout is reached, WS0 is triggered and WCV is reloaded with value in
>>>>> + * WOR. WS0 interrupt will be ignored, then the second watch period starts;
>>>>> + * when second timeout is reached, then WS1 is triggered, system resets. WCV
>>>>> + * and WOR are programmed in such a way that total time corresponding to
>>>>> + * WCV+WOR becomes equivalent to user programmed "timeout".
>>>>> + * If action is 1, then we expect to call panic() at user programmed
>>>>> + * "timeout". Therefore, we program both first and second stage using WCV
>>>>> + * only.
>>>>
>>>> So I'm not sure I understand how this works yet, but there was an earlier
>>>> version of Fu's driver that did something similar.  It depended on being
>>>> able to reprogram the hardware during the WS0 interrupt, and that was
>>>> rejected by the community.
>>>>
>>>> How is what you are doing different?
>>>
>>> * Following was the comment for Fu Wei's primitive version of patch [1], because
>>> * of which community rejected it.
>>>
>>>> The triggering of the hardware reset should never depend on an interrupt being
>>>> handled properly.  You should always program WCV correctly in advance.
>>>
>>> Now, there are couple of things different:
>>>
>>> (1) There is an important difference in upstreamed version than the version [1]
>>> which was rejected on above ground. In upstreamed version,  there would be no
>>> interrupt handler when we are in normal mode ie action=0.  So, there is no
>>> possibility of doing any thing in ISR for all normal usage of this timer. In
>>> this mode WCV is always programmed well in advance now.
>>>
>>> (2)action=1 mechanism was introduced to implement a dump saving mechanism if
>>> watchdog timeout expires before next kick. So, the current upstream version
>>> calls panic() in ISR. When action=1, then we do write WCV now in ISR, but there
>>> too some precaution have been taken.
>>>
>>> When action=1, and we land into isr handler sbsa_gwdt_interrupt() we can not
>>> trust watchdog data structure any more. That might have been corrupted.
>>
>> Why ?
>>
>>> (i) So it might happen that gwdt or wdd pointers have a corrupted value and as
>>> soon as we access gwdt->wdd or wdd->timeout, kernel panics. *No harm*, just
>>> panic() is called a bit early, which dump saving mechanism would be able to
>>> find. So, in fact it will give an extra information to dump saving mechanism
>>> that watchdog structure was corrupted as well.
>>> (ii) Another case, It might happen that wdd->timeout has been corrupted with
>>> large values. This patch does a protection while programming WCV in ISR. It
>>
>> How would wdd->timeout be corrupted ?
>
> Most likely it will not be corrupted. But, if ISR has been called it means
> something went wrong, and watchdog was not kicked for the time programmed as
> "timeout". So, probably we should be extra careful.
>
This logic would apply to _every_ watchdog driver implementing interrupts.
Actually, it would apply to _all_ kernel code, and the logic could be used
to introduce hyper-defensive programming all over the place, bloat the kernel
and ultimately make it all but unusable. I do not believe in such programming
in an operating system kernel.

On top of that, the assumption that the kernel would be still sane enough
to call the interrupt handler, but not sane enough to actually execute it,
seems to be a bit far-fetched.

> Anyway, there is no side effect of checking wdd->timeout against MAX_TIMEOUT.
>
The side effect is increased code size.

> Moreover, I was unable to envisage any regression with this patch other than
> that it introduces a bit complexity. But, then it gives us an advantage of
> having any timeout value.
>
>> If what you are say is correct, the interrupt handler can not be trusted, period,
>> and should be disabled entirely.
>
> Yes, When system is hang, then probably we can not trust even interrupt handler.
> We can not be 100% sure that handler will be called, but that is the case with
> existing upstream code as well. In such situation upstream code will also
> misbehave. There is little we can do for such cases.
>
... other than wait for the hard reset, which will be just fine, at least
with the current code.

> If one wants to be fully assured for a hardware watchdog reset then the choice
> is action=0. Even after current patch, action=0 will ensure that there would be
> a hardware reset upon timeout.
>
Sorry, I don't buy into this. You'll have to convince Wim, or simply
change the driver to use the infrastructure to achieve larger timeouts.

Thanks,
Guenter

Re: [PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Timur Tabi]

Guenter Roeck wrote:
>> Most likely it will not be corrupted. But, if ISR has been called it
>> means
>> something went wrong, and watchdog was not kicked for the time
>> programmed as
>> "timeout". So, probably we should be extra careful.
>>
> This logic would apply to _every_ watchdog driver implementing interrupts.
> Actually, it would apply to _all_ kernel code, and the logic could be used
> to introduce hyper-defensive programming all over the place, bloat the
> kernel
> and ultimately make it all but unusable. I do not believe in such
> programming
> in an operating system kernel.
>
> On top of that, the assumption that the kernel would be still sane enough
> to call the interrupt handler, but not sane enough to actually execute it,
> seems to be a bit far-fetched.

Agreed.  Pratyush, have you ever seen any watchdog register get 
corrupted, like you describe?  It just seems like you're imagining a 
problem that has never occurred.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

Re: [PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Timur Tabi]

Pratyush Anand wrote:
> In fact after supporting max_hw_heartbeat_ms, there should be no change for
> action=0 functionally. However, we would still need some changes for action=1.

IMHO, action=1 is more of a debugging option, and not something that 
would be used normally.  I would need to see some evidence that real 
users want to have action=1 and a longer timeout.

I've never been a fan of the action=1 option, and I'm certainly not keen 
any patches that make action=1 more complicated than it already is.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

Re: [PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Pratyush Anand]

On 03/05/2016:10:07:48 AM, Timur Tabi wrote:
> Pratyush Anand wrote:
> >In fact after supporting max_hw_heartbeat_ms, there should be no change for
> >action=0 functionally. However, we would still need some changes for action=1.
> 
> IMHO, action=1 is more of a debugging option, and not something that would
> be used normally.  I would need to see some evidence that real users want to
> have action=1 and a longer timeout.
> 
If action=1 need to be used effectively, then we should have something which
would help to increase timeout values.

Currently you have only 10 second to execute secondary kernel, which might not
be sufficient.

> I've never been a fan of the action=1 option, and I'm certainly not keen any
> patches that make action=1 more complicated than it already is.

I think, with max_hw_heartbeat_ms it would be far more simpler. Will attempt and
send another RFC.

~Pratyush

Re: [PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Guenter Roeck]

On Tue, May 03, 2016 at 09:21:41PM +0530, Pratyush Anand wrote:
> On 03/05/2016:10:07:48 AM, Timur Tabi wrote:
> > Pratyush Anand wrote:
> > >In fact after supporting max_hw_heartbeat_ms, there should be no change for
> > >action=0 functionally. However, we would still need some changes for action=1.
> > 
> > IMHO, action=1 is more of a debugging option, and not something that would
> > be used normally.  I would need to see some evidence that real users want to
> > have action=1 and a longer timeout.
> > 
> If action=1 need to be used effectively, then we should have something which
> would help to increase timeout values.
> 
> Currently you have only 10 second to execute secondary kernel, which might not
> be sufficient.
> 
Previously the argument was that the 10 seconds (assuming the clock runs at
maximum speed) would not be sufficient to load the watchdog application. Now it
seems the 10 seconds are deemed insufficient to load the watchdog driver (since
the infrastructure can handle the heartbeats). Is there actual evidence that
this is the case ?

Guenter

> > I've never been a fan of the action=1 option, and I'm certainly not keen any
> > patches that make action=1 more complicated than it already is.
> 
> I think, with max_hw_heartbeat_ms it would be far more simpler. Will attempt and
> send another RFC.
> 
> ~Pratyush

Re: [PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Pratyush Anand]

Hi Guenter,

On 03/05/2016:10:16:02 AM, Guenter Roeck wrote:
> On Tue, May 03, 2016 at 09:21:41PM +0530, Pratyush Anand wrote:
> > On 03/05/2016:10:07:48 AM, Timur Tabi wrote:
> > > Pratyush Anand wrote:
> > > >In fact after supporting max_hw_heartbeat_ms, there should be no change for
> > > >action=0 functionally. However, we would still need some changes for action=1.
> > > 
> > > IMHO, action=1 is more of a debugging option, and not something that would
> > > be used normally.  I would need to see some evidence that real users want to
> > > have action=1 and a longer timeout.
> > > 
> > If action=1 need to be used effectively, then we should have something which
> > would help to increase timeout values.
> > 
> > Currently you have only 10 second to execute secondary kernel, which might not
> > be sufficient.
> > 
> Previously the argument was that the 10 seconds (assuming the clock runs at
> maximum speed) would not be sufficient to load the watchdog application. Now it

May be you meant "would be sufficient". OK..let me clarify on it.

Currently it takes 7-8 second from the point 1st kernel panics to the point
second kernel boots. (Given, we have D-cache enabled in kexec-tools, for which
community is not yet agreed), anyway..so, it is safe for me as of now. But,
there is only 2-3 second margin. So, I am not sure if all sort of secondary
kernel will be able to make it in that time.

Following minimal code will be able to extend timeout for secondary kernel, and
I do not see anything wrong in it. We are anyway, panicking in ISR, so what
could be disadvantage if we write a wdt register just before panicking?

--- a/drivers/watchdog/sbsa_gwdt.c
+++ b/drivers/watchdog/sbsa_gwdt.c
@@ -221,6 +221,13 @@ static int sbsa_gwdt_stop(struct watchdog_device *wdd)
 
static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
{
+    struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
+    struct watchdog_device *wdd = &gwdt->wdd;
+    u64 timeout = (u64)gwdt->clk * wdd->timeout;
+
+    writeq(timeout + arch_counter_get_cntvct(),
+                    gwdt->control_base + SBSA_GWDT_WCV);
+
     panic(WATCHDOG_NAME " timeout");
      
             return IRQ_HANDLED;

~Pratyush

Re: [PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Timur Tabi]

Pratyush Anand wrote:
> static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
> {
> +    struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
> +    struct watchdog_device *wdd = &gwdt->wdd;
> +    u64 timeout = (u64)gwdt->clk * wdd->timeout;
> +
> +    writeq(timeout + arch_counter_get_cntvct(),
> +                    gwdt->control_base + SBSA_GWDT_WCV);
> +
>       panic(WATCHDOG_NAME " timeout");

I'm on the fence about this.

On one hand, I have always opposed the idea that the interrupt handler 
needs to function properly in order for the timeout to be correct.  Fu's 
original patch required this for every timeout.

The current code, however, only uses the interrupt when action=1.  In 
this case, WCV is only reprogrammed in order to prevent the system from 
resetting during the kexec.  Technically, the watchdog timeout has 
already been handled.

However, this should be unnecessary, because it can't be a problem 
that's unique to the SBSA watchdog.  Every system that kexecs another 
kernel needs to be able to handle a watchdog timeout.  Shouldn't the 
kexec code already ping or disable the watchdog?  We need a 
cross-platform solution.  Drivers should not need to do this.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Pratyush Anand]

+Dave

Hi Timur,

On 04/05/2016:09:21:43 AM, Timur Tabi wrote:
> Pratyush Anand wrote:
> >static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
> >{
> >+    struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
> >+    struct watchdog_device *wdd = &gwdt->wdd;
> >+    u64 timeout = (u64)gwdt->clk * wdd->timeout;
> >+
> >+    writeq(timeout + arch_counter_get_cntvct(),
> >+                    gwdt->control_base + SBSA_GWDT_WCV);
> >+
> >      panic(WATCHDOG_NAME " timeout");
> 
> I'm on the fence about this.
> 
> On one hand, I have always opposed the idea that the interrupt handler needs
> to function properly in order for the timeout to be correct.  Fu's original
> patch required this for every timeout.
> 
> The current code, however, only uses the interrupt when action=1.  In this
> case, WCV is only reprogrammed in order to prevent the system from resetting
> during the kexec.  Technically, the watchdog timeout has already been
> handled.

Yes.

> 
> However, this should be unnecessary, because it can't be a problem that's
> unique to the SBSA watchdog.  Every system that kexecs another kernel needs
> to be able to handle a watchdog timeout.  Shouldn't the kexec code already
> ping or disable the watchdog?  We need a cross-platform solution.  Drivers
> should not need to do this.

Its unique to SBSA because you have very little timeout here. kexec-tools
upstream does not have any mechanism to handle watchdog timeout. Lets say even
if we implement a framework there, the best it can do is to ping the watchdog
again. Disabling should not be an option in kexec-tools, because in that case if
kexec-tools or secondary kernel stuck, we won't have a way out.
Now, even if we ping it once in kexec tools, we will have to make sure that
watchdog driver's probe is called before timeout. Therefore, user must have a
way to specify this timeout, so that if a particular kernel take more time to
boot then he can increase the timeout. Given, these variable conditions I do not
see much advantage of implementing it in kexec-tools.

However fedora/rhel kedumpctl mechanism does some  best case correction. It
makes sure that watchdog module is loaded in second kernel if watchdog was
active during first kernel, and loaded as early as possible [1].

~Pratyush

[1] https://github.com/pratyushanand/kexec-tools/commits/watchdog_fmaster

Re: [PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Timur Tabi]

Pratyush Anand wrote:
> Its unique to SBSA because you have very little timeout here. kexec-tools
> upstream does not have any mechanism to handle watchdog timeout. Lets say even
> if we implement a framework there, the best it can do is to ping the watchdog
> again.

Ok, so it's more accurate to say that kexec has a minimum watchdog 
timeout requirement.  What happens if the system admin sets the timeout 
to 5 seconds arbitrarily?  The system will reset during kexec, no matter 
which hardware is used.

This still sounds like a band-aid to me.  We're just assuming that we 
need a timeout of at least 20 seconds to support kexec.  Frankly, this 
still sounds like a problem the kexec developers needs to acknowledge 
and deal with.

Still I'm okay with a patch that extends the timeout by programming WCV, 
but it has to be commented as a hack specifically to support kexec 
because the timeout might be too short.  Then Wim can decide whether he 
supports such changes.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

Re: [PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Guenter Roeck]

On Wed, May 04, 2016 at 11:17:29AM -0500, Timur Tabi wrote:
> Pratyush Anand wrote:
> >Its unique to SBSA because you have very little timeout here. kexec-tools
> >upstream does not have any mechanism to handle watchdog timeout. Lets say even
> >if we implement a framework there, the best it can do is to ping the watchdog
> >again.
> 
> Ok, so it's more accurate to say that kexec has a minimum watchdog timeout
> requirement.  What happens if the system admin sets the timeout to 5 seconds
> arbitrarily?  The system will reset during kexec, no matter which hardware
> is used.
> 
> This still sounds like a band-aid to me.  We're just assuming that we need a
> timeout of at least 20 seconds to support kexec.  Frankly, this still sounds
> like a problem the kexec developers needs to acknowledge and deal with.
> 
> Still I'm okay with a patch that extends the timeout by programming WCV, but
> it has to be commented as a hack specifically to support kexec because the
> timeout might be too short.  Then Wim can decide whether he supports such
> changes.
> 
I don't even understand how kexec-tools is involved in the first place.
kexec-tools sounds like user space, which should execute _after_ the kernel
and its modules are loaded (assuming modules are loaded from initramfs).
If kexec-tools can somehow ping the watchdog (presumably by writing into
the HW directly), I don't understand why it doesn't simply load the watchdog
driver instead and let the watchdog core handle the heartbeats.

I am really missing something here. How can kexec-tools do anything before
the kernel is loaded ?

Guenter

Re: [PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Pratyush Anand]

+kexec list

Hi Guenter,

On 05/05/2016:09:43:00 AM, Guenter Roeck wrote:
> On Wed, May 04, 2016 at 11:17:29AM -0500, Timur Tabi wrote:
> > Pratyush Anand wrote:
> > >Its unique to SBSA because you have very little timeout here. kexec-tools
> > >upstream does not have any mechanism to handle watchdog timeout. Lets say even
> > >if we implement a framework there, the best it can do is to ping the watchdog
> > >again.
> > 
> > Ok, so it's more accurate to say that kexec has a minimum watchdog timeout
> > requirement.  What happens if the system admin sets the timeout to 5 seconds
> > arbitrarily?  The system will reset during kexec, no matter which hardware
> > is used.
> > 
> > This still sounds like a band-aid to me.  We're just assuming that we need a
> > timeout of at least 20 seconds to support kexec.  Frankly, this still sounds
> > like a problem the kexec developers needs to acknowledge and deal with.
> > 
> > Still I'm okay with a patch that extends the timeout by programming WCV, but
> > it has to be commented as a hack specifically to support kexec because the
> > timeout might be too short.  Then Wim can decide whether he supports such
> > changes.
> > 
> I don't even understand how kexec-tools is involved in the first place.
> kexec-tools sounds like user space, which should execute _after_ the kernel

So _after_ the 1st kernel and _before_ the second kernel. It is an application
for the 1st kernel, which creates a tiny boot loader for 2nd kernel. After the
1st kernel is loaded, kexec-tools is executed in user space, which provides a
sane 2nd kernel and initramfs to the kernel using kexec() system call. Now 1st
kernel keep these information loaded into a specific memory called "Crash
Kernel" memory. When 1st kernel crashes, kernel kexec code passes control to
kexec boot loader, which does sha verification of 2nd kernel and initramfs and
passes control to 2nd kernel.

> and its modules are loaded (assuming modules are loaded from initramfs).
> If kexec-tools can somehow ping the watchdog (presumably by writing into
> the HW directly), I don't understand why it doesn't simply load the watchdog
> driver instead and let the watchdog core handle the heartbeats.

Because that tiny boot loader (which called purgatory) does not have any
knowledge about driver.

> 
> I am really missing something here. How can kexec-tools do anything before
> the kernel is loaded ?

So, if we _do_  _not_ go with the current version of patch, probably this could
be the only available option. However, Even when we would kick watchdog once in
kexec boot loader, we will have to make sure the 2nd kernel is light enough to
load watchdog module before timeout.  I think, in the long run we must have SBSA
watchdog specification improvement to keep WOR as 64 bit.

~Pratyush

Re: [PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Timur Tabi]

Pratyush Anand wrote:
> I think, in the long run we must have SBSA
> watchdog specification improvement to keep WOR as 64 bit.

I agree with this 100%.  IMHO, using a 32-bit WOR was just a bad decision.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

Re: [PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Guenter Roeck]

On 05/05/2016 11:22 AM, Timur Tabi wrote:
> Pratyush Anand wrote:
>> I think, in the long run we must have SBSA
>> watchdog specification improvement to keep WOR as 64 bit.
>
> I agree with this 100%.  IMHO, using a 32-bit WOR was just a bad decision.
>

A 32-bit counter is absolutely fine. Letting it run with a 400MHz clock
(or was it 200 MHz ?) is the problem. A resolution of 2.5ns for a watchdog
timer does not really make any sense.

Guenter

Re: [PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Timur Tabi]

Guenter Roeck wrote:
> A 32-bit counter is absolutely fine. Letting it run with a 400MHz clock
> (or was it 200 MHz ?) is the problem. A resolution of 2.5ns for a watchdog
> timer does not really make any sense.

The 10 second limit is based on a 20MHz clock.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

Re: [PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Timur Tabi]

Timur Tabi wrote:
>
>> A 32-bit counter is absolutely fine. Letting it run with a 400MHz clock
>> (or was it 200 MHz ?) is the problem. A resolution of 2.5ns for a
>> watchdog
>> timer does not really make any sense.
>
> The 10 second limit is based on a 20MHz clock.

No, that's not true.  I misread the code.  I knew something was wrong, 
but it didn't click until just now.

The default timeout is 10 seconds.  The max timeout on a 20MHz system 
(which is what we're running) is over 200 seconds.

The problem is that Pratyush's system is running at a clock that's way 
too fast:

  [  131.187562] sbsa-gwdt sbsa-gwdt.0: Initialized with 40s timeout @ 
250000000 Hz, action=1.

250MHz is unreasonable.  Pratyush, why is your system counter so high? 
On our ARM64 system, it's set to 20MHz.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

Re: [PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Guenter Roeck]

On 05/05/2016 04:38 PM, Timur Tabi wrote:
> Guenter Roeck wrote:
>> A 32-bit counter is absolutely fine. Letting it run with a 400MHz clock
>> (or was it 200 MHz ?) is the problem. A resolution of 2.5ns for a watchdog
>> timer does not really make any sense.
>
> The 10 second limit is based on a 20MHz clock.
>
Not that a resolution of 50 ns makes sense either, but 4294967296 / 20971520 = 204,
and 20971520 * 10 = 209715200 = 0x0c800000. Where does the resolution get lost ?

Guenter

Re: [PATCH RFC] Watchdog: sbsa_gwdt: Enhance timeout range

[Guenter Roeck]

On 05/05/2016 04:45 PM, Timur Tabi wrote:
> Timur Tabi wrote:
>>
>>> A 32-bit counter is absolutely fine. Letting it run with a 400MHz clock
>>> (or was it 200 MHz ?) is the problem. A resolution of 2.5ns for a
>>> watchdog
>>> timer does not really make any sense.
>>
>> The 10 second limit is based on a 20MHz clock.
>
> No, that's not true.  I misread the code.  I knew something was wrong, but it didn't click until just now.
>
> The default timeout is 10 seconds.  The max timeout on a 20MHz system (which is what we're running) is over 200 seconds.
>
> The problem is that Pratyush's system is running at a clock that's way too fast:
>
>   [  131.187562] sbsa-gwdt sbsa-gwdt.0: Initialized with 40s timeout @ 250000000 Hz, action=1.
>
> 250MHz is unreasonable.  Pratyush, why is your system counter so high? On our ARM64 system, it's set to 20MHz.
>

Guess that answers my earlier question. Problem is that the specification
_permits_ those unreasonable frequencies, and quite obviously they are
being used, no matter if it makes sense or not.

With a (still unreasonable) maximum frequency of 100 MHz, the problem
would not exist. So, if anything, someone with influence on the standard
might suggest to reduce the maximum permitted frequency.

Guenter