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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <93f063fa-0012-4018-8c37-a2bfa623d2bf@linaro.org>
Date: Fri, 2 May 2025 13:29:12 +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 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.

>>>>> +    /*
>>>>> +     * 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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