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@xxxxxxx>
---
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;
