diff mbox series

[net-next,5/7] bnxt_en: Get the RX packet timestamp.

Message ID 1622249601-7106-6-git-send-email-michael.chan@broadcom.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series bnxt_en: Add hardware PTP timestamping support on 575XX devices. | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 4 of 4 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 83 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Michael Chan May 29, 2021, 12:53 a.m. UTC 
From: Pavan Chebbi <pavan.chebbi@broadcom.com>

If the RX packet is timestamped by the hardware, the RX completion
record will contain the lower 32-bit of the timestamp.  This needs
to be combined with the upper 16-bit of the periodic timestamp that
we get from the timer.  The previous snapshot in ptp->old_timer is
used to make sure that the snapshot is not ahead of the RX timestamp
and we adjust for wrap-around if needed.

Reviewed-by: Edwin Peer <edwin.peer@broadcom.com>
Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 19 +++++++++++++++++--
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  3 ++-
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 16 ++++++++++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h |  1 +
 4 files changed, 36 insertions(+), 3 deletions(-)

Comments

Jakub Kicinski May 29, 2021, 1:39 a.m. UTC | #1 
On Fri, 28 May 2021 20:53:19 -0400 Michael Chan wrote:
> +	struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
> +	u64 time;
> +
> +	if (!ptp)
> +		return -ENODEV;
> +
> +	time = READ_ONCE(ptp->old_time);

READ_ONCE() on a u64? That's not gonna prevent tearing the read on 32
bit architectures, right?

> +	*ts = (time & BNXT_HI_TIMER_MASK) | pkt_ts;
> +	if (pkt_ts < (time & BNXT_LO_TIMER_MASK))
> +		*ts += BNXT_LO_TIMER_MASK + 1;

The stamp is from the MAC, I hope, or otherwise packet which could have
been sitting on the ring for some approximation of eternity. You can
easily see a packet stamp older than the value stashed in ptp->old_time
if you run soon after the refresh.
Michael Chan May 29, 2021, 2:31 a.m. UTC | #2 
On Fri, May 28, 2021 at 6:39 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 28 May 2021 20:53:19 -0400 Michael Chan wrote:
> > +     struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
> > +     u64 time;
> > +
> > +     if (!ptp)
> > +             return -ENODEV;
> > +
> > +     time = READ_ONCE(ptp->old_time);
>
> READ_ONCE() on a u64? That's not gonna prevent tearing the read on 32
> bit architectures, right?

Right, we should add a conditional lock for 32-bit architectures.

>
> > +     *ts = (time & BNXT_HI_TIMER_MASK) | pkt_ts;
> > +     if (pkt_ts < (time & BNXT_LO_TIMER_MASK))
> > +             *ts += BNXT_LO_TIMER_MASK + 1;
>
> The stamp is from the MAC, I hope, or otherwise packet which could have
> been sitting on the ring for some approximation of eternity. You can
> easily see a packet stamp older than the value stashed in ptp->old_time
> if you run soon after the refresh.

The hardware returns the low 32-bit timestamp of the packet.
ptp->old_time contains the full 48-bit of the time counter that we
sample periodically.  We're getting the upper 16-bit from
ptp->old_time to form the complete timestamp for the packet.
ptp->old_time is between 1 and 2 sampling periods before the current
time.  The sampling period should be 1 second.

Yeah, if the RX packet is older than 1 to 2 seconds, the upper part
can potentially be wrong if it has wrapped around.
Jakub Kicinski May 29, 2021, 4:11 a.m. UTC | #3 
On Fri, 28 May 2021 19:31:30 -0700 Michael Chan wrote:
> On Fri, May 28, 2021 at 6:39 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > On Fri, 28 May 2021 20:53:19 -0400 Michael Chan wrote:  
> > > +     struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
> > > +     u64 time;
> > > +
> > > +     if (!ptp)
> > > +             return -ENODEV;
> > > +
> > > +     time = READ_ONCE(ptp->old_time);  
> >
> > READ_ONCE() on a u64? That's not gonna prevent tearing the read on 32
> > bit architectures, right?  
> 
> Right, we should add a conditional lock for 32-bit architectures.

Or only store the top 32 bit of the full counter. I don't think you 
need the bottom 16.

> > > +     *ts = (time & BNXT_HI_TIMER_MASK) | pkt_ts;
> > > +     if (pkt_ts < (time & BNXT_LO_TIMER_MASK))
> > > +             *ts += BNXT_LO_TIMER_MASK + 1;  
> >
> > The stamp is from the MAC, I hope, or otherwise packet which could have
> > been sitting on the ring for some approximation of eternity. You can
> > easily see a packet stamp older than the value stashed in ptp->old_time
> > if you run soon after the refresh.  
> 
> The hardware returns the low 32-bit timestamp of the packet.
> ptp->old_time contains the full 48-bit of the time counter that we
> sample periodically.  We're getting the upper 16-bit from
> ptp->old_time to form the complete timestamp for the packet.
> ptp->old_time is between 1 and 2 sampling periods before the current
> time.  The sampling period should be 1 second.
> 
> Yeah, if the RX packet is older than 1 to 2 seconds, the upper part
> can potentially be wrong if it has wrapped around.

