| Message ID | 20241212113014.1075414-1-phil@zankapfel.net |
|---|---|
| State | New |
| Headers | show |
| Series | watchdog: aspeed: replace mdelay with msleep | expand |
From: Guenter Roeck > Sent: 12 December 2024 13:56 > To: Phil Eichinger <phil@zankapfel.net>; wim@linux-watchdog.org; joel@jms.id.au; > andrew@codeconstruct.com.au; linux-watchdog@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] watchdog: aspeed: replace mdelay with msleep > > On 12/12/24 03:30, Phil Eichinger wrote: > > Since it is not called in an atomic context the mdelay function > > can be replaced with msleep to avoid busy wait. > > > > Signed-off-by: Phil Eichinger <phil@zankapfel.net> > > --- > > drivers/watchdog/aspeed_wdt.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c > > index b4773a6aaf8c..98ef341408f7 100644 > > --- a/drivers/watchdog/aspeed_wdt.c > > +++ b/drivers/watchdog/aspeed_wdt.c > > @@ -208,7 +208,7 @@ static int aspeed_wdt_restart(struct watchdog_device *wdd, > > wdt->ctrl &= ~WDT_CTRL_BOOT_SECONDARY; > > aspeed_wdt_enable(wdt, 128 * WDT_RATE_1MHZ / 1000); > > > > - mdelay(1000); > > + msleep(1000); > > > > return 0; > > } > This is a _restart_ handler. The only purpose of the delay is to wait > for the reset to trigger. It is not supposed to sleep. With the recent scheduler changes isn't the code likely to get pre-empted? Which (effectively) converts is to a sleep? David > > NACK. > > Guenter > > > - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 12/14/24 13:21, David Laight wrote: > From: Guenter Roeck >> Sent: 12 December 2024 13:56 >> To: Phil Eichinger <phil@zankapfel.net>; wim@linux-watchdog.org; joel@jms.id.au; >> andrew@codeconstruct.com.au; linux-watchdog@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org >> Subject: Re: [PATCH] watchdog: aspeed: replace mdelay with msleep >> >> On 12/12/24 03:30, Phil Eichinger wrote: >>> Since it is not called in an atomic context the mdelay function >>> can be replaced with msleep to avoid busy wait. >>> >>> Signed-off-by: Phil Eichinger <phil@zankapfel.net> >>> --- >>> drivers/watchdog/aspeed_wdt.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c >>> index b4773a6aaf8c..98ef341408f7 100644 >>> --- a/drivers/watchdog/aspeed_wdt.c >>> +++ b/drivers/watchdog/aspeed_wdt.c >>> @@ -208,7 +208,7 @@ static int aspeed_wdt_restart(struct watchdog_device *wdd, >>> wdt->ctrl &= ~WDT_CTRL_BOOT_SECONDARY; >>> aspeed_wdt_enable(wdt, 128 * WDT_RATE_1MHZ / 1000); >>> >>> - mdelay(1000); >>> + msleep(1000); >>> >>> return 0; >>> } >> This is a _restart_ handler. The only purpose of the delay is to wait >> for the reset to trigger. It is not supposed to sleep. > > With the recent scheduler changes isn't the code likely to get > pre-empted? > Which (effectively) converts is to a sleep? > This code is called from do_kernel_restart(), which in turn is called from machine_restart(). I'd think that the kernel has a severe problem if it decides to preempt that function. Guenter
diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c index b4773a6aaf8c..98ef341408f7 100644 --- a/drivers/watchdog/aspeed_wdt.c +++ b/drivers/watchdog/aspeed_wdt.c @@ -208,7 +208,7 @@ static int aspeed_wdt_restart(struct watchdog_device *wdd, wdt->ctrl &= ~WDT_CTRL_BOOT_SECONDARY; aspeed_wdt_enable(wdt, 128 * WDT_RATE_1MHZ / 1000); - mdelay(1000); + msleep(1000); return 0; }
Since it is not called in an atomic context the mdelay function can be replaced with msleep to avoid busy wait. Signed-off-by: Phil Eichinger <phil@zankapfel.net> --- drivers/watchdog/aspeed_wdt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)