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

[Heinrich Schuchardt]

On 8/1/22 21:13, Simon Glass wrote:
> Hi Heinrich,
>
> On Mon, 1 Aug 2022 at 08:58, Heinrich Schuchardt <xypron.glpk at 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 at 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 at gmail.com>
>>>>> Reported-by: Tom Rini <trini at konsulko.com>
>>>>> Suggested-by: Rasmus Villemoes <rasmus.villemoes at prevas.dk>
>>>>> Signed-off-by: Simon Glass <sjg at 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. As I

U-Boot neither knows the time zone of the user nor the time zone of the
RTC not does it hold the time zone definitions needed to manage daylight
saving time.

The idea of showing local time is doomed.

Typically the RTC will be using UTC and not local time. The sandbox
should do the same.

Best regards

Heinrich

> mentioned, we could expand the API to allow control over that, e.g. to
> show local times but use UTC in tests?
>
>>
>> If you dual boot into Windows, you should better adjust Windows to use UTC.
>
> I'm not the world's most dedicated Windows user.
>
> Regards,
> Simon
>
>
>>
>> Best regards
>>
>> Heinrich
>>
>>> test).
>>>
>>> I've got an idea how to deal with daylight savings, and an alternative
>>> approach for this patch, so will send a series.
>>>
>>>>
>>>> Reviewed-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>>>
>>>>>
>>>>>         return 0;
>>>>>     }
>>>>
>>>
>>> Regards,
>>> Simon
>>