Re: [Intel-wired-lan] [PATCH iwl-net v1] i40e: fix races in PTP external timestamp work handling
Vadim Fedorenko <[email protected]> Wed, 5 Aug 2026 10:49:23 +0100
| Newsgroups | org.osuosl.intel-wired-lan,org.kernel.vger.netdev |
|---|---|
| Message-ID | <[email protected]> |
On 05/08/2026 08:13, luoxuanqiang wrote: > > 在 2026/8/5 05:33, Vadim Fedorenko 写道: >> On 24/07/2026 10:34, [email protected] wrote: >>> >>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ptp.c b/drivers/ >>> net/ethernet/intel/i40e/i40e_ptp.c >>> index ff62b5f2c8150..fd51ab8c10c20 100644 >>> --- a/drivers/net/ethernet/intel/i40e/i40e_ptp.c >>> +++ b/drivers/net/ethernet/intel/i40e/i40e_ptp.c >>> @@ -169,6 +169,17 @@ static void i40e_ptp_extts0_work(struct >>> work_struct *work) >>> ptp_clock_event(pf->ptp_clock, &event); >>> } >>> +/** >>> + * i40e_ptp_init_work - Initialize PTP work for a PF >>> + * @pf: Board private structure >>> + * >>> + * Initialize work which must remain valid for the lifetime of the PF. >>> + */ >>> +void i40e_ptp_init_work(struct i40e_pf *pf) >>> +{ >>> + INIT_WORK(&pf->ptp_extts0_work, i40e_ptp_extts0_work); >>> +} >>> + >> >> why do you need extra function for a single line of code? why cannot >> you put it into i40e_ptp_init? >> >> > Thanks for pointing this out. > > i40e_ptp_init() is also called from the reset/rebuild path: > > i40e_service_task() > -> i40e_reset_subtask() > -> i40e_reset_and_rebuild() > -> i40e_rebuild() > -> i40e_setup_pf_switch() > -> i40e_ptp_init() > > Putting INIT_WORK() there would reinitialize the work item during reset or > recovery. If the work is pending or running, this races with the workqueue, > resets its bookkeeping, and breaks the workqueue's non-reentrance > guarantee. ok, but then this driver has bigger problem. the same i40e_ptp_init() re-inits tmreg_lock mutex and ptp_rx_lock spinlock. While latter might be ok during reset (hopefully no traffic processed while the nic is in reset), tmreg_lock mutex can be used for adjfine/adjtime/gettimex64/ settime64 callbacks as ptp device is still visible to user space. I believe ptp part of the driver has to be redesigned properly. > > The work item is therefore initialized once per PF from i40e_sw_init(). > The helper also keeps the work callback private to i40e_ptp.c.