Re: [PATCH net v2 05/15] ice: call PTP link change only from link events

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

On 10/12/2026 4:15 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 (1) · Medium: 4 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] In the non-READY branch of ice_ptp_release() (state ERROR after
>   a failed ice_ptp_rebuild_owner(), or RESETTING after a rebuild that…
> - [Medium] On E82x, every non-PFR reset (CORER/GLOBR/EMPR) now restarts
>   the PHY timestamp block twice.
> - [Medium] A link event that arrives during the INITIALIZING window of
>   ice_ptp_init() is now lost for good.
> - [Medium] Now that the ice_up_complete() call is gone, edge detection in
>   ice_link_event() is the only thing that updates PTP link state at…
> - [Medium] The new block at the end of ice_rebuild() programs the PTP PHY
>   from pf->hw.port_info->phy.link_info.link_info without asking firmware…
> 
> Pre-existing issues:
> - [High] ice_ptp_init() publishes the port on adapter->ports.list
>   (ice_ptp_setup_pf() -> list_add_rcu) before ice_ptp_init_work()…
> 
> --
> 
> ice: call PTP link change only from link events
> 
> This patch removes the ice_ptp_link_change() calls from ice_up_complete()
> and ice_down(), so the link event handler is the only runtime source of
> PTP link state. It seeds ptp->port.link_up in ice_ptp_init() and adds an
> ice_ptp_link_change() call at the end of ice_rebuild(). It also creates
> the kworker before the first PHY restart in ice_ptp_init().
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
>> index d88835482d3aa..f32041dd8b274 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
>> @@ -6745,7 +6745,6 @@ static int ice_up_complete(struct ice_vsi *vsi)
>>  		ice_print_link_msg(vsi, true);
>>  		netif_tx_start_all_queues(vsi->netdev);
>>  		netif_carrier_on(vsi->netdev);
>> -		ice_ptp_link_change(pf, true);
>>  	}
> 
> [Severity: Medium]
> With this call gone, can PTP miss a link-up that some code other than the
> LSE handler notices first?
> 
> ice_link_event() only notifies PTP on an edge, and it compares against the
> cached pi->phy.link_info:
> 
> ice_link_event() {
>     ...
> 	if (link_up == old_link && link_speed == old_link_speed)
> 		return 0;
>     ...
> 	ice_ptp_link_change(pf, link_up);
> }
> 
> Other paths refresh that cache with Get Link Status:
> ice_open_internal() -> ice_update_link_info(), ice_check_media_subtask()
> while ICE_FLAG_NO_MEDIA is set, and ethtool ice_set_link_ksettings().
> 
> Here is one sequence:
> 
> - The netdev is down and the cable is pulled. PTP link_up becomes false,
>   and on E82x ice_ptp_port_phy_stop() stops the PHY timer.
> - The cable is plugged back in.
> - 'ip link set up' runs before the service task drains the ARQ.
> 
> ice_open_internal() then refreshes the cache to up, and ice_up_complete()
> turns the carrier on without telling PTP. When the queued LSE is handled,
> it sees old == new and returns early.
> 
> Would ptp_port->link_up then stay false while carrier is on, until the next
> physical link transition? The E82x PHY timer would stay stopped, and
> ice_ptp_process_tx_tstamp() would drop every Tx timestamp. Later in the
> series, ice_ptp_is_tx_tracker_up() rejects new requests instead. Before
> this patch, the open path handled this case.
> 
>> @@ -7273,7 +7272,6 @@ int ice_down(struct ice_vsi *vsi)
>>  
>>  	if (vsi->netdev) {
>>  		vlan_err = ice_vsi_del_vlan_zero(vsi);
>> -		ice_ptp_link_change(vsi->back, false);
>>  		netif_carrier_off(vsi->netdev);
>>  		netif_tx_disable(vsi->netdev);
>>  	}
> 
> [Severity: High]
> ice_down() no longer clears link_up during reset preparation, so it stays
> true through a failed reset. Can ice_ptp_restart_all_phy() then reach a
> port whose kworker has already been destroyed?
> 
> ice_ptp_release() has a non-READY branch. It runs in state ERROR after a
> failed ice_ptp_rebuild_owner(), or in state RESETTING after a rebuild that
> failed early. That branch destroys the worker before it removes the port
> from adapter->ports.list:
> 
> ice_ptp_release() {
>     ...
> 	kthread_destroy_worker(pf->ptp.kworker);
> 	pf->ptp.kworker = NULL;
>     ...
> 	ice_ptp_cleanup_pf(pf);
> }
> 
> ice_ptp_settime64() does not check the PTP state, and on E82x it calls
> ice_ptp_restart_all_phy(). If that runs during this window, it takes a
> reference on the port, which is still on the list. With link_up true it
> then goes down this path:
> 
> ice_ptp_settime64()
>   ice_ptp_restart_all_phy()
>     ice_ptp_port_phy_restart()
>       kthread_cancel_delayed_work_sync(&ptp_port->ov_work)
>       kthread_queue_delayed_work(pf->ptp.kworker, ...)
> 
> The cancel goes through work->worker, which can point at the freed worker.
> The queue call passes a NULL worker.
> 
> Before this patch, ice_down() cleared link_up from ice_prepare_for_reset(),
> so ice_ptp_restart_all_phy() skipped the port.
> 
> The order of destroying the worker before removing the port comes from the
> earlier patch in this series, "ice: fix removal of PTP timestamp tracker
> during reset". This change makes it reachable for ports that had a running
> netdev. Can this end in a use-after-free or a NULL dereference in
> kthread_queue_delayed_work()?
>

