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

[Jon Hunter <jonathanh@nvidia.com>]

On 02/05/2025 11:51, Daniel Lezcano wrote:
> On Fri, May 02, 2025 at 11:16:17AM +0100, Jon Hunter wrote:
>>
>>
>> On 02/05/2025 10:19, Daniel Lezcano wrote:
>>> On Fri, May 02, 2025 at 12:37:25PM +0800, 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 | 64 +++++++++++++++++++++++++++-
>>>>    1 file changed, 63 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/clocksource/timer-tegra186.c
>>>> index ea742889ee06..8d5698770cbd 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,69 @@ 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 expiration, val;
>>>> +	u64 timeleft;
>>>> +
>>>> +	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;
>>>
>>> Each call will generate a big warning in the message. May be simpler
>>> to add a pr_err() with an explicit message.
>>
>> I prefer the WARN. This should never happen. If we do change this, then
>> dev_WARN() might be more appropriate, but I think that this is fine too.
> 
> The function tegra186_wdt_get_timeleft() is triggered from userspace
> via an ioctl or sysfs. The documentation process/coding-style.rst says:
> 
> """
> Do not WARN lightly
> *******************
> 
> WARN*() is intended for unexpected, this-should-never-happen situations.
> WARN*() macros are not to be used for anything that is expected to happen
> during normal operation. These are not pre- or post-condition asserts, for
> example. Again: WARN*() must not be used for a condition that is expected
> to trigger easily, for example, by user space actions. pr_warn_once() is a
> possible alternative, if you need to notify the user of a problem.
> """
> 
> I understand it is important to check the return value in order to
> have a consistent result when computing the remaining time but that
> should not trigger a warning each time.

Yes so WARN sounds appropriate. It should never happen. I don't see the 
issue.

>>>> +	/*
>>>> +	 * Convert the current counter value to seconds,
>>>> +	 * rounding up to the nearest second. Cast u64 to
>>>> +	 * u32 under the assumption that no overflow happens
>>>> +	 * when coverting to seconds.
>>>> +	 */
>>>> +	timeleft = (timeleft + USEC_PER_SEC / 2) / USEC_PER_SEC;
>>>
>>> Did you check there is a macro fitting the need in math.h ?
>>
>> I had a quick look, but looking again, I see we can use
>> DIV_ROUND_CLOSEST_ULL() here.
> 
> What about 'roundup()' ?

'roundup' does not look the same as what is being done above. However, 
DIV_ROUND_CLOSEST_ULL() does.

-- 
nvpublic