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

[Simon Glass]

Hi Heinrich,

On Tue, 2 Aug 2022 at 07:51, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> 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.

Right, that's what I'm wondering, whether we should add that.

>
> 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.

We'll have to disagree on that one. I find GM time just such a pain.

Let's review the series on its merits, then. I can drop or rephase the
comments if that helps.

This whole thing came about because you brought up the potential for a
test failure when the time zone changes. Let's just ignore that
problem as it is very rare, as you said.

Regards,
Simon

>
> 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
> >>
>