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

[Jiri Pirko <jiri@resnulli.us>]

Wed, Feb 28, 2024 at 06:43:03PM CET, jacob.e.keller@intel.com wrote:
>
>
>> -----Original Message-----
>> From: Oleksij Rempel <o.rempel@pengutronix.de>
>> Sent: Wednesday, February 28, 2024 4:28 AM
>> To: Jiri Pirko <jiri@resnulli.us>
>> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net;
>> kuba@kernel.org; pabeni@redhat.com; edumazet@google.com;
>> netdev@vger.kernel.org; richardcochran@gmail.com; nathan.sullivan@ni.com;
>> Keller, Jacob E <jacob.e.keller@intel.com>; Pucha, HimasekharX Reddy
>> <himasekharx.reddy.pucha@intel.com>
>> 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, anthony.l.nguyen@intel.com wrote:
>> > >From: Oleksij Rempel <o.rempel@pengutronix.de>
>> > >
>> > >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 |