On 6/15/24 08:14, L.Q wrote:
Well
If I first set a timeout value greater than 128 and then start the watchdog, the watchdog timeout value is illegal.
In the function 'imx2_wdt_start', there is no validity check on the timeout value
static int imx2_wdt_start(struct watchdog_device *wdog)
{
struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
if (imx2_wdt_is_running(wdev))
imx2_wdt_set_timeout(wdog, wdog->timeout);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
else
imx2_wdt_setup(wdog);
...
static int imx2_wdt_set_timeout(struct watchdog_device *wdog,
unsigned int new_timeout)
{
unsigned int actual;
actual = min(new_timeout, IMX2_WDT_MAX_TIME);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
__imx2_wdt_set_timeout(wdog, actual);
^^^^^^
wdog->timeout = new_timeout;
return 0;
}
Please point out the code path where an attempt is made to write
a value larger than IMX2_WDT_MAX_TIME into the chip.
Guenter
---- Replied Message ----
From Guenter Roeck<linux@xxxxxxxxxxxx> <mailto:undefined>
Date 6/15/2024 22:18
To LongQiang<lqking7735@xxxxxxx>,
<mailto:lqking7735@xxxxxxx><wim@xxxxxxxxxxxxxxxxxx> <mailto:wim@xxxxxxxxxxxxxxxxxx>
Cc <shawnguo@xxxxxxxxxx>,
<mailto:shawnguo@xxxxxxxxxx><s.hauer@xxxxxxxxxxxxxx>,
<mailto:s.hauer@xxxxxxxxxxxxxx><kernel@xxxxxxxxxxxxxx>,
<mailto:kernel@xxxxxxxxxxxxxx><festevam@xxxxxxxxx>,
<mailto:festevam@xxxxxxxxx><linux-watchdog@xxxxxxxxxxxxxxx>,
<mailto:linux-watchdog@xxxxxxxxxxxxxxx><imx@xxxxxxxxxxxxxxx>,
<mailto:imx@xxxxxxxxxxxxxxx><linux-arm-kernel@xxxxxxxxxxxxxxxxxxx> <mailto:linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>
Subject Re: [PATCH] watchdog: imx2_wdg: Save the actual timeout value
On 6/15/24 07:10, LongQiang wrote:
When setting the timeout, the effective timeout value should be saved.
Otherwise, the illegal timeout will take effect at 'start'.
Signed-off-by: LongQiang <lqking7735@xxxxxxx>
---
drivers/watchdog/imx2_wdt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index 42e8ffae18dd..d4a4d4c58c3f 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -196,7 +196,7 @@ static int imx2_wdt_set_timeout(struct watchdog_device *wdog,
actual = min(new_timeout, IMX2_WDT_MAX_TIME);
__imx2_wdt_set_timeout(wdog, actual);
- wdog->timeout = new_timeout;
+ wdog->timeout = actual;
return 0;
}
No, that would be wrong.
NACK.
Guenter
