Re: [Intel-wired-lan] [PATCH iwl-net] ice: fix E810 low latency timestamp interrupt handling

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

On 6/2/2026 3:15 AM, Marcin Szycik wrote:
> 
> 
> On 29/05/2026 22:22, Jacob Keller wrote:
>> The driver has supported a low latency timestamp interface since commit
>> 82e71b226e0e ("ice: Enable SW interrupt from FW for LL TS").
>>
>> This interface is triggered by calling ice_ptp_req_tx_single_tstamp(),
>> which is called by ice_ptp_ts_irq(), which in turn is called when the
>> driver gets an interrupt from hardware indicating that a timestamp is
>> available.
>>
>> This function doesn't check if a timestamp request is already in progress,
>> and as a result could trash an existing outstanding requests when called.
>> It turns out that this is unlikely in practice due to a number of
>> circumstances that prevent most of the ways that could happen.
>>
>> 1. The ice_misc_intr_thread_fn() might trigger a software-generated
>>    interrupt with the PFINT_OICR_TSYN_TX flag. However, we don't enter the
>>    thread function since ice_ptp_ts_irq() always returns IRQ_HANDLED for
>>    E810 devices which support the low latency firmware interface.
>>
>> 2. The ice_ptp_maybe_trigger_tx_interrupt() function might trigger a
>>    software-generated interrupt if it detects waiting timetstamps. However
>>    it checks ptp.port.tx.has_ready_bitmap which is always false for E810,
>>    so never enters the code path.
>>
>> However, it is still possible that another Tx timestamp request could
>> happen and complete and race with the firmware completing the outstanding
>> low latency timestamp request.
>>
>> This doesn't happen often in practice because many applications only
>> trigger a single outstanding Tx timestamp at once. However, if the user
>> runs multiple copies of ptp4l or uses other userspace stack which does,
>> they might miss timestamps or get corrupted timestamp data.
>>
>> To fix this, have the ice_ptp_req_tX_single_tstamp() function check and
>> only begin the operation if the ATQBAL_FLAGS_INTR_IN_PROGRESS flag was not
>> yet set. This prevents a new possible request from trashing an outstanding
>> request. Note that on completion of a request, the ice_ll_ts_intr()
>> function will initiate a request for the next outstanding timestamp, so no
>> timestamps will be lost.
>>
>> Additionally, although the ice_ptp_tx_tstamps_pending() function doesn't
>> currently get called for E810 devices, it should still not return true for
>> devices which support the low latency interrupt. If for some reason code is
>> refactored and the miscellaneous thread function does execute, it should
>> not trigger a new software interrupt for devices using the low latency
>> interrupt interface. Add an explicit check to make this function always
>> return false when the device is operating in this mode.
>>
>> Finally, convert the atqbal_flags to DECLARE_BITMAP and use test/set bit
>> functions. This helps in clarity as we can use test_and_set_bit and
>> test_and_clear_bit.
>>
>> Fixes: 82e71b226e0e ("ice: Enable SW interrupt from FW for LL TS")
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> 
> Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> + some nits
> 

