| Message ID | 20191117200325.142419-1-brandon.maier@rockwellcollins.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | watchdog: omap: Fix early_enable to start watchdogd | expand |
On 11/17/19 12:03 PM, Brandon Maier wrote: > When the 'early_enable' module_param is enabled, Linux's watchdogd > thread does not start, causing the watchdog to eventually fire. > What does early_enable have to do with watchdogd ? Why would watchdogd not start if this flag is set ? The purpose of early_enable in this driver, as I understand it, was to force watchdogd to start within the timeout period. So it does exactly what it is supposed to be doing. Guenter > For the watchdogd to be started, the WDOG_HW_RUNNING flag must be set > before watchdog_register_device(). > > Signed-off-by: Brandon Maier <brandon.maier@rockwellcollins.com> > --- > drivers/watchdog/omap_wdt.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c > index 9b91882fe3c4..ecc8592c00a5 100644 > --- a/drivers/watchdog/omap_wdt.c > +++ b/drivers/watchdog/omap_wdt.c > @@ -268,8 +268,13 @@ static int omap_wdt_probe(struct platform_device *pdev) > wdev->wdog.bootstatus = WDIOF_CARDRESET; > } > > - if (!early_enable) > + if (!early_enable) { > omap_wdt_disable(wdev); > + clear_bit(WDOG_HW_RUNNING, &wdev->wdog.status); > + } else { > + omap_wdt_start(&wdev->wdog); > + set_bit(WDOG_HW_RUNNING, &wdev->wdog.status); > + } > > ret = watchdog_register_device(&wdev->wdog); > if (ret) { > @@ -281,9 +286,6 @@ static int omap_wdt_probe(struct platform_device *pdev) > readl_relaxed(wdev->base + OMAP_WATCHDOG_REV) & 0xFF, > wdev->wdog.timeout); > > - if (early_enable) > - omap_wdt_start(&wdev->wdog); > - > pm_runtime_put(wdev->dev); > > return 0; >
On Sun, Nov 17, 2019 at 3:44 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On 11/17/19 12:03 PM, Brandon Maier wrote: > > When the 'early_enable' module_param is enabled, Linux's watchdogd > > thread does not start, causing the watchdog to eventually fire. > > > > What does early_enable have to do with watchdogd ? The early_enable doesn't directly control the watchdogd, it only controls starting the watchdog. The problem is that, if the watchdog is started, we need the watchdogd to ping the watchdog. It watchdogd is not running, then the watchdog will trip and reset the system. > Why would watchdogd not start if this flag is set ? watchdog_register_device() checks this flag, and only enables the watchdogd if it's set. See following: https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/watchdog_dev.c#L1031 > > The purpose of early_enable in this driver, as I understand it, > was to force watchdogd to start within the timeout period. So > it does exactly what it is supposed to be doing. I'm not sure what you're referring to by "timeout period". But early_enable does force the watchdog to start during module insertion, that works correctly. The issue is that the driver doesn't tell the watchdog core to launch the watchdogd. This change is based off many of the other watchdog drivers. They also set the WDOG_HW_RUNNING flag if the watchdog is running. The only significant difference is that those drivers don't typically have an early_enable, they just detect if the watchdog was already running before insertion. But the end result is the same, the watchdog is running and will trigger if watchdogd isn't. > > Guenter > > > For the watchdogd to be started, the WDOG_HW_RUNNING flag must be set > > before watchdog_register_device(). > > > > Signed-off-by: Brandon Maier <brandon.maier@rockwellcollins.com> > > --- > > drivers/watchdog/omap_wdt.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c > > index 9b91882fe3c4..ecc8592c00a5 100644 > > --- a/drivers/watchdog/omap_wdt.c > > +++ b/drivers/watchdog/omap_wdt.c > > @@ -268,8 +268,13 @@ static int omap_wdt_probe(struct platform_device *pdev) > > wdev->wdog.bootstatus = WDIOF_CARDRESET; > > } > > > > - if (!early_enable) > > + if (!early_enable) { > > omap_wdt_disable(wdev); > > + clear_bit(WDOG_HW_RUNNING, &wdev->wdog.status); > > + } else { > > + omap_wdt_start(&wdev->wdog); > > + set_bit(WDOG_HW_RUNNING, &wdev->wdog.status); > > + } > > > > ret = watchdog_register_device(&wdev->wdog); > > if (ret) { > > @@ -281,9 +286,6 @@ static int omap_wdt_probe(struct platform_device *pdev) > > readl_relaxed(wdev->base + OMAP_WATCHDOG_REV) & 0xFF, > > wdev->wdog.timeout); > > > > - if (early_enable) > > - omap_wdt_start(&wdev->wdog); > > - > > pm_runtime_put(wdev->dev); > > > > return 0; > > > Brandon
On 11/17/19 2:19 PM, Brandon Maier wrote: > On Sun, Nov 17, 2019 at 3:44 PM Guenter Roeck <linux@roeck-us.net> wrote: >> >> On 11/17/19 12:03 PM, Brandon Maier wrote: >>> When the 'early_enable' module_param is enabled, Linux's watchdogd >>> thread does not start, causing the watchdog to eventually fire. >>> >> >> What does early_enable have to do with watchdogd ? > > The early_enable doesn't directly control the watchdogd, it only > controls starting the watchdog. The problem is that, if the watchdog > is started, we need the watchdogd to ping the watchdog. It watchdogd > is not running, then the watchdog will trip and reset the system. > >> Why would watchdogd not start if this flag is set ? > > watchdog_register_device() checks this flag, and only enables the > watchdogd if it's set. See following: > https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/watchdog_dev.c#L1031 > >> >> The purpose of early_enable in this driver, as I understand it, >> was to force watchdogd to start within the timeout period. So >> it does exactly what it is supposed to be doing. > > I'm not sure what you're referring to by "timeout period". But > early_enable does force the watchdog to start during module insertion, > that works correctly. The issue is that the driver doesn't tell the > watchdog core to launch the watchdogd. > If you refer to the kernel "watchdogd", that is not of interest here. _Userspace_ is supposed to start a watchdog daemon if early_enable is set. If userspace doesn't do that, it is a userspace problem, not a kernel problem. The whole point of early_enable is to force _userspace_ to start its watchdog daemon. Your proposed patch would defeat that by pinging the watchdog in the kernel, and it would keep doing so forever even if userspace hangs completely. This would be just wrong. Guenter > This change is based off many of the other watchdog drivers. They also > set the WDOG_HW_RUNNING flag if the watchdog is running. The only > significant difference is that those drivers don't typically have an > early_enable, they just detect if the watchdog was already running > before insertion. But the end result is the same, the watchdog is > running and will trigger if watchdogd isn't. > >> >> Guenter >> >>> For the watchdogd to be started, the WDOG_HW_RUNNING flag must be set >>> before watchdog_register_device(). >>> >>> Signed-off-by: Brandon Maier <brandon.maier@rockwellcollins.com> >>> --- >>> drivers/watchdog/omap_wdt.c | 10 ++++++---- >>> 1 file changed, 6 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c >>> index 9b91882fe3c4..ecc8592c00a5 100644 >>> --- a/drivers/watchdog/omap_wdt.c >>> +++ b/drivers/watchdog/omap_wdt.c >>> @@ -268,8 +268,13 @@ static int omap_wdt_probe(struct platform_device *pdev) >>> wdev->wdog.bootstatus = WDIOF_CARDRESET; >>> } >>> >>> - if (!early_enable) >>> + if (!early_enable) { >>> omap_wdt_disable(wdev); >>> + clear_bit(WDOG_HW_RUNNING, &wdev->wdog.status); >>> + } else { >>> + omap_wdt_start(&wdev->wdog); >>> + set_bit(WDOG_HW_RUNNING, &wdev->wdog.status); >>> + } >>> >>> ret = watchdog_register_device(&wdev->wdog); >>> if (ret) { >>> @@ -281,9 +286,6 @@ static int omap_wdt_probe(struct platform_device *pdev) >>> readl_relaxed(wdev->base + OMAP_WATCHDOG_REV) & 0xFF, >>> wdev->wdog.timeout); >>> >>> - if (early_enable) >>> - omap_wdt_start(&wdev->wdog); >>> - >>> pm_runtime_put(wdev->dev); >>> >>> return 0; >>> >> > > Brandon >
On Sun, Nov 17, 2019 at 6:23 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On 11/17/19 2:19 PM, Brandon Maier wrote: > > On Sun, Nov 17, 2019 at 3:44 PM Guenter Roeck <linux@roeck-us.net> wrote: > >> > >> On 11/17/19 12:03 PM, Brandon Maier wrote: > >>> When the 'early_enable' module_param is enabled, Linux's watchdogd > >>> thread does not start, causing the watchdog to eventually fire. > >>> > >> > >> What does early_enable have to do with watchdogd ? > > > > The early_enable doesn't directly control the watchdogd, it only > > controls starting the watchdog. The problem is that, if the watchdog > > is started, we need the watchdogd to ping the watchdog. It watchdogd > > is not running, then the watchdog will trip and reset the system. > > > >> Why would watchdogd not start if this flag is set ? > > > > watchdog_register_device() checks this flag, and only enables the > > watchdogd if it's set. See following: > > https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_latest_source_drivers_watchdog_watchdog-5Fdev.c-23L1031&d=DwICaQ&c=ilBQI1lupc9Y65XwNblLtw&r=bIwUnEkCqKFQQ0RVQaaY0gBWY7SIAhmiWLyMS82_mSU&m=xNCU_zPoNl2ll56Vs7usphsCSGx4KDrV83ykbUzBHig&s=kiZuvwneuc7XR_vqsiab4GnglNk6j9xk6Njy5nluF1o&e= > > > >> > >> The purpose of early_enable in this driver, as I understand it, > >> was to force watchdogd to start within the timeout period. So > >> it does exactly what it is supposed to be doing. > > > > I'm not sure what you're referring to by "timeout period". But > > early_enable does force the watchdog to start during module insertion, > > that works correctly. The issue is that the driver doesn't tell the > > watchdog core to launch the watchdogd. > > > > If you refer to the kernel "watchdogd", that is not of interest here. > _Userspace_ is supposed to start a watchdog daemon if early_enable > is set. If userspace doesn't do that, it is a userspace problem, > not a kernel problem. > > The whole point of early_enable is to force _userspace_ to start its watchdog > daemon. Your proposed patch would defeat that by pinging the watchdog in the > kernel, and it would keep doing so forever even if userspace hangs completely. > This would be just wrong. The kernel watchdogd is what I care about here. The kernel has a Kconfig option, CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED[1]. If this setting is enabled, then the kernel is supposed to ping the watchdog until userspace takes over. This is what other watchdog drivers do, and is what this patch is trying to fix. What you're describing is the behaviour if that config is disabled. That is unrelated to early_enable, early_enable only controls if the driver is enabled during initialization. [1] https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/Kconfig#L50 > > Guenter > > > This change is based off many of the other watchdog drivers. They also > > set the WDOG_HW_RUNNING flag if the watchdog is running. The only > > significant difference is that those drivers don't typically have an > > early_enable, they just detect if the watchdog was already running > > before insertion. But the end result is the same, the watchdog is > > running and will trigger if watchdogd isn't. > > > >> > >> Guenter > >> > >>> For the watchdogd to be started, the WDOG_HW_RUNNING flag must be set > >>> before watchdog_register_device(). > >>> > >>> Signed-off-by: Brandon Maier <brandon.maier@rockwellcollins.com> > >>> --- > >>> drivers/watchdog/omap_wdt.c | 10 ++++++---- > >>> 1 file changed, 6 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c > >>> index 9b91882fe3c4..ecc8592c00a5 100644 > >>> --- a/drivers/watchdog/omap_wdt.c > >>> +++ b/drivers/watchdog/omap_wdt.c > >>> @@ -268,8 +268,13 @@ static int omap_wdt_probe(struct platform_device *pdev) > >>> wdev->wdog.bootstatus = WDIOF_CARDRESET; > >>> } > >>> > >>> - if (!early_enable) > >>> + if (!early_enable) { > >>> omap_wdt_disable(wdev); > >>> + clear_bit(WDOG_HW_RUNNING, &wdev->wdog.status); > >>> + } else { > >>> + omap_wdt_start(&wdev->wdog); > >>> + set_bit(WDOG_HW_RUNNING, &wdev->wdog.status); > >>> + } > >>> > >>> ret = watchdog_register_device(&wdev->wdog); > >>> if (ret) { > >>> @@ -281,9 +286,6 @@ static int omap_wdt_probe(struct platform_device *pdev) > >>> readl_relaxed(wdev->base + OMAP_WATCHDOG_REV) & 0xFF, > >>> wdev->wdog.timeout); > >>> > >>> - if (early_enable) > >>> - omap_wdt_start(&wdev->wdog); > >>> - > >>> pm_runtime_put(wdev->dev); > >>> > >>> return 0; > >>> > >> > > > > Brandon > > >
diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c index 9b91882fe3c4..ecc8592c00a5 100644 --- a/drivers/watchdog/omap_wdt.c +++ b/drivers/watchdog/omap_wdt.c @@ -268,8 +268,13 @@ static int omap_wdt_probe(struct platform_device *pdev) wdev->wdog.bootstatus = WDIOF_CARDRESET; } - if (!early_enable) + if (!early_enable) { omap_wdt_disable(wdev); + clear_bit(WDOG_HW_RUNNING, &wdev->wdog.status); + } else { + omap_wdt_start(&wdev->wdog); + set_bit(WDOG_HW_RUNNING, &wdev->wdog.status); + } ret = watchdog_register_device(&wdev->wdog); if (ret) { @@ -281,9 +286,6 @@ static int omap_wdt_probe(struct platform_device *pdev) readl_relaxed(wdev->base + OMAP_WATCHDOG_REV) & 0xFF, wdev->wdog.timeout); - if (early_enable) - omap_wdt_start(&wdev->wdog); - pm_runtime_put(wdev->dev); return 0;
When the 'early_enable' module_param is enabled, Linux's watchdogd thread does not start, causing the watchdog to eventually fire. For the watchdogd to be started, the WDOG_HW_RUNNING flag must be set before watchdog_register_device(). Signed-off-by: Brandon Maier <brandon.maier@rockwellcollins.com> --- drivers/watchdog/omap_wdt.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)