Re: [PATCH net v2 12/15] ice: keep Tx timestamp slots tracked until completion or timeout

[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 more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] This patch adds a sentence to the ice_ptp_process_tx_tstamp()
>   kernel-doc: "in the unusual case where the PHY continuously fails to…
> - [Low] The ice_ptp_maybe_trigger_tx_interrupt() kernel-doc still says
>   the function will "periodically check if the PHY has outstanding…
> 
> Pre-existing issues:
> - [Medium] ice_ptp_init() sets link_up from port_info under ps_lock but
>   sets ptp->state = ICE_PTP_READY only after dropping the lock.
> - [Low] Because the has_ready_bitmap gate is removed from
>   ice_ptp_maybe_trigger_tx_interrupt(), the periodic worker on non-LL…
> 
> --
> 
> ice: keep Tx timestamp slots tracked until completion or timeout
> 
> When the link goes down, outstanding Tx timestamp requests are no longer
> dropped. They are marked stale instead, new requests are rejected while
> the link is down, and a slot that is not ready yet is freed only after the
> two second timeout. The patch also changes ice_ptp_tx_tstamps_pending() so
> stale entries are ignored when deciding whether to re-arm the IRQ, and lets
> ice_ptp_maybe_trigger_tx_interrupt() run on devices that handle their own
> Tx interrupt.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> index 5220de274819a..a5ee8c8edf3d6 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> 
> [ ... ]
> 
>> @@ -554,7 +557,9 @@ void ice_ptp_complete_tx_single_tstamp(struct ice_ptp_tx *tx)
>>   * extremely unlikely that a packet will ever take this long to timestamp. If
>>   * we detect a Tx timestamp request that has waited for this long we assume
>>   * the packet will never be sent by hardware and discard it without reading
>> - * the timestamp register.
>> + * the timestamp register. Note that in the unusual case where the PHY
>> + * continuously fails to clear the ready bitmap index, the slot may remained
>> + * locked for more than two seconds.
>>   */
>>  static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx)
>>  {
> 
> [Severity: Low]
> Does this new comment describe what the code does at this commit?
> 

This is likely correct after the whole series and a rebase error made it
squash into the wrong change. I'll try to fix that.

> In ice_ptp_process_tx_tstamp(), nothing re-reads the PHY ready bitmap
> after ice_read_phy_tstamp() and before the slot is released:
> 
> 	err = ice_read_phy_tstamp(hw, tx->block, phy_idx, &raw_tstamp);
> 	if (err && !drop_ts)
> 		continue;
> 	...
> skip_ts_read:
> 	...
> 	clear_bit(idx, tx->in_use);
> 
> Suppose the PHY does not clear the ready bit, for example for an entry
> with VALID clear that was captured while P_REG_TX_OR=0. Then the index is
> freed and the skb released, which leaves a set ready bit with no in_use
> owner. The slot does not stay locked.
> 
> At this commit ice_ptp_tx_tstamps_pending(pf, true) still falls back to
> the hardware check:
> 
> 	/* Check hardware indicator */
> 	ret = ice_check_phy_tx_tstamp_ready(hw);
> 
> That would re-arm PFINT_OICR_TSYN_TX for a bit the processing loop never
> visits.
> 
> A later patch in this series, "ice: don't clear in_use until HW clears
> ready bitmap", adds the "verify ready bit cleared" re-read. That re-read
> keeps the index in_use and marks it stale. The same patch also removes the
> hardware fallback from ice_ptp_tx_tstamps_pending(). After the full series
> the comment matches the code.
> 
> Could this comment move into that later patch so each commit is consistent
> on its own?
> 
> [ ... ]
> 
>> @@ -1426,6 +1428,9 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
>>  
>>  	WRITE_ONCE(ptp_port->link_up, linkup);
>>  
>> +	if (!linkup)
>> +		ice_ptp_mark_tx_tracker_stale(&ptp_port->tx);
>> +
>>  	/* Skip HW writes if reset is in progress */
>>  	if (pf->hw.reset_ongoing)
>>  		goto out_unlock;
> 
> [Severity: Medium]
> This is a pre-existing issue and this patch did not introduce it. This
> patch does make link_up more important, though.
> 
> ice_ptp_init() seeds link_up while holding ps_lock, but it publishes
> ICE_PTP_READY only after dropping the lock:
> 
> ice_ptp_init() {
>     ...
> 	mutex_lock(&pf->adapter->ps_lock);
> 	if (pf->hw.port_info)
> 		WRITE_ONCE(ptp->port.link_up, ...);
> 	ice_ptp_port_phy_restart(&ptp->port);
> 	mutex_unlock(&pf->adapter->ps_lock);
> 	ice_ptp_cfg_tx_interrupt(pf);
> 	ptp->state = ICE_PTP_READY;
>     ...
> }
> 
> ice_ptp_link_change() checks the state without holding the lock:
> 
> 	if (pf->ptp.state != ICE_PTP_READY)
> 		return;
> 
> ice_init() starts the service timer, and the netdev is registered before
> ice_init_features()->ice_ptp_init() runs, so link events can arrive during
> this window. Any link event in that window is dropped.
> 
> With this patch, ice_ptp_is_tx_tracker_up() and the link-down stale marking
> both depend on link_up. A stale link_up=false now means every new request
> is rejected until the next link flap. The race itself came from the
> earlier commit "ice: call PTP link change only from link events".
> 
> Should ptp->state be set while ps_lock is held, or should link_up be
> re-seeded after the state becomes ICE_PTP_READY?
> 

I'm still trying to figure out how best to address that sequencing.
Perhaps it makes sense to re-check firmware and handle link state after
we set ICE_PTP_READY.

> [ ... ]
> 
>> @@ -2984,19 +2990,16 @@ static void ice_ptp_maybe_trigger_tx_interrupt(struct ice_pf *pf)
>>  {
>>  	struct device *dev = ice_pf_to_dev(pf);
>>  	struct ice_hw *hw = &pf->hw;
>> -	int ret;
>>  
>> -	if (!pf->ptp.port.tx.has_ready_bitmap)
>> +	/* Avoid re-triggering OICR on E810 with low latency interrupt path */
>> +	if (hw->dev_caps.ts_dev_info.ts_ll_int_read)
>>  		return;
>>  
> 
> [Severity: Low]
> This is a pre-existing issue and was not introduced by this patch. It is
> a kernel-doc wording problem.
> 
> With the has_ready_bitmap gate gone, the periodic worker on non-LL E810
> now reaches ice_check_phy_tx_tstamp_ready_e810(). At this commit it gets
> there through ice_ptp_tx_tstamps_pending(). Later in the series it calls
> it directly. The kernel-doc of that helper says it "should not be called
> for E810 hardware", but the body is only:
> 

I'll try to clean up the kdoc here.

> static int ice_check_phy_tx_tstamp_ready_e810(struct ice_hw *hw)
> {
> 	return 0;
> }
> 
> ice_misc_intr_thread_fn() already reached it on E810 before this patch,
> so nothing changes at runtime.
> 
> Should that kernel-doc be updated, now that this is a regular call path?
> 
>> -	if (!ice_pf_src_tmr_owned(pf))
>> +	if (pf->ptp.tx_interrupt_mode != ICE_PTP_TX_INTERRUPT_SELF &&
>> +	    !ice_pf_src_tmr_owned(pf))
>>  		return;
>>  
>> -	ret = ice_check_phy_tx_tstamp_ready(hw);
>> -	if (ret < 0) {
>> -		dev_dbg(dev, "PTP periodic task unable to read PHY timestamp ready bitmap, err %d\n",
>> -			ret);
>> -	} else if (ret) {
>> +	if (ice_ptp_tx_tstamps_pending(pf, false)) {
>>  		dev_dbg(dev, "PTP periodic task detected waiting timestamps. Triggering Tx timestamp interrupt now.\n");
>>  
>>  		wr32(hw, PFINT_OICR, PFINT_OICR_TSYN_TX_M);
> 
> [Severity: Low]
> Should the kernel-doc of ice_ptp_maybe_trigger_tx_interrupt() be updated
> to match the new trigger condition? It still says:
> 
>  * To avoid getting permanently stuck, periodically check if the PHY has
>  * outstanding timestamp data. If so, trigger an interrupt from software to
>  * process this data.
> 
> With in_irq=false, ice_port_has_timestamps() returns:
> 
> 	return !bitmap_empty(tx->in_use, tx->len);
> 
> That is true for any outstanding software request, including stale
> requests and requests the PHY has not captured yet. So the interrupt now
> fires on every 500 msec tick even when the PHY has no data. The commit
> message describes this ("one extra IRQ every 500 msec until the timeout"),
> but the function comment was not updated.
> 
> At the end of the series the condition is
> "ret > 0 || ice_ptp_tx_tstamps_pending(pf, false)" and the comment text is
> the same, so the mismatch remains.
> 

I'll try to reword this.