diff mbox series

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

Message ID 20240419080547.10682-1-r-gunasekaran@ti.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net] net: ethernet: ti: am65-cpts: Fix PTPv1 message type on TX packets | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present fail Series targets non-next tree, but doesn't contain any Fixes tags
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch warning WARNING: line length of 85 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-04-21--03-00 (tests: 995)

Commit Message

Ravi Gunasekaran April 19, 2024, 8:05 a.m. UTC 
From: Jason Reeder <jreeder@ti.com>

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@ti.com>
Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
---
 drivers/net/ethernet/ti/am65-cpts.c | 5 +++++
 1 file changed, 5 insertions(+)


base-commit: a35e92ef04c07bd473404b9b73d489aea19a60a8

Comments

Jason Reeder April 19, 2024, 5:03 p.m. UTC | #1 
On 4/19/24 02:05, Ravi Gunasekaran wrote:
> From: Jason Reeder <jreeder@ti.com>
> 
> 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@ti.com>
> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
> ---
>   drivers/net/ethernet/ti/am65-cpts.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
> index c66618d91c28..f89716b1cfb6 100644
> --- a/drivers/net/ethernet/ti/am65-cpts.c
> +++ b/drivers/net/ethernet/ti/am65-cpts.c
> @@ -784,6 +784,11 @@ static bool am65_cpts_match_tx_ts(struct am65_cpts *cpts,
>   		struct am65_cpts_skb_cb_data *skb_cb =
>   					(struct am65_cpts_skb_cb_data *)skb->cb;
>   
> +		if ((ptp_classify_raw(skb) & PTP_CLASS_V1) &&
> +		    ((mtype_seqid & AM65_CPTS_EVENT_1_SEQUENCE_ID_MASK) ==
> +		     (skb_cb->skb_mtype_seqid & AM65_CPTS_EVENT_1_SEQUENCE_ID_MASK)))
> +			mtype_seqid = skb_cb->skb_mtype_seqid;
> +
>   		if (mtype_seqid == skb_cb->skb_mtype_seqid) {
>   			u64 ns = event->timestamp;
>   
> 
> base-commit: a35e92ef04c07bd473404b9b73d489aea19a60a8

++ Ed Trexel at HP and the Audinate support team for visibility and 
'Tested-by' tags.

Jason Reeder
Paolo Abeni April 23, 2024, 10:01 a.m. UTC | #2 
On Fri, 2024-04-19 at 13:35 +0530, Ravi Gunasekaran wrote:
> From: Jason Reeder <jreeder@ti.com>
> 
> 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@ti.com>
> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>

Please provide a suitable fixes tag, thanks!

Paolo
Ravi Gunasekaran April 23, 2024, 11:06 a.m. UTC | #3 
Paolo,

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@ti.com>
>>
>> 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@ti.com>
>> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
> 
> 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? I understand that the purpose of
the fixes tag is to know to which all previous kernels, the fix needs to
be applied. 

Please let me know, if it is ok to provide first commit as fixes tag, so that
I can send a v2.

> 
> Paolo
>
Paolo Abeni April 23, 2024, 11:44 a.m. UTC | #4 
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@ti.com>
> > > 
> > > 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@ti.com>
> > > Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
> > 
> > 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
diff mbox series

Patch

diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
index c66618d91c28..f89716b1cfb6 100644
--- a/drivers/net/ethernet/ti/am65-cpts.c
+++ b/drivers/net/ethernet/ti/am65-cpts.c
@@ -784,6 +784,11 @@  static bool am65_cpts_match_tx_ts(struct am65_cpts *cpts,
 		struct am65_cpts_skb_cb_data *skb_cb =
 					(struct am65_cpts_skb_cb_data *)skb->cb;
 
+		if ((ptp_classify_raw(skb) & PTP_CLASS_V1) &&
+		    ((mtype_seqid & AM65_CPTS_EVENT_1_SEQUENCE_ID_MASK) ==
+		     (skb_cb->skb_mtype_seqid & AM65_CPTS_EVENT_1_SEQUENCE_ID_MASK)))
+			mtype_seqid = skb_cb->skb_mtype_seqid;
+
 		if (mtype_seqid == skb_cb->skb_mtype_seqid) {
 			u64 ns = event->timestamp;