[PATCH iwl-net] idpf: fix read_dev_clk_lock spinlock init in idpf_ptp_init()

[PATCH iwl-net] idpf: fix read_dev_clk_lock spinlock init in idpf_ptp_init()

[Emil Tantilov]

In idpf_ptp_init(), read_dev_clk_lock is initialized after
ptp_schedule_worker() had already been called (and after
idpf_ptp_settime64() could reach the lock). The PTP aux worker
fires immediately upon scheduling and can call into
idpf_ptp_read_src_clk_reg_direct(), which takes
spin_lock(&ptp->read_dev_clk_lock) on an uninitialized lock, triggering
the lockdep "non-static key" warning:

[12973.796587] idpf 0000:83:00.0: Device HW Reset initiated
[12974.094507] INFO: trying to register non-static key.
...
[12974.097208] Call Trace:
[12974.097213]  <TASK>
[12974.097218]  dump_stack_lvl+0x93/0xe0
[12974.097234]  register_lock_class+0x4c4/0x4e0
[12974.097249]  ? __lock_acquire+0x427/0x2290
[12974.097259]  __lock_acquire+0x98/0x2290
[12974.097272]  lock_acquire+0xc6/0x310
[12974.097281]  ? idpf_ptp_read_src_clk_reg+0xb7/0x150 [idpf]
[12974.097311]  ? lockdep_hardirqs_on_prepare+0xde/0x190
[12974.097318]  ? finish_task_switch.isra.0+0xd2/0x350
[12974.097330]  ? __pfx_ptp_aux_kworker+0x10/0x10 [ptp]
[12974.097343]  _raw_spin_lock+0x30/0x40
[12974.097353]  ? idpf_ptp_read_src_clk_reg+0xb7/0x150 [idpf]
[12974.097373]  idpf_ptp_read_src_clk_reg+0xb7/0x150 [idpf]
[12974.097391]  ? kthread_worker_fn+0x88/0x3d0
[12974.097404]  ? kthread_worker_fn+0x4e/0x3d0
[12974.097411]  idpf_ptp_update_cached_phctime+0x26/0x120 [idpf]
[12974.097428]  ? _raw_spin_unlock_irq+0x28/0x50
[12974.097436]  idpf_ptp_do_aux_work+0x15/0x20 [idpf]
[12974.097454]  ptp_aux_kworker+0x20/0x40 [ptp]
[12974.097464]  kthread_worker_fn+0xd5/0x3d0
[12974.097474]  ? __pfx_kthread_worker_fn+0x10/0x10
[12974.097482]  kthread+0xf4/0x130
[12974.097489]  ? __pfx_kthread+0x10/0x10
[12974.097498]  ret_from_fork+0x32c/0x410
[12974.097512]  ? __pfx_kthread+0x10/0x10
[12974.097519]  ret_from_fork_asm+0x1a/0x30
[12974.097540]  </TASK>

Move the call to spin_lock_init() up a bit to make sure read_dev_clk_lock
is not touched before it's been initialized.

Fixes: 5cb8805d2366 ("idpf: negotiate PTP capabilities and get PTP clock")
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
---
 drivers/net/ethernet/intel/idpf/idpf_ptp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_ptp.c b/drivers/net/ethernet/intel/idpf/idpf_ptp.c
index eec91c4f0a75..4a51d2727547 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_ptp.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_ptp.c
@@ -952,6 +952,8 @@ int idpf_ptp_init(struct idpf_adapter *adapter)
 		goto free_ptp;
 	}
 
+	spin_lock_init(&adapter->ptp->read_dev_clk_lock);
+
 	err = idpf_ptp_create_clock(adapter);
 	if (err)
 		goto free_ptp;
@@ -977,8 +979,6 @@ int idpf_ptp_init(struct idpf_adapter *adapter)
 			goto remove_clock;
 	}
 
-	spin_lock_init(&adapter->ptp->read_dev_clk_lock);
-
 	pci_dbg(adapter->pdev, "PTP init successful\n");
 
 	return 0;
-- 
2.37.3

