| Message ID | 20240615141059.345076-1-lqking7735@163.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | watchdog: imx2_wdg: Save the actual timeout value | expand |
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@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
On 6/15/24 07:18, Guenter Roeck wrote: > 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@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. > To add more detail, ->timeout is the soft timeout, handled by the watchdog core. The start function calls imx2_wdt_set_timeout() again, which will set the chip timeout to no more than IMX2_WDT_MAX_TIME. So the claim that "the illegal timeout will take effect at 'start'" is simply not correct. Guenter
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@roeck-us.net> <mailto:undefined> > Date 6/15/2024 22:18 > To LongQiang<lqking7735@163.com>, > <mailto:lqking7735@163.com><wim@linux-watchdog.org> <mailto:wim@linux-watchdog.org> > Cc <shawnguo@kernel.org>, > <mailto:shawnguo@kernel.org><s.hauer@pengutronix.de>, > <mailto:s.hauer@pengutronix.de><kernel@pengutronix.de>, > <mailto:kernel@pengutronix.de><festevam@gmail.com>, > <mailto:festevam@gmail.com><linux-watchdog@vger.kernel.org>, > <mailto:linux-watchdog@vger.kernel.org><imx@lists.linux.dev>, > <mailto:imx@lists.linux.dev><linux-arm-kernel@lists.infradead.org> <mailto:linux-arm-kernel@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@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 >
On 6/15/24 08:51, L.Q wrote: > Step 1: > Call imx2_wdt_set_timeout with a timeout value greater than 128 > This illegal value will be stored in wdog->timeout This is not an illegal value because the driver sets max_hw_heartbeat_ms which lets the watchdog core handle timeout values exceeding the maximum timeout supported by the chip. [ ... ] > static inline void imx2_wdt_setup(struct watchdog_device *wdog) > { > struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog); > u32 val; > regmap_read(wdev->regmap, IMX2_WDT_WCR, &val); > /* Suspend timer in low power mode, write once-only */ > val |= IMX2_WDT_WCR_WDZST; > /* Strip the old watchdog Time-Out value */ > val &= ~IMX2_WDT_WCR_WT; > /* Generate internal chip-level reset if WDOG times out */ > if (!wdev->ext_reset) > val &= ~IMX2_WDT_WCR_WRE; > /* Or if external-reset assert WDOG_B reset only on time-out */ > else > val |= IMX2_WDT_WCR_WRE; > /* Keep Watchdog Disabled */ > val &= ~IMX2_WDT_WCR_WDE; > /* Set the watchdog's Time-Out value */ > val |= WDOG_SEC_TO_COUNT(wdog->timeout); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ That is the bug. It needs to be val |= WDOG_SEC_TO_COUNT(min(wdog->timeout, IMX2_WDT_MAX_TIME)); Guenter
On 6/15/24 09:21, Guenter Roeck wrote: > On 6/15/24 08:51, L.Q wrote: >> Step 1: >> Call imx2_wdt_set_timeout with a timeout value greater than 128 >> This illegal value will be stored in wdog->timeout > > This is not an illegal value because the driver sets max_hw_heartbeat_ms > which lets the watchdog core handle timeout values exceeding the maximum > timeout supported by the chip. > From Documentation/watchdog/watchdog-kernel-api.rst: * set_timeout: this routine checks and changes the timeout of the watchdog timer device. It returns 0 on success, -EINVAL for "parameter out of range" and -EIO for "could not write value to the watchdog". On success this routine should set the timeout value of the watchdog_device to the achieved timeout value (which may be different from the requested one because the watchdog does not necessarily have a 1 second resolution). Drivers implementing max_hw_heartbeat_ms set the hardware watchdog heartbeat ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ to the minimum of timeout and max_hw_heartbeat_ms. Those drivers set the ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ timeout value of the watchdog_device either to the requested timeout value ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ (if it is larger than max_hw_heartbeat_ms), or to the achieved timeout value. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ If any of this is difficult to understand, please feel free to suggest a better wording. Thanks, Guenter
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; }
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@163.com> --- drivers/watchdog/imx2_wdt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)