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

[Robert Lin]

> -----Original Message-----
> From: Jon Hunter <jonathanh@xxxxxxxxxx>
> Sent: Friday, May 2, 2025 8:37 PM
> To: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> Cc: Robert Lin <robelin@xxxxxxxxxx>; thierry.reding@xxxxxxxxx;
> tglx@xxxxxxxxxxxxx; Pohsun Su <pohsuns@xxxxxxxxxx>; linux-
> kernel@xxxxxxxxxxxxxxx; linux-tegra@xxxxxxxxxxxxxxx; Sumit Gupta
> <sumitg@xxxxxxxxxx>
> Subject: Re: [PATCH v7 1/3] clocksource/drivers/timer-tegra186: add
> WDIOC_GETTIMELEFT support
> 
> 
> 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@xxxxxxxxxx>
> >>>>>>
> >>>>>> 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@xxxxxxxxxx>
> >>>>>> Signed-off-by: Robert Lin <robelin@xxxxxxxxxx>
> >>>>>> ---
> >>>>>>    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.
> 

I believe the two WARN_ON I used here are the "this-should-never-happen" situations. In my opinion, we are good to keep. But I am also good with WARN_ON_ONCE for compromise. I wonder if Daniel is good with this then I can fix them.

> Jon
> 
> --
> nvpublic