Re: [PATCH net 10/15] ice: wait for in-flight Tx timestamps before flushing the tracker

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

On 9/15/2026 6:12 PM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> Here is the report:
> 
> commit fea79aeeb67734d73dbd19b0b88b152cd016c86e
> Author: Petr Oros <poros@redhat.com>
> 
> ice: wait for in-flight Tx timestamps before flushing the tracker
> 
> This adds ice_ptp_wait_for_tracker_drain(), called from
> ice_ptp_flush_tx_tracker(), which polls the PHY Tx timestamp ready bitmap
> for up to 10 ms so that in-flight captures publish their ready bits before
> the tracker entries are freed and the ready bits are cleared.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> index 4dec728e219d8..b142b779dc105 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> @@ -740,6 +740,37 @@ ice_ptp_alloc_tx_tracker(struct ice_ptp_tx *tx)
>>  	return 0;
>>  }
>>  
>> +static void
>> +ice_ptp_wait_for_tracker_drain(struct ice_pf *pf, struct ice_ptp_tx *tx)
>> +{
>> +	unsigned long deadline = jiffies + msecs_to_jiffies(10);
> 
> [Severity: Low]
> Is the 10 ms budget the commit message describes actually available at every
> HZ setting?  With CONFIG_HZ=100, msecs_to_jiffies(10) is 1, so the deadline
> is a single tick ahead and the remaining fraction of the current tick is
> arbitrary.
> 
> Since the loop body only sleeps 500-1000 us before re-testing
> time_before(jiffies, deadline), can the whole wait end after one poll of
> roughly 500 us?
> 
> Would a microsecond-resolution deadline (ktime_get() plus ktime_add_us(), or
> read_poll_timeout()) match the documented budget better here?
> 

I'll see about switching to read_poll_timeout().

>> +	struct ice_hw *hw = &pf->hw;
>> +	u64 tstamp_ready;
>> +	bool pending;
>> +	u8 idx;
>> +
>> +	if (hw->reset_ongoing)
>> +		return;
>> +
>> +	do {
>> +		if (ice_get_phy_tx_tstamp_ready(hw, tx->block, &tstamp_ready))
>> +			return;
>> +
>> +		pending = false;
>> +		for_each_set_bit(idx, tx->in_use, tx->len) {
>> +			if (!(tstamp_ready & BIT_ULL(idx + tx->offset)))
>> +				pending = true;
>> +		}
>> +		if (!pending)
>> +			return;
> 
> [Severity: High]
> Should this read of the ready bitmap be gated on tx->has_ready_bitmap?
> 
> On ICE_MAC_E810 the flag is cleared in ice_ptp_init_tx():
> 
> 	/* The E810 PHY does not provide a timestamp ready bitmap. Instead,
> 	 * verify new timestamps against cached copy of the last read
> 	 * timestamp.
> 	 */
> 	tx->has_ready_bitmap = pf->hw.mac_type != ICE_MAC_E810;
> 
> and the backing helper in ice_ptp_hw.c is a stub:
> 
> 	static int
> 	ice_get_phy_tx_tstamp_ready_e810(struct ice_hw *hw, u8 port, u64 *tstamp_ready)
> 	{
> 		*tstamp_ready = 0xFFFFFFFFFFFFFFFF;
> 		return 0;
> 	}
> 
> With every bit set, doesn't the for_each_set_bit() loop above leave pending
> false on the first iteration, so the helper returns immediately without ever
> sleeping?  That would make the wait dead code on E810, which is the device
> named by the Fixes tag ea9b847cda64 ("ice: enable transmit timestamps for
> E810 devices").
> 
> The other consumer of the same register in ice_ptp_process_tx_tstamp() does
> gate both the read and the per-index test:
> 
> 	if (tx->has_ready_bitmap) {
> 		err = ice_get_phy_tx_tstamp_ready(hw, tx->block, &tstamp_ready);
> 		if (err)
> 			return;
> 	}
> 	...
> 		if (tx->has_ready_bitmap &&
> 		    !(tstamp_ready & BIT_ULL(phy_idx))) {
> 
> and the kernel-doc in ice_ptp.h states the register is only valid when the
> flag is set:
> 
>  * @has_ready_bitmap: if true, the hardware has a valid Tx timestamp ready
>  *                    bitmap register. If false, fall back to verifying new
>  *                    timestamp values against previously cached copy.
> 
> Would it make sense to either skip the wait when
> !tx->has_ready_bitmap and use the cached-timestamp fallback on E810, or
> adjust the Fixes tag and commit message to describe which devices are
> actually covered?
> 
> Also, a device with has_ready_bitmap == 0 whose stub returned zeros instead
> of all-ones would make this loop burn the entire budget on every flush,
> wouldn't it?
> 

Yes, this really shouldn't be done on E810 device without the ready
bitmap. Will fix.

>> +
>> +		usleep_range(500, 1000);
>> +	} while (time_before(jiffies, deadline));
>> +
>> +	dev_dbg(ice_pf_to_dev(pf), "Timed out waiting for in-flight Tx timestamps on block %u\n",
>> +		tx->block);
>> +}
>> +
> 
> [ ... ]