| Message ID | cb068bbba92347b2ab3190fda5d85ebf@chdua14.duagon.ads (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [BUG] omap_wdt.c: Watchdog not serviced by kernel | expand |
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); >
> -----Ursprüngliche Nachricht----- > Von: Guenter Roeck <groeck7@gmail.com> Im Auftrag von Guenter Roeck > Gesendet: Freitag, 17. September 2021 15:47 > An: Walter Stoll <Walter.Stoll@duagon.com>; wim@linux-watchdog.org > Cc: linux-watchdog@vger.kernel.org > 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
On 9/17/21 8:00 AM, Walter Stoll wrote: >> -----Ursprüngliche Nachricht----- >> Von: Guenter Roeck <groeck7@gmail.com> Im Auftrag von Guenter Roeck >> Gesendet: Freitag, 17. September 2021 15:47 >> An: Walter Stoll <Walter.Stoll@duagon.com>; wim@linux-watchdog.org >> Cc: linux-watchdog@vger.kernel.org >> 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. > Excellent. > From my point of view, this works exactly as expected. Any chance to get > that mainline ? > Submit a patch, I'll review it, and Wim will pick it up for v5.16. Guenter
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); + } + ret = watchdog_register_device(&wdev->wdog); if (ret) { pm_runtime_disable(wdev->dev);