* [PATCH] dm: rtc: Avoid a race in the rtc_reset test @ 2022-07-31 18:27 Simon Glass 2022-08-01 8:11 ` Heinrich Schuchardt 0 siblings, 1 reply; 9+ messages in thread From: Simon Glass @ 2022-07-31 18:27 UTC (permalink / raw) To: U-Boot Mailing List Cc: Heinrich Schuchardt, Tom Rini, Simon Glass, Bin Meng, Rasmus Villemoes, Marek Vasut, Pavel Herrmann 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); 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); return 0; } -- 2.37.1.455.g008518b4e5-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] dm: rtc: Avoid a race in the rtc_reset test 2022-07-31 18:27 [PATCH] dm: rtc: Avoid a race in the rtc_reset test Simon Glass @ 2022-08-01 8:11 ` Heinrich Schuchardt 2022-08-01 13:59 ` Simon Glass 0 siblings, 1 reply; 9+ messages in thread From: Heinrich Schuchardt @ 2022-08-01 8:11 UTC (permalink / raw) To: Simon Glass Cc: Tom Rini, Bin Meng, Rasmus Villemoes, Marek Vasut, Pavel Herrmann, U-Boot Mailing List 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. Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > return 0; > } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dm: rtc: Avoid a race in the rtc_reset test 2022-08-01 8:11 ` Heinrich Schuchardt @ 2022-08-01 13:59 ` Simon Glass 2022-08-01 14:58 ` Heinrich Schuchardt 0 siblings, 1 reply; 9+ messages in thread From: Simon Glass @ 2022-08-01 13:59 UTC (permalink / raw) To: Heinrich Schuchardt Cc: Tom Rini, Bin Meng, Rasmus Villemoes, Marek Vasut, Pavel Herrmann, U-Boot Mailing List 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 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@gmx.de> > > > > > return 0; > > } > Regards, Simon ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dm: rtc: Avoid a race in the rtc_reset test 2022-08-01 13:59 ` Simon Glass @ 2022-08-01 14:58 ` Heinrich Schuchardt 2022-08-01 19:13 ` Simon Glass 0 siblings, 1 reply; 9+ messages in thread From: Heinrich Schuchardt @ 2022-08-01 14:58 UTC (permalink / raw) To: Simon Glass Cc: Tom Rini, Bin Meng, Rasmus Villemoes, Marek Vasut, Pavel Herrmann, U-Boot Mailing List 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. If you dual boot into Windows, you should better adjust Windows to use UTC. 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@gmx.de> >> >>> >>> return 0; >>> } >> > > Regards, > Simon ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dm: rtc: Avoid a race in the rtc_reset test 2022-08-01 14:58 ` Heinrich Schuchardt @ 2022-08-01 19:13 ` Simon Glass 2022-08-02 7:08 ` Rasmus Villemoes 2022-08-02 13:51 ` Heinrich Schuchardt 0 siblings, 2 replies; 9+ messages in thread From: Simon Glass @ 2022-08-01 19:13 UTC (permalink / raw) To: Heinrich Schuchardt Cc: Tom Rini, Bin Meng, Rasmus Villemoes, Marek Vasut, Pavel Herrmann, U-Boot Mailing List 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. As I 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@gmx.de> > >> > >>> > >>> return 0; > >>> } > >> > > > > Regards, > > Simon > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dm: rtc: Avoid a race in the rtc_reset test 2022-08-01 19:13 ` Simon Glass @ 2022-08-02 7:08 ` Rasmus Villemoes 2022-08-02 12:41 ` Simon Glass 2022-08-02 13:51 ` Heinrich Schuchardt 1 sibling, 1 reply; 9+ messages in thread From: Rasmus Villemoes @ 2022-08-02 7:08 UTC (permalink / raw) To: Simon Glass, Heinrich Schuchardt Cc: Tom Rini, Bin Meng, Marek Vasut, Pavel Herrmann, U-Boot Mailing List 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 :) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dm: rtc: Avoid a race in the rtc_reset test 2022-08-02 7:08 ` Rasmus Villemoes @ 2022-08-02 12:41 ` Simon Glass 0 siblings, 0 replies; 9+ messages in thread From: Simon Glass @ 2022-08-02 12:41 UTC (permalink / raw) To: Rasmus Villemoes Cc: Heinrich Schuchardt, Tom Rini, Bin Meng, Marek Vasut, Pavel Herrmann, U-Boot Mailing List Hi Rasmus, On Tue, 2 Aug 2022 at 01:08, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > > 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"? Me, at least. E.g. the 'date' command on sandbox shows the current local time. > > 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). OK, we have an is_dst but not many drivers actually set it, Anyway, it doesn't specify the offset. So if we wanted to support a DST offset we would have to add that to the API. The patches in this series deal with the 'second' problem. They happen to also deal with the DST issue mentioned by Heinrich in v1. So one option is just to remove any mention of DST, since the code changes are the same for the 'second' problem as the DST problem. For changing sandbox to use gmtime, we could add the concept of a timezone offset to U-Boot, along with an option to the 'date' command to allow GMT or local time. Then we could change sandbox to internally use gmtime(). Would that be useful? As I said, it doesn't really affect the code, as I see it, but I could certainly drop the DST comments if that helps. > > 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 :) From what I understand in the 1800s every town/city had its own time based on the position of the sun, so things were even worse! I'm not sure I like DST, actually. Perhaps I should move to Arizona? Regards, Simon ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dm: rtc: Avoid a race in the rtc_reset test 2022-08-01 19:13 ` Simon Glass 2022-08-02 7:08 ` Rasmus Villemoes @ 2022-08-02 13:51 ` Heinrich Schuchardt 2022-08-02 14:01 ` Simon Glass 1 sibling, 1 reply; 9+ messages in thread From: Heinrich Schuchardt @ 2022-08-02 13:51 UTC (permalink / raw) To: Simon Glass Cc: Tom Rini, Bin Meng, Rasmus Villemoes, Marek Vasut, Pavel Herrmann, U-Boot Mailing List On 8/1/22 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. 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@gmx.de> >>>> >>>>> >>>>> return 0; >>>>> } >>>> >>> >>> Regards, >>> Simon >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dm: rtc: Avoid a race in the rtc_reset test 2022-08-02 13:51 ` Heinrich Schuchardt @ 2022-08-02 14:01 ` Simon Glass 0 siblings, 0 replies; 9+ messages in thread From: Simon Glass @ 2022-08-02 14:01 UTC (permalink / raw) To: Heinrich Schuchardt Cc: Tom Rini, Bin Meng, Rasmus Villemoes, Marek Vasut, Pavel Herrmann, U-Boot Mailing List Hi Heinrich, On Tue, 2 Aug 2022 at 07:51, Heinrich Schuchardt <xypron.glpk@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@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. 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@gmx.de> > >>>> > >>>>> > >>>>> return 0; > >>>>> } > >>>> > >>> > >>> Regards, > >>> Simon > >> > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-08-02 14:02 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-07-31 18:27 [PATCH] dm: rtc: Avoid a race in the rtc_reset test Simon Glass 2022-08-01 8:11 ` Heinrich Schuchardt 2022-08-01 13:59 ` Simon Glass 2022-08-01 14:58 ` Heinrich Schuchardt 2022-08-01 19:13 ` Simon Glass 2022-08-02 7:08 ` Rasmus Villemoes 2022-08-02 12:41 ` Simon Glass 2022-08-02 13:51 ` Heinrich Schuchardt 2022-08-02 14:01 ` Simon Glass
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.