> -----Original Message----- > From: Jacob Keller <[email protected]> > Sent: Friday, September 25, 2026 4:57 PM > To: Intel Wired LAN <[email protected]>; Machnikowski, Maciej > <[email protected]>; Keller, Jacob E <[email protected]>; > Korba, Przemyslaw <[email protected]>; Nguyen, Anthony L > <[email protected]>; Nitka, Grzegorz <[email protected]>; > Kubalewski, Arkadiusz <[email protected]> > Cc: Keller, Jacob E <[email protected]> > Subject: [PATCH iwl-net v3 14/15] ice: don't clear in_use until HW clears > ready bitmap > > During a link down transition, there is a small window where hardware does > not properly respond to reading the PHY timestamp registers. When this > occurs, the PHY does not automatically clear the ready bitmap or the valid > bit for the timestamp. This begins happening slightly before a link > transition even before the firmware has notified the driver of the state > change. > > The driver happily completes the timestamp, releasing the in_use bit. This > allows another request to reuse the bit potentially reporting an invalid > stale timestamp. Additionally, with the ready bit still set high the > driver > continues to re-trigger the IRQ and check for timestamps in a tight loop, > wasting CPU cycles. > > To fix this, re-read the PHY timestamp memory status after each read of a PHY > index. Double check if the hardware cleared the index properly. If it hasn't, > mark the timestamp index as stale and keep the index locked. The index will > be re-checked once another interrupt occurs (either from a real timestamp or > from the watchdog kick). Marking the packet as stale makes sense since we > know this begins happening when link is going down. > > If the ice_get_phy_tx_tstamp_ready() fails, treat it the same as if the > hardware hadn't cleared the bitmap. Continuing here is safe since the > ice_get_phy_tx_tstamp_ready() device implementations do not update the output > parameter except on success. Note that "transient" errors to access the PHY > will result in marking packets as stale. This is the simplest approach and > avoids risking a potential reuse of the stuck ready bit. > > The extra reads of the timestamp ready bitmap are saved in the same > tstamp_ready field. This effectively updates the ready bitmap for all > timestamps remaining in the loop. This is safe, and actually increases the > changes that a given timestamp will be processed by the loop in the event > that a timestamp completed after the initial read. Update the comments to > remove some text that might imply otherwise. > > This flow has been observed on E825, but allowing the driver to free an index > which is not cleared would be incorrect regardless of which device type it > occurs on. Thus, this re-read is applied to all device types. > > In the unlikely event that a timestamp has timed out the 2 second wait > *and* somehow suddenly has its ready bit set but unable to clear on read, > this could accidentally increment the timeout counter. It is intentional that > we do *not* release the index even in a timed out case, as we must not allow > reuse of that index until we can be certain it has cleared. Instead, refactor > so that the timeout counter is only incremented once the index is released, > (after the skip_tx_read label). This ensures > that we don't double count > timeouts or count them prematurely. The "stuck" ready indexes remain locked > *indefinitely* until the hardware reaches a state where the clear works. > > Stale timestamps are already ignored by the ice_any_port_has_timestamps() > function. However, the ice_ptp_tx_tstamps_pending() function also checks the > ready bitmap. Instead, modify it to only check the software tracker. > Additionally, stop re-triggering the interrupt from the IRQ if the timestamp > tracker is calibrating or has the link marked as down. Continue to check the > hardware ready bitmap from the watchdog to catch cases of unexpected > timestamps. > > With these changes, the timestamp processing no longer triggers a repeated > spamming of the IRQ during link down events where timestamps get stuck as the > PHY transitions to link down. Once link is restored, the PHY will be reset > and the stuck timestamps are cleared. > > Measuring CPU utilization of the miscellaneous IRQ thread function during > timestamp storms near a link reset shows that this prevents the spikes caused > by the "stuck" ready bit. Without this fix, the CPU handling the IRQ becomes > slammed due to the IRQ re-triggering logic. > > Measuring latency using the ice Tx timestamp traces does show that this fix > comes at a latency cost. Latency is measured using the ice Tx timestamp > traces for the request to completion time. I measured a couple of different > workloads both before and after this fix: > > * ptp4l using a profile with ~16 SYNC messages per second > > before: 189.71 microseconds mean, stdev 43.24 > after: 195.92 microseconds mean, stdev 25.28 > > * a C program generating 16 timestamp requests every 10 milliseconds on > two different ports: > > before: 457.11 microseconds mean, stdev 180.63 > after: 717.77 microseconds mean, stdev 321.58 > > In the normal work flows this comes with about a 10-20 microsecond penalty on > the average, and the standard deviation remains similar (with some variance > between run to run comparison). > > For heavy workloads with many more timestamps than expected for typical > applications this comes at a significant cost. This is because we handle all > timestamps in a single thread. If there are many concurrent timestamps being > requested at once, any which use the later slots on ports later in the port > list will take much longer to be processed once the interrupt is fired. Since > each timestamp now requires an additional PHY register access, this cost is > much higher in the case where the device is under unusually heavy load. The > high standard deviation indicates a very high variance in timestamp latency, > with many timestamps completing in the usual time but some taking > significantly longer when multiple timestamps are outstanding in a single IRQ. > > Ultimately, *correctness* is more important than speed here. Additionally, we > still remain well below the default limit of 10 milliseconds that ptp4l will > wait before complaining about missing timestamps. > > Fixes: 7cab44f1c35f ("ice: Introduce ETH56G PHY model for E825C products") > Signed-off-by: Jacob Keller <[email protected]> > --- > drivers/net/ethernet/intel/ice/ice_ptp.c | 78 > ++++++++++++++++---------------- > 1 file changed, 39 insertions(+), 39 deletions(-)
Tested-by: Alexander Nowlin <[email protected]>
