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

[Jon Hunter]

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.

Jon

--
nvpublic