On Sat, 2026-10-31 at 01:34 -0700, Simon Glass wrote: > Hi Felix, > > On 2026-10-30T14:38:40+11:00, Felix Moessbauer > <[email protected]> wrote: > > efi_selftest: add test for EFI_TIMESTAMP_PROTOCOL > > > > Make sure the protocol is usable: the counter must advance at roughly > > the reported frequency, otherwise applications get wrong time > > measurements. > > I don't think the test can actually check this. See below. > > > diff --git a/lib/efi_selftest/efi_selftest_timestamp.c > > b/lib/efi_selftest/efi_selftest_timestamp.c > > @@ -0,0 +1,99 @@ > > + t0 = ts->get_timestamp(); > > + ret = boottime->stall(10000); > > + if (ret != EFI_SUCCESS) { > > + efi_st_error("Stall failed\n"); > > + return EFI_ST_FAILURE; > > + } > > + t1 = ts->get_timestamp(); > > + > > + if (t1 <= t0) { > > + efi_st_error("Timestamp did not advance\n"); > > + return EFI_ST_FAILURE; > > + } > > + /* 10 ms correspond to frequency / 100 ticks, allow 50 % slack */ > > + expected = props.frequency / 100; > > + if (t1 - t0 < expected / 2) { > > + efi_st_error("Timestamp advanced too slowly\n"); > > + return EFI_ST_FAILURE; > > + } > > This check is circular. efi_stall() sets 'end_tick = get_ticks() + > usec_to_tick(microseconds)' and spins until get_ticks() reaches it, > and usec_to_tick() uses get_tbclk(). So when stall() returns, the > counter has already advanced by at least frequency / 100 ticks, > whatever the real frequency is. The check can only fail if > get_timestamp() stops using get_ticks(); a wrong get_tbclk() value > (the case the commit message describes) still passes. The 50% slack > suggests tolerance for a real-time measurement, but there isn't one > here.
Right... thanks for pointing out. > > Checking the frequency needs an independent time source, e.g. the > runtime get_time() service when CONFIG_EFI_GET_TIME is enabled, > measuring over a second or two, though that makes the test slower. > Otherwise, please can you reword the commit message and the comment to > claim only that the counter is monotonic and consistent with Stall(), > and drop the slack, since stall() already guarantees 't1 - t0 >= > expected'. What do you think? If a test duration of ~1.2 seconds is fine, I'll just implement the comparison with get_time(). Just checking for monotonicity IMHO does not make much sense. > > There is also no upper bound, so a reported frequency that is far too > low would not be caught either way. Yes, but with your proposed 1s+ test this would be correctly detected. Felix > > > > > Signed-off-by: Felix Moessbauer <[email protected]> > > > > lib/efi_selftest/Makefile | 1 + > > lib/efi_selftest/efi_selftest_timestamp.c | 99 > > +++++++++++++++++++++++++++++++ > > 2 files changed, 100 insertions(+) > > Regards, > Simon
