Re: [PATCH net] net: ethernet: ti: am65-cpts: Fix PTPv1 message type on TX packets

[Paolo Abeni]

On Tue, 2024-04-23 at 16:36 +0530, Ravi Gunasekaran wrote:
> On 4/23/24 3:31 PM, Paolo Abeni wrote:
> > On Fri, 2024-04-19 at 13:35 +0530, Ravi Gunasekaran wrote:
> > > From: Jason Reeder <jreeder@xxxxxx>
> > > 
> > > The CPTS, by design, captures the messageType (Sync, Delay_Req, etc.)
> > > field from the second nibble of the PTP header which is defined in the
> > > PTPv2 (1588-2008) specification. In the PTPv1 (1588-2002) specification
> > > the first two bytes of the PTP header are defined as the versionType
> > > which is always 0x0001. This means that any PTPv1 packets that are
> > > tagged for TX timestamping by the CPTS will have their messageType set
> > > to 0x0 which corresponds to a Sync message type. This causes issues
> > > when a PTPv1 stack is expecting a Delay_Req (messageType: 0x1)
> > > timestamp that never appears.
> > > 
> > > Fix this by checking if the ptp_class of the timestamped TX packet is
> > > PTP_CLASS_V1 and then matching the PTP sequence ID to the stored
> > > sequence ID in the skb->cb data structure. If the sequence IDs match
> > > and the packet is of type PTPv1 then there is a chance that the
> > > messageType has been incorrectly stored by the CPTS so overwrite the
> > > messageType stored by the CPTS with the messageType from the skb->cb
> > > data structure. This allows the PTPv1 stack to receive TX timestamps
> > > for Delay_Req packets which are necessary to lock onto a PTP Leader.
> > > 
> > > Signed-off-by: Jason Reeder <jreeder@xxxxxx>
> > > Signed-off-by: Ravi Gunasekaran <r-gunasekaran@xxxxxx>
> > 
> > Please provide a suitable fixes tag, thanks!
> 
> am65_cpts_match_tx_ts() was added in the very first commit of the file.
> Would that be a suitable fixes tag? 

It looks like it is, since such function exposed the buggy behaviour
since the beginning.

> I understand that the purpose of
> the fixes tag is to know to which all previous kernels, the fix needs to
> be applied. 

Yes, it's used by stable team(s) to filter fixes relevant for a given
stable tree. A correct hash allow to do that automatically.


Cheers,

Paolo