Thanks for the review. The nits are minor, but I think its probably best
to clean them up because of the possible confusion.
>> ---
>> This was originally motivated by changes in our out-of-tree release where
>> the issue of a software-generated interrupt was causing significantly more
>> issues. After further investigation it seems that the upstream
>> implementation is more robust, preventing the thread function from running
>> for E810. However, there appears to be a small window where issues can crop
>> up if multiple outstanding timestamps are requested near concurrently. This
>> change is motivated at closing that gap and ensuring consistency of
>> timestamps returned through the low latency interface.
>>
>> To trigger the issue you will need to issue multiple Tx timestamp requests
>> near but not quite simultaneously, and it may be quite a rare race
>> condition.
>> ---
>>  drivers/net/ethernet/intel/ice/ice_type.h   |  8 ++++++--
>>  drivers/net/ethernet/intel/ice/ice_ptp.c    | 24 +++++++++++++++++++-----
>>  drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 12 ++++++------
>>  3 files changed, 31 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
>> index 1e82f4c40b32..7035ea6c59db 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_type.h
>> +++ b/drivers/net/ethernet/intel/ice/ice_type.h
>> @@ -859,12 +859,16 @@ struct ice_mbx_data {
>>  #define ICE_PORTS_PER_QUAD	4
>>  #define ICE_GET_QUAD_NUM(port) ((port) / ICE_PORTS_PER_QUAD)
>>  
>> -#define ATQBAL_FLAGS_INTR_IN_PROGRESS	BIT(0)
>> +enum ice_atqbal_flags {
>> +	ATQBAL_FLAGS_INTR_IN_PROGRESS,
>> +
>> +	ATQBAL_FLAGS_NBITS, /* must be last */
> 
> Nit: no comma
> 

Fair. This one never changes and is always last so no comma makes sense.
My habbit is generally to always include commas on the last element in
enums since it keeps things uniform but I agree it makes sense in this case.

>> +};
>>  
>>  struct ice_e810_params {
>>  	/* The wait queue lock also protects the low latency interface */
>>  	wait_queue_head_t atqbal_wq;
>> -	unsigned int atqbal_flags;
>> +	DECLARE_BITMAP(atqbal_flags, ATQBAL_FLAGS_NBITS);
>>  };
>>  
>>  struct ice_eth56g_params {
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> index 36df742c326c..11059deb5d41 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> @@ -382,6 +382,7 @@ void ice_ptp_req_tx_single_tstamp(struct ice_ptp_tx *tx, u8 idx)
>>  	struct ice_ptp_port *ptp_port;
>>  	unsigned long flags;
>>  	struct sk_buff *skb;
>> +	struct device *dev;
>>  	struct ice_pf *pf;
>>  
>>  	if (!tx->init)
>> @@ -389,6 +390,7 @@ void ice_ptp_req_tx_single_tstamp(struct ice_ptp_tx *tx, u8 idx)
>>  
>>  	ptp_port = container_of(tx, struct ice_ptp_port, tx);
>>  	pf = ptp_port_to_pf(ptp_port);
>> +	dev = ice_pf_to_dev(pf);
>>  	params = &pf->hw.ptp.phy.e810;
>>  
>>  	/* Drop packets which have waited for more than 2 seconds */
>> @@ -408,7 +410,13 @@ void ice_ptp_req_tx_single_tstamp(struct ice_ptp_tx *tx, u8 idx)
>>  
>>  	spin_lock_irqsave(&params->atqbal_wq.lock, flags);
>>  
>> -	params->atqbal_flags |= ATQBAL_FLAGS_INTR_IN_PROGRESS;
>> +	if (test_and_set_bit(ATQBAL_FLAGS_INTR_IN_PROGRESS,
>> +			     params->atqbal_flags)) {
>> +		dev_dbg(dev, "%s: low latency interrupt request already in progress?\n",
> 
> Why the '?', are we not certain it's in progress?
> 

It was more intended to invoke a note of "surprise" as in "why is this
happening??". Its obvious that it *has* happened, but it wasn't supposed
to be possible. However, I think that was a statement made before I
realized the case where two near simultaneous timestamps could cause us
to enter this function again. In the original "bug" this was happening
because our interrupt handler was being forcibly re-scheduled via
software triggered interrupt.. but that only happened due to flukes in
code that never got in-tree, and the assessment was wrong.

I think this is worth fixing, so I'll send a v2 after I double check
feedback on Sashiko.

>> +			__func__);
>> +		spin_unlock_irqrestore(&params->atqbal_wq.lock, flags);
>> +		return;
> 
> Nit: could be an unroll.
> 

Fair. I had also considered using (scoped_)guard but netdev maintainers
don't like that as much. I often don't bother with an unroll goto unless
there are multiple places where we have to cleanup.