| Message ID | 20241202214343.2425341-1-mac@mcrowe.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | watchdog: sp805: Report correct timeleft at maximum | expand |
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);
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 += wdt->load_val + 1; + } spin_unlock(&wdt->lock); return div_u64(load, wdt->rate);
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.