This is the same issue as reported on the first patch, and should be
fixed by re-ordering the teardown properly.

>> @@ -7794,6 +7792,14 @@ 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);
>> +		ice_ptp_link_change(pf, link_up);
>> +	}
>> +
> 
> [Severity: Medium]
> Does this restart the E82x PHY timestamp block a second time on every
> CORER, GLOBR or EMPR?
> 
> Before this patch, ice_prepare_for_reset() -> ice_pf_dis_all_vsi() ->
> ice_vsi_close() -> ice_down() cleared ptp_port->link_up while the state was
> still READY. ice_ptp_restart_all_phy() then skipped the port, and the one
> restart came later, from ice_up_complete().
> 
> Now link_up keeps its pre-reset value, so the earlier ice_ptp_rebuild()
> call already restarts the port:
> 
> ice_rebuild()
>   ice_ptp_rebuild()
>     ice_ptp_rebuild_owner()
>       ice_ptp_restart_all_phy()
>         if (READ_ONCE(port->link_up))
>             ice_ptp_port_phy_restart(port);
> 
> That cancels ov_work, sets calibrating, starts the PHY timer and queues
> ov_work. Then this new block calls ice_ptp_link_change().
> 
> hw.reset_ongoing was already cleared in ice_reset_subtask() before
> ice_rebuild() ran, so the HW writes go ahead. On ICE_MAC_GENERIC this calls
> ice_ptp_port_phy_restart() again every time. On E825 it does so when
> linkup is true:
> 
> ice_ptp_link_change() {
>     ...
> 	case ICE_MAC_GENERIC:
> 		ice_ptp_port_phy_restart(ptp_port);
> 		break;
>     ...
> }
> 
> So the calibration that was just started is thrown away and started again.
> The owner's ice_ptp_restart_all_phy() also restarts the ports of other PFs.
> Each of those PFs then restarts its own port again at the end of its own
> rebuild.
> 
> The commit message names this as the first problem being fixed:
> 
>     1. Double initialization on link-up: ice_handle_link_event() calls
>        ice_ptp_link_change(true), then ice_up_complete() calls it again.
> 
> Does the reset path now get the same kind of double restart?
> 


:\ This whole flow is incredibly tricky. The old flow had a double
re-init on every up, while this only results in a double init on a reset.

Perhaps we could delay the restart in the earlier part of rebuild, but
there is fundamentally a problem of ordering here: We need to restart
after the PF clock owner finishes rebuilding, but devices reset
independently and link changes independently so we may need to restart
after a PF rebuild. These two conflicting requirements are what
ultimately drive this collision. Since each PF is mostly independent it
tries to reset it self, but the clock owner must also reset everyone and
cannot necessarily rely on the PFs having already reset or not.

I'm not certain there is a safe way to remove all these double re-init
flows with the current driver design. I can update the commit message
for clarity.

