Re: [PATCH] timekeeping: Always initialize use_nsecs when querying time from phc drivers

[Thomas Gleixner]

On Wed, Jul 09 2025 at 10:32, Markus Blöchl wrote:
> On Tue, Jul 08, 2025 at 12:09:40PM -0700, John Stultz wrote:
>> On Tue, Jul 8, 2025 at 9:46 AM Markus Blöchl
>> <markus.bloechl@xxxxxxxxxxxxx> wrote:
>> >
>> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
>> > index a009c91f7b05..be0da807329f 100644
>> > --- a/kernel/time/timekeeping.c
>> > +++ b/kernel/time/timekeeping.c
>> > @@ -1269,6 +1269,8 @@ int get_device_system_crosststamp(int (*get_time_fn)
>> >
>> >         do {
>> >                 seq = read_seqcount_begin(&tk_core.seq);
>> > +               system_counterval.use_nsecs = false;
>> > +
>>
>> So if the argument is the local system_counterval structure isn't
>> being fully initialized by the get_time_fn() functions it is passed
>> to, it seems like it would be better to do so at the top of
>> get_device_system_crosststamp(), and not inside the seqloop.
>
> Probably, I was just afraid of the case where get_time_fn() would take
> like *very* different paths during different iterations.
> But that seems really unlikely, indeed.

It's impossible. xtstamp->device and the related get_time_fn() are
immutable during the call.

>> But having the responsibility to initialize/fill in the structure
>> being split across the core and the implementation logic (leaving some
>> of the fields as optional) feels prone to mistakes, so it makes me
>> wonder if those drivers implementing the get_time_fn() really ought to
>> fully fill out the structure, and thus the fix would be better done in
>> those drivers.
>
> Yes, they should.

No, they should not.

The data structure is instantiated in get_device_system_crosststamp()
and then handed in un-initialized to get_time_fn(), which is wrong to
begin with. Why?

That means if the structure is ever expanded, then you'd have to fix up
all of the get_time_fn() implementations.

Seriously?

The obviously correct and future proof thing to do is:

-	struct system_counterval_t system_counterval;
+	struct system_counterval_t system_counterval = { };

Which fixes the problem you discovered once and forever, no?

Thanks,

        tglx