| 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: <a6a45384-18b4-4500-8b8e-8cdd8df5ac1f@nvidia.com>
Date: Fri, 2 May 2025 13:37:25 +0100
From: Jon Hunter <jonathanh@...dia.com>
To: Daniel Lezcano <daniel.lezcano@...aro.org>
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 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@...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.
>>
>> 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
Powered by blists - more mailing lists