> -----Ursprüngliche Nachricht----- > Von: Guenter Roeck <groeck7@xxxxxxxxx> Im Auftrag von Guenter Roeck > Gesendet: Freitag, 17. September 2021 15:47 > An: Walter Stoll <Walter.Stoll@xxxxxxxxxx>; wim@xxxxxxxxxxxxxxxxxx > Cc: linux-watchdog@xxxxxxxxxxxxxxx > Betreff: Re: [BUG] omap_wdt.c: Watchdog not serviced by kernel > > On 9/17/21 1:36 AM, Walter Stoll wrote: > > Effect observed > > --------------- > > > > We use the watchdog timer on a AM335x controller. When U-Boot runs, we enable > > the watchdog and want the kernel to service the watchdog until userspace takes > > it over. > > > > We compile the watchdog directly into the kernel and add the parameter > > "omap_wdt.early_enable=1" to the kernel command line. We furthermore set > > "CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED=y" in the kernel configuration. > > > > Our expectation is, that the watchdog is serviced by the kernel as long as > > userspace does not touch the device /dev/watchdog. However, this is not the > > case. The watchdog always expires. It is obviously not serviced by the kernel. > > > > We observed the effect with kernel version v5.4.138-rt62. However, we think > > that the most recent kernel exhibits the same behavior because the structure of > > the sources in question (see below) did not change. This also holds for the non > > realtime kernel. > > > > > > Root cause > > ---------- > > > > The CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED configuration option is not implemented > > in omap_wdt.c. > > > > > > Fix proposal > > ------------ > > > > Interestingly we found only one single driver that implements this featrue, > > namely the driver from STM, see > > https://elixir.bootlin.com/linux/v5.4.138/source/drivers/watchdog/stm32_iwdg.c#L274 > > > > This makes us wonder if there might be a good reason not to implement it??? > > > It is primarily a watchdog core feature. Handling running watchdogs in the core > used to be enabled by default. Not everyone was happy with that, so > WATCHDOG_HANDLE_BOOT_ENABLED was added to be able to _disable_ that functionality. > It was never intended to be a driver feature. The STM driver (mis)uses it > because it wants to be able to support WDOG_HW_RUNNING, but the HW has no means > to detect if it is running. That doesn't mean that other drivers need to do > the same. > > > However we think this feature should be available. Our use case is to make > > software updates more robust. If an updated kernel hangs for whatever reason, > > then U-Boot gets the chance to boot the old one provided there is a reboot. > > > > Based on the STM implementation, we created a patch (see below) which resolves > > the issue. The watchdog is now correctly handled by the kernel until userspace > > first accesses it. > > > > diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c > > index 9b91882fe3c4..94e2e1b494d2 100644 > > --- a/drivers/watchdog/omap_wdt.c > > +++ b/drivers/watchdog/omap_wdt.c > > @@ -271,6 +271,11 @@ static int omap_wdt_probe(struct platform_device *pdev) > > if (!early_enable) > > omap_wdt_disable(wdev); > > > > + if (IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED)) { > > + /* Make sure the watchdog is serviced */ > > + set_bit(WDOG_HW_RUNNING, &wdev->wdog.status); > > + } > > + > > No, this is wrong. The driver should set WDOG_HW_RUNNING if the watchdog is running, > independently of CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED. > > The omap_wdt driver has a boot option "early_enable" which does start the watchdog > during probe, but it doesn't set WDOG_HW_RUNNING (which is the real problem). > Plus, if early_enable is not set, the driver explicitly disables the watchdog > (see code above), and setting WDOG_HW_RUNNING would be wrong in that case. > > The fix would be something like > > if (early_enable) { > omap_wdt_start(&wdev->wdog); > set_bit(WDOG_HW_RUNNING, &wdev->wdog.status); > } else { > omap_wdt_disable(wdev); > } > > That needs to be ahead of watchdog_register_device(), and is independent > of CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED. > > Guenter > > > ret = watchdog_register_device(&wdev->wdog); > > if (ret) { > > pm_runtime_disable(wdev->dev); > > Hello Guenter Thank you very much for your fast response. I checked your fix with all combinations of early_enable and CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED. >From my point of view, this works exactly as expected. Any chance to get that mainline ? Walter
