| Message ID | 20230207044603.1011555-1-george.cherian@marvell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] watchdog: sbsa_wdog: Fix the timeout calculation while programming | expand |
On 2/6/23 20:46, George Cherian wrote: > The time out calculation done in sbsa_gwdt_set_timeout() would always > return a 32-bit value. Use proper typecasting to make sure the overflow > values are captured. > > Fixes: abd3ac7902fb ("watchdog: sbsa: Support architecture version 1") > > Signed-off-by: George Cherian <george.cherian@marvell.com> > --- > drivers/watchdog/sbsa_gwdt.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c > index 9791c74aebd4..aaa3f5631f29 100644 > --- a/drivers/watchdog/sbsa_gwdt.c > +++ b/drivers/watchdog/sbsa_gwdt.c > @@ -152,14 +152,14 @@ static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd, > wdd->timeout = timeout; > > if (action) > - sbsa_gwdt_reg_write(gwdt->clk * timeout, gwdt); > + sbsa_gwdt_reg_write((u64)gwdt->clk * (u64)timeout, gwdt); > else > /* > * In the single stage mode, The first signal (WS0) is ignored, > * the timeout is (WOR * 2), so the WOR should be configured > * to half value of timeout. > */ > - sbsa_gwdt_reg_write(gwdt->clk / 2 * timeout, gwdt); > + sbsa_gwdt_reg_write((u64)gwdt->clk / 2 * (u64)timeout, gwdt); > > return 0; > } The driver sets max_hw_heartbeat_ms. It is its responsibility to clamp the timeout value written into the controller to the configured limit to avoid confusing the watchdog core. Something like timeout = clamp(timeout, 0, wdd->max_hw_heartbeat_ms / 1000); This also solves the problem in patch 2 since it guarantees that the resulting register value is <= U32_MAX for version 0. Guenter
> -----Original Message----- > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > Sent: Tuesday, February 7, 2023 9:34 PM > To: George Cherian <gcherian@marvell.com>; wim@linux-watchdog.org; > zhangshaokun@hisilicon.com > Cc: linux-watchdog@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH 1/2] watchdog: sbsa_wdog: Fix the timeout > calculation while programming > > > ---------------------------------------------------------------------- > On 2/6/23 20:46, George Cherian wrote: > > The time out calculation done in sbsa_gwdt_set_timeout() would always > > return a 32-bit value. Use proper typecasting to make sure the overflow > > values are captured. > > > > Fixes: abd3ac7902fb ("watchdog: sbsa: Support architecture version 1") > > > > Signed-off-by: George Cherian <george.cherian@marvell.com> > > --- > > drivers/watchdog/sbsa_gwdt.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/watchdog/sbsa_gwdt.c > b/drivers/watchdog/sbsa_gwdt.c > > index 9791c74aebd4..aaa3f5631f29 100644 > > --- a/drivers/watchdog/sbsa_gwdt.c > > +++ b/drivers/watchdog/sbsa_gwdt.c > > @@ -152,14 +152,14 @@ static int sbsa_gwdt_set_timeout(struct > watchdog_device *wdd, > > wdd->timeout = timeout; > > > > if (action) > > - sbsa_gwdt_reg_write(gwdt->clk * timeout, gwdt); > > + sbsa_gwdt_reg_write((u64)gwdt->clk * (u64)timeout, gwdt); > > else > > /* > > * In the single stage mode, The first signal (WS0) is ignored, > > * the timeout is (WOR * 2), so the WOR should be > configured > > * to half value of timeout. > > */ > > - sbsa_gwdt_reg_write(gwdt->clk / 2 * timeout, gwdt); > > + sbsa_gwdt_reg_write((u64)gwdt->clk / 2 * (u64)timeout, > gwdt); > > > > return 0; > > } > > The driver sets max_hw_heartbeat_ms. It is its responsibility to clamp > the timeout value written into the controller to the configured limit > to avoid confusing the watchdog core. Something like > > timeout = clamp(timeout, 0, wdd->max_hw_heartbeat_ms / 1000); > > This also solves the problem in patch 2 since it guarantees that the > resulting register value is <= U32_MAX for version 0. Thanks for the review. I will update accordingly and post a v2. > > Guenter Regards, -George
diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c index 9791c74aebd4..aaa3f5631f29 100644 --- a/drivers/watchdog/sbsa_gwdt.c +++ b/drivers/watchdog/sbsa_gwdt.c @@ -152,14 +152,14 @@ static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd, wdd->timeout = timeout; if (action) - sbsa_gwdt_reg_write(gwdt->clk * timeout, gwdt); + sbsa_gwdt_reg_write((u64)gwdt->clk * (u64)timeout, gwdt); else /* * In the single stage mode, The first signal (WS0) is ignored, * the timeout is (WOR * 2), so the WOR should be configured * to half value of timeout. */ - sbsa_gwdt_reg_write(gwdt->clk / 2 * timeout, gwdt); + sbsa_gwdt_reg_write((u64)gwdt->clk / 2 * (u64)timeout, gwdt); return 0; }
The time out calculation done in sbsa_gwdt_set_timeout() would always return a 32-bit value. Use proper typecasting to make sure the overflow values are captured. Fixes: abd3ac7902fb ("watchdog: sbsa: Support architecture version 1") Signed-off-by: George Cherian <george.cherian@marvell.com> --- drivers/watchdog/sbsa_gwdt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)