* [BUG] omap_wdt.c: Watchdog not serviced by kernel @ 2021-09-17 8:36 Walter Stoll 2021-09-17 13:47 ` Guenter Roeck 0 siblings, 1 reply; 4+ messages in thread From: Walter Stoll @ 2021-09-17 8:36 UTC (permalink / raw) To: wim, linux; +Cc: linux-watchdog 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??? 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); + } + ret = watchdog_register_device(&wdev->wdog); if (ret) { pm_runtime_disable(wdev->dev); ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [BUG] omap_wdt.c: Watchdog not serviced by kernel 2021-09-17 8:36 [BUG] omap_wdt.c: Watchdog not serviced by kernel Walter Stoll @ 2021-09-17 13:47 ` Guenter Roeck 2021-09-17 15:00 ` AW: " Walter Stoll 0 siblings, 1 reply; 4+ messages in thread From: Guenter Roeck @ 2021-09-17 13:47 UTC (permalink / raw) To: Walter Stoll, wim; +Cc: linux-watchdog 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); > ^ permalink raw reply [flat|nested] 4+ messages in thread
* AW: [BUG] omap_wdt.c: Watchdog not serviced by kernel 2021-09-17 13:47 ` Guenter Roeck @ 2021-09-17 15:00 ` Walter Stoll 2021-09-17 15:08 ` Guenter Roeck 0 siblings, 1 reply; 4+ messages in thread From: Walter Stoll @ 2021-09-17 15:00 UTC (permalink / raw) To: Guenter Roeck, wim; +Cc: linux-watchdog > -----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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: AW: [BUG] omap_wdt.c: Watchdog not serviced by kernel 2021-09-17 15:00 ` AW: " Walter Stoll @ 2021-09-17 15:08 ` Guenter Roeck 0 siblings, 0 replies; 4+ messages in thread From: Guenter Roeck @ 2021-09-17 15:08 UTC (permalink / raw) To: Walter Stoll, wim; +Cc: linux-watchdog 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-09-17 15:08 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-17 8:36 [BUG] omap_wdt.c: Watchdog not serviced by kernel Walter Stoll 2021-09-17 13:47 ` Guenter Roeck 2021-09-17 15:00 ` AW: " Walter Stoll 2021-09-17 15:08 ` Guenter Roeck
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).