[PATCH] x86/tsc: Fix misplaced seqcount_latch_init() in cyc2ns_init_secondary_cpus()

[PATCH] x86/tsc: Fix misplaced seqcount_latch_init() in cyc2ns_init_secondary_cpus()

[Bo Li]

In cyc2ns_init_secondary_cpus(), seqcount_latch_init(&c2n->seq) is
called _before_ c2n is advanced to the next CPU via per_cpu_ptr().
As a result:

 1. On the first iteration, c2n still points at the BSP's struct, so
    the BSP's seqcount_latch is re-initialized. cyc2ns_init_boot_cpu()
    already did this correctly, so it happens to work by accident
    because no concurrent readers are live at __init time.

 2. On subsequent iterations, seqcount_latch_init() initializes the
    previous CPU's seqcount (because c2n was advanced by the prior
    per_cpu_ptr()), so all CPUs except the last one in the for_each
    loop happen to get initialized. The last secondary CPU's seqcount
    is left uninitialized.

Move seqcount_latch_init() _after_ c2n is pointed at the target CPU's
struct, so each secondary CPU's seqcount is correctly initialized.

Fixes: e2a9ca29b5ed ("x86/tsc: Initialize cyc2ns when tsc frequency is determined")
Cc: stable@vger.kernel.org
Signed-off-by: Bo Li <libo.gcs85@bytedance.com>
---
 arch/x86/kernel/tsc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index ce10ae4b298b..84fb80492b01 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -223,8 +223,8 @@ static void __init cyc2ns_init_secondary_cpus(void)
 
 	for_each_possible_cpu(cpu) {
 		if (cpu != this_cpu) {
-			seqcount_latch_init(&c2n->seq);
 			c2n = per_cpu_ptr(&cyc2ns, cpu);
+			seqcount_latch_init(&c2n->seq);
 			c2n->data[0] = data[0];
 			c2n->data[1] = data[1];
 		}
-- 
2.20.1

Re: [PATCH] x86/tsc: Fix misplaced seqcount_latch_init() in cyc2ns_init_secondary_cpus()

[Peter Zijlstra]

On Tue, Aug 04, 2026 at 03:11:28PM +0800, Bo Li wrote:
> In cyc2ns_init_secondary_cpus(), seqcount_latch_init(&c2n->seq) is
> called _before_ c2n is advanced to the next CPU via per_cpu_ptr().
> As a result:
> 
>  1. On the first iteration, c2n still points at the BSP's struct, so
>     the BSP's seqcount_latch is re-initialized. cyc2ns_init_boot_cpu()
>     already did this correctly, so it happens to work by accident
>     because no concurrent readers are live at __init time.
> 
>  2. On subsequent iterations, seqcount_latch_init() initializes the
>     previous CPU's seqcount (because c2n was advanced by the prior
>     per_cpu_ptr()), so all CPUs except the last one in the for_each
>     loop happen to get initialized. The last secondary CPU's seqcount
>     is left uninitialized.
> 
> Move seqcount_latch_init() _after_ c2n is pointed at the target CPU's
> struct, so each secondary CPU's seqcount is correctly initialized.
> 
> Fixes: e2a9ca29b5ed ("x86/tsc: Initialize cyc2ns when tsc frequency is determined")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bo Li <libo.gcs85@bytedance.com>
> ---
>  arch/x86/kernel/tsc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index ce10ae4b298b..84fb80492b01 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -223,8 +223,8 @@ static void __init cyc2ns_init_secondary_cpus(void)
>  
>  	for_each_possible_cpu(cpu) {
>  		if (cpu != this_cpu) {
> -			seqcount_latch_init(&c2n->seq);
>  			c2n = per_cpu_ptr(&cyc2ns, cpu);
> +			seqcount_latch_init(&c2n->seq);
>  			c2n->data[0] = data[0];
>  			c2n->data[1] = data[1];
>  		}

No, this looks wrong. Note that per the 'cpu != this_cpu' there are only
N-1 invocations. One CPU will not be initialized. Further note the name
of this function, and the name of the function above it.

Re: [PATCH] x86/tsc: Fix misplaced seqcount_latch_init() in cyc2ns_init_secondary_cpus()

[Bo Li]

On 8/4/26 3:22 PM, Peter Zijlstra wrote:
> On Tue, Aug 04, 2026 at 03:11:28PM +0800, Bo Li wrote:
>> In cyc2ns_init_secondary_cpus(), seqcount_latch_init(&c2n->seq) is
>> called _before_ c2n is advanced to the next CPU via per_cpu_ptr().
>> As a result:
>>
>>   1. On the first iteration, c2n still points at the BSP's struct, so
>>      the BSP's seqcount_latch is re-initialized. cyc2ns_init_boot_cpu()
>>      already did this correctly, so it happens to work by accident
>>      because no concurrent readers are live at __init time.
>>
>>   2. On subsequent iterations, seqcount_latch_init() initializes the
>>      previous CPU's seqcount (because c2n was advanced by the prior
>>      per_cpu_ptr()), so all CPUs except the last one in the for_each
>>      loop happen to get initialized. The last secondary CPU's seqcount
>>      is left uninitialized.
>>
>> Move seqcount_latch_init() _after_ c2n is pointed at the target CPU's
>> struct, so each secondary CPU's seqcount is correctly initialized.
>>
>> Fixes: e2a9ca29b5ed ("x86/tsc: Initialize cyc2ns when tsc frequency is determined")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Bo Li <libo.gcs85@bytedance.com>
>> ---
>>   arch/x86/kernel/tsc.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
>> index ce10ae4b298b..84fb80492b01 100644
>> --- a/arch/x86/kernel/tsc.c
>> +++ b/arch/x86/kernel/tsc.c
>> @@ -223,8 +223,8 @@ static void __init cyc2ns_init_secondary_cpus(void)
>>   
>>   	for_each_possible_cpu(cpu) {
>>   		if (cpu != this_cpu) {
>> -			seqcount_latch_init(&c2n->seq);
>>   			c2n = per_cpu_ptr(&cyc2ns, cpu);
>> +			seqcount_latch_init(&c2n->seq);
>>   			c2n->data[0] = data[0];
>>   			c2n->data[1] = data[1];
>>   		}
> 
> No, this looks wrong. Note that per the 'cpu != this_cpu' there are only
> N-1 invocations. One CPU will not be initialized. Further note the name
> of this function, and the name of the function above it.
> 

Agreed - the loop is N-1 and deliberately skips this_cpu, which is
initialized by cyc2ns_init_boot_cpu(). That CPU is not my concern.
Sorry, my changelog was unclear. The issue is an off-by-one in the
existing code: seqcount_latch_init(&c2n->seq) runs before c2n is
advanced to the target CPU, so each iteration initializes the seqcount
of the previous c2n rather than the current one:

	c2n = this_cpu_ptr(&cyc2ns);		/* c2n -> boot cpu */
	for_each_possible_cpu(cpu) {
		if (cpu != this_cpu) {
			seqcount_latch_init(&c2n->seq);   /* inits the previous c2n */
			c2n = per_cpu_ptr(&cyc2ns, cpu);  /* only now advanced */
			...
		}
	}

Concretely, with this_cpu=0 and possible cpus {0,1,2,3}:
	cpu=1: inits cpu0 (boot, already done by cyc2ns_init_boot_cpu())
	cpu=2: inits cpu1
	cpu=3: inits cpu2
	=> cpu3, the last secondary, is never initialized, while boot is
	   redundantly re-initialized.

Moving seqcount_latch_init() after per_cpu_ptr() makes each secondary
initialize its own seqcount.