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

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

On 10/19/2026 1:14 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 4 · Low: 1
> 
> 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] The commit message says the new ice_ptp_link_change() call in
>   ice_rebuild() (ice_main.c:7803-7808) exists 'to reflect the current…
> - [Medium] This patch removes ice_ptp_link_change(pf, true) from
>   ice_up_complete(), which leaves the PTP link state with no resync on…
> - [Medium] The new PTP link_up seed is only in ice_init_link()
>   (ice_main.c:4817).
> - [Medium] ice_ptp_link_change() used to start with `if (pf->ptp.state !=
>   ICE_PTP_READY) return;`.
> - [Medium] ice_ptp_init() now sets `ptp->state = ICE_PTP_READY`
>   (ice_ptp.c:3425) before the PHY restart and before…
> - [Low] The comment above ice_ptp_rebuild() in ice_rebuild() still says
>   'PTP init needs to happen before the VSI rebuild.
> 
> --
> 
> ice: call PTP link change only from link events
> 
> This patch stops calling ice_ptp_link_change() from ice_up_complete() and
> ice_down(). Instead it seeds the PTP link_up field in ice_init_link() and
> resyncs it from ice_rebuild(). ice_ptp_link_change() now records link_up
> even when PTP is not ready, and ice_ptp_init() sets ICE_PTP_READY before
> it restarts the PHY timestamping block.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
>> index d88835482d3aa..b4ee2cf7cd704 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
>> @@ -4810,6 +4810,14 @@ static void ice_init_link(struct ice_pf *pf)
>>  
>>  	ice_init_link_dflt_override(pf->hw.port_info);
>>  
>> +	/* Seed PTP link status from current PHY status, since link may
>> +	 * already be up (e.g. after PXE boot) without a link-change edge to
>> +	 * catch it later.
>> +	 */
>> +	WRITE_ONCE(pf->ptp.port.link_up,
>> +		   !!(pf->hw.port_info->phy.link_info.link_info &
>> +		      ICE_AQ_LINK_UP));
>> +
> 
> [Severity: Medium]
> Does the devlink reload path need this seed too?
> 
> ice_init_link() is only reached from ice_init() on probe. The reload path
> skips it:
> 
> ice_devlink_reinit_up()
>   ice_init_hw()
>   ice_init_dev()
>   ice_init_pf()
>   ice_load()
>     ice_init_features()
>       ice_ptp_init()
>         ice_ptp_port_phy_restart()
> 
> ice_ptp_port_phy_restart() then uses whatever pf->ptp.port.link_up was
> left from before the unload, because ice_ptp_release() does not reset it.
> 
> While the reload is in progress, the service task is stopped and the
> control queues are shut down, so link events are lost. ice_init_hw() then
> refreshes the link_info cache with ice_aq_get_link_info(), so no later
> ice_link_event() sees a difference.
> 
> Before this patch, the ice_ptp_link_change(pf, true) call in
> ice_up_complete() fixed a stale value on the next open. That call is now
> gone. Say the link was down before devlink dev reload and came up during
> the reload. Does PTP then stay at link_up == false until the next physical
> link flap or reset? On E82x that means the PHY timer stays stopped and Tx
> timestamps are dropped.
> 
>> @@ -6745,7 +6753,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 removed, can PTP miss a link-up event that is still queued
> when the interface is opened?
> 
> ice_open_internal() refreshes the cached link_info directly from firmware:
> 
> ice_open_internal() {
>     ...
> 	pi = vsi->port_info;
> 	err = ice_update_link_info(pi);
>     ...
> }
> 
> ice_link_event() compares against that same cache. If nothing changed, it
> returns before it reaches ice_ptp_link_change():
> 
> ice_link_event() {
>     ...
> 	if (link_up == old_link && link_speed == old_link_speed)
> 		return 0;
>     ...
> 	ice_ptp_link_change(pf, link_up);
>     ...
> }
> 
> Take an admin-down netdev whose PTP link_up is false from an earlier
> link-down event. A link-up LSE has been posted to the ARQ but the service
> task has not handled it yet:
> 
> ice_open()
>   ice_open_internal()
>     ice_update_link_info()     <- cache now says up
>     ice_up_complete()          <- carrier on, PTP not told
> 
> service task
>   ice_link_event()             <- up == up, returns early
> 
> Wouldn't pf->ptp.port.link_up stay false while the physical link is up?
> 
> On E82x, ice_ptp_port_phy_restart() would keep the PHY timer stopped.
> ice_ptp_process_tx_tstamp() would drop Tx timestamps because link_up is
> false. Later in the series the same link_up value gates Tx timestamp
> requests, so those requests would be refused instead.
> 
> A later ifdown/ifup does not fix this either, because ice_down() and
> ice_up_complete() no longer touch PTP. Only a link flap or a reset does.
> This seems to contradict the commit message statement that "the link
> status will reflect the current state of the physical link". Later
> patches in the series leave ice_up_complete() unchanged.
> 

...

It seems like there is just no way to get Sashiko to be happy with any
of this mess and we need a completely different model for handling these
link events...

This is a very theoretical case which we haven't observed that would be
blocking legitimate issues we have with customers (the fact that certain
configuration changes like MTU were causing outage on startup of PTP).

I have tried now ~5 different ways to make Sashiko happy and each time
it spits out some pathway that looks like a link event is lost. As far
as I can tell there isn't any ordering that works properly without going
in and re-writing a bunch of other stuff that is even more problematic
as a fix :\

> [ ... ]
> 
>> @@ -7794,6 +7800,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: High]
> When the netdev is administratively down, is link_info re-read from
> firmware anywhere before this read?
> 
> The commit message says this call is there "to reflect the current link
> status in case it changed and an edge was missed during a reset". However,
> ice_ena_vsi() only calls ice_open_internal(), and through it
> ice_update_link_info(), when netif_running(vsi->netdev) is true.
> 
> ice_update_pf_netdev_link() -> ice_get_link_status() only queries firmware
> when pi->phy.get_link_info is set. Only the ethtool set_link_ksettings path
> sets that flag. Nothing else in ice_rebuild() issues Get Link Status.
> 
> If so, for an admin-down interface this call just replays the pre-reset
> state:
> 
> - the link is down before the reset (PTP link_up == false, PHY stopped)
> - the link comes up during the reset, and the LSE is lost while the
>   control queues are down
> - the cache still says down, so this calls ice_ptp_link_change(pf, false)
> - a later ice_open() refreshes the cache to up and turns the carrier on,
>   but ice_up_complete() no longer notifies PTP
> 
> Wouldn't PTP then stay at link_up == false while the carrier is up, until
> another link flap or reset?
> 
> The reverse case also looks possible. A stale "up" value makes
> ice_ptp_link_change(pf, true) start the PHY timer before the link has
> retrained. The real link-up LSE is then suppressed by the unchanged-cache
> check in ice_link_event().
> 
> The end of the series does not change this code. Would calling
> ice_update_link_info(pf->hw.port_info) before computing link_up fix this?
> 

Same frustration from above. I need to be able to stop the
ice_down/ice_up flow from causing an unnecessary PTP restart, but so far
every solution has had some complaint and we've iterated the series
multiple times now :\

> [Severity: Low]
> This isn't a bug, but the comment above ice_ptp_rebuild() earlier in
> ice_rebuild() still reads:
> 
> 	/* If the PF previously had enabled PTP, PTP init needs to happen before
> 	 * the VSI rebuild. If not, this causes the PTP link status events to
> 	 * fail.
> 	 */
> 
> After this patch the VSI rebuild no longer sends PTP link status updates,
> because ice_ptp_link_change() was removed from ice_up_complete() and
> ice_down(). The ordering that matters now is that this new
> ice_ptp_link_change() call runs after ice_ptp_rebuild() has set
> ICE_PTP_READY. Should the comment say that instead?
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> index 11416e23dadeb..776f211290eca 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> 
> [ ... ]
> 
>> @@ -1315,17 +1315,13 @@ 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;
>>  
>>  	mutex_lock(&pf->adapter->ps_lock);
> 
> [Severity: Medium]
> The state check now comes after mutex_lock(), so every call dereferences
> pf->adapter, even after ice_ptp_release() has set ICE_PTP_UNINIT. Can
> this touch a freed ice_adapter during removal?
> 
> In ice_remove(), the adapter reference is dropped before the service task
> is drained:
> 
> ice_remove() {
>     ...
> 	ice_adapter_put(pdev);
> 	ice_deinit_hw(&pf->hw);
> 
> 	ice_deinit_dev(pf);    <- ice_service_task_stop() happens here

This really should be done in a different order and only remove the
adapter once we have finished removing stuff :\ I think this is a
pre-existing issue.

>     ...
> }
> 
> ice_deinit() only sets ICE_SERVICE_DIS. It does not wait for a service
> task that is already running. For the last PF on an adapter,
> ice_adapter_put() can free the adapter. A service task already inside
> ice_link_event() past the ice_get_main_vsi() check could then reach
> ice_ptp_link_change() and lock the freed adapter's ps_lock. For example,
> the task could be stalled in ice_set_link() AQ traffic.
> 
> Before this patch, the early state check made this a no-op once
> ice_ptp_release() had run.
> 
> The race itself is older: the same window already has a pre-existing use
> of the freed VSI from the concurrent service task. What this change adds
> is the adapter access. At the end of the series the lock is still taken
> unconditionally.
> 

This needs to be fixed via a combination of flushing the service work,
and then removing the adapter I think.

>>  
>> -	ptp_port->link_up = linkup;
>> +	WRITE_ONCE(ptp_port->link_up, linkup);
>>  
>> -	/* Skip HW writes if reset is in progress */
>> -	if (pf->hw.reset_ongoing)
>> +	if (pf->ptp.state != ICE_PTP_READY || pf->hw.reset_ongoing)
>>  		goto out_unlock;
> 
> [ ... ]
> 
>> @@ -3426,7 +3422,9 @@ void ice_ptp_init(struct ice_pf *pf)
>>  	if (err)
>>  		goto err_release_tx_tracker;
>>  
>> -	/* Start the PHY timestamping block */
>> +	ptp->state = ICE_PTP_READY;
>> +
>> +	/* (Re)start the PHY timestamping block */
>>  	mutex_lock(&pf->adapter->ps_lock);
>>  	ice_ptp_port_phy_restart(&ptp->port);
>>  	mutex_unlock(&pf->adapter->ps_lock);
>> @@ -3434,8 +3432,6 @@ void ice_ptp_init(struct ice_pf *pf)
>>  	/* Configure initial Tx interrupt settings */
>>  	ice_ptp_cfg_tx_interrupt(pf);
>>  
>> -	ptp->state = ICE_PTP_READY;
>> -
> 
> [Severity: Medium]
> Setting ICE_PTP_READY before ice_ptp_cfg_tx_interrupt() runs: can
> ndo_hwtstamp_set now race with that call?
> 
> ice_ptp_init() runs from ice_load() -> ice_init_features() after
> ice_register_netdev(), so the netdev is already visible. The only check in
> ice_ptp_hwtstamp_set() is:
> 
> 	if (pf->ptp.state != ICE_PTP_READY)
> 		return -EAGAIN;
> 
> Both paths end up in ice_ptp_cfg_tx_interrupt(). The user path gets there
> through ice_ptp_set_timestamp_mode() -> ice_ptp_restore_timestamp_mode().
> The function computes enable from pf->ptp.tstamp_config.tx_type and then
> does an unlocked read-modify-write:
> 
> 	val = rd32(hw, PFINT_OICR_ENA);
> 	if (enable)
> 		val |= PFINT_OICR_TSYN_TX_M;
> 	else
> 		val &= ~PFINT_OICR_TSYN_TX_M;
> 	wr32(hw, PFINT_OICR_ENA, val);
> 
> ice_ptp_init() runs under devl_lock and the ndo runs under rtnl/netdev
> locking, so nothing keeps the two apart. In ICE_PTP_TX_INTERRUPT_SELF
> mode:
> 
> ice_ptp_init()                        ice_ptp_hwtstamp_set()
>   enable = false (tx_type OFF)
>                                         tx_type = HWTSTAMP_TX_ON
>                                         sets PFINT_OICR_TSYN_TX_M
>   rd32(PFINT_OICR_ENA)
>   clears PFINT_OICR_TSYN_TX_M
>   wr32(PFINT_OICR_ENA)
> 
> Wouldn't the config then report HWTSTAMP_TX_ON while the Tx timestamp
> interrupt is masked? Tx timestamps would then time out until the next
> hwtstamp_set or reset.
> 
> Before this patch, hwtstamp_set returned -EAGAIN until init had finished
> ice_ptp_cfg_tx_interrupt(). The end of the series keeps the new ordering.
> 

It is becoming increasingly frustrating that I cannot replicate a lot of
these reports from the netdev sashiko on my own runs, (despite numerous
re-runs of Sashiko). I don't know if this is because of different
prompting or lack of model access.

This particular issue partly falls under the situation that we shouldn't
be registering the access methods before we've finished initialization...

It might be safe to just move the ptp_cfg_tx_interrupt earlier... I
can't remember exactly why I did this order. I guess the function got
lumped in with starting the kthread that needs to go after marking the
PTP as ready.