> [Severity: Medium]
> Is link_info current at this point after the reset?
> 
> ice_update_pf_netdev_link(), just above, uses ice_get_link_status(). That
> only sends Get Link Status when phy.get_link_info is set, and in the driver
> only ethtool sets that flag:
> 
> ice_get_link_status() {
>     ...
> 	if (phy_info->get_link_info) {
>     ...
> }
> 
> During rebuild, the cache is refreshed only when the netdev is running
> (ice_ena_vsi() -> ice_open_internal() -> ice_update_link_info()).
> ice_init_all_ctrlq() also drops any LSE that was queued before the reset.
> 
> So for a netdev that is down, this seeds PTP from the pre-reset snapshot.
> If the reset changed the real link state, the E82x/E825 PHY timer can be
> set up for the wrong state.
> 
> ice_up_complete() no longer notifies PTP. A later open refreshes the cache
> without telling PTP, and the next LSE may then match the cache and be
> ignored.
> 
> The commit message says:
> 
>     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.
> 
> Should this re-read link_info from firmware before using it? As written, it
> restores a cached software value rather than the hardware link status.
> 

Yes this probably does need to check the firmware state.

>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> index bbb57edcec0f5..220d174397bab 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> 
> [ ... ]
> 
>> @@ -3414,8 +3415,23 @@ void ice_ptp_init(struct ice_pf *pf)
>>  	if (err)
>>  		goto err_release_tx_tracker;
>>  
>> -	/* Start the PHY timestamping block */
>> +	/* Create the kworker before restarting the PHY, which queues work on
>> +	 * it in the E82x restart path.
>> +	 */
>> +	err = ice_ptp_init_work(pf, ptp);
>> +	if (err)
>> +		goto err_clean_pf;
>> +
> 
> [Severity: High]
> This is a pre-existing issue, but this reorder doesn't fully close the
> window it is aimed at. Can another PF still restart this port before
> ptp->kworker is assigned?
> 
> ice_ptp_setup_pf() adds the port to adapter->ports.list with
> list_add_rcu() before ice_ptp_init_work() runs. The kworker is then
> assigned without ps_lock.
> 
> ice_ptp_release() sets pf->ptp.kworker = NULL but never clears
> pf->ptp.port.link_up. ice_unload() also runs ice_deinit_features() before
> ice_unregister_netdev(), so ice_down() never clears it either. After a
> devlink reload, the PF therefore re-enters ice_ptp_init() with link_up
> still true.
> 
> The window includes kthread_run_worker() creating a thread. If the E82x
> clock owner handles clock_settime during it:
> 
> ice_ptp_settime64()
>   ice_ptp_restart_all_phy()
>     kref_get_unless_zero(&port->ref)   /* succeeds, port is listed */
>     if (READ_ONCE(port->link_up))      /* stale true */
>         ice_ptp_port_phy_restart(port)
>           kthread_queue_delayed_work(pf->ptp.kworker /* NULL */, ...)
> 
> then kthread_queue_delayed_work() dereferences a NULL worker. The window
> was larger before this patch.
> 
> Would creating the kworker before ice_ptp_setup_pf(), or clearing link_up
> in ice_ptp_release(), close it?
> 

This appears to be related (but not equivalent) to the issue in the
first patch. We need to make sure that the kworker exists before we're
in the list and we need to make sure it is not removed until we're
removed from the list. Additionally, it may make sense for some portions
of the code to check PTP state.

>> +	/* Seed link_up from current PHY status, since link may already be up
>> +	 * (e.g. after PXE boot) with no link-change edge to catch it later.
>> +	 */
>>  	mutex_lock(&pf->adapter->ps_lock);
>> +	if (pf->hw.port_info)
>> +		WRITE_ONCE(ptp->port.link_up,
>> +			   !!(pf->hw.port_info->phy.link_info.link_info &
>> +			      ICE_AQ_LINK_UP));
>> +
>> +	/* Start the PHY timestamping block */
>>  	ice_ptp_port_phy_restart(&ptp->port);
>>  	mutex_unlock(&pf->adapter->ps_lock);
>>  
>> @@ -3424,9 +3440,10 @@ void ice_ptp_init(struct ice_pf *pf)
>>  
>>  	ptp->state = ICE_PTP_READY;
>>  
> 
> [Severity: Medium]
> What happens to a link event that is handled after link_up is seeded above
> but before ptp->state is set to ICE_PTP_READY here?
> 
> The service task is already running at this point. ice_init() clears
> ICE_SERVICE_DIS and arms serv_tmr, and ice_init_link() has already called
> ice_phy_cfg(). The window covers ice_ptp_port_phy_restart() and
> ice_ptp_cfg_tx_interrupt().
> 
> If an LSE is handled in that window, ice_link_event() updates the cached
> link_info and calls ice_ptp_link_change(pf, true). That call returns
> before it records the new state:
> 
> ice_ptp_link_change() {
> 	if (pf->ptp.state != ICE_PTP_READY)
> 		return;
>     ...
> 	WRITE_ONCE(ptp_port->link_up, linkup);
>     ...
> }
> 
> Later LSEs then see link_up == old_link in ice_link_event() and return
> early. Before this patch, the ice_up_complete() call fixed this on the next
> open, but that call is now removed.
> 
> Would ptp_port->link_up stay false while the link is up, until the link
> flaps or a reset runs? The E82x/E825 PHY timer would never be started, and
> Tx timestamps would be dropped at this commit. Later in the series,
> ice_ptp_is_tx_tracker_up() rejects new requests instead.
> 

Seems like there is a gap here. But the big challenge is that just
calling ptp_link_change or setting ptp_port->link_up inside of ice_open
is that we *don't* want to be toggling the link state for events which
aren't a true open/close of the device, (for example ice_down_up used
for certain internal reconfigurations).

Several other iterations of this series have had other complaints
outlined by changing order, such as link state not reflecting the port
start state, or issues with sequencing. :\ I guess I'll see what I can
figure out for v3...