[PATCH] watchdog: imx2_wdg: Save the actual timeout value

[Guenter Roeck]

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 at roeck-us.net> <mailto:undefined>
> Date	6/15/2024 22:18
> To	LongQiang<lqking7735 at 163.com>,
> <mailto:lqking7735 at 163.com><wim at linux-watchdog.org> <mailto:wim at linux-watchdog.org>
> Cc	<shawnguo at kernel.org>,
> <mailto:shawnguo at kernel.org><s.hauer at pengutronix.de>,
> <mailto:s.hauer at pengutronix.de><kernel at pengutronix.de>,
> <mailto:kernel at pengutronix.de><festevam at gmail.com>,
> <mailto:festevam at gmail.com><linux-watchdog at vger.kernel.org>,
> <mailto:linux-watchdog at vger.kernel.org><imx at lists.linux.dev>,
> <mailto:imx at lists.linux.dev><linux-arm-kernel at lists.infradead.org> <mailto:linux-arm-kernel at lists.infradead.org>
> 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 at 163.com>
>     ---
>     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
>