| Message ID | 20241027105323.93699-3-wahrenst@gmx.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | watchdog: imx7ulp_wdt: Improve readability | expand |
On 10/27/24 03:53, Stefan Wahren wrote: > The WDOG Timeout Value (TOVAL) is a 16 bit value, which is stored > at the beginning of a 32 bit register. So add a range check to > prevent writing in the reserved register area. > > Signed-off-by: Stefan Wahren <wahrenst@gmx.net> > --- > drivers/watchdog/imx7ulp_wdt.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c > index 0f92d2217088..a7574f9c9150 100644 > --- a/drivers/watchdog/imx7ulp_wdt.c > +++ b/drivers/watchdog/imx7ulp_wdt.c > @@ -48,6 +48,8 @@ > > #define RETRY_MAX 5 > > +#define TOVAL_MAX 0xFFFF > + > static bool nowayout = WATCHDOG_NOWAYOUT; > module_param(nowayout, bool, 0000); > MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=" > @@ -192,6 +194,9 @@ static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog, > int ret; > u32 loop = RETRY_MAX; > > + if (toval > TOVAL_MAX) > + return -EINVAL; > + The whole idea of having max_timeout in struct watchdog_device is to avoid the need for this check. max_timeout should be set to 0xffff / wdt->hw->wdog_clock_rate. It is currently set to 128. With wdt->hw->wdog_clock_rate set to either 125 or 1000, it can indeed overflow. However, checking the value above is wrong. max_timeout should be initialized correctly instead. Even better would be to set max_hw_heartbeat_ms and let the watchdog core handle larger timeouts. Another question is why the driver enables a clock but doesn't use its actual frequency. Guenter
Am 27.10.24 um 14:36 schrieb Guenter Roeck: > On 10/27/24 03:53, Stefan Wahren wrote: >> The WDOG Timeout Value (TOVAL) is a 16 bit value, which is stored >> at the beginning of a 32 bit register. So add a range check to >> prevent writing in the reserved register area. >> >> Signed-off-by: Stefan Wahren <wahrenst@gmx.net> >> --- >> drivers/watchdog/imx7ulp_wdt.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/watchdog/imx7ulp_wdt.c >> b/drivers/watchdog/imx7ulp_wdt.c >> index 0f92d2217088..a7574f9c9150 100644 >> --- a/drivers/watchdog/imx7ulp_wdt.c >> +++ b/drivers/watchdog/imx7ulp_wdt.c >> @@ -48,6 +48,8 @@ >> >> #define RETRY_MAX 5 >> >> +#define TOVAL_MAX 0xFFFF >> + >> static bool nowayout = WATCHDOG_NOWAYOUT; >> module_param(nowayout, bool, 0000); >> MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started >> (default=" >> @@ -192,6 +194,9 @@ static int imx7ulp_wdt_set_timeout(struct >> watchdog_device *wdog, >> int ret; >> u32 loop = RETRY_MAX; >> >> + if (toval > TOVAL_MAX) >> + return -EINVAL; >> + > > The whole idea of having max_timeout in struct watchdog_device is to > avoid the need > for this check. max_timeout should be set to 0xffff / > wdt->hw->wdog_clock_rate. > It is currently set to 128. With wdt->hw->wdog_clock_rate set to > either 125 or 1000, > it can indeed overflow. However, checking the value above is wrong. > max_timeout should > be initialized correctly instead. > > Even better would be to set max_hw_heartbeat_ms and let the watchdog > core handle > larger timeouts. It's funny because I tried this on a i.MX93 board but it didn't work for me. But I must confess that I didn't spend much time in the investigation. > > Another question is why the driver enables a clock but doesn't use its > actual > frequency. Yes, this would be better Regards > > Guenter > >
On 10/27/24 08:54, Stefan Wahren wrote: > Am 27.10.24 um 14:36 schrieb Guenter Roeck: >> On 10/27/24 03:53, Stefan Wahren wrote: >>> The WDOG Timeout Value (TOVAL) is a 16 bit value, which is stored >>> at the beginning of a 32 bit register. So add a range check to >>> prevent writing in the reserved register area. >>> >>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net> >>> --- >>> drivers/watchdog/imx7ulp_wdt.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/drivers/watchdog/imx7ulp_wdt.c >>> b/drivers/watchdog/imx7ulp_wdt.c >>> index 0f92d2217088..a7574f9c9150 100644 >>> --- a/drivers/watchdog/imx7ulp_wdt.c >>> +++ b/drivers/watchdog/imx7ulp_wdt.c >>> @@ -48,6 +48,8 @@ >>> >>> #define RETRY_MAX 5 >>> >>> +#define TOVAL_MAX 0xFFFF >>> + >>> static bool nowayout = WATCHDOG_NOWAYOUT; >>> module_param(nowayout, bool, 0000); >>> MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started >>> (default=" >>> @@ -192,6 +194,9 @@ static int imx7ulp_wdt_set_timeout(struct >>> watchdog_device *wdog, >>> int ret; >>> u32 loop = RETRY_MAX; >>> >>> + if (toval > TOVAL_MAX) >>> + return -EINVAL; >>> + >> >> The whole idea of having max_timeout in struct watchdog_device is to >> avoid the need >> for this check. max_timeout should be set to 0xffff / >> wdt->hw->wdog_clock_rate. >> It is currently set to 128. With wdt->hw->wdog_clock_rate set to >> either 125 or 1000, >> it can indeed overflow. However, checking the value above is wrong. >> max_timeout should >> be initialized correctly instead. >> >> Even better would be to set max_hw_heartbeat_ms and let the watchdog >> core handle >> larger timeouts. > It's funny because I tried this on a i.MX93 board but it didn't work for > me. But I must confess that I didn't spend much time in the investigation. I can't test it, but something like the diff below should do. Guenter --- diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c index 0f13a3053357..e672d27af63e 100644 --- a/drivers/watchdog/imx7ulp_wdt.c +++ b/drivers/watchdog/imx7ulp_wdt.c @@ -187,11 +187,16 @@ static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog, unsigned int timeout) { struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog); - u32 toval = wdt->hw->wdog_clock_rate * timeout; + u32 toval; u32 val; int ret; u32 loop = RETRY_MAX; + if (timeout > 0xffff / wdt->hw->wdog_clock_rate) + toval = 0xffff; + else + toval = wdt->hw->wdog_clock_rate * timeout; + do { ret = _imx7ulp_wdt_set_timeout(wdt, toval); val = readl(wdt->base + WDOG_TOVAL); @@ -338,7 +343,6 @@ static int imx7ulp_wdt_probe(struct platform_device *pdev) wdog->info = &imx7ulp_wdt_info; wdog->ops = &imx7ulp_wdt_ops; wdog->min_timeout = 1; - wdog->max_timeout = MAX_TIMEOUT; wdog->parent = dev; wdog->timeout = DEFAULT_TIMEOUT; @@ -348,6 +352,7 @@ static int imx7ulp_wdt_probe(struct platform_device *pdev) watchdog_set_drvdata(wdog, imx7ulp_wdt); imx7ulp_wdt->hw = of_device_get_match_data(dev); + wdog->max_hw_heartbeat_ms = 0xffff * 1000 / imx7ulp_wdt->hw->wdog_clock_rate; ret = imx7ulp_wdt_init(imx7ulp_wdt, wdog->timeout * imx7ulp_wdt->hw->wdog_clock_rate); if (ret) return ret;
Hi Guenter, Am 27.10.24 um 18:35 schrieb Guenter Roeck: > On 10/27/24 08:54, Stefan Wahren wrote: >> Am 27.10.24 um 14:36 schrieb Guenter Roeck: >>> On 10/27/24 03:53, Stefan Wahren wrote: >>>> The WDOG Timeout Value (TOVAL) is a 16 bit value, which is stored >>>> at the beginning of a 32 bit register. So add a range check to >>>> prevent writing in the reserved register area. >>>> >>>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net> >>>> --- >>>> drivers/watchdog/imx7ulp_wdt.c | 8 ++++++++ >>>> 1 file changed, 8 insertions(+) >>>> >>>> diff --git a/drivers/watchdog/imx7ulp_wdt.c >>>> b/drivers/watchdog/imx7ulp_wdt.c >>>> index 0f92d2217088..a7574f9c9150 100644 >>>> --- a/drivers/watchdog/imx7ulp_wdt.c >>>> +++ b/drivers/watchdog/imx7ulp_wdt.c >>>> @@ -48,6 +48,8 @@ >>>> >>>> #define RETRY_MAX 5 >>>> >>>> +#define TOVAL_MAX 0xFFFF >>>> + >>>> static bool nowayout = WATCHDOG_NOWAYOUT; >>>> module_param(nowayout, bool, 0000); >>>> MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started >>>> (default=" >>>> @@ -192,6 +194,9 @@ static int imx7ulp_wdt_set_timeout(struct >>>> watchdog_device *wdog, >>>> int ret; >>>> u32 loop = RETRY_MAX; >>>> >>>> + if (toval > TOVAL_MAX) >>>> + return -EINVAL; >>>> + >>> >>> The whole idea of having max_timeout in struct watchdog_device is to >>> avoid the need >>> for this check. max_timeout should be set to 0xffff / >>> wdt->hw->wdog_clock_rate. >>> It is currently set to 128. With wdt->hw->wdog_clock_rate set to >>> either 125 or 1000, >>> it can indeed overflow. However, checking the value above is wrong. >>> max_timeout should >>> be initialized correctly instead. >>> >>> Even better would be to set max_hw_heartbeat_ms and let the watchdog >>> core handle >>> larger timeouts. >> It's funny because I tried this on a i.MX93 board but it didn't work for >> me. But I must confess that I didn't spend much time in the >> investigation. > > I can't test it, but something like the diff below should do. Thanks, I will give it a try but it will take some time. Regards > > Guenter > > --- > diff --git a/drivers/watchdog/imx7ulp_wdt.c > b/drivers/watchdog/imx7ulp_wdt.c > index 0f13a3053357..e672d27af63e 100644 > --- a/drivers/watchdog/imx7ulp_wdt.c > +++ b/drivers/watchdog/imx7ulp_wdt.c > @@ -187,11 +187,16 @@ static int imx7ulp_wdt_set_timeout(struct > watchdog_device *wdog, > unsigned int timeout) > { > struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog); > - u32 toval = wdt->hw->wdog_clock_rate * timeout; > + u32 toval; > u32 val; > int ret; > u32 loop = RETRY_MAX; > > + if (timeout > 0xffff / wdt->hw->wdog_clock_rate) > + toval = 0xffff; > + else > + toval = wdt->hw->wdog_clock_rate * timeout; > + > do { > ret = _imx7ulp_wdt_set_timeout(wdt, toval); > val = readl(wdt->base + WDOG_TOVAL); > @@ -338,7 +343,6 @@ static int imx7ulp_wdt_probe(struct > platform_device *pdev) > wdog->info = &imx7ulp_wdt_info; > wdog->ops = &imx7ulp_wdt_ops; > wdog->min_timeout = 1; > - wdog->max_timeout = MAX_TIMEOUT; > wdog->parent = dev; > wdog->timeout = DEFAULT_TIMEOUT; > > @@ -348,6 +352,7 @@ static int imx7ulp_wdt_probe(struct > platform_device *pdev) > watchdog_set_drvdata(wdog, imx7ulp_wdt); > > imx7ulp_wdt->hw = of_device_get_match_data(dev); > + wdog->max_hw_heartbeat_ms = 0xffff * 1000 / > imx7ulp_wdt->hw->wdog_clock_rate; > ret = imx7ulp_wdt_init(imx7ulp_wdt, wdog->timeout * > imx7ulp_wdt->hw->wdog_clock_rate); > if (ret) > return ret; > >
diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c index 0f92d2217088..a7574f9c9150 100644 --- a/drivers/watchdog/imx7ulp_wdt.c +++ b/drivers/watchdog/imx7ulp_wdt.c @@ -48,6 +48,8 @@ #define RETRY_MAX 5 +#define TOVAL_MAX 0xFFFF + static bool nowayout = WATCHDOG_NOWAYOUT; module_param(nowayout, bool, 0000); MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=" @@ -192,6 +194,9 @@ static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog, int ret; u32 loop = RETRY_MAX; + if (toval > TOVAL_MAX) + return -EINVAL; + do { ret = _imx7ulp_wdt_set_timeout(wdt, toval); val = readl(wdt->base + WDOG_TOVAL); @@ -286,6 +291,9 @@ static int imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, int ret; u32 loop = RETRY_MAX; + if (timeout_clks > TOVAL_MAX) + return -EINVAL; + if (wdt->hw->prescaler_enable) val |= WDOG_CS_PRES;
The WDOG Timeout Value (TOVAL) is a 16 bit value, which is stored at the beginning of a 32 bit register. So add a range check to prevent writing in the reserved register area. Signed-off-by: Stefan Wahren <wahrenst@gmx.net> --- drivers/watchdog/imx7ulp_wdt.c | 8 ++++++++ 1 file changed, 8 insertions(+) -- 2.34.1