Re: [PATCH v4 1/3] clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support

[Jon Hunter <jonathanh@nvidia.com>]

On 17/04/2025 12:34, Robert Lin wrote:
> 
> 
>> -----Original Message-----
>> From: Jon Hunter <jonathanh@nvidia.com>
>> Sent: Thursday, April 17, 2025 7:10 PM
>> To: Robert Lin <robelin@nvidia.com>; thierry.reding@gmail.com;
>> daniel.lezcano@linaro.org; tglx@linutronix.de; Pohsun Su
>> <pohsuns@nvidia.com>
>> Cc: linux-kernel@vger.kernel.org; linux-tegra@vger.kernel.org; Sumit Gupta
>> <sumitg@nvidia.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@nvidia.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@nvidia.com>
>>> Signed-off-by: Robert Lin <robelin@nvidia.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.


I was looking at the code, and if it is 4, then the following ...

timeleft += wdt->base.timeout * (4 - expiration) / 5;

... becomes 0. However, given that in this case something very bad has 
happened, then the other alternative may be to just ...

     if (WARN_ON(expiration > 4)
             return 0;

Jon

-- 
nvpublic