RE: [PATCH iwl-net] idpf: fix read_dev_clk_lock spinlock init in idpf_ptp_init()

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Tantilov, Emil S <emil.s.tantilov@intel.com>
> Sent: Friday, April 3, 2026 5:36 PM
> To: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; Loktionov, Aleksandr
> <aleksandr.loktionov@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>; andrew+netdev@lunn.ch;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; richardcochran@gmail.com; Olech, Milena
> <milena.olech@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>;
> Ilichev, Konstantin <konstantin.ilichev@intel.com>
> Subject: [PATCH iwl-net] idpf: fix read_dev_clk_lock spinlock init in
> idpf_ptp_init()
> 
> In idpf_ptp_init(), read_dev_clk_lock is initialized after
> ptp_schedule_worker() had already been called (and after
> idpf_ptp_settime64() could reach the lock). The PTP aux worker fires
> immediately upon scheduling and can call into
> idpf_ptp_read_src_clk_reg_direct(), which takes
> spin_lock(&ptp->read_dev_clk_lock) on an uninitialized lock,
> triggering the lockdep "non-static key" warning:
> 
> [12973.796587] idpf 0000:83:00.0: Device HW Reset initiated
> [12974.094507] INFO: trying to register non-static key.
> ...
> [12974.097208] Call Trace:
> [12974.097213]  <TASK>
> [12974.097218]  dump_stack_lvl+0x93/0xe0 [12974.097234]
> register_lock_class+0x4c4/0x4e0 [12974.097249]  ?
> __lock_acquire+0x427/0x2290 [12974.097259]  __lock_acquire+0x98/0x2290
> [12974.097272]  lock_acquire+0xc6/0x310 [12974.097281]  ?
> idpf_ptp_read_src_clk_reg+0xb7/0x150 [idpf] [12974.097311]  ?
> lockdep_hardirqs_on_prepare+0xde/0x190
> [12974.097318]  ? finish_task_switch.isra.0+0xd2/0x350
> [12974.097330]  ? __pfx_ptp_aux_kworker+0x10/0x10 [ptp] [12974.097343]
> _raw_spin_lock+0x30/0x40 [12974.097353]  ?
> idpf_ptp_read_src_clk_reg+0xb7/0x150 [idpf] [12974.097373]
> idpf_ptp_read_src_clk_reg+0xb7/0x150 [idpf] [12974.097391]  ?
> kthread_worker_fn+0x88/0x3d0 [12974.097404]  ?
> kthread_worker_fn+0x4e/0x3d0 [12974.097411]
> idpf_ptp_update_cached_phctime+0x26/0x120 [idpf] [12974.097428]  ?
> _raw_spin_unlock_irq+0x28/0x50 [12974.097436]
> idpf_ptp_do_aux_work+0x15/0x20 [idpf] [12974.097454]
> ptp_aux_kworker+0x20/0x40 [ptp] [12974.097464]
> kthread_worker_fn+0xd5/0x3d0 [12974.097474]  ?
> __pfx_kthread_worker_fn+0x10/0x10 [12974.097482]  kthread+0xf4/0x130
> [12974.097489]  ? __pfx_kthread+0x10/0x10 [12974.097498]
> ret_from_fork+0x32c/0x410 [12974.097512]  ? __pfx_kthread+0x10/0x10
> [12974.097519]  ret_from_fork_asm+0x1a/0x30 [12974.097540]  </TASK>
> 
> Move the call to spin_lock_init() up a bit to make sure
> read_dev_clk_lock is not touched before it's been initialized.
> 
> Fixes: 5cb8805d2366 ("idpf: negotiate PTP capabilities and get PTP
> clock")
> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
> Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
> ---
>  drivers/net/ethernet/intel/idpf/idpf_ptp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_ptp.c
> b/drivers/net/ethernet/intel/idpf/idpf_ptp.c
> index eec91c4f0a75..4a51d2727547 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_ptp.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_ptp.c
> @@ -952,6 +952,8 @@ int idpf_ptp_init(struct idpf_adapter *adapter)
>  		goto free_ptp;
>  	}
> 
> +	spin_lock_init(&adapter->ptp->read_dev_clk_lock);
> +
>  	err = idpf_ptp_create_clock(adapter);
>  	if (err)
>  		goto free_ptp;
> @@ -977,8 +979,6 @@ int idpf_ptp_init(struct idpf_adapter *adapter)
>  			goto remove_clock;
>  	}
> 
> -	spin_lock_init(&adapter->ptp->read_dev_clk_lock);
> -
>  	pci_dbg(adapter->pdev, "PTP init successful\n");
> 
>  	return 0;
> --
> 2.37.3

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

