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

[Jon Hunter <jonathanh@nvidia.com>]

On 29/04/2025 07:45, 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 | 60 +++++++++++++++++++++++++++-
>   1 file changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/clocksource/timer-tegra186.c
> index ea742889ee06..800f6650ebd4 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,65 @@ 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;
> +	u64 timeleft_us;
> +
> +	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;

There is a problem with the indent here ...

WARNING: suspect code indent for conditional statements (8, 13)
#83: FILE: drivers/clocksource/timer-tegra186.c:270:
+	if (WARN_ON(expiration > 4))
+             return 0;

ERROR: code indent should use tabs where possible
#84: FILE: drivers/clocksource/timer-tegra186.c:271:
+             return 0;$

WARNING: please, no spaces at the start of a line
#84: FILE: drivers/clocksource/timer-tegra186.c:271:
+             return 0;$

> +
> +	/* Get the current counter value in microsecond.
> +	 */

No newline needed here, the comment can fit on a single line ...

        /* Get the current counter value in microsecond. */

> +	val = readl_relaxed(wdt->tmr->regs + TMRSR);
> +	timeleft_us = FIELD_GET(TMRSR_PCV, val);
> +
> +	/*
> +	 * Calculate the time remaining by adding the time for the
> +	 * counter value to the time of the counter expirations that
> +	 * remain.
> +	 */
> +	timeleft_us += (((u64)wdt->base.timeout * USEC_PER_SEC) / 5) * (4 - expiration);

There should be a newline here.


> +	/*
> +	 * 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_us + USEC_PER_SEC / 2) / USEC_PER_SEC;

I am always a bit concerned about possible overflows that can be 
silently missed. So probably best to make 'timeleft' a u64 too and then ...

  timeleft = (timeleft_us + USEC_PER_SEC / 2) / USEC_PER_SEC;

  if (WARN_ON(timeleft > U32_MAX))
          return U32_MAX;

  return lower_32_bits(timeleft);

> +	return timeleft;
> +}
> +
>   static const struct watchdog_ops tegra186_wdt_ops = {
>   	.owner = THIS_MODULE,
>   	.start = tegra186_wdt_start,
>   	.stop = tegra186_wdt_stop,
>   	.ping = tegra186_wdt_ping,
>   	.set_timeout = tegra186_wdt_set_timeout,
> +	.get_timeleft = tegra186_wdt_get_timeleft,
>   };
>   
>   static struct tegra186_wdt *tegra186_wdt_create(struct tegra186_timer *tegra,

-- 
nvpublic