Re: [PATCH net v2 08/15] ice: E825: stop clearing PHY_REG_TX_OFFSET_READY

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · 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] Concern: the new ice_ptp_mark_tx_tracker_stale() calls in the
  E825 branches of ice_ptp_port_phy_restart() (line 1322) and…
- [Low] The new error message in ice_ptp_port_phy_restart()
  (drivers/net/ethernet/intel/ice/ice_ptp.c:1340) blames a link-up…

--

ice: E825: stop clearing PHY_REG_TX_OFFSET_READY

On E825, ice_stop_phy_timer_eth56g() no longer clears
PHY_REG_TX_OFFSET_READY, because clearing it leaves the hardware Tx
timestamp counter out of sync. The E825 stop and restart paths now set
tx.calibrating and mark outstanding Tx timestamps stale, then clear
tx.calibrating once the PHY start succeeds. The patch also drops the
unused soft_reset argument and rewords the restart failure message.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 7cc151b01a13e..818e2e265a7e5 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c

[ ... ]

> @@ -1302,15 +1310,35 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port)
>  					   0);
>  		break;
>  	case ICE_MAC_GENERIC_3K_E825:
> +		/* ice_ptp_port_phy_stop() may have already disabled
> +		 * timestamps, but some restarts occur without first stopping
> +		 * the timer, so we ensure that new requests are disabled
> +		 * here.
> +		 */
> +		spin_lock_irqsave(&ptp_port->tx.lock, flags);
> +		ptp_port->tx.calibrating = true;
> +		spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
> +
> +		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);

[Severity: High]
Can this call race with ice_ptp_release() freeing the Tx tracker bitmaps?

At this commit, ice_ptp_mark_tx_tracker_stale() does not check tx->init:

ice_ptp_mark_tx_tracker_stale() {
	spin_lock_irqsave(&tx->lock, flags);
	bitmap_or(tx->stale, tx->stale, tx->in_use, tx->len);
	spin_unlock_irqrestore(&tx->lock, flags);
}

ice_ptp_release_tx_tracker() clears tx->init under tx->lock. It then
frees the bitmaps without holding tx->lock or ps_lock:

	bitmap_free(tx->in_use);
	tx->in_use = NULL;

	bitmap_free(tx->stale);
	tx->stale = NULL;

	tx->len = 0;

ice_ptp_link_change() checks pf->ptp.state against ICE_PTP_READY without
a lock, before it takes ps_lock. ice_ptp_release() sets ICE_PTP_UNINIT
without holding ps_lock.

So on E825, the service task could handle a link-up event during driver
removal and reach this path:

ice_ptp_link_change()
  ice_ptp_port_phy_restart()
    ice_ptp_mark_tx_tracker_stale()
      bitmap_or(tx->stale, tx->stale, tx->in_use, tx->len)

If that happens while ice_ptp_release_tx_tracker() is freeing or clearing
tx->stale and tx->in_use, this would touch freed or NULL bitmaps. The new
call in the E825 branch of ice_ptp_port_phy_stop() has the same
exposure.

The direct call ice_ptp_release() -> ice_ptp_port_phy_stop() is safe,
because it runs after tx->len has already been set to 0.

A later patch in the series, "ice: keep Tx timestamp slots tracked until
completion or timeout", appears to fix this. It wraps the bitmap_or() in
ice_ptp_mark_tx_tracker_stale() with an if (tx->init) check under
tx->lock.

Could that check be added here, or could that patch be ordered before
this one, so the tree stays safe at every commit for bisection?

> +
>  		err = ice_start_phy_timer_eth56g(hw, port);
> +		if (err)
> +			break;
> +
> +		spin_lock_irqsave(&ptp_port->tx.lock, flags);
> +		ptp_port->tx.calibrating = false;
> +		spin_unlock_irqrestore(&ptp_port->tx.lock, flags);
> +
>  		break;
>  	default:
> -		err = -ENODEV;
> +		dev_dbg(ice_pf_to_dev(pf), "PTP failed to restart PHY port %u with unknown MAC type %d\n",
> +			port, hw->mac_type);
> +		return -ENODEV;
>  	}
>  
>  	if (err)
> -		dev_err(ice_pf_to_dev(pf), "PTP failed to set PHY port %d up, err %d\n",
> -			port, err);
> +		dev_err(ice_pf_to_dev(pf), "PTP failed to restart PHY port %u on link-up with err %pe; Timestamping remains disabled; A link-toggle may recover.\n",
> +			port, ERR_PTR(err));

[Severity: Low]
Is "on link-up" accurate for every caller of ice_ptp_port_phy_restart()?

ice_ptp_link_change() calls it, but so does ice_ptp_restart_all_phy(),
which runs from the reset rebuild path and from ice_ptp_settime64():

ice_ptp_restart_all_phy() {
	...
		if (READ_ONCE(port->link_up))
			ice_ptp_port_phy_restart(port);
	...
}

ice_ptp_init() also calls it directly:

	/* Start the PHY timestamping block */
	ice_ptp_port_phy_restart(&ptp->port);

At this commit, the settime64 path only applies to E822. Later in the
series, "ice: Recalibrate PHY after settime64 on E825-C" extends it to
E825 as well.

If the PHY start fails during rebuild, init or settime, this message
says a link-up event caused it. The wording is still the same at the end
of the series.

Would something like "PTP failed to restart PHY port %u" without the "on
link-up" part be more accurate?

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