* [PATCH 0/2] watchdog: imx7ulp_wdt: Improve readability @ 2024-10-27 10:53 Stefan Wahren 2024-10-27 10:53 ` [PATCH 1/2] watchdog: imx7ulp_wdt: Clarify timing units Stefan Wahren 2024-10-27 10:53 ` [PATCH 2/2] watchdog: imx7ulp_wdt: Add TOVAL range check Stefan Wahren 0 siblings, 2 replies; 9+ messages in thread From: Stefan Wahren @ 2024-10-27 10:53 UTC (permalink / raw) To: Alice Guo, Wim Van Sebroeck, Guenter Roeck, Shawn Guo, Sascha Hauer, Fabio Estevam Cc: Pengutronix Kernel Team, linux-watchdog, imx, linux-arm-kernel, Stefan Wahren This small series tries to improve the readability of imx7ulp_wdt. For someone who didn't read the reference manual, some parts of the watchdog driver are not clear. Stefan Wahren (2): watchdog: imx7ulp_wdt: Clarify timing units watchdog: imx7ulp_wdt: Add TOVAL range check drivers/watchdog/imx7ulp_wdt.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] watchdog: imx7ulp_wdt: Clarify timing units 2024-10-27 10:53 [PATCH 0/2] watchdog: imx7ulp_wdt: Improve readability Stefan Wahren @ 2024-10-27 10:53 ` Stefan Wahren 2024-10-27 14:28 ` Guenter Roeck 2024-10-27 10:53 ` [PATCH 2/2] watchdog: imx7ulp_wdt: Add TOVAL range check Stefan Wahren 1 sibling, 1 reply; 9+ messages in thread From: Stefan Wahren @ 2024-10-27 10:53 UTC (permalink / raw) To: Alice Guo, Wim Van Sebroeck, Guenter Roeck, Shawn Guo, Sascha Hauer, Fabio Estevam Cc: Pengutronix Kernel Team, linux-watchdog, imx, linux-arm-kernel, Stefan Wahren imx7ulp_wdt mixes a lot of timing units (frequency, clocks, seconds) in a not obvious way. So improve readability of imx7ulp_wdt by clarifying the relevant units. Signed-off-by: Stefan Wahren <wahrenst@gmx.net> --- drivers/watchdog/imx7ulp_wdt.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c index 0f13a3053357..0f92d2217088 100644 --- a/drivers/watchdog/imx7ulp_wdt.c +++ b/drivers/watchdog/imx7ulp_wdt.c @@ -19,7 +19,7 @@ #define WDOG_CS_PRES BIT(12) #define WDOG_CS_ULK BIT(11) #define WDOG_CS_RCS BIT(10) -#define LPO_CLK 0x1 +#define LPO_CLK 0x1 /* 32 kHz */ #define LPO_CLK_SHIFT 8 #define WDOG_CS_CLK (LPO_CLK << LPO_CLK_SHIFT) #define WDOG_CS_EN BIT(7) @@ -39,8 +39,8 @@ #define UNLOCK_SEQ1 0xD928 #define UNLOCK ((UNLOCK_SEQ1 << 16) | UNLOCK_SEQ0) -#define DEFAULT_TIMEOUT 60 -#define MAX_TIMEOUT 128 +#define DEFAULT_TIMEOUT 60 /* seconds */ +#define MAX_TIMEOUT 128 /* seconds */ #define WDOG_CLOCK_RATE 1000 #define WDOG_ULK_WAIT_TIMEOUT 1000 #define WDOG_RCS_WAIT_TIMEOUT 10000 @@ -240,7 +240,8 @@ static const struct watchdog_info imx7ulp_wdt_info = { WDIOF_MAGICCLOSE, }; -static int _imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, unsigned int timeout, unsigned int cs) +static int _imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, + unsigned int timeout_clks, unsigned int cs) { u32 val; int ret; @@ -263,7 +264,7 @@ static int _imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, unsigned int timeou goto init_out; /* set an initial timeout value in TOVAL */ - writel(timeout, wdt->base + WDOG_TOVAL); + writel(timeout_clks, wdt->base + WDOG_TOVAL); writel(cs, wdt->base + WDOG_CS); local_irq_enable(); ret = imx7ulp_wdt_wait_rcs(wdt); @@ -275,7 +276,8 @@ static int _imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, unsigned int timeou return ret; } -static int imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, unsigned int timeout) +static int imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, + unsigned int timeout_clks) { /* enable 32bit command sequence and reconfigure */ u32 val = WDOG_CS_CMD32EN | WDOG_CS_CLK | WDOG_CS_UPDATE | @@ -296,11 +298,11 @@ static int imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, unsigned int timeout } do { - ret = _imx7ulp_wdt_init(wdt, timeout, val); + ret = _imx7ulp_wdt_init(wdt, timeout_clks, val); toval = readl(wdt->base + WDOG_TOVAL); cs = readl(wdt->base + WDOG_CS); cs &= ~(WDOG_CS_FLG | WDOG_CS_ULK | WDOG_CS_RCS); - } while (--loop > 0 && (cs != val || toval != timeout || ret)); + } while (--loop > 0 && (cs != val || toval != timeout_clks || ret)); if (loop == 0) return -EBUSY; -- 2.34.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] watchdog: imx7ulp_wdt: Clarify timing units 2024-10-27 10:53 ` [PATCH 1/2] watchdog: imx7ulp_wdt: Clarify timing units Stefan Wahren @ 2024-10-27 14:28 ` Guenter Roeck 2024-10-27 15:56 ` Stefan Wahren 0 siblings, 1 reply; 9+ messages in thread From: Guenter Roeck @ 2024-10-27 14:28 UTC (permalink / raw) To: Stefan Wahren, Alice Guo, Wim Van Sebroeck, Shawn Guo, Sascha Hauer, Fabio Estevam Cc: Pengutronix Kernel Team, linux-watchdog, imx, linux-arm-kernel On 10/27/24 03:53, Stefan Wahren wrote: > imx7ulp_wdt mixes a lot of timing units (frequency, clocks, seconds) > in a not obvious way. So improve readability of imx7ulp_wdt by > clarifying the relevant units. > > Signed-off-by: Stefan Wahren <wahrenst@gmx.net> > --- > drivers/watchdog/imx7ulp_wdt.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c > index 0f13a3053357..0f92d2217088 100644 > --- a/drivers/watchdog/imx7ulp_wdt.c > +++ b/drivers/watchdog/imx7ulp_wdt.c > @@ -19,7 +19,7 @@ > #define WDOG_CS_PRES BIT(12) > #define WDOG_CS_ULK BIT(11) > #define WDOG_CS_RCS BIT(10) > -#define LPO_CLK 0x1 > +#define LPO_CLK 0x1 /* 32 kHz */ This configures the clock source to be the LPO clock, which according to the chip datasheets is a 1kHz clock for all chips except IMX93. It is only 32kHz for IMX93, and the prescaler is enabled for that chip. The comment is not only misleading because it selects the clock source, not the rate, but wrong, because it only selects a 32kHz clock for IMX93, and that value is then prescaled. > #define LPO_CLK_SHIFT 8 > #define WDOG_CS_CLK (LPO_CLK << LPO_CLK_SHIFT) > #define WDOG_CS_EN BIT(7) > @@ -39,8 +39,8 @@ > #define UNLOCK_SEQ1 0xD928 > #define UNLOCK ((UNLOCK_SEQ1 << 16) | UNLOCK_SEQ0) > > -#define DEFAULT_TIMEOUT 60 > -#define MAX_TIMEOUT 128 > +#define DEFAULT_TIMEOUT 60 /* seconds */ > +#define MAX_TIMEOUT 128 /* seconds */ > #define WDOG_CLOCK_RATE 1000 > #define WDOG_ULK_WAIT_TIMEOUT 1000 > #define WDOG_RCS_WAIT_TIMEOUT 10000 > @@ -240,7 +240,8 @@ static const struct watchdog_info imx7ulp_wdt_info = { > WDIOF_MAGICCLOSE, > }; > > -static int _imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, unsigned int timeout, unsigned int cs) > +static int _imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, > + unsigned int timeout_clks, unsigned int cs) I don't think "_clks" adds any clarity because the value doesn't set a "clock". "_ticks", maybe. > { > u32 val; > int ret; > @@ -263,7 +264,7 @@ static int _imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, unsigned int timeou > goto init_out; > > /* set an initial timeout value in TOVAL */ > - writel(timeout, wdt->base + WDOG_TOVAL); > + writel(timeout_clks, wdt->base + WDOG_TOVAL); > writel(cs, wdt->base + WDOG_CS); > local_irq_enable(); > ret = imx7ulp_wdt_wait_rcs(wdt); > @@ -275,7 +276,8 @@ static int _imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, unsigned int timeou > return ret; > } > > -static int imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, unsigned int timeout) > +static int imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, > + unsigned int timeout_clks) > { > /* enable 32bit command sequence and reconfigure */ > u32 val = WDOG_CS_CMD32EN | WDOG_CS_CLK | WDOG_CS_UPDATE | > @@ -296,11 +298,11 @@ static int imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, unsigned int timeout > } > > do { > - ret = _imx7ulp_wdt_init(wdt, timeout, val); > + ret = _imx7ulp_wdt_init(wdt, timeout_clks, val); > toval = readl(wdt->base + WDOG_TOVAL); > cs = readl(wdt->base + WDOG_CS); > cs &= ~(WDOG_CS_FLG | WDOG_CS_ULK | WDOG_CS_RCS); > - } while (--loop > 0 && (cs != val || toval != timeout || ret)); > + } while (--loop > 0 && (cs != val || toval != timeout_clks || ret)); > > if (loop == 0) > return -EBUSY; > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] watchdog: imx7ulp_wdt: Clarify timing units 2024-10-27 14:28 ` Guenter Roeck @ 2024-10-27 15:56 ` Stefan Wahren 0 siblings, 0 replies; 9+ messages in thread From: Stefan Wahren @ 2024-10-27 15:56 UTC (permalink / raw) To: Guenter Roeck, Alice Guo, Wim Van Sebroeck, Shawn Guo, Sascha Hauer, Fabio Estevam Cc: Pengutronix Kernel Team, linux-watchdog, imx, linux-arm-kernel Am 27.10.24 um 15:28 schrieb Guenter Roeck: > On 10/27/24 03:53, Stefan Wahren wrote: >> imx7ulp_wdt mixes a lot of timing units (frequency, clocks, seconds) >> in a not obvious way. So improve readability of imx7ulp_wdt by >> clarifying the relevant units. >> >> Signed-off-by: Stefan Wahren <wahrenst@gmx.net> >> --- >> drivers/watchdog/imx7ulp_wdt.c | 18 ++++++++++-------- >> 1 file changed, 10 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/watchdog/imx7ulp_wdt.c >> b/drivers/watchdog/imx7ulp_wdt.c >> index 0f13a3053357..0f92d2217088 100644 >> --- a/drivers/watchdog/imx7ulp_wdt.c >> +++ b/drivers/watchdog/imx7ulp_wdt.c >> @@ -19,7 +19,7 @@ >> #define WDOG_CS_PRES BIT(12) >> #define WDOG_CS_ULK BIT(11) >> #define WDOG_CS_RCS BIT(10) >> -#define LPO_CLK 0x1 >> +#define LPO_CLK 0x1 /* 32 kHz */ > > This configures the clock source to be the LPO clock, which according > to the > chip datasheets is a 1kHz clock for all chips except IMX93. It is only > 32kHz > for IMX93, and the prescaler is enabled for that chip. > > The comment is not only misleading because it selects the clock source, > not the rate, but wrong, because it only selects a 32kHz clock for IMX93, > and that value is then prescaled. Okay, I will drop it > >> #define LPO_CLK_SHIFT 8 >> #define WDOG_CS_CLK (LPO_CLK << LPO_CLK_SHIFT) >> #define WDOG_CS_EN BIT(7) >> @@ -39,8 +39,8 @@ >> #define UNLOCK_SEQ1 0xD928 >> #define UNLOCK ((UNLOCK_SEQ1 << 16) | UNLOCK_SEQ0) >> >> -#define DEFAULT_TIMEOUT 60 >> -#define MAX_TIMEOUT 128 >> +#define DEFAULT_TIMEOUT 60 /* seconds */ >> +#define MAX_TIMEOUT 128 /* seconds */ >> #define WDOG_CLOCK_RATE 1000 >> #define WDOG_ULK_WAIT_TIMEOUT 1000 >> #define WDOG_RCS_WAIT_TIMEOUT 10000 >> @@ -240,7 +240,8 @@ static const struct watchdog_info >> imx7ulp_wdt_info = { >> WDIOF_MAGICCLOSE, >> }; >> >> -static int _imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, >> unsigned int timeout, unsigned int cs) >> +static int _imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, >> + unsigned int timeout_clks, unsigned int cs) > > I don't think "_clks" adds any clarity because the value doesn't set a > "clock". > "_ticks", maybe. I'm fine with _ticks Thanks > >> { >> u32 val; >> int ret; >> @@ -263,7 +264,7 @@ static int _imx7ulp_wdt_init(struct >> imx7ulp_wdt_device *wdt, unsigned int timeou >> goto init_out; >> >> /* set an initial timeout value in TOVAL */ >> - writel(timeout, wdt->base + WDOG_TOVAL); >> + writel(timeout_clks, wdt->base + WDOG_TOVAL); >> writel(cs, wdt->base + WDOG_CS); >> local_irq_enable(); >> ret = imx7ulp_wdt_wait_rcs(wdt); >> @@ -275,7 +276,8 @@ static int _imx7ulp_wdt_init(struct >> imx7ulp_wdt_device *wdt, unsigned int timeou >> return ret; >> } >> >> -static int imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, unsigned >> int timeout) >> +static int imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, >> + unsigned int timeout_clks) >> { >> /* enable 32bit command sequence and reconfigure */ >> u32 val = WDOG_CS_CMD32EN | WDOG_CS_CLK | WDOG_CS_UPDATE | >> @@ -296,11 +298,11 @@ static int imx7ulp_wdt_init(struct >> imx7ulp_wdt_device *wdt, unsigned int timeout >> } >> >> do { >> - ret = _imx7ulp_wdt_init(wdt, timeout, val); >> + ret = _imx7ulp_wdt_init(wdt, timeout_clks, val); >> toval = readl(wdt->base + WDOG_TOVAL); >> cs = readl(wdt->base + WDOG_CS); >> cs &= ~(WDOG_CS_FLG | WDOG_CS_ULK | WDOG_CS_RCS); >> - } while (--loop > 0 && (cs != val || toval != timeout || ret)); >> + } while (--loop > 0 && (cs != val || toval != timeout_clks || >> ret)); >> >> if (loop == 0) >> return -EBUSY; >> -- >> 2.34.1 >> > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] watchdog: imx7ulp_wdt: Add TOVAL range check 2024-10-27 10:53 [PATCH 0/2] watchdog: imx7ulp_wdt: Improve readability Stefan Wahren 2024-10-27 10:53 ` [PATCH 1/2] watchdog: imx7ulp_wdt: Clarify timing units Stefan Wahren @ 2024-10-27 10:53 ` Stefan Wahren 2024-10-27 13:36 ` Guenter Roeck 1 sibling, 1 reply; 9+ messages in thread From: Stefan Wahren @ 2024-10-27 10:53 UTC (permalink / raw) To: Alice Guo, Wim Van Sebroeck, Guenter Roeck, Shawn Guo, Sascha Hauer, Fabio Estevam Cc: Pengutronix Kernel Team, linux-watchdog, imx, linux-arm-kernel, Stefan Wahren 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; + 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; -- 2.34.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] watchdog: imx7ulp_wdt: Add TOVAL range check 2024-10-27 10:53 ` [PATCH 2/2] watchdog: imx7ulp_wdt: Add TOVAL range check Stefan Wahren @ 2024-10-27 13:36 ` Guenter Roeck 2024-10-27 15:54 ` Stefan Wahren 0 siblings, 1 reply; 9+ messages in thread From: Guenter Roeck @ 2024-10-27 13:36 UTC (permalink / raw) To: Stefan Wahren, Alice Guo, Wim Van Sebroeck, Shawn Guo, Sascha Hauer, Fabio Estevam Cc: Pengutronix Kernel Team, linux-watchdog, imx, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] watchdog: imx7ulp_wdt: Add TOVAL range check 2024-10-27 13:36 ` Guenter Roeck @ 2024-10-27 15:54 ` Stefan Wahren 2024-10-27 17:35 ` Guenter Roeck 0 siblings, 1 reply; 9+ messages in thread From: Stefan Wahren @ 2024-10-27 15:54 UTC (permalink / raw) To: Guenter Roeck, Alice Guo, Wim Van Sebroeck, Shawn Guo, Sascha Hauer, Fabio Estevam Cc: Pengutronix Kernel Team, linux-watchdog, imx, linux-arm-kernel 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 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] watchdog: imx7ulp_wdt: Add TOVAL range check 2024-10-27 15:54 ` Stefan Wahren @ 2024-10-27 17:35 ` Guenter Roeck 2024-10-28 16:52 ` Stefan Wahren 0 siblings, 1 reply; 9+ messages in thread From: Guenter Roeck @ 2024-10-27 17:35 UTC (permalink / raw) To: Stefan Wahren, Alice Guo, Wim Van Sebroeck, Shawn Guo, Sascha Hauer, Fabio Estevam Cc: Pengutronix Kernel Team, linux-watchdog, imx, linux-arm-kernel 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; ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] watchdog: imx7ulp_wdt: Add TOVAL range check 2024-10-27 17:35 ` Guenter Roeck @ 2024-10-28 16:52 ` Stefan Wahren 0 siblings, 0 replies; 9+ messages in thread From: Stefan Wahren @ 2024-10-28 16:52 UTC (permalink / raw) To: Guenter Roeck, Alice Guo, Wim Van Sebroeck, Shawn Guo, Sascha Hauer, Fabio Estevam Cc: Pengutronix Kernel Team, linux-watchdog, imx, linux-arm-kernel 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; > > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-10-28 17:11 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-10-27 10:53 [PATCH 0/2] watchdog: imx7ulp_wdt: Improve readability Stefan Wahren 2024-10-27 10:53 ` [PATCH 1/2] watchdog: imx7ulp_wdt: Clarify timing units Stefan Wahren 2024-10-27 14:28 ` Guenter Roeck 2024-10-27 15:56 ` Stefan Wahren 2024-10-27 10:53 ` [PATCH 2/2] watchdog: imx7ulp_wdt: Add TOVAL range check Stefan Wahren 2024-10-27 13:36 ` Guenter Roeck 2024-10-27 15:54 ` Stefan Wahren 2024-10-27 17:35 ` Guenter Roeck 2024-10-28 16:52 ` Stefan Wahren
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).