Ah, you're comparing to the previous sample, I see.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 2b24d927fd07..1fbfc0326dda 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -1709,9 +1709,9 @@  static int bnxt_rx_pkt(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
 	u8 *data_ptr, agg_bufs, cmp_type;
 	dma_addr_t dma_addr;
 	struct sk_buff *skb;
+	u32 flags, misc;
 	void *data;
 	int rc = 0;
-	u32 misc;
 
 	rxcmp = (struct rx_cmp *)
 			&cpr->cp_desc_ring[CP_RING(cp_cons)][CP_IDX(cp_cons)];
@@ -1809,7 +1809,8 @@  static int bnxt_rx_pkt(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
 		goto next_rx_no_len;
 	}
 
-	len = le32_to_cpu(rxcmp->rx_cmp_len_flags_type) >> RX_CMP_LEN_SHIFT;
+	flags = le32_to_cpu(rxcmp->rx_cmp_len_flags_type);
+	len = flags >> RX_CMP_LEN_SHIFT;
 	dma_addr = rx_buf->mapping;
 
 	if (bnxt_rx_xdp(bp, rxr, cons, data, &data_ptr, &len, event)) {
@@ -1886,6 +1887,20 @@  static int bnxt_rx_pkt(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
 		}
 	}
 
+	if (unlikely((flags & RX_CMP_FLAGS_ITYPES_MASK) ==
+		     RX_CMP_FLAGS_ITYPE_PTP_W_TS)) {
+		if (bp->flags & BNXT_FLAG_CHIP_P5) {
+			u32 cmpl_ts = le32_to_cpu(rxcmp1->rx_cmp_timestamp);
+			u64 ns, ts;
+
+			if (!bnxt_get_rx_ts_p5(bp, &ts, cmpl_ts)) {
+				ns = timecounter_cyc2time(&bp->ptp_cfg->tc, ts);
+				memset(skb_hwtstamps(skb), 0,
+				       sizeof(*skb_hwtstamps(skb)));
+				skb_hwtstamps(skb)->hwtstamp = ns_to_ktime(ns);
+			}
+		}
+	}
 	bnxt_deliver_skb(bp, bnapi, skb);
 	rc = 1;
 
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index cf10cfff5c1b..dbefa77279ef 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -159,6 +159,7 @@  struct rx_cmp {
 	#define RX_CMP_FLAGS_RSS_VALID				(1 << 10)
 	#define RX_CMP_FLAGS_UNUSED				(1 << 11)
 	 #define RX_CMP_FLAGS_ITYPES_SHIFT			 12
+	 #define RX_CMP_FLAGS_ITYPES_MASK			 0xf000
 	 #define RX_CMP_FLAGS_ITYPE_UNKNOWN			 (0 << 12)
 	 #define RX_CMP_FLAGS_ITYPE_IP				 (1 << 12)
 	 #define RX_CMP_FLAGS_ITYPE_TCP				 (2 << 12)
@@ -240,7 +241,7 @@  struct rx_cmp_ext {
 	#define RX_CMPL_CFA_CODE_MASK				(0xffff << 16)
 	 #define RX_CMPL_CFA_CODE_SFT				 16
 
-	__le32 rx_cmp_unused3;
+	__le32 rx_cmp_timestamp;
 };
 
 #define RX_CMP_L2_ERRORS						\
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
index bdf79850fc2d..538b3c8f97f1 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
@@ -247,6 +247,22 @@  static u64 bnxt_cc_read(const struct cyclecounter *cc)
 	return ns;
 }
 
+int bnxt_get_rx_ts_p5(struct bnxt *bp, u64 *ts, u32 pkt_ts)
+{
+	struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
+	u64 time;
+
+	if (!ptp)
+		return -ENODEV;
+
+	time = READ_ONCE(ptp->old_time);
+	*ts = (time & BNXT_HI_TIMER_MASK) | pkt_ts;
+	if (pkt_ts < (time & BNXT_LO_TIMER_MASK))
+		*ts += BNXT_LO_TIMER_MASK + 1;
+
+	return 0;
+}
+
 int bnxt_ptp_start(struct bnxt *bp)
 {
 	struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
index 7ea58a26720d..3bea71111231 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
@@ -53,6 +53,7 @@  struct bnxt_ptp_cfg {
 int bnxt_ptp_get_current_time(struct bnxt *bp);
 int bnxt_hwtstamp_set(struct net_device *dev, struct ifreq *ifr);
 int bnxt_hwtstamp_get(struct net_device *dev, struct ifreq *ifr);
+int bnxt_get_rx_ts_p5(struct bnxt *bp, u64 *ts, u32 pkt_ts);
 int bnxt_ptp_start(struct bnxt *bp);
 int bnxt_ptp_init(struct bnxt *bp);
 void bnxt_ptp_clear(struct bnxt *bp);