linux-kernel - Re: [PATCH] clocksource: timer-tegra186: Enable WDT at probe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bd2oz44lcrtdixmzqhcmespqjye5s5gvgzh4j6pqqj3bycktmv@r5gp66jjraxr>
Date: Thu, 3 Jul 2025 15:40:41 +0200
From: Thierry Reding <thierry.reding@...il.com>
To: Jon Hunter <jonathanh@...dia.com>
Cc: Kartik Rajput <kkartik@...dia.com>, daniel.lezcano@...aro.org, 
	tglx@...utronix.de, linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org
Subject: Re: [PATCH] clocksource: timer-tegra186: Enable WDT at probe

On Thu, Jul 03, 2025 at 11:36:16AM +0100, Jon Hunter wrote:
> 
> 
> On 30/06/2025 12:01, Kartik Rajput wrote:
> > Currently, if the system crashes or hangs during kernel boot before
> > userspace initializes and configures the watchdog timer, then the
> > watchdog won’t be able to recover the system as it’s not running. This
> > becomes crucial during an over-the-air update, where if the newly
> > updated kernel crashes on boot, the watchdog is needed to reset the
> > device and boot into an alternative system partition. If the watchdog
> > is disabled in such scenarios, it can lead to the system getting
> > bricked.
> > 
> > Enable the WDT during driver probe to allow recovery from any crash/hang
> > seen during early kernel boot. Also, disable interrupts once userspace
> > starts pinging the watchdog.
> > 
> > Signed-off-by: Kartik Rajput <kkartik@...dia.com>
> > ---
> >   drivers/clocksource/timer-tegra186.c | 42 ++++++++++++++++++++++++++++
> >   1 file changed, 42 insertions(+)
> > 
> > diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/clocksource/timer-tegra186.c
> > index e5394f98a02e..59abb5dab8f1 100644
> > --- a/drivers/clocksource/timer-tegra186.c
> > +++ b/drivers/clocksource/timer-tegra186.c
> > @@ -57,6 +57,8 @@
> >   #define WDTUR 0x00c
> >   #define  WDTUR_UNLOCK_PATTERN 0x0000c45a
> > +#define WDT_DEFAULT_TIMEOUT 120
> > +
> >   struct tegra186_timer_soc {
> >   	unsigned int num_timers;
> >   	unsigned int num_wdts;
> > @@ -74,6 +76,7 @@ struct tegra186_wdt {
> >   	void __iomem *regs;
> >   	unsigned int index;
> > +	bool enable_irq;
> >   	bool locked;
> >   	struct tegra186_tmr *tmr;
> > @@ -174,6 +177,12 @@ static void tegra186_wdt_enable(struct tegra186_wdt *wdt)
> >   		value &= ~WDTCR_PERIOD_MASK;
> >   		value |= WDTCR_PERIOD(1);
> > +		/* configure local interrupt for WDT petting */
> 
> It might be a bit clearer if this comment states ...
> 
> 'If enable_irq is set then enable the watchdog IRQ for kernel petting,
> otherwise userspace is responsible for petting the watchdog.'
> 
> > +		if (wdt->enable_irq)
> > +			value |= WDTCR_LOCAL_INT_ENABLE;
> > +		else
> > +			value &= ~WDTCR_LOCAL_INT_ENABLE;
> > +
> >   		/* enable system POR reset */
> >   		value |= WDTCR_SYSTEM_POR_RESET_ENABLE;
> > @@ -205,6 +214,10 @@ static int tegra186_wdt_ping(struct watchdog_device *wdd)
> >   {
> >   	struct tegra186_wdt *wdt = to_tegra186_wdt(wdd);
> > +	/* Disable WDT interrupt once userspace takes over. */
> 
> Technically userspace is taking over at this point and so we should be more
> assertive here ...
> 
> 'Disable the watchdog IRQ now userspace is taking over'
> 
> > +	if (wdt->enable_irq)
> > +		wdt->enable_irq = false;
> > +
> >   	tegra186_wdt_disable(wdt);
> >   	tegra186_wdt_enable(wdt);
> > @@ -315,6 +328,8 @@ static struct tegra186_wdt *tegra186_wdt_create(struct tegra186_timer *tegra,
> >   	if (value & WDTCR_LOCAL_INT_ENABLE)
> >   		wdt->locked = true;
> > +	wdt->enable_irq = true;
> > +
> >   	source = value & WDTCR_TIMER_SOURCE_MASK;
> >   	wdt->tmr = tegra186_tmr_create(tegra, source);
> > @@ -339,6 +354,13 @@ static struct tegra186_wdt *tegra186_wdt_create(struct tegra186_timer *tegra,
> >   		return ERR_PTR(err);
> >   	}
> > +	/*
> > +	 * Start the watchdog to recover the system if it crashes before
> > +	 * userspace initialize the WDT.
> > +	 */
> > +	tegra186_wdt_set_timeout(&wdt->base, WDT_DEFAULT_TIMEOUT);
> > +	tegra186_wdt_start(&wdt->base);

If we need to stick to the single watchdog, then it's probably better to
explicitly enable the local interrupt here and explicitly disable it
when userspace takes over. That would allow us to avoid tracking this in
the enable_irq state variable.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)