Re: [PATCH iwl-net] idpf: fix read_dev_clk_lock spinlock init in idpf_ptp_init()

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev

Simon says: I don't agree with the regression characterisation made by
the AI review - I think this patch is good. But I do think the issues
flagged by the AI review warrant investigation.

---
idpf: fix read_dev_clk_lock spinlock init in idpf_ptp_init()

This commit moves the initialization of read_dev_clk_lock earlier in
idpf_ptp_init() to ensure it is initialized before the PTP auxiliary
worker is scheduled.

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_ptp.c b/drivers/net/ethernet/intel/idpf/idpf_ptp.c
> index eec91c4f0a75a..4a51d2727547d 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_ptp.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_ptp.c
> @@ -952,6 +952,8 @@ int idpf_ptp_init(struct idpf_adapter *adapter)
>  		goto free_ptp;
>  	}
>  
> +	spin_lock_init(&adapter->ptp->read_dev_clk_lock);
> +
>  	err = idpf_ptp_create_clock(adapter);

This patch moves the lock initialization to prevent the worker from accessing
an uninitialized lock. However, idpf_ptp_create_clock() registers the PTP
device, and shortly after, ptp_schedule_worker() is called. Both happen
before the hardware clock baseline and initial time are set later in
idpf_ptp_init().

Could this introduce a regression where userspace opens the PTP device and
interacts with an uninitialized clock, or has its settings overwritten by
the driver's delayed idpf_ptp_settime64() call?

Additionally, could the scheduled worker read the uninitialized zero-time
and overwrite the software cache after the main thread sets the real time?

Moving the device registration and worker scheduling to the very end of
idpf_ptp_init() might resolve these races and naturally fix the lock
initialization ordering.

Also, since this code manages worker synchronization, is there a regression
when updating cached_phc_time and cached_phc_jiffies?

In idpf_ptp_update_cached_phctime(), these variables are updated
sequentially using the WRITE_ONCE() macro.

Because this function can be executed concurrently by the PTP worker
thread and ioctls, thread interleaving could result in an inconsistent
state where one variable is from one thread and the other from another.

Readers like idpf_ptp_extend_ts() load these variables using plain C
accesses without mutual exclusion or a seqlock.

Could this cause valid timestamps to be extended using an incorrect or
outdated baseline, silently corrupting network timestamps?

[ ... ]

Re: [Intel-wired-lan] [PATCH iwl-net] idpf: fix read_dev_clk_lock spinlock init in idpf_ptp_init()

[Tantilov, Emil S]

On 4/7/2026 9:02 AM, Simon Horman wrote:
> From: 'Simon Horman' <horms@kernel.org>
> 
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> 
> Simon says: I don't agree with the regression characterisation made by
> the AI review - I think this patch is good. But I do think the issues
> flagged by the AI review warrant investigation.

The point of the change is to resolve the use of uninitialized spinlock. 
The questions below appear to be generated around that code, which would
be out of scope for this patch, but I will address them anyway ...

