| Message ID | 20250225143638.1989755-2-regis.dargent@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] watchdog: sunxi_wdt: Allow watchdog to remain enabled after probe | expand |
On 2/25/25 06:36, Regis Dargent wrote: > If the watchdog is already running during probe, let it run on, read its > configured timeout, and set its status so that it is correctly handled by the > kernel. > > Signed-off-by: Regis Dargent <regis.dargent@gmail.com> > > -- > > Changelog v1..v2: > - add sunxi_wdt_read_timeout function > - add signed-off-by tag > > Changelog v2..v3: > - WDIOF_SETTIMEOUT was set twice, and other code cleanup > --- > drivers/watchdog/sunxi_wdt.c | 45 ++++++++++++++++++++++++++++++++++-- > 1 file changed, 43 insertions(+), 2 deletions(-) > > diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c > index b85354a99582..d509dbcb77ce 100644 > --- a/drivers/watchdog/sunxi_wdt.c > +++ b/drivers/watchdog/sunxi_wdt.c > @@ -140,6 +140,7 @@ static int sunxi_wdt_set_timeout(struct watchdog_device *wdt_dev, > timeout++; > > sunxi_wdt->wdt_dev.timeout = timeout; > + sunxi_wdt->wdt_dev.max_hw_heartbeat_ms = 0; > > reg = readl(wdt_base + regs->wdt_mode); > reg &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift); > @@ -152,6 +153,32 @@ static int sunxi_wdt_set_timeout(struct watchdog_device *wdt_dev, > return 0; > } > > +static int sunxi_wdt_read_timeout(struct watchdog_device *wdt_dev) > +{ > + struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev); > + void __iomem *wdt_base = sunxi_wdt->wdt_base; > + const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs; > + int i; > + u32 reg; > + > + reg = readl(wdt_base + regs->wdt_mode); > + reg >>= regs->wdt_timeout_shift; > + reg &= WDT_TIMEOUT_MASK; > + > + /* Start at 0 which actually means 0.5s */ > + for (i = 0; (i < WDT_MAX_TIMEOUT) && (wdt_timeout_map[i] != reg); i++) Unnecessary (). On top of that, its complexity is unnecessary. The timeout in seconds, except for reg == 0, is wdt_timeout_map[reg], with values of 0x0c..0x0f undefined. Worse, the above code can access beyond the size of wdt_timeout_map[] if reg >= 0x0c. > + ; > + if (i == 0) { > + wdt_dev->timeout = 1; > + wdt_dev->max_hw_heartbeat_ms = 500; This is an unacceptable API abuse. max_hw_heartbeat_ms, if set, should be 16000, not 500. You could set the timeout to 1 second instead. Guenter
Le mar. 25 févr. 2025 à 22:51, Guenter Roeck <linux@roeck-us.net> a écrit : > > On 2/25/25 06:36, Regis Dargent wrote: > > If the watchdog is already running during probe, let it run on, read its > > configured timeout, and set its status so that it is correctly handled by the > > kernel. > > > > Signed-off-by: Regis Dargent <regis.dargent@gmail.com> > > > > -- > > > > Changelog v1..v2: > > - add sunxi_wdt_read_timeout function > > - add signed-off-by tag > > > > Changelog v2..v3: > > - WDIOF_SETTIMEOUT was set twice, and other code cleanup > > --- > > drivers/watchdog/sunxi_wdt.c | 45 ++++++++++++++++++++++++++++++++++-- > > 1 file changed, 43 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c > > index b85354a99582..d509dbcb77ce 100644 > > --- a/drivers/watchdog/sunxi_wdt.c > > +++ b/drivers/watchdog/sunxi_wdt.c > > @@ -140,6 +140,7 @@ static int sunxi_wdt_set_timeout(struct watchdog_device *wdt_dev, > > timeout++; > > > > sunxi_wdt->wdt_dev.timeout = timeout; > > + sunxi_wdt->wdt_dev.max_hw_heartbeat_ms = 0; > > > > reg = readl(wdt_base + regs->wdt_mode); > > reg &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift); > > @@ -152,6 +153,32 @@ static int sunxi_wdt_set_timeout(struct watchdog_device *wdt_dev, > > return 0; > > } > > > > +static int sunxi_wdt_read_timeout(struct watchdog_device *wdt_dev) > > +{ > > + struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev); > > + void __iomem *wdt_base = sunxi_wdt->wdt_base; > > + const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs; > > + int i; > > + u32 reg; > > + > > + reg = readl(wdt_base + regs->wdt_mode); > > + reg >>= regs->wdt_timeout_shift; > > + reg &= WDT_TIMEOUT_MASK; > > + > > + /* Start at 0 which actually means 0.5s */ > > + for (i = 0; (i < WDT_MAX_TIMEOUT) && (wdt_timeout_map[i] != reg); i++) > > Unnecessary (). On top of that, its complexity is unnecessary. > The timeout in seconds, except for reg == 0, is wdt_timeout_map[reg], > with values of 0x0c..0x0f undefined. Worse, the above code can access > beyond the size of wdt_timeout_map[] if reg >= 0x0c. Ok, I prefer parenthesis for (my) readability, but I will remove them. wdt_timeout_map is not linear, so timeout in seconds is i when wdt_timeout_map[i] == reg, some values of i not corresponding to any reg value. reg values of 0x0c and more will end with a timeout of 16s (i == WDT_MAX_TIMEOUT) which will lead to unknown behaviour only because these values are marked "reserved" in the HW timer documentation. > > + ; > > + if (i == 0) { > > + wdt_dev->timeout = 1; > > + wdt_dev->max_hw_heartbeat_ms = 500; > > This is an unacceptable API abuse. max_hw_heartbeat_ms, if set, > should be 16000, not 500. You could set the timeout to 1 second instead. sorry about that, my comprehension of the documentation on min/max_hw_heartbeat_ms suggested that they define the time window during which the HW timer must receive a heartbeat or it will reset. 0-'timeout' defines the time window for the userspace point of view, meaning that if 'timeout' > max_hw_heartbeat_ms, extra heartbeats must be sent to HW timer so that the reset can occur only after 'timeout' seconds. Just to be clear, setting max_hw_heartbeat_ms to the maximum HW possible value (here 16000ms) means that one cannot use sub-second HW timer values, while all the watchdog API actually handles them well enough (my tests on the 500ms HW timer worked good). For this sunxi driver case, this implies that instead of just reading the current timeout value from the HW timer (if it was enabled before the kernel start), I also have to reconfigure the HW timer (to 1sec) if its timeout is less than a second (the 0 register value). > Guenter >
diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c index b85354a99582..d509dbcb77ce 100644 --- a/drivers/watchdog/sunxi_wdt.c +++ b/drivers/watchdog/sunxi_wdt.c @@ -140,6 +140,7 @@ static int sunxi_wdt_set_timeout(struct watchdog_device *wdt_dev, timeout++; sunxi_wdt->wdt_dev.timeout = timeout; + sunxi_wdt->wdt_dev.max_hw_heartbeat_ms = 0; reg = readl(wdt_base + regs->wdt_mode); reg &= ~(WDT_TIMEOUT_MASK << regs->wdt_timeout_shift); @@ -152,6 +153,32 @@ static int sunxi_wdt_set_timeout(struct watchdog_device *wdt_dev, return 0; } +static int sunxi_wdt_read_timeout(struct watchdog_device *wdt_dev) +{ + struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev); + void __iomem *wdt_base = sunxi_wdt->wdt_base; + const struct sunxi_wdt_reg *regs = sunxi_wdt->wdt_regs; + int i; + u32 reg; + + reg = readl(wdt_base + regs->wdt_mode); + reg >>= regs->wdt_timeout_shift; + reg &= WDT_TIMEOUT_MASK; + + /* Start at 0 which actually means 0.5s */ + for (i = 0; (i < WDT_MAX_TIMEOUT) && (wdt_timeout_map[i] != reg); i++) + ; + if (i == 0) { + wdt_dev->timeout = 1; + wdt_dev->max_hw_heartbeat_ms = 500; + } else { + wdt_dev->timeout = i; + wdt_dev->max_hw_heartbeat_ms = 0; + } + + return 0; +} + static int sunxi_wdt_stop(struct watchdog_device *wdt_dev) { struct sunxi_wdt_dev *sunxi_wdt = watchdog_get_drvdata(wdt_dev); @@ -192,6 +219,16 @@ static int sunxi_wdt_start(struct watchdog_device *wdt_dev) return 0; } +static bool sunxi_wdt_enabled(struct sunxi_wdt_dev *wdt) +{ + void __iomem *wdt_base = wdt->wdt_base; + const struct sunxi_wdt_reg *regs = wdt->wdt_regs; + u32 reg; + + reg = readl(wdt_base + regs->wdt_mode); + return !!(reg & WDT_MODE_EN); +} + static const struct watchdog_info sunxi_wdt_info = { .identity = DRV_NAME, .options = WDIOF_SETTIMEOUT | @@ -275,8 +312,12 @@ static int sunxi_wdt_probe(struct platform_device *pdev) watchdog_set_drvdata(&sunxi_wdt->wdt_dev, sunxi_wdt); - sunxi_wdt_stop(&sunxi_wdt->wdt_dev); - + if (sunxi_wdt_enabled(sunxi_wdt)) { + sunxi_wdt_read_timeout(&sunxi_wdt->wdt_dev); + set_bit(WDOG_HW_RUNNING, &sunxi_wdt->wdt_dev.status); + } else { + sunxi_wdt_stop(&sunxi_wdt->wdt_dev); + } watchdog_stop_on_reboot(&sunxi_wdt->wdt_dev); err = devm_watchdog_register_device(dev, &sunxi_wdt->wdt_dev); if (unlikely(err))
If the watchdog is already running during probe, let it run on, read its configured timeout, and set its status so that it is correctly handled by the kernel. Signed-off-by: Regis Dargent <regis.dargent@gmail.com> -- Changelog v1..v2: - add sunxi_wdt_read_timeout function - add signed-off-by tag Changelog v2..v3: - WDIOF_SETTIMEOUT was set twice, and other code cleanup --- drivers/watchdog/sunxi_wdt.c | 45 ++++++++++++++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 2 deletions(-)