Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
From: Brandon Maier <brandon.maier@rockwellcollins.com>
To: Guenter Roeck <linux@roeck-us.net>
Cc: linux-watchdog@vger.kernel.org, wim@linux-watchdog.org
Subject: Re: [External] Re: [PATCH] watchdog: omap: Fix early_enable to start watchdogd
Date: Mon, 18 Nov 2019 10:10:05 -0600
Message-ID: <CA+fik51ni5y1K_ZDa1PdZ1873sN5eKrhFQP+xNU=JQft+0Yxwg@mail.gmail.com> (raw)
In-Reply-To: <0338087a-07cf-5673-7c3b-5d1211f359e2@roeck-us.net>

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

      reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-17 20:03 Brandon Maier
2019-11-17 21:44 ` Guenter Roeck
2019-11-17 22:19   ` Brandon Maier
2019-11-18  0:23     ` Guenter Roeck
2019-11-18 16:10       ` Brandon Maier [this message]

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CA+fik51ni5y1K_ZDa1PdZ1873sN5eKrhFQP+xNU=JQft+0Yxwg@mail.gmail.com' \
    --to=brandon.maier@rockwellcollins.com \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=wim@linux-watchdog.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org
	public-inbox-index linux-watchdog

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git