Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
* [RFC PATCH] watchdog: renesas_wdt: don't sleep in atomic context
@ 2020-10-02 15:09 Wolfram Sang
  2020-10-02 15:12 ` Wolfram Sang
  2020-10-02 16:47 ` Guenter Roeck
  0 siblings, 2 replies; 3+ messages in thread
From: Wolfram Sang @ 2020-10-02 15:09 UTC (permalink / raw)
  To: linux-watchdog; +Cc: linux-renesas-soc, Yoshihiro Shimoda, Wolfram Sang

In the restart handler, we hit the following OOPS because rwdt_start()
schedules because of usleep_range():

[   16.071855] BUG: scheduling while atomic: init/1184/0x00000002
...
[   16.159955] [<c05b6b3c>] (schedule) from [<c05b9884>] (schedule_hrtimeout_range_clock+0x108/0x15c)
[   16.168918]  r5:00000003 r4:bd855014
[   16.172498] [<c05b977c>] (schedule_hrtimeout_range_clock) from [<c05b9904>] (schedule_hrtimeout_range+0x2c/0x34)
[   16.182679]  r7:ea0edd48 r6:ea0ec000 r5:0000fa00 r4:00000000
[   16.188343] [<c05b98d8>] (schedule_hrtimeout_range) from [<c05b92dc>] (usleep_range+0x84/0xb8)
[   16.196973] [<c05b9258>] (usleep_range) from [<c049d4a8>] (rwdt_start+0x58/0xa8)

Convert usleep_range() to udelay() to avoid the bug. The downside is
busy looping when the watchdog is started or stopped (64us or 96us in my
case). However, this happens rarely enough, so it seems acceptable.

Fixes: b836005b4f95 ("watchdog: renesas_wdt: Add a few cycles delay")
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

We could also add a new flag in the watchdog core "IS_RESTARTING" or
similar and check that in the driver. Depending on the flag, we use
udelay() or usleep_range(). Not sure, if this is feasible or
over-engineered, though. Looking forward for comments.

 drivers/watchdog/renesas_wdt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index 00662a8e039c..2b0da9b76375 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -76,8 +76,7 @@ static void rwdt_wait_cycles(struct rwdt_priv *priv, unsigned int cycles)
 	unsigned int delay;
 
 	delay = DIV_ROUND_UP(cycles * 1000000, priv->clk_rate);
-
-	usleep_range(delay, 2 * delay);
+	udelay(delay);
 }
 
 static int rwdt_start(struct watchdog_device *wdev)
-- 
2.28.0


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

* Re: [RFC PATCH] watchdog: renesas_wdt: don't sleep in atomic context
  2020-10-02 15:09 [RFC PATCH] watchdog: renesas_wdt: don't sleep in atomic context Wolfram Sang
@ 2020-10-02 15:12 ` Wolfram Sang
  2020-10-02 16:47 ` Guenter Roeck
  1 sibling, 0 replies; 3+ messages in thread
From: Wolfram Sang @ 2020-10-02 15:12 UTC (permalink / raw)
  To: linux-watchdog; +Cc: linux-renesas-soc, Yoshihiro Shimoda


[-- Attachment #1: Type: text/plain, Size: 185 bytes --]


>  	delay = DIV_ROUND_UP(cycles * 1000000, priv->clk_rate);
> -
> -	usleep_range(delay, 2 * delay);
> +	udelay(delay);

I should probably add a comment here explaining the situation.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

* Re: [RFC PATCH] watchdog: renesas_wdt: don't sleep in atomic context
  2020-10-02 15:09 [RFC PATCH] watchdog: renesas_wdt: don't sleep in atomic context Wolfram Sang
  2020-10-02 15:12 ` Wolfram Sang
@ 2020-10-02 16:47 ` Guenter Roeck
  1 sibling, 0 replies; 3+ messages in thread
From: Guenter Roeck @ 2020-10-02 16:47 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: linux-watchdog, linux-renesas-soc, Yoshihiro Shimoda

On Fri, Oct 02, 2020 at 05:09:44PM +0200, Wolfram Sang wrote:
> In the restart handler, we hit the following OOPS because rwdt_start()
> schedules because of usleep_range():
> 
> [   16.071855] BUG: scheduling while atomic: init/1184/0x00000002
> ...
> [   16.159955] [<c05b6b3c>] (schedule) from [<c05b9884>] (schedule_hrtimeout_range_clock+0x108/0x15c)
> [   16.168918]  r5:00000003 r4:bd855014
> [   16.172498] [<c05b977c>] (schedule_hrtimeout_range_clock) from [<c05b9904>] (schedule_hrtimeout_range+0x2c/0x34)
> [   16.182679]  r7:ea0edd48 r6:ea0ec000 r5:0000fa00 r4:00000000
> [   16.188343] [<c05b98d8>] (schedule_hrtimeout_range) from [<c05b92dc>] (usleep_range+0x84/0xb8)
> [   16.196973] [<c05b9258>] (usleep_range) from [<c049d4a8>] (rwdt_start+0x58/0xa8)
> 
> Convert usleep_range() to udelay() to avoid the bug. The downside is
> busy looping when the watchdog is started or stopped (64us or 96us in my
> case). However, this happens rarely enough, so it seems acceptable.
> 
> Fixes: b836005b4f95 ("watchdog: renesas_wdt: Add a few cycles delay")
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> We could also add a new flag in the watchdog core "IS_RESTARTING" or
> similar and check that in the driver. Depending on the flag, we use
> udelay() or usleep_range(). Not sure, if this is feasible or
> over-engineered, though. Looking forward for comments.

Seems to be neither to me. I don't know if there are many drivers who sleep
in the stop function, but even if there are none (today) it would be a
generic problem.

On a side note, I am quite sure there is a system wide flag indicating
that the system is rebooting. Maybe we can just use that ?

> 
>  drivers/watchdog/renesas_wdt.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 00662a8e039c..2b0da9b76375 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -76,8 +76,7 @@ static void rwdt_wait_cycles(struct rwdt_priv *priv, unsigned int cycles)
>  	unsigned int delay;
>  
>  	delay = DIV_ROUND_UP(cycles * 1000000, priv->clk_rate);
> -
> -	usleep_range(delay, 2 * delay);
> +	udelay(delay);

I think there should be a comment such as "we can't sleep here because the
function may be called from the restart handler and thus must not sleep".
Either case, this happens so rarely and the delay is so minor that we
should just apply this patch (with comment added) and bother about the more
generic version if the problem is seen again.

Thanks,
Guenter

>  }
>  
>  static int rwdt_start(struct watchdog_device *wdev)
> -- 
> 2.28.0
> 

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

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-02 15:09 [RFC PATCH] watchdog: renesas_wdt: don't sleep in atomic context Wolfram Sang
2020-10-02 15:12 ` Wolfram Sang
2020-10-02 16:47 ` Guenter Roeck

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org
	public-inbox-index linux-watchdog

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git