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;
+
+ /* Get the current counter value in microsecond. */
+ val = readl_relaxed(wdt->tmr->regs + TMRSR);
+ timeleft = 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 += (((u64)wdt->base.timeout * USEC_PER_SEC) / 5) * (4 - expiration);
+
+ /*
+ * 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;
+
+ if (WARN_ON(timeleft > U32_MAX))
+ return U32_MAX;
+
+ return lower_32_bits(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,
--
2.34.1On 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.
> + /* Get the current counter value in microsecond. */
> + val = readl_relaxed(wdt->tmr->regs + TMRSR);
> + timeleft = 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 += (((u64)wdt->base.timeout * USEC_PER_SEC) / 5) * (4 - expiration);
> +
> + /*
> + * 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 ?
> + if (WARN_ON(timeleft > U32_MAX))
s/WARN_ON/pr_err/
> + return U32_MAX;
> +
> + return lower_32_bits(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,
> --
> 2.34.1
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
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.
>
>> + /* Get the current counter value in microsecond. */
>> + val = readl_relaxed(wdt->tmr->regs + TMRSR);
>> + timeleft = 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 += (((u64)wdt->base.timeout * USEC_PER_SEC) / 5) * (4 - expiration);
>> +
>> + /*
>> + * 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.
>
>> + if (WARN_ON(timeleft > U32_MAX))
>
> s/WARN_ON/pr_err/
Why? Again this is not expected. I think that this is fine.
>
>> + return U32_MAX;
>> +
>> + return lower_32_bits(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,
>> --
>> 2.34.1
>>
>
--
nvpublic
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.
[ ... ]
> > > + /*
> > > + * 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()' ?
> > > + if (WARN_ON(timeleft > U32_MAX))
> >
> > s/WARN_ON/pr_err/
>
> Why? Again this is not expected. I think that this is fine.
[ ... ]
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
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
On 02/05/2025 13:06, Jon Hunter wrote:
>
>
> 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.
The issue is if there is an userspace application reading the ioctl and
or the sysfs, then the warning will be emitted each time if the
never-happen condition exists. Preferably replace the WARN_ON by
pr_warn_once() as suggested if the bug must be reported.
>>>>> + /*
>>>>> + * 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.
Ok, thanks for checking
-- Daniel
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 02/05/2025 12:29, Daniel Lezcano wrote:
> On 02/05/2025 13:06, Jon Hunter wrote:
>>
>>
>> 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.
>
> The issue is if there is an userspace application reading the ioctl and
> or the sysfs, then the warning will be emitted each time if the never-
> happen condition exists. Preferably replace the WARN_ON by
> pr_warn_once() as suggested if the bug must be reported.
Sounds a bit funny 'if the never-happen condition exists' :-)
However, I will be fine with WARN_ON_ONCE(). I think that this warrants
more of a large WARN splat than pr_warn() because it should never happen.
Jon
--
nvpublic
© 2016 - 2026 Red Hat, Inc.