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

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

That change could likely be merged here.

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

I'll drop that phrasing in v3.