[PATCH] watchdog: omap: Fix early_enable to start watchdogd

[PATCH] watchdog: omap: Fix early_enable to start watchdogd

[Brandon Maier]

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(-)

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;
-- 
2.23.0

Re: [PATCH] watchdog: omap: Fix early_enable to start watchdogd

[Guenter Roeck]

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;
> 

Re: [PATCH] watchdog: omap: Fix early_enable to start watchdogd

[Brandon Maier]

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

Re: [PATCH] watchdog: omap: Fix early_enable to start watchdogd

[Guenter Roeck]

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
> 

Re: [External] Re: [PATCH] watchdog: omap: Fix early_enable to start watchdogd

[Brandon Maier]

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
> >
>