Re: [PATCH net-next v3 3/5] eth: fbnic: add RX packets timestamping support

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 05/10/2024 00:14, Jacob Keller wrote:
> 
> 
> On 10/3/2024 5:39 AM, Vadim Fedorenko wrote:
>> Add callbacks to support timestamping configuration via ethtool.
>> Add processing of RX timestamps.
>>
>> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
>>   
>> +/**
>> + * fbnic_ts40_to_ns() - convert descriptor timestamp to PHC time
>> + * @fbn: netdev priv of the FB NIC
>> + * @ts40: timestamp read from a descriptor
>> + *
>> + * Return: u64 value of PHC time in nanoseconds
>> + *
>> + * Convert truncated 40 bit device timestamp as read from a descriptor
>> + * to the full PHC time in nanoseconds.
>> + */
>> +static __maybe_unused u64 fbnic_ts40_to_ns(struct fbnic_net *fbn, u64 ts40)
>> +{
>> +	unsigned int s;
>> +	u64 time_ns;
>> +	s64 offset;
>> +	u8 ts_top;
>> +	u32 high;
>> +
>> +	do {
>> +		s = u64_stats_fetch_begin(&fbn->time_seq);
>> +		offset = READ_ONCE(fbn->time_offset);
>> +	} while (u64_stats_fetch_retry(&fbn->time_seq, s));
>> +
>> +	high = READ_ONCE(fbn->time_high);
>> +
>> +	/* Bits 63..40 from periodic clock reads, 39..0 from ts40 */
>> +	time_ns = (u64)(high >> 8) << 40 | ts40;
>> +
>> +	/* Compare bits 32-39 between periodic reads and ts40,
>> +	 * see if HW clock may have wrapped since last read
>> +	 */
>> +	ts_top = ts40 >> 32;
>> +	if (ts_top < (u8)high && (u8)high - ts_top > U8_MAX / 2)
>> +		time_ns += 1ULL << 40;
>> +
>> +	return time_ns + offset;
>> +}
>> +
> 
> This logic doesn't seem to match the logic used by the cyclecounter
> code, and Its not clear to me if this safe against a race between
> time_high updating and the packet timestamp arriving.
> 
> the timestamp could arrive either before or after the time_high update,
> and the logic needs to ensure the appropriate high bits are applied in
> both cases.

To avoid this race condition we decided to make sure that incoming
timestamps are always later then cached high bits. That will make the
logic above correct.

> Again, I think your use case makes sense to just implement with a
> timecounter and cyclecounter, since you're not modifying the hardware
> cycle counter and are leaving it as free-running.

After discussion with Jakub we decided to keep simple logic without
timecounter + cyclecounter, as it's pretty straight forward.