Subject: Re: [PATCH net] igb: extend PTP timestamp adjustments to i211

From: Jiri Pirko <[email-masked]>
Date: Thu, 29 Feb 2024 09:29:05 +0100

Wed, Feb 28, 2024 at 06:43:03PM CET, [email-masked] wrote:
>
>
>> -----Original Message-----
>> From: Oleksij Rempel <[email-masked]>
>> Sent: Wednesday, February 28, 2024 4:28 AM
>> To: Jiri Pirko <[email-masked]>
>> Cc: Nguyen, Anthony L <[email-masked]>; [email-masked];
>> [email-masked]; [email-masked]; [email-masked];
>> [email-masked]; [email-masked]; [email-masked];
>> Keller, Jacob E <[email-masked]>; Pucha, HimasekharX Reddy
>> <[email-masked]>
>> Subject: Re: [PATCH net] igb: extend PTP timestamp adjustments to i211
>> 
>> On Wed, Feb 28, 2024 at 10:37:56AM +0100, Jiri Pirko wrote:
>> > Tue, Feb 27, 2024 at 07:49:41PM CET, [email-masked] wrote:
>> > >From: Oleksij Rempel <[email-masked]>
>> > >
>> > >The i211 requires the same PTP timestamp adjustments as the i210,
>> > >according to its datasheet. To ensure consistent timestamping across
>> > >different platforms, this change extends the existing adjustments to
>> > >include the i211.
>> > >
>> > >The adjustment result are tested and comparable for i210 and i211 based
>> > >systems.
>> > >
>> > >Fixes: 3f544d2a4d5c ("igb: adjust PTP timestamps for Tx/Rx latency")
>> >
>> > IIUC, you are just extending the timestamp adjusting to another HW, not
>> > actually fixing any error, don't you? In that case, I don't see why not
>> > to rather target net-next and avoid "Fixes" tag. Or do I misunderstand
>> > this?
>> 
>> From my perspective, it was an error, since two nearly identical systems
>> with only one difference (one used i210 other i211) showed different PTP
>> measurements. So, it would be nice if distributions would include this
>> fix. On other hand, I'm ok with what ever maintainer would decide how
>> to handle this patch.
>> 
>> Regards,
>> Oleksij
>
>Without this, the i211 doesn't apply the Tx/Rx latency adjustments, so the timestamps would be less accurate than if the corrections are applied. On the one hand I guess this is a "feature" and the lack of a feature isn't a bug, so maybe its not viewed as a bug fix then.

The behaviour of i211 is the same as it always was. I mean, 3f544d2a4d5c
didn't cause any regression. From that perspective, it is clearly a
feature.

I know that in netdev we are taking the meaning of "Fixes" quite on the
edge often, but I think this is off the cliff :)

>
>Another interpretation is that lacking those corrections is a bug which this patch fixes.
>
>Thanks,
>Jake
>
>> --
>> Pengutronix e.K.                           |                             |
>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |