Re: [PATCH] dm: rtc: Avoid a race in the rtc_reset test

[Rasmus Villemoes <rasmus.villemoes@prevas.dk>]

On 01/08/2022 21.13, Simon Glass wrote:
> Hi Heinrich,
> 
> On Mon, 1 Aug 2022 at 08:58, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 8/1/22 15:59, Simon Glass wrote:
>>> Hi Heinrich,
>>>
>>> On Mon, 1 Aug 2022 at 02:11, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>
>>>> On 7/31/22 20:27, Simon Glass wrote:
>>>>> Since resetting the RTC on sandbox causes it to read the base time from
>>>>> the system, we cannot rely on this being unchanged since it was last read.
>>>>> Allow for a one-second delay.
>>>>>
>>>>> Fixes: https://source.denx.de/u-boot/u-boot/-/issues/4
>>>>> Reported-by: Bin Meng <bmeng.cn@gmail.com>
>>>>> Reported-by: Tom Rini <trini@konsulko.com>
>>>>> Suggested-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>> ---
>>>>>
>>>>>    test/dm/rtc.c | 11 ++++++++---
>>>>>    1 file changed, 8 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/test/dm/rtc.c b/test/dm/rtc.c
>>>>> index c7f9f8f0ce7..403bf5c640a 100644
>>>>> --- a/test/dm/rtc.c
>>>>> +++ b/test/dm/rtc.c
>>>>> @@ -245,16 +245,21 @@ static int dm_test_rtc_reset(struct unit_test_state *uts)
>>>>>        ut_assertok(dm_rtc_get(dev, &now));
>>>>>
>>>>>        ut_assertok(i2c_emul_find(dev, &emul));
>>>>> -     ut_assert(emul != NULL);
>>>>> +     ut_assertnonnull(emul);
>>>>
>>>> This is an unrelated change. It would be preferable to describe it in
>>>> the commit message.
>>>>
>>>>>
>>>>>        old_base_time = sandbox_i2c_rtc_get_set_base_time(emul, 0);
>>>>>
>>>>>        ut_asserteq(0, sandbox_i2c_rtc_get_set_base_time(emul, -1));
>>>>>
>>>>> -     /* Resetting the RTC should put he base time back to normal */
>>>>> +     /*
>>>>> +      * Resetting the RTC should put the base time back to normal. Allow for
>>>>> +      * a one-second adjustment in case the time flips over while this
>>>>> +      * test process is pre-empted, since reset_time() in i2c_rtc_emul.c
>>>>> +      * reads the time from the OS.
>>>>> +      */
>>>>>        ut_assertok(dm_rtc_reset(dev));
>>>>>        base_time = sandbox_i2c_rtc_get_set_base_time(emul, -1);
>>>>> -     ut_asserteq(old_base_time, base_time);
>>>>> +     ut_assert(base_time - old_base_time <= 1);
>>>>
>>>> If the operating system uses daylight saving time, this may still fail
>>>> (very rarely).
>>>>
>>>> How about using gmtime() instead of localtime()? But that would be a
>>>> separate patch.
>>>
>>> I'm not sure how to do this, as U-Boot expects local time. Of course
>>> we could enhance the rtc API to support both (and use gmtime for the
>>
>> What makes you think that U-Boot expects local time? U-Boot has no
>> notion of time zones. Linux systems tend to use UTC on the RTC. Why
>> should sandbox_defconfig deviate?
>>
>> $ sudo hwclock --show -v
>> hwclock from util-linux 2.38
>> System Time: 1659365754.792540
>> Trying to open: /dev/rtc0
>> Using the rtc interface to the clock.
>> Assuming hardware clock is kept in UTC time.
> 
> Well the thing is, we want to show local times in U-Boot.

Who's "we"?

I'm with Heinrich. I'd much rather U-Boot showed the time actually
stored in the RTC, and if we emulate an RTC, that RTC should emulate
what a real RTC does, namely store UTC. If you're worried about that
that might confuse somebody, there's no harm adding " UTC" or "+00" or
whatever to the output-for-humans (wherever that is).

Especially when doing anything else causes weird and still-fragile hacks
to be sprinkled throughout the testing code (that DST hack is ugly, and
the "only twice a year" isn't accurate, because something on the host
may also change /etc/localtime at any time).

Rasmus

PS: I loved working in Iceland. Apart from the beautiful nature, their
timezone is UTC+0 all year round, so all questions of UTC vs localtime
vs DST were moot :)