linux-kernel - RE: [PATCH v5 1/3] clocksource/drivers/timer-tegra186: add WDIOC

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:
 <PH7PR12MB6418840127C215779DB63CE3D9802@PH7PR12MB6418.namprd12.prod.outlook.com>
Date: Tue, 29 Apr 2025 03:50:35 +0000
From: Robert Lin <robelin@...dia.com>
To: Jon Hunter <jonathanh@...dia.com>, "thierry.reding@...il.com"
	<thierry.reding@...il.com>, "daniel.lezcano@...aro.org"
	<daniel.lezcano@...aro.org>, "tglx@...utronix.de" <tglx@...utronix.de>,
	Pohsun Su <pohsuns@...dia.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>, Sumit Gupta
	<sumitg@...dia.com>
Subject: RE: [PATCH v5 1/3] clocksource/drivers/timer-tegra186: add
 WDIOC_GETTIMELEFT support

> -----Original Message-----
> From: Jon Hunter <jonathanh@...dia.com>
> Sent: Monday, April 28, 2025 10:04 PM
> To: Robert Lin <robelin@...dia.com>; thierry.reding@...il.com;
> daniel.lezcano@...aro.org; tglx@...utronix.de; Pohsun Su
> <pohsuns@...dia.com>
> Cc: linux-kernel@...r.kernel.org; linux-tegra@...r.kernel.org; Sumit Gupta
> <sumitg@...dia.com>
> Subject: Re: [PATCH v5 1/3] clocksource/drivers/timer-tegra186: add
> WDIOC_GETTIMELEFT support
> 
> Hi Robert,
> 
> On 21/04/2025 11:08, Robert Lin wrote:
> > From: Pohsun Su <pohsuns@...dia.com>
> >
> > This change adds support for WDIOC_GETTIMELEFT so userspace programs
> > can get the number of seconds before system reset by the watchdog
> > timer via ioctl.
> >
> > Signed-off-by: Pohsun Su <pohsuns@...dia.com>
> > Signed-off-by: Robert Lin <robelin@...dia.com>
> > ---
> >   drivers/clocksource/timer-tegra186.c | 58
> +++++++++++++++++++++++++++-
> >   1 file changed, 57 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clocksource/timer-tegra186.c
> > b/drivers/clocksource/timer-tegra186.c
> > index ea742889ee06..56d08bf1b6b0 100644
> > --- a/drivers/clocksource/timer-tegra186.c
> > +++ b/drivers/clocksource/timer-tegra186.c
> > @@ -1,8 +1,9 @@
> >   // SPDX-License-Identifier: GPL-2.0-only
> >   /*
> > - * Copyright (c) 2019-2020 NVIDIA Corporation. All rights reserved.
> > + * Copyright (c) 2019-2025 NVIDIA Corporation. All rights reserved.
> >    */
> >
> > +#include <linux/bitfield.h>
> >   #include <linux/clocksource.h>
> >   #include <linux/module.h>
> >   #include <linux/interrupt.h>
> > @@ -30,6 +31,7 @@
> >
> >   #define TMRSR 0x004
> >   #define  TMRSR_INTR_CLR BIT(30)
> > +#define  TMRSR_PCV GENMASK(28, 0)
> >
> >   #define TMRCSSR 0x008
> >   #define  TMRCSSR_SRC_USEC (0 << 0)
> > @@ -46,6 +48,9 @@
> >   #define  WDTCR_TIMER_SOURCE_MASK 0xf
> >   #define  WDTCR_TIMER_SOURCE(x) ((x) & 0xf)
> >
> > +#define WDTSR 0x004
> > +#define  WDTSR_CURRENT_EXPIRATION_COUNT GENMASK(14, 12)
> > +
> >   #define WDTCMDR 0x008
> >   #define  WDTCMDR_DISABLE_COUNTER BIT(1)
> >   #define  WDTCMDR_START_COUNTER BIT(0) @@ -235,12 +240,63 @@
> static
> > int tegra186_wdt_set_timeout(struct watchdog_device *wdd,
> >   	return 0;
> >   }
> >
> > +static unsigned int tegra186_wdt_get_timeleft(struct watchdog_device
> > +*wdd) {
> > +	struct tegra186_wdt *wdt = to_tegra186_wdt(wdd);
> > +	u32 timeleft, expiration, val;
> > +
> > +	if (!watchdog_active(&wdt->base)) {
> > +		/* return zero if the watchdog timer is not activated. */
> > +		return 0;
> > +	}
> > +
> > +	/*
> > +	 * Reset occurs on the fifth expiration of the
> > +	 * watchdog timer and so when the watchdog timer is configured,
> > +	 * the actual value programmed into the counter is 1/5 of the
> > +	 * timeout value. Once the counter reaches 0, expiration count
> > +	 * will be increased by 1 and the down counter restarts.
> > +	 * Hence to get the time left before system reset we must
> > +	 * combine 2 parts:
> > +	 * 1. value of the current down counter
> > +	 * 2. (number of counter expirations remaining) * (timeout/5)
> > +	 */
> > +
> > +	/* Get the current number of counter expirations. Should be a
> > +	 * value between 0 and 4
> > +	 */
> > +	val = readl_relaxed(wdt->regs + WDTSR);
> > +	expiration = FIELD_GET(WDTSR_CURRENT_EXPIRATION_COUNT, val);
> > +	if (WARN_ON(expiration > 4))
> > +             return 0;
> > +
> > +	/* Get the current counter value in microsecond.
> > +	 */
> > +	val = readl_relaxed(wdt->tmr->regs + TMRSR);
> > +	timeleft = FIELD_GET(TMRSR_PCV, val);
> 
> So this value is in microseconds.
> 
> > +
> > +	/*
> > +	 * Calculate the time remaining by adding the time for the
> > +	 * counter value to the time of the counter expirations that
> > +	 * remain. Do the multiplication first on purpose just to keep
> > +	 * the precision due to the integer division.
> > +	 */
> > +	timeleft += wdt->base.timeout * (4 - expiration) / 5;
> 
> However, wdt->base.timeout is in seconds. So I don't think we can simply add
> this. Don't we need to ...
> 
> timeleft += (wdt->base.timeout * USEC_PER_SEC * (4 - expiration)) / 5;
> 

You are right. As watchdog timer can still be triggered at the right time, even if get_timeleft callback return the inaccurate value, I thought my change is fine. But it turns out get_timeleft is independent to this.

I'll fix and validate this carefully.

> Given that this could be quite a big number, we probably want to make
> timeleft a 64-bit type too. So we may want to define a 'u64 timeleft_usecs'
> and 'u32 timeleft_secs' that we return.
> 

We can do this.

> Jon
> 
> --
> nvpublic