On 19/09/2025 08:17, Choong, Chwee Lin wrote:
On Friday, September 19, 2025 6:11 AM, Keller, Jacob E
<[email protected]> wrote:
On 9/18/2025 1:47 PM, Vadim Fedorenko wrote:
On 18/09/2025 19:38, Chwee-Lin Choong wrote:
The current HW bug workaround checks the TXTT_0 ready bit first, then
reads LOW -> HIGH -> LOW from register 0 to detect if a timestamp was
captured.
This sequence has a race: if a new timestamp is latched after reading
the TXTT mask but before the first LOW read, both old and new
timestamp match, causing the driver to drop a valid timestamp.
Fix by reading the LOW register first, then the TXTT mask, so a newly
latched timestamp will always be detected.
This fix also prevents TX unit hangs observed under heavy
timestamping load.
Fixes: c789ad7cbebc ("igc: Work around HW bug causing missing
timestamps")
Suggested-by: Avi Shalev <[email protected]>
Signed-off-by: Song Yoong Siang <[email protected]>
Signed-off-by: Chwee-Lin Choong <[email protected]>
---
drivers/net/ethernet/intel/igc/igc_ptp.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
[...]
* timestamp was captured, we can read the "high"
* register again.
*/
This comment begins with 'read the "high" register (to latch a new
timestamp)' ...
- u32 txstmpl_old, txstmpl_new;
+ u32 txstmpl_new;
- txstmpl_old = rd32(IGC_TXSTMPL);
rd32(IGC_TXSTMPH);
txstmpl_new = rd32(IGC_TXSTMPL);
and a couple of lines later in this function you have
regval = txstmpl_new;
regval |= (u64)rd32(IGC_TXSTMPH) << 32;
According to the comment above, the value in the register will be
latched after reading IGC_TXSTMPH. As there will be no read of "low"
part of the register, it will stay latched with old value until the
next call to the same function. Could it be the reason of unit hangs?
It looks like the value of previous read of IGC_TXSTMPH should be
stored and used to construct new timestamp, right?
I wouldn't trust the comment, but instead double check the data sheets.
Unfortunately, I don't seem to have a copy of the igc hardware data sheet handy
:(
Thanks,
Jake
Flow before this patch:
1. Read the TXTT bits into mask
2. if TXTT_0 == 0, go to workaround ->If at this point register 0
captures TX timestamp, and TXTT_0 is set but we think it is 0.
3. Read LOW to OLD
4. Read HIGH – this clears the TXTT_0
5. Read LOW again , now to NEW.
6. NEW==OLD, so the timestamp is discarded -> causing timestamp timeout
Flow after this patch:
1. Read LOW to OLD
2. Read the TXTT bits into mask
3. if TXTT_0 == 0, go to workaround -> If at this point register 0
captures TX timestamp, and TXTT_0 is set but we think it is 0.
4. Read HIGH – this clears the TXTT_0
5. Read LOW again , now to NEW.
6. NEW!=OLD, so we detect this is a valid timestamp
7. Read HIGH again and use the timestamp
Let me know if this address your questions?
Unfortunately, it doesn't. The question is "what will happen to register
after step 7?" The comment above says it will stay latched until LOW is
read, will it affect performance/stability?
