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?
Regards,
CL
