| Message ID | 20220927074544.207443-1-rasmus.villemoes@prevas.dk |
|---|---|
| State | Accepted |
| Commit | 51443c9a499989f698a2921e72156713fe1a6bc2 |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | watchdog: gpio_wdt: use __udelay() to avoid recursion | expand |
Hi Rasmus, (add Pali to Cc) On 27.09.22 09:45, Rasmus Villemoes wrote: > The udelay() function in lib/time.c contains a WATCHDOG_RESET() > call. The only reason this doesn't lead to a catastrophic infinite > recursion is due to the rate-limiting in wdt-uclass.c: > > if (time_after_eq(now, priv->next_reset)) { > priv->next_reset = now + priv->reset_period; > wdt_reset(dev); > } > > But this would fall apart if ->next_reset was updated after calling the > device's reset method. > > This is needlessly fragile, and it's easy enough to avoid that > recursion in the first place by just using __udelay() directly. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- > drivers/watchdog/gpio_wdt.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c > index fe06ec8cc9..2920c2c751 100644 > --- a/drivers/watchdog/gpio_wdt.c > +++ b/drivers/watchdog/gpio_wdt.c > @@ -31,7 +31,7 @@ static int gpio_wdt_reset(struct udevice *dev) > case HW_ALGO_LEVEL: > /* Pulse */ > dm_gpio_set_value(&priv->gpio, 1); > - udelay(1); > + __udelay(1); > dm_gpio_set_value(&priv->gpio, 0); > break; > } Yes, good catch: Reviewed-by: Stefan Roese <sr@denx.de> The max6370_wdt.c driver has the same problem AFAICT. Pali, could you please take a look a this? Thanks, Stefan
On Tuesday 27 September 2022 10:00:00 Stefan Roese wrote: > The max6370_wdt.c driver has the same problem AFAICT. Pali, could you > please take a look a this? Done, here is patch: https://patchwork.ozlabs.org/project/uboot/patch/20220927101919.6999-1-pali@kernel.org/
On 27.09.22 09:45, Rasmus Villemoes wrote: > The udelay() function in lib/time.c contains a WATCHDOG_RESET() > call. The only reason this doesn't lead to a catastrophic infinite > recursion is due to the rate-limiting in wdt-uclass.c: > > if (time_after_eq(now, priv->next_reset)) { > priv->next_reset = now + priv->reset_period; > wdt_reset(dev); > } > > But this would fall apart if ->next_reset was updated after calling the > device's reset method. > > This is needlessly fragile, and it's easy enough to avoid that > recursion in the first place by just using __udelay() directly. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> Applied to u-boot-watchdog/master Thanks, Stefan > --- > drivers/watchdog/gpio_wdt.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c > index fe06ec8cc9..2920c2c751 100644 > --- a/drivers/watchdog/gpio_wdt.c > +++ b/drivers/watchdog/gpio_wdt.c > @@ -31,7 +31,7 @@ static int gpio_wdt_reset(struct udevice *dev) > case HW_ALGO_LEVEL: > /* Pulse */ > dm_gpio_set_value(&priv->gpio, 1); > - udelay(1); > + __udelay(1); > dm_gpio_set_value(&priv->gpio, 0); > break; > } Viele Grüße, Stefan Roese
diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c index fe06ec8cc9..2920c2c751 100644 --- a/drivers/watchdog/gpio_wdt.c +++ b/drivers/watchdog/gpio_wdt.c @@ -31,7 +31,7 @@ static int gpio_wdt_reset(struct udevice *dev) case HW_ALGO_LEVEL: /* Pulse */ dm_gpio_set_value(&priv->gpio, 1); - udelay(1); + __udelay(1); dm_gpio_set_value(&priv->gpio, 0); break; }
The udelay() function in lib/time.c contains a WATCHDOG_RESET() call. The only reason this doesn't lead to a catastrophic infinite recursion is due to the rate-limiting in wdt-uclass.c: if (time_after_eq(now, priv->next_reset)) { priv->next_reset = now + priv->reset_period; wdt_reset(dev); } But this would fall apart if ->next_reset was updated after calling the device's reset method. This is needlessly fragile, and it's easy enough to avoid that recursion in the first place by just using __udelay() directly. Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- drivers/watchdog/gpio_wdt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)