| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aBSjyZJ0__ctBa-4@mai.linaro.org>
Date: Fri, 2 May 2025 12:51:53 +0200
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: Jon Hunter <jonathanh@...dia.com>
Cc: Robert Lin <robelin@...dia.com>, thierry.reding@...il.com,
tglx@...utronix.de, pohsuns@...dia.com,
linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org,
sumitg@...dia.com
Subject: Re: [PATCH v7 1/3] clocksource/drivers/timer-tegra186: add
WDIOC_GETTIMELEFT support
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@...dia.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@...dia.com>
> > > Signed-off-by: Robert Lin <robelin@...dia.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
Powered by blists - more mailing lists