diff mbox series

[net-next,v6,09/13] net: dsa: microchip: ptp: move pdelay_rsp correction field to tail tag

Message ID 20230102050459.31023-10-arun.ramadoss@microchip.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: dsa: microchip: add PTP support for KSZ9563/KSZ8563 and LAN937x | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 11 of 11 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
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: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 85 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Arun Ramadoss Jan. 2, 2023, 5:04 a.m. UTC 
From: Christian Eggers <ceggers@arri.de>

For PDelay_Resp messages we will likely have a negative value in the
correction field. The switch hardware cannot correctly update such
values (produces an off by one error in the UDP checksum), so it must be
moved to the time stamp field in the tail tag. Format of the correction
field is 48 bit ns + 16 bit fractional ns.  After updating the
correction field, clone is no longer required hence it is freed.

Signed-off-by: Christian Eggers <ceggers@arri.de>
Co-developed-by: Arun Ramadoss <arun.ramadoss@microchip.com>
Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
---
v2 -> v3
- used update_correction variable in skb->cb instead of ptp_msg_type

v1 -> v2
- added fallthrough keyword in switch case to suppress checkpatch
warning

RFC v3 -> Patch v1
- Patch is separated from transmission logic patch
---
 drivers/net/dsa/microchip/ksz_ptp.c |  5 ++++
 include/linux/dsa/ksz_common.h      |  2 ++
 net/dsa/tag_ksz.c                   | 41 ++++++++++++++++++++++++++++-
 3 files changed, 47 insertions(+), 1 deletion(-)

Comments

Vladimir Oltean Jan. 3, 2023, 5:40 p.m. UTC | #1 
On Mon, Jan 02, 2023 at 10:34:55AM +0530, Arun Ramadoss wrote:
> From: Christian Eggers <ceggers@arri.de>
> 
> For PDelay_Resp messages we will likely have a negative value in the
> correction field. The switch hardware cannot correctly update such
> values (produces an off by one error in the UDP checksum), so it must be
> moved to the time stamp field in the tail tag. Format of the correction
> field is 48 bit ns + 16 bit fractional ns.  After updating the
> correction field, clone is no longer required hence it is freed.
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> Co-developed-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> ---
> v2 -> v3
> - used update_correction variable in skb->cb instead of ptp_msg_type
> 
> v1 -> v2
> - added fallthrough keyword in switch case to suppress checkpatch
> warning

I don't think checkpatch asks for fallthrough keyword if there is no
code in between the cases. That would be silly, because it's obvious
that the code falls through. It's most likely that the fallthrough is
needed, but not _here_.

> 
> RFC v3 -> Patch v1
> - Patch is separated from transmission logic patch
> ---
>  drivers/net/dsa/microchip/ksz_ptp.c |  5 ++++
>  include/linux/dsa/ksz_common.h      |  2 ++
>  net/dsa/tag_ksz.c                   | 41 ++++++++++++++++++++++++++++-
>  3 files changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
> index 8f5e099b1b99..5d5b8d4ed17b 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.c
> +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> @@ -267,6 +267,8 @@ void ksz_port_txtstamp(struct dsa_switch *ds, int port,
>  
>  	switch (ptp_msg_type) {
>  	case PTP_MSGTYPE_PDELAY_REQ:
> +		 fallthrough;

On the other hand, I would expect checkpatch to warn about the
superfluous space. There should be just 2 leading tabs here, not 2 tabs
and 1 space.

> +	case PTP_MSGTYPE_PDELAY_RESP:
>  		break;
>  
>  	default:
> @@ -279,6 +281,9 @@ void ksz_port_txtstamp(struct dsa_switch *ds, int port,
>  
>  	/* caching the value to be used in tag_ksz.c */
>  	KSZ_SKB_CB(skb)->clone = clone;
> +	KSZ_SKB_CB(clone)->ptp_type = type;
> +	if (ptp_msg_type == PTP_MSGTYPE_PDELAY_RESP)
> +		KSZ_SKB_CB(clone)->update_correction = true;
>  }
>  
>  static void ksz_ptp_txtstamp_skb(struct ksz_device *dev,
> diff --git a/include/linux/dsa/ksz_common.h b/include/linux/dsa/ksz_common.h
> index b91beab5e138..576a99ca698d 100644
> --- a/include/linux/dsa/ksz_common.h
> +++ b/include/linux/dsa/ksz_common.h
> @@ -36,6 +36,8 @@ struct ksz_tagger_data {
>  
>  struct ksz_skb_cb {
>  	struct sk_buff *clone;
> +	unsigned int ptp_type;
> +	bool update_correction;
>  	u32 tstamp;
>  };
>  
> diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
> index e14ee26bf6a0..7dd2133b0820 100644
> --- a/net/dsa/tag_ksz.c
> +++ b/net/dsa/tag_ksz.c
> @@ -7,6 +7,7 @@
>  #include <linux/dsa/ksz_common.h>
>  #include <linux/etherdevice.h>
>  #include <linux/list.h>
> +#include <linux/ptp_classify.h>
>  #include <net/dsa.h>
>  
>  #include "tag.h"
> @@ -194,14 +195,52 @@ static void ksz_rcv_timestamp(struct sk_buff *skb, u8 *tag)
>   */
>  static void ksz_xmit_timestamp(struct dsa_port *dp, struct sk_buff *skb)
>  {
> +	struct sk_buff *clone = KSZ_SKB_CB(skb)->clone;
>  	struct ksz_tagger_private *priv;
> +	struct ptp_header *ptp_hdr;
> +	bool update_correction;
> +	unsigned int ptp_type;
> +	u32 tstamp_raw = 0;
> +	s64 correction;
>  
>  	priv = ksz_tagger_private(dp->ds);
>  
>  	if (!test_bit(KSZ_HWTS_EN, &priv->state))
>  		return;
>  
> -	put_unaligned_be32(0, skb_put(skb, KSZ_PTP_TAG_LEN));
> +	if (!clone)
> +		goto output_tag;
> +
> +	update_correction = KSZ_SKB_CB(clone)->update_correction;
> +	if (!update_correction)

I don't think the on-stack variable "update_correction" really serves a
purpose here, since it's used only once; it would be simpler if you just
used "if (KSZ_SKB_CB()->update...)".

> +		goto output_tag;
> +
> +	ptp_type = KSZ_SKB_CB(clone)->ptp_type;
> +
> +	ptp_hdr = ptp_parse_header(skb, ptp_type);
> +	if (!ptp_hdr)
> +		goto output_tag;
> +
> +	correction = (s64)get_unaligned_be64(&ptp_hdr->correction);
> +
> +	if (correction < 0) {
> +		struct timespec64 ts;
> +
> +		ts = ns_to_timespec64(-correction >> 16);
> +		tstamp_raw = ((ts.tv_sec & 3) << 30) | ts.tv_nsec;
> +
> +		/* Set correction field to 0 and update UDP checksum.  */
> +		ptp_header_update_correction(skb, ptp_type, ptp_hdr, 0);
> +	}
> +
> +	/* For PDelay_Resp messages, the clone is not required in
> +	 * skb_complete_tx_timestamp() and should be freed here.
> +	 */
> +	kfree_skb(clone);
> +	KSZ_SKB_CB(skb)->clone = NULL;

Why do you clone the skb in the first place, then?
Just extend KSZ_SKB_CB() with some other flag to denote that this skb
needs processing here, and replace the "if (!clone)" test with that.

> +
> +output_tag:
> +	put_unaligned_be32(tstamp_raw, skb_put(skb, KSZ_PTP_TAG_LEN));
>  }
>  
>  /* Defer transmit if waiting for egress time stamp is required.  */
> -- 
> 2.36.1
>
diff mbox series

Patch

diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
index 8f5e099b1b99..5d5b8d4ed17b 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.c
+++ b/drivers/net/dsa/microchip/ksz_ptp.c
@@ -267,6 +267,8 @@  void ksz_port_txtstamp(struct dsa_switch *ds, int port,
 
 	switch (ptp_msg_type) {
 	case PTP_MSGTYPE_PDELAY_REQ:
+		 fallthrough;
+	case PTP_MSGTYPE_PDELAY_RESP:
 		break;
 
 	default:
@@ -279,6 +281,9 @@  void ksz_port_txtstamp(struct dsa_switch *ds, int port,
 
 	/* caching the value to be used in tag_ksz.c */
 	KSZ_SKB_CB(skb)->clone = clone;
+	KSZ_SKB_CB(clone)->ptp_type = type;
+	if (ptp_msg_type == PTP_MSGTYPE_PDELAY_RESP)
+		KSZ_SKB_CB(clone)->update_correction = true;
 }
 
 static void ksz_ptp_txtstamp_skb(struct ksz_device *dev,
diff --git a/include/linux/dsa/ksz_common.h b/include/linux/dsa/ksz_common.h
index b91beab5e138..576a99ca698d 100644
--- a/include/linux/dsa/ksz_common.h
+++ b/include/linux/dsa/ksz_common.h
@@ -36,6 +36,8 @@  struct ksz_tagger_data {
 
 struct ksz_skb_cb {
 	struct sk_buff *clone;
+	unsigned int ptp_type;
+	bool update_correction;
 	u32 tstamp;
 };
 
diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index e14ee26bf6a0..7dd2133b0820 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c
@@ -7,6 +7,7 @@ 
 #include <linux/dsa/ksz_common.h>
 #include <linux/etherdevice.h>
 #include <linux/list.h>
+#include <linux/ptp_classify.h>
 #include <net/dsa.h>
 
 #include "tag.h"
@@ -194,14 +195,52 @@  static void ksz_rcv_timestamp(struct sk_buff *skb, u8 *tag)
  */
 static void ksz_xmit_timestamp(struct dsa_port *dp, struct sk_buff *skb)
 {
+	struct sk_buff *clone = KSZ_SKB_CB(skb)->clone;
 	struct ksz_tagger_private *priv;
+	struct ptp_header *ptp_hdr;
+	bool update_correction;
+	unsigned int ptp_type;
+	u32 tstamp_raw = 0;
+	s64 correction;
 
 	priv = ksz_tagger_private(dp->ds);
 
 	if (!test_bit(KSZ_HWTS_EN, &priv->state))
 		return;
 
-	put_unaligned_be32(0, skb_put(skb, KSZ_PTP_TAG_LEN));
+	if (!clone)
+		goto output_tag;
+
+	update_correction = KSZ_SKB_CB(clone)->update_correction;
+	if (!update_correction)
+		goto output_tag;
+
+	ptp_type = KSZ_SKB_CB(clone)->ptp_type;
+
+	ptp_hdr = ptp_parse_header(skb, ptp_type);
+	if (!ptp_hdr)
+		goto output_tag;
+
+	correction = (s64)get_unaligned_be64(&ptp_hdr->correction);
+
+	if (correction < 0) {
+		struct timespec64 ts;
+
+		ts = ns_to_timespec64(-correction >> 16);
+		tstamp_raw = ((ts.tv_sec & 3) << 30) | ts.tv_nsec;
+
+		/* Set correction field to 0 and update UDP checksum.  */
+		ptp_header_update_correction(skb, ptp_type, ptp_hdr, 0);
+	}
+
+	/* For PDelay_Resp messages, the clone is not required in
+	 * skb_complete_tx_timestamp() and should be freed here.
+	 */
+	kfree_skb(clone);
+	KSZ_SKB_CB(skb)->clone = NULL;
+
+output_tag:
+	put_unaligned_be32(tstamp_raw, skb_put(skb, KSZ_PTP_TAG_LEN));
 }
 
 /* Defer transmit if waiting for egress time stamp is required.  */