> 
> ---
> idpf: fix read_dev_clk_lock spinlock init in idpf_ptp_init()
> 
> This commit moves the initialization of read_dev_clk_lock earlier in
> idpf_ptp_init() to ensure it is initialized before the PTP auxiliary
> worker is scheduled.
> 
>> diff --git a/drivers/net/ethernet/intel/idpf/idpf_ptp.c b/drivers/net/ethernet/intel/idpf/idpf_ptp.c
>> index eec91c4f0a75a..4a51d2727547d 100644
>> --- a/drivers/net/ethernet/intel/idpf/idpf_ptp.c
>> +++ b/drivers/net/ethernet/intel/idpf/idpf_ptp.c
>> @@ -952,6 +952,8 @@ int idpf_ptp_init(struct idpf_adapter *adapter)
>>   		goto free_ptp;
>>   	}
>>   
>> +	spin_lock_init(&adapter->ptp->read_dev_clk_lock);
>> +
>>   	err = idpf_ptp_create_clock(adapter);
> 
> This patch moves the lock initialization to prevent the worker from accessing
> an uninitialized lock. However, idpf_ptp_create_clock() registers the PTP
> device, and shortly after, ptp_schedule_worker() is called. Both happen
> before the hardware clock baseline and initial time are set later in
> idpf_ptp_init().
> 
> Could this introduce a regression where userspace opens the PTP device and
> interacts with an uninitialized clock, or has its settings overwritten by
> the driver's delayed idpf_ptp_settime64() call?

If such an issue existed it would not be because of this change (see my
point above). There is a check in idpf_ptp_settime64() that should 
protect against this case:
https://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue.git/tree/drivers/net/ethernet/intel/idpf/idpf_ptp.c?h=dev-queue#n424

	access = adapter->ptp->set_dev_clk_time_access;
	if (access != IDPF_PTP_MAILBOX)
		return -EOPNOTSUPP;

basically, the initial time write will only happen when the capability
is actually set.

> 
> Additionally, could the scheduled worker read the uninitialized zero-time
> and overwrite the software cache after the main thread sets the real time?
> 
> Moving the device registration and worker scheduling to the very end of
> idpf_ptp_init() might resolve these races and naturally fix the lock
> initialization ordering.

I think this is a valid observation, but I am not sure it would be a
problem in practice, because the vport queues are not receiving at that
point. I can take a look in more details, but if a fix is needed, it 
will not be part of this patch.

> 
> Also, since this code manages worker synchronization, is there a regression
> when updating cached_phc_time and cached_phc_jiffies?
> 
> In idpf_ptp_update_cached_phctime(), these variables are updated
> sequentially using the WRITE_ONCE() macro.
> 
> Because this function can be executed concurrently by the PTP worker
> thread and ioctls, thread interleaving could result in an inconsistent
> state where one variable is from one thread and the other from another.
> 
> Readers like idpf_ptp_extend_ts() load these variables using plain C
> accesses without mutual exclusion or a seqlock.
> 
> Could this cause valid timestamps to be extended using an incorrect or
> outdated baseline, silently corrupting network timestamps?

Not sure why the AI went on the "regression" tangent here. I think it is
obvious none of these cases are regressions caused by the patch. That 
being said, I think this looks like a valid concern, but again, will be
addressed via separate patch if needed.

Thanks,
Emil

> 
> [ ... ]

Re: [Intel-wired-lan] [PATCH iwl-net] idpf: fix read_dev_clk_lock spinlock init in idpf_ptp_init()

[Simon Horman]

On Tue, Apr 07, 2026 at 03:00:22PM -0700, Tantilov, Emil S wrote:
> 
> 
> On 4/7/2026 9:02 AM, Simon Horman wrote:
> > From: 'Simon Horman' <horms@kernel.org>
> > 
> > This is an AI-generated review of your patch. The human sending this
> > email has considered the AI review valid, or at least plausible.
> > Full review at: https://sashiko.dev
> > 
> > Simon says: I don't agree with the regression characterisation made by
> > the AI review - I think this patch is good. But I do think the issues
> > flagged by the AI review warrant investigation.
> 
> The point of the change is to resolve the use of uninitialized spinlock. The
> questions below appear to be generated around that code, which would
> be out of scope for this patch, but I will address them anyway ...

Right, I agree with that general statement on the review: it muddles
up potential problems in nearby code, with problems introduced by
your patch (none seen).

I do thank you for analysing the problems raised. And I'll leave it up
to you to provide follow-up patches as you see fit.

For this patch, I think we are good.

Reviewed-by: Simon Horman <horms@kernel.org>