Re: [PATCH iwl-net 04/12] ice: call PTP link change only from link events

[Jacob Keller <jacob.e.keller@intel.com>]

On 8/21/2026 5:13 PM, Jacob Keller wrote:
> From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> 
> Remove redundant ice_ptp_link_change() calls from ice_up_complete() and
> ice_down(). These duplicate the call already made from
> ice_handle_link_event(), creating three problems:
> 
> 1. Double initialization on link-up: ice_handle_link_event() calls
>    ice_ptp_link_change(true), then ice_up_complete() calls it again.
>    The second call re-enters ice_ptp_port_phy_restart(), re-setting the
>    calibrating flag and restarting the PHY timer while the first
>    invocation's offset verification work (ov_work) may still be running.
> 
> 2. Premature cleanup on administrative down: ice_down() calls
>    ice_ptp_link_change(false) during ifconfig down or reset preparation,
>    even when the physical link is still up. This clears timestamp state
>    unnecessarily and can interfere with ongoing PTP operations.
> 
> 3. Ordering dependency: ice_down()/ice_up_complete() are called during
>    reset sequences where PTP may not be fully initialized, creating
>    edge cases with partially configured state.
> 
> The link event handler is the correct and sufficient place to drive PTP
> link state changes, as it reflects actual physical link transitions. Remove
> the calls of ice_ptp_link_change from the ice_down()/ice_up() flows.
> 
> Initialize the link_up in ice_ptp_init() and ensure that we check and
> restore the link status at the end of the rebuild flow, ensuring that we
> initialize the PHY timer appropriately after a reset.
> 
> Fixes: 6b1ff5d39228 ("ice: always call ice_ptp_link_change and make it void")
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> Signed-off-by: Przemyslaw Korba <przemyslaw.korba@intel.com>
> Signed-off-by: Petr Oros <poros@redhat.com>
> Reviewed-by: Maciek Machnikowski <maciej.machnikowski@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_main.c | 11 +++++++--
>  drivers/net/ethernet/intel/ice/ice_ptp.c  | 38 +++++++++++++++++++++++--------
>  2 files changed, 37 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index d88835482d3a..bb631ae9e67d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -7794,6 +7792,15 @@ static void ice_rebuild(struct ice_pf *pf, enum ice_reset_req reset_type)
>  
>  	ice_update_pf_netdev_link(pf);
>  
> +	if (test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags) && pf->hw.port_info) {
> +		bool link_up;
> +
> +		link_up = !!(pf->hw.port_info->phy.link_info.link_info &
> +			     ICE_AQ_LINK_UP);
> +		if (pf->ptp.port.link_up != link_up)
> +			ice_ptp_link_change(pf, link_up);
> +	}
> +


Sashiko says:

> Does conditionally skipping ice_ptp_link_change() here leave the non-owner PF
> PHY timer stopped indefinitely after a global reset if the link remains up?
> During a global reset (like EMPR or GLOBR), concurrent rebuilds are triggered
> across all PFs. The clock-owner PF iterates over all adapter ports in
> ice_ptp_rebuild_owner() and calls ice_ptp_port_phy_restart().
> If the clock-owner PF executes this before a non-owner PF has finished
> ice_init_all_ctrlq() or cleared its reset state, the SBQ command fails.
> Previously, the unconditional call to ice_ptp_link_change() masked this race
> by forcing a successful retry. Since ice_down() no longer clears link_up, and
> the physical link may stay UP across the reset, the condition
> pf->ptp.port.link_up != link_up will evaluate to false, leaving the non-owner
> PF's PHY timer stopped indefinitely.

I think this is correct. We need to ensure that we restart the PHY
somewhere in the chain after the CQ is active and the timer has been
reconfigured by the clock owner. Will investigate.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index b8647a39db6d..8aa49dda90a2 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -1327,14 +1327,14 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
>  	struct ice_ptp_port *ptp_port;
>  	struct ice_hw *hw = &pf->hw;
>  
> -	if (pf->ptp.state != ICE_PTP_READY)
> -		return;
> -
>  	ptp_port = &pf->ptp.port;
>  
>  	/* Update cached link status for this port immediately */
>  	ptp_port->link_up = linkup;
>  
> +	if (pf->ptp.state != ICE_PTP_READY)
> +		return;
> +


Sashiko says:

> This is a pre-existing issue, but is there a Time-of-Check to Time-of-Use
> (TOCTOU) race between checking ICE_PTP_READY here and module unload?
> In ice_remove(), ice_unload() is called which invokes ice_ptp_release().
> This destroys the PTP port mutex and the kworker. However, the background
> service task is not stopped until ice_deinit_dev() is called much later.
> If the service task processes a link event concurrently, ice_ptp_link_change()
> checks ICE_PTP_READY without synchronization. If preempted immediately after
> this check, ice_ptp_release() can destroy the resources.
> When ice_ptp_link_change() resumes, it calls ice_ptp_port_phy_restart()
> which attempts to acquire the destroyed ps_lock and queues work on the freed
> kworker, potentially causing a use-after-free or NULL pointer dereference.

I think this is best fixed by having ice_ptp_link_change acquire a
reference using the kref in the first patch to ensure that the port
can't be removed until the function exits. Will fix this in the first patch.