watchdog: sp805: Report correct timeleft at maximum

sp805_wdt::load_val is of type unsigned int. When the interrupt is
inactive wdt_timeleft adds one to the value, and then adds that to the
value used to calculate the time remaining. Unfortunately it's not
unlikely that load_val contains LOAD_MAX, which is 0xFFFFFFFF and wraps
to zero when one is added to it, resulting in the time left being
understated by about 21.7s.

It would be possible to widen load_val to 64-bit, or cast the value
before adding it, but it's easy to just cap the value one tick lower at
0xFFFFFFFE so that the addition is guaranteed to succeed. Add a
static_assert to ensure this remains true.

Signed-off-by: Mike Crowe <mac@mcrowe.com>
---
 drivers/watchdog/sp805_wdt.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Of course a simple cast to u64 would work just as well. Please let me
know if you'd like me to fix this in a different way.

On 12/2/24 13:43, Mike Crowe wrote:
> sp805_wdt::load_val is of type unsigned int. When the interrupt is
> inactive wdt_timeleft adds one to the value, and then adds that to the
> value used to calculate the time remaining. Unfortunately it's not
> unlikely that load_val contains LOAD_MAX, which is 0xFFFFFFFF and wraps
> to zero when one is added to it, resulting in the time left being
> understated by about 21.7s.
> 
> It would be possible to widen load_val to 64-bit, or cast the value
> before adding it, but it's easy to just cap the value one tick lower at
> 0xFFFFFFFE so that the addition is guaranteed to succeed. Add a
> static_assert to ensure this remains true.
> 
> Signed-off-by: Mike Crowe <mac@mcrowe.com>
> ---
>   drivers/watchdog/sp805_wdt.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> Of course a simple cast to u64 would work just as well. Please let me
> know if you'd like me to fix this in a different way.
> 
> diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
> index 109e2e37e8f0..1f9a7001d0d6 100644
> --- a/drivers/watchdog/sp805_wdt.c
> +++ b/drivers/watchdog/sp805_wdt.c
> @@ -39,7 +39,7 @@
>   /* watchdog register offsets and masks */
>   #define WDTLOAD			0x000
>   	#define LOAD_MIN	0x00000001
> -	#define LOAD_MAX	0xFFFFFFFF
> +	#define LOAD_MAX	0xFFFFFFFE
>   #define WDTVALUE		0x004
>   #define WDTCONTROL		0x008
>   	/* control register masks */
> @@ -127,8 +127,10 @@ static unsigned int wdt_timeleft(struct watchdog_device *wdd)
>   	load = readl_relaxed(wdt->base + WDTVALUE);
>   
>   	/*If the interrupt is inactive then time left is WDTValue + WDTLoad. */
> -	if (!(readl_relaxed(wdt->base + WDTRIS) & INT_MASK))
> +	if (!(readl_relaxed(wdt->base + WDTRIS) & INT_MASK)) {
> +		static_assert((__typeof__(wdt->load_val))LOAD_MAX + 1 != 0);

"load" is already an u64. A typecast of wdt->load_val to u64 in the assignment below
would be much simpler and much less confusing.

Guenter

>   		load += wdt->load_val + 1;
> +	}
>   	spin_unlock(&wdt->lock);
>   
>   	return div_u64(load, wdt->rate);

On 12/3/24 01:57, Mike Crowe wrote:
> sp805_wdt::load_val is of type unsigned int. When the interrupt is
> inactive wdt_timeleft adds one to the value, and then adds that to the
> value used to calculate the time remaining. Unfortunately it's not
> unlikely that load_val contains LOAD_MAX, which is 0xFFFFFFFF and wraps
> to zero when one is added to it, resulting in the time left being
> understated by about 21.7s. Fix this by ensuring the addition happens as
> 64-bit.
> 
> Signed-off-by: Mike Crowe <mac@mcrowe.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Please provide change logs in future patches.

Guenter

> ---
>   drivers/watchdog/sp805_wdt.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
> index 109e2e37e8f0..4400c439c718 100644
> --- a/drivers/watchdog/sp805_wdt.c
> +++ b/drivers/watchdog/sp805_wdt.c
> @@ -128,7 +128,7 @@ static unsigned int wdt_timeleft(struct watchdog_device *wdd)
>   
>   	/*If the interrupt is inactive then time left is WDTValue + WDTLoad. */
>   	if (!(readl_relaxed(wdt->base + WDTRIS) & INT_MASK))
> -		load += wdt->load_val + 1;
> +		load += (u64)wdt->load_val + 1;
>   	spin_unlock(&wdt->lock);
>   
>   	return div_u64(load, wdt->rate);