[PATCH] watchdog: gpio_wdt: use __udelay() to avoid recursion

[PATCH] watchdog: gpio_wdt: use __udelay() to avoid recursion

[Rasmus Villemoes]

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;
 	}
-- 
2.37.2

Re: [PATCH] watchdog: gpio_wdt: use __udelay() to avoid recursion

[Stefan Roese]

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

Re: [PATCH] watchdog: gpio_wdt: use __udelay() to avoid recursion

[Pali Rohár]

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/

Re: [PATCH] watchdog: gpio_wdt: use __udelay() to avoid recursion

[Stefan Roese]

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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de