linux-kernel - RE: [PATCH v4 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:
 <PH7PR12MB641816B01C956C123FDC5C09D9BC2@PH7PR12MB6418.namprd12.prod.outlook.com>
Date: Thu, 17 Apr 2025 11:34:48 +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 v4 1/3] clocksource/drivers/timer-tegra186: add
 WDIOC_GETTIMELEFT support



> -----Original Message-----
> From: Jon Hunter <jonathanh@...dia.com>
> Sent: Thursday, April 17, 2025 7:10 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 v4 1/3] clocksource/drivers/timer-tegra186: add
> WDIOC_GETTIMELEFT support
> 
> 
> On 17/04/2025 10:31, 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 | 56
> +++++++++++++++++++++++++++-
> >   1 file changed, 55 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clocksource/timer-tegra186.c
> > b/drivers/clocksource/timer-tegra186.c
> > index ea742889ee06..201b24ca59f4 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,61 @@
> 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);
> 
> The above says 'should be between 0 and 4', however, we never check.
> 
> I am wondering if we should ...
> 
> if (WARN_ON(expiration > 4)
>      expiration = 4;
> 
> To avoid any overflow later on.
> 

Warning for the bad value seems to be good. But for the part to forcibly bound the value to 4, I'm not sure if this makes sense. Using the bad value from WDTSR or 4 both lead to wrong timeleft value at the end.

> 
> --
> nvpublic