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

[netdev-bot+sashiko@kernel.org]

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.

[ ... ]

> @@ -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?

[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
    ...
}

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.

>  
> -	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.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015191102.1174111-1-anthony.l.nguyen%40intel.com