[PATCH net-next 00/10] bnxt_en: PTP updates for net-next

[PATCH net-next 00/10] bnxt_en: PTP updates for net-next

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 1438 bytes --]

The first 5 patches implement the PTP feature on the new BCM5760X
chips.  The main new hardware feature is the new TX timestamp
completion which enables the driver to retrieve the TX timestamp
in NAPI without deferring to the PTP worker.

The last 5 patches increase the number of TX PTP packets in-flight
from 1 to 4 on the older BCM5750X chips.  On these older chips, we
need to call firmware in the PTP worker to retrieve the timestamp.
We use an arry to keep track of the in-flight TX PTP packets.

Michael Chan (4):
  bnxt_en: Add new TX timestamp completion definitions
  bnxt_en: Add is_ts_pkt field to struct bnxt_sw_tx_bd
  bnxt_en: Allow some TX packets to be unprocessed in NAPI
  bnxt_en: Add TX timestamp completion logic

Pavan Chebbi (6):
  bnxt_en: Add BCM5760X specific PHC registers mapping
  bnxt_en: Refactor all PTP TX timestamp fields into a struct
  bnxt_en: Remove an impossible condition check for PTP TX pending SKB
  bnxt_en: Let bnxt_stamp_tx_skb() return error code
  bnxt_en: Increase the max total outstanding PTP TX packets to 4
  bnxt_en: Remove atomic operations on ptp->tx_avail

 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 109 ++++++++-----
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  42 ++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 151 +++++++++++++-----
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h |  36 ++++-
 4 files changed, 254 insertions(+), 84 deletions(-)

-- 
2.30.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

[PATCH net-next 01/10] bnxt_en: Add new TX timestamp completion definitions

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 1980 bytes --]

The new BCM5760X chips will generate this new TX timestamp completion
when a TX packet's timestamp has been taken right before transmission.
The driver logic to retreive the timestamp will be added in the next
few patches.

Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.h | 26 +++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 9cf0acfa04e5..d3ad73d4c00a 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -181,6 +181,32 @@ struct tx_cmp {
 #define TX_CMP_SQ_CONS_IDX(txcmp)					\
 	(le32_to_cpu((txcmp)->sq_cons_idx) & TX_CMP_SQ_CONS_IDX_MASK)
 
+struct tx_ts_cmp {
+	__le32 tx_ts_cmp_flags_type;
+	#define TX_TS_CMP_FLAGS_ERROR				(1 << 6)
+	#define TX_TS_CMP_FLAGS_TS_TYPE				(1 << 7)
+	 #define TX_TS_CMP_FLAGS_TS_TYPE_PM			 (0 << 7)
+	 #define TX_TS_CMP_FLAGS_TS_TYPE_PA			 (1 << 7)
+	#define TX_TS_CMP_FLAGS_TS_FALLBACK			(1 << 8)
+	#define TX_TS_CMP_TS_SUB_NS				(0xf << 12)
+	#define TX_TS_CMP_TS_NS_MID				(0xffff << 16)
+	#define TX_TS_CMP_TS_NS_MID_SFT				16
+	u32 tx_ts_cmp_opaque;
+	__le32 tx_ts_cmp_errors_v;
+	#define TX_TS_CMP_V					(1 << 0)
+	#define TX_TS_CMP_TS_INVALID_ERR			(1 << 10)
+	__le32 tx_ts_cmp_ts_ns_lo;
+};
+
+#define BNXT_GET_TX_TS_48B_NS(tscmp)					\
+	(le32_to_cpu((tscmp)->tx_ts_cmp_ts_ns_lo) |			\
+	 ((u64)(le32_to_cpu((tscmp)->tx_ts_cmp_flags_type) &		\
+	  TX_TS_CMP_TS_NS_MID) << TX_TS_CMP_TS_NS_MID_SFT))
+
+#define BNXT_TX_TS_ERR(tscmp)						\
+	(((tscmp)->tx_ts_cmp_flags_type & cpu_to_le32(TX_TS_CMP_FLAGS_ERROR)) &&\
+	 ((tscmp)->tx_ts_cmp_errors_v & cpu_to_le32(TX_TS_CMP_TS_INVALID_ERR)))
+
 struct rx_cmp {
 	__le32 rx_cmp_len_flags_type;
 	#define RX_CMP_CMP_TYPE					(0x3f << 0)
-- 
2.30.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next 01/10] bnxt_en: Add new TX timestamp completion definitions

[Przemek Kitszel]

On 6/26/24 18:42, Michael Chan wrote:
> The new BCM5760X chips will generate this new TX timestamp completion
> when a TX packet's timestamp has been taken right before transmission.

Tx

> The driver logic to retreive the timestamp will be added in the next

retrieve

> few patches.
> 
> Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
>   drivers/net/ethernet/broadcom/bnxt/bnxt.h | 26 +++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> index 9cf0acfa04e5..d3ad73d4c00a 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> @@ -181,6 +181,32 @@ struct tx_cmp {
>   #define TX_CMP_SQ_CONS_IDX(txcmp)					\
>   	(le32_to_cpu((txcmp)->sq_cons_idx) & TX_CMP_SQ_CONS_IDX_MASK)
>   
> +struct tx_ts_cmp {
> +	__le32 tx_ts_cmp_flags_type;
> +	#define TX_TS_CMP_FLAGS_ERROR				(1 << 6)

those should be BIT(6)

> +	#define TX_TS_CMP_FLAGS_TS_TYPE				(1 << 7)
> +	 #define TX_TS_CMP_FLAGS_TS_TYPE_PM			 (0 << 7)

weird way to spell 0

> +	 #define TX_TS_CMP_FLAGS_TS_TYPE_PA			 (1 << 7)
> +	#define TX_TS_CMP_FLAGS_TS_FALLBACK			(1 << 8)
> +	#define TX_TS_CMP_TS_SUB_NS				(0xf << 12)

GENMASK(),
please use through the series, same for BIT()

> +	#define TX_TS_CMP_TS_NS_MID				(0xffff << 16)
> +	#define TX_TS_CMP_TS_NS_MID_SFT				16
> +	u32 tx_ts_cmp_opaque;
> +	__le32 tx_ts_cmp_errors_v;
> +	#define TX_TS_CMP_V					(1 << 0)
> +	#define TX_TS_CMP_TS_INVALID_ERR			(1 << 10)
> +	__le32 tx_ts_cmp_ts_ns_lo;
> +};
> +
> +#define BNXT_GET_TX_TS_48B_NS(tscmp)					\
> +	(le32_to_cpu((tscmp)->tx_ts_cmp_ts_ns_lo) |			\
> +	 ((u64)(le32_to_cpu((tscmp)->tx_ts_cmp_flags_type) &		\
> +	  TX_TS_CMP_TS_NS_MID) << TX_TS_CMP_TS_NS_MID_SFT))
> +
> +#define BNXT_TX_TS_ERR(tscmp)						\
> +	(((tscmp)->tx_ts_cmp_flags_type & cpu_to_le32(TX_TS_CMP_FLAGS_ERROR)) &&\
> +	 ((tscmp)->tx_ts_cmp_errors_v & cpu_to_le32(TX_TS_CMP_TS_INVALID_ERR)))
> +
>   struct rx_cmp {
>   	__le32 rx_cmp_len_flags_type;
>   	#define RX_CMP_CMP_TYPE					(0x3f << 0)

Re: [PATCH net-next 01/10] bnxt_en: Add new TX timestamp completion definitions

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 1777 bytes --]

On Thu, Jun 27, 2024 at 2:00 AM Przemek Kitszel
<przemyslaw.kitszel@intel.com> wrote:
>
> On 6/26/24 18:42, Michael Chan wrote:
> > The new BCM5760X chips will generate this new TX timestamp completion
> > when a TX packet's timestamp has been taken right before transmission.
>
> Tx
>
> > The driver logic to retreive the timestamp will be added in the next
>
> retrieve
>
> > few patches.
> >
> > Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> > Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
> > Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> > ---
> >   drivers/net/ethernet/broadcom/bnxt/bnxt.h | 26 +++++++++++++++++++++++
> >   1 file changed, 26 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> > index 9cf0acfa04e5..d3ad73d4c00a 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> > @@ -181,6 +181,32 @@ struct tx_cmp {
> >   #define TX_CMP_SQ_CONS_IDX(txcmp)                                   \
> >       (le32_to_cpu((txcmp)->sq_cons_idx) & TX_CMP_SQ_CONS_IDX_MASK)
> >
> > +struct tx_ts_cmp {
> > +     __le32 tx_ts_cmp_flags_type;
> > +     #define TX_TS_CMP_FLAGS_ERROR                           (1 << 6)
>
> those should be BIT(6)
>
> > +     #define TX_TS_CMP_FLAGS_TS_TYPE                         (1 << 7)
> > +      #define TX_TS_CMP_FLAGS_TS_TYPE_PM                      (0 << 7)
>
> weird way to spell 0

These hardware interface structures are generated internally from
yaml.  All similar structures in this .h file have the same format.  I
think it will be better to convert them all together in the future to
keep everything consistent.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

[PATCH net-next 02/10] bnxt_en: Add is_ts_pkt field to struct bnxt_sw_tx_bd

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 3431 bytes --]

Remove the unused is_gso field and add the is_ts_pkt field to struct
bnxt_sw_tx_bd.  This field will mark the TX BD that has requested
HW TX timestamp.  The field needs to be cleared if the timestamp packet
is later aborted.  This field will be useful when processing the
new TX timestamp completion from the hardware in the next patches.

Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 11 +++++++++--
 drivers/net/ethernet/broadcom/bnxt/bnxt.h |  2 +-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 1bd0c5973252..996d9fe80495 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -522,6 +522,7 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
 				if (vlan_tag_flags)
 					ptp->tx_hdr_off += VLAN_HLEN;
 				lflags |= cpu_to_le32(TX_BD_FLAGS_STAMP);
+				tx_buf->is_ts_pkt = 1;
 				skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 			} else {
 				atomic_inc(&bp->ptp_cfg->tx_avail);
@@ -612,9 +613,11 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
 normal_tx:
 	if (length < BNXT_MIN_PKT_SIZE) {
 		pad = BNXT_MIN_PKT_SIZE - length;
-		if (skb_pad(skb, pad))
+		if (skb_pad(skb, pad)) {
 			/* SKB already freed. */
+			tx_buf->is_ts_pkt = 0;
 			goto tx_kick_pending;
+		}
 		length = BNXT_MIN_PKT_SIZE;
 	}
 
@@ -741,6 +744,7 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* start back at beginning and unmap skb */
 	prod = txr->tx_prod;
 	tx_buf = &txr->tx_buf_ring[RING_TX(bp, prod)];
+	tx_buf->is_ts_pkt = 0;
 	dma_unmap_single(&pdev->dev, dma_unmap_addr(tx_buf, mapping),
 			 skb_headlen(skb), DMA_TO_DEVICE);
 	prod = NEXT_TX(prod);
@@ -781,6 +785,7 @@ static void __bnxt_tx_int(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
 	while (RING_TX(bp, cons) != hw_cons) {
 		struct bnxt_sw_tx_bd *tx_buf;
 		struct sk_buff *skb;
+		bool is_ts_pkt;
 		int j, last;
 
 		tx_buf = &txr->tx_buf_ring[RING_TX(bp, cons)];
@@ -800,6 +805,8 @@ static void __bnxt_tx_int(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
 			tx_buf->is_push = 0;
 			goto next_tx_int;
 		}
+		is_ts_pkt = tx_buf->is_ts_pkt;
+		tx_buf->is_ts_pkt = 0;
 
 		dma_unmap_single(&pdev->dev, dma_unmap_addr(tx_buf, mapping),
 				 skb_headlen(skb), DMA_TO_DEVICE);
@@ -814,7 +821,7 @@ static void __bnxt_tx_int(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
 				skb_frag_size(&skb_shinfo(skb)->frags[j]),
 				DMA_TO_DEVICE);
 		}
-		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS)) {
+		if (unlikely(is_ts_pkt)) {
 			if (BNXT_CHIP_P5(bp)) {
 				/* PTP worker takes ownership of the skb */
 				if (!bnxt_get_tx_ts_p5(bp, skb)) {
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index d3ad73d4c00a..00976e8a1e6a 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -874,7 +874,7 @@ struct bnxt_sw_tx_bd {
 	DEFINE_DMA_UNMAP_ADDR(mapping);
 	DEFINE_DMA_UNMAP_LEN(len);
 	struct page		*page;
-	u8			is_gso;
+	u8			is_ts_pkt;
 	u8			is_push;
 	u8			action;
 	unsigned short		nr_frags;
-- 
2.30.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next 02/10] bnxt_en: Add is_ts_pkt field to struct bnxt_sw_tx_bd

[Jakub Kicinski]

On Wed, 26 Jun 2024 09:42:59 -0700 Michael Chan wrote:
> @@ -612,9 +613,11 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  normal_tx:
>  	if (length < BNXT_MIN_PKT_SIZE) {
>  		pad = BNXT_MIN_PKT_SIZE - length;
> -		if (skb_pad(skb, pad))
> +		if (skb_pad(skb, pad)) {
>  			/* SKB already freed. */
> +			tx_buf->is_ts_pkt = 0;
>  			goto tx_kick_pending;
> +		}
>  		length = BNXT_MIN_PKT_SIZE;
>  	}

There is a jump to tx_free in between these two, when DMA mapping
head fails. It appears not to clear is_ts_pkt.

Why not add the clearing above the line on the error patch which 
clears the skb pointer?

@@ -771,6 +770,7 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
        if (txr->kick_pending)
                bnxt_txr_db_kick(bp, txr, txr->tx_prod);
        txr->tx_buf_ring[txr->tx_prod].skb = NULL;
+       txr->tx_buf_ring[txr->tx_prod].is_ts_pkt = 0;
        dev_core_stats_tx_dropped_inc(dev);
        return NETDEV_TX_OK;
 }

> @@ -741,6 +744,7 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	/* start back at beginning and unmap skb */
>  	prod = txr->tx_prod;
>  	tx_buf = &txr->tx_buf_ring[RING_TX(bp, prod)];
> +	tx_buf->is_ts_pkt = 0;
>  	dma_unmap_single(&pdev->dev, dma_unmap_addr(tx_buf, mapping),
>  			 skb_headlen(skb), DMA_TO_DEVICE);
>  	prod = NEXT_TX(prod);

Re: [PATCH net-next 02/10] bnxt_en: Add is_ts_pkt field to struct bnxt_sw_tx_bd

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 1114 bytes --]

On Thu, Jun 27, 2024 at 5:08 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 26 Jun 2024 09:42:59 -0700 Michael Chan wrote:
> > @@ -612,9 +613,11 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
> >  normal_tx:
> >       if (length < BNXT_MIN_PKT_SIZE) {
> >               pad = BNXT_MIN_PKT_SIZE - length;
> > -             if (skb_pad(skb, pad))
> > +             if (skb_pad(skb, pad)) {
> >                       /* SKB already freed. */
> > +                     tx_buf->is_ts_pkt = 0;
> >                       goto tx_kick_pending;
> > +             }
> >               length = BNXT_MIN_PKT_SIZE;
> >       }
>
> There is a jump to tx_free in between these two, when DMA mapping
> head fails. It appears not to clear is_ts_pkt.
>

You are absolutely right.

> Why not add the clearing above the line on the error patch which
> clears the skb pointer?

Yes, that looks like the best place to clear it for all error
conditions.  I think I will consolidate all the PTP cleanup around
that location to make the code look cleaner.  Thanks.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

[PATCH net-next 03/10] bnxt_en: Allow some TX packets to be unprocessed in NAPI

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 3872 bytes --]

The driver's current logic will always free all the TX SKBs up to
txr->tx_hw_cons within NAPI.  In the next patches, we'll be adding
logic to handle TX timestamp completion and we may need to hold
some remaining TX SKBs if we don't have the timestamp completions
yet.

Modify __bnxt_poll_work_done() to clear each event bit separately to
allow bnapi->tx_int() to decide whether to clear BNXT_TX_CMP_EVENT or
not.  bnapi->tx_int() will not clear BNXT_TX_CMP_EVENT if some TX
SKBs are held waiting for TX timestamps.  Note that legacy chips will
never hold any SKBs this way.  The SKB is always deferred to the PTP
worker slow path to retrieve the timestamp from firmware.  On the new
P7 chips, the timestamp is returned by the hardware directly and we
can retrieve it directly from NAPI.

Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 996d9fe80495..784787a09dba 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -772,7 +772,8 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	return NETDEV_TX_OK;
 }
 
-static void __bnxt_tx_int(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
+/* Returns true if some remaining TX packets not processed. */
+static bool __bnxt_tx_int(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
 			  int budget)
 {
 	struct netdev_queue *txq = netdev_get_tx_queue(bp->dev, txr->txq_index);
@@ -795,7 +796,7 @@ static void __bnxt_tx_int(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
 
 		if (unlikely(!skb)) {
 			bnxt_sched_reset_txr(bp, txr, cons);
-			return;
+			return false;
 		}
 
 		tx_pkts++;
@@ -844,18 +845,22 @@ static void __bnxt_tx_int(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
 	__netif_txq_completed_wake(txq, tx_pkts, tx_bytes,
 				   bnxt_tx_avail(bp, txr), bp->tx_wake_thresh,
 				   READ_ONCE(txr->dev_state) == BNXT_DEV_STATE_CLOSING);
+
+	return false;
 }
 
 static void bnxt_tx_int(struct bnxt *bp, struct bnxt_napi *bnapi, int budget)
 {
 	struct bnxt_tx_ring_info *txr;
+	bool more = false;
 	int i;
 
 	bnxt_for_each_napi_tx(i, bnapi, txr) {
 		if (txr->tx_hw_cons != RING_TX(bp, txr->tx_cons))
-			__bnxt_tx_int(bp, txr, budget);
+			more |= __bnxt_tx_int(bp, txr, budget);
 	}
-	bnapi->events &= ~BNXT_TX_CMP_EVENT;
+	if (!more)
+		bnapi->events &= ~BNXT_TX_CMP_EVENT;
 }
 
 static struct page *__bnxt_alloc_rx_page(struct bnxt *bp, dma_addr_t *mapping,
@@ -2952,8 +2957,10 @@ static int __bnxt_poll_work(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
 		}
 	}
 
-	if (event & BNXT_REDIRECT_EVENT)
+	if (event & BNXT_REDIRECT_EVENT) {
 		xdp_do_flush();
+		event &= ~BNXT_REDIRECT_EVENT;
+	}
 
 	if (event & BNXT_TX_EVENT) {
 		struct bnxt_tx_ring_info *txr = bnapi->tx_ring[0];
@@ -2963,6 +2970,7 @@ static int __bnxt_poll_work(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
 		wmb();
 
 		bnxt_db_write_relaxed(bp, &txr->tx_db, prod);
+		event &= ~BNXT_TX_EVENT;
 	}
 
 	cpr->cp_raw_cons = raw_cons;
@@ -2980,13 +2988,14 @@ static void __bnxt_poll_work_done(struct bnxt *bp, struct bnxt_napi *bnapi,
 		struct bnxt_rx_ring_info *rxr = bnapi->rx_ring;
 
 		bnxt_db_write(bp, &rxr->rx_db, rxr->rx_prod);
+		bnapi->events &= ~BNXT_RX_EVENT;
 	}
 	if (bnapi->events & BNXT_AGG_EVENT) {
 		struct bnxt_rx_ring_info *rxr = bnapi->rx_ring;
 
 		bnxt_db_write(bp, &rxr->rx_agg_db, rxr->rx_agg_prod);
+		bnapi->events &= ~BNXT_AGG_EVENT;
 	}
-	bnapi->events &= BNXT_TX_CMP_EVENT;
 }
 
 static int bnxt_poll_work(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
-- 
2.30.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

[PATCH net-next 04/10] bnxt_en: Add TX timestamp completion logic

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 8080 bytes --]

The new BCM5760X chips will return the timestamp of TX packets in a
new completion.  Add logic in __bnxt_poll_work() to handle this
completion type to retrieve the timestamp.  This feature eliminates
the limit on the number of in-flight PTP TX packets.

Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 39 +++++++++++++------
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  1 +
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 32 +++++++++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h |  2 +
 4 files changed, 63 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 784787a09dba..e16f50d822c8 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -456,6 +456,7 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	dma_addr_t mapping;
 	unsigned int length, pad = 0;
 	u32 len, free_size, vlan_tag_flags, cfa_action, flags;
+	struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
 	u16 prod, last_frag;
 	struct pci_dev *pdev = bp->pdev;
 	struct bnxt_tx_ring_info *txr;
@@ -509,10 +510,12 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
 			vlan_tag_flags |= 1 << TX_BD_CFA_META_TPID_SHIFT;
 	}
 
-	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
-		struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
-
-		if (ptp && ptp->tx_tstamp_en && !skb_is_gso(skb)) {
+	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) && ptp &&
+	    ptp->tx_tstamp_en) {
+		if (bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP) {
+			lflags |= cpu_to_le32(TX_BD_FLAGS_STAMP);
+			tx_buf->is_ts_pkt = 1;
+		} else if (!skb_is_gso(skb)) {
 			if (atomic_dec_if_positive(&ptp->tx_avail) < 0) {
 				atomic64_inc(&ptp->stats.ts_err);
 				goto tx_no_ts;
@@ -782,6 +785,7 @@ static bool __bnxt_tx_int(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
 	unsigned int tx_bytes = 0;
 	u16 cons = txr->tx_cons;
 	int tx_pkts = 0;
+	bool rc = false;
 
 	while (RING_TX(bp, cons) != hw_cons) {
 		struct bnxt_sw_tx_bd *tx_buf;
@@ -790,24 +794,29 @@ static bool __bnxt_tx_int(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
 		int j, last;
 
 		tx_buf = &txr->tx_buf_ring[RING_TX(bp, cons)];
-		cons = NEXT_TX(cons);
 		skb = tx_buf->skb;
-		tx_buf->skb = NULL;
 
 		if (unlikely(!skb)) {
 			bnxt_sched_reset_txr(bp, txr, cons);
-			return false;
+			return rc;
+		}
+
+		is_ts_pkt = tx_buf->is_ts_pkt;
+		if (is_ts_pkt && (bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP)) {
+			rc = true;
+			break;
 		}
 
+		cons = NEXT_TX(cons);
 		tx_pkts++;
 		tx_bytes += skb->len;
+		tx_buf->skb = NULL;
+		tx_buf->is_ts_pkt = 0;
 
 		if (tx_buf->is_push) {
 			tx_buf->is_push = 0;
 			goto next_tx_int;
 		}
-		is_ts_pkt = tx_buf->is_ts_pkt;
-		tx_buf->is_ts_pkt = 0;
 
 		dma_unmap_single(&pdev->dev, dma_unmap_addr(tx_buf, mapping),
 				 skb_headlen(skb), DMA_TO_DEVICE);
@@ -846,7 +855,7 @@ static bool __bnxt_tx_int(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
 				   bnxt_tx_avail(bp, txr), bp->tx_wake_thresh,
 				   READ_ONCE(txr->dev_state) == BNXT_DEV_STATE_CLOSING);
 
-	return false;
+	return rc;
 }
 
 static void bnxt_tx_int(struct bnxt *bp, struct bnxt_napi *bnapi, int budget)
@@ -2926,6 +2935,8 @@ static int __bnxt_poll_work(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
 					cpr->has_more_work = 1;
 				break;
 			}
+		} else if (cmp_type == CMP_TYPE_TX_L2_PKT_TS_CMP) {
+			bnxt_tx_ts_cmp(bp, bnapi, (struct tx_ts_cmp *)txcmp);
 		} else if (cmp_type >= CMP_TYPE_RX_L2_CMP &&
 			   cmp_type <= CMP_TYPE_RX_L2_TPA_START_V3_CMP) {
 			if (likely(budget))
@@ -6804,6 +6815,7 @@ static int hwrm_ring_alloc_send_msg(struct bnxt *bp,
 	switch (ring_type) {
 	case HWRM_RING_ALLOC_TX: {
 		struct bnxt_tx_ring_info *txr;
+		u16 flags = 0;
 
 		txr = container_of(ring, struct bnxt_tx_ring_info,
 				   tx_ring_struct);
@@ -6817,6 +6829,9 @@ static int hwrm_ring_alloc_send_msg(struct bnxt *bp,
 		if (bp->flags & BNXT_FLAG_TX_COAL_CMPL)
 			req->cmpl_coal_cnt =
 				RING_ALLOC_REQ_CMPL_COAL_CNT_COAL_64;
+		if ((bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP) && bp->ptp_cfg)
+			flags |= RING_ALLOC_REQ_FLAGS_TX_PKT_TS_CMPL_ENABLE;
+		req->flags = cpu_to_le16(flags);
 		break;
 	}
 	case HWRM_RING_ALLOC_RX:
@@ -9111,6 +9126,8 @@ static int __bnxt_hwrm_func_qcaps(struct bnxt *bp)
 		bp->fw_cap |= BNXT_FW_CAP_RX_ALL_PKT_TS;
 	if (flags_ext2 & FUNC_QCAPS_RESP_FLAGS_EXT2_UDP_GSO_SUPPORTED)
 		bp->flags |= BNXT_FLAG_UDP_GSO_CAP;
+	if (flags_ext2 & FUNC_QCAPS_RESP_FLAGS_EXT2_TX_PKT_TS_CMPL_SUPPORTED)
+		bp->fw_cap |= BNXT_FW_CAP_TX_TS_CMP;
 
 	bp->tx_push_thresh = 0;
 	if ((flags & FUNC_QCAPS_RESP_FLAGS_PUSH_MODE_SUPPORTED) &&
@@ -12152,7 +12169,7 @@ static int __bnxt_open_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init)
 	/* VF-reps may need to be re-opened after the PF is re-opened */
 	if (BNXT_PF(bp))
 		bnxt_vf_reps_open(bp);
-	if (bp->ptp_cfg)
+	if (bp->ptp_cfg && !(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
 		atomic_set(&bp->ptp_cfg->tx_avail, BNXT_MAX_TX_TS);
 	bnxt_ptp_init_rtc(bp, true);
 	bnxt_ptp_cfg_tstamp_filters(bp);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 00976e8a1e6a..12d6d17936a9 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -2410,6 +2410,7 @@ struct bnxt {
 	#define BNXT_FW_CAP_CFA_RFS_RING_TBL_IDX_V2	BIT_ULL(16)
 	#define BNXT_FW_CAP_PCIE_STATS_SUPPORTED	BIT_ULL(17)
 	#define BNXT_FW_CAP_EXT_STATS_SUPPORTED		BIT_ULL(18)
+	#define BNXT_FW_CAP_TX_TS_CMP			BIT_ULL(19)
 	#define BNXT_FW_CAP_ERR_RECOVER_RELOAD		BIT_ULL(20)
 	#define BNXT_FW_CAP_HOT_RESET			BIT_ULL(21)
 	#define BNXT_FW_CAP_PTP_RTC			BIT_ULL(22)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
index a14d46b9bfdf..0ca7bc75616b 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
@@ -768,6 +768,38 @@ int bnxt_get_rx_ts_p5(struct bnxt *bp, u64 *ts, u32 pkt_ts)
 	return 0;
 }
 
+void bnxt_tx_ts_cmp(struct bnxt *bp, struct bnxt_napi *bnapi,
+		    struct tx_ts_cmp *tscmp)
+{
+	struct skb_shared_hwtstamps timestamp = {};
+	struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
+	u32 opaque = tscmp->tx_ts_cmp_opaque;
+	struct bnxt_tx_ring_info *txr;
+	struct bnxt_sw_tx_bd *tx_buf;
+	u64 ts, ns;
+	u16 cons;
+
+	txr = bnapi->tx_ring[TX_OPAQUE_RING(opaque)];
+	ts = BNXT_GET_TX_TS_48B_NS(tscmp);
+	cons = TX_OPAQUE_IDX(opaque);
+	tx_buf = &txr->tx_buf_ring[RING_TX(bp, cons)];
+	if (tx_buf->is_ts_pkt) {
+		if (BNXT_TX_TS_ERR(tscmp)) {
+			netdev_err(bp->dev,
+				   "timestamp completion error 0x%x 0x%x\n",
+				   le32_to_cpu(tscmp->tx_ts_cmp_flags_type),
+				   le32_to_cpu(tscmp->tx_ts_cmp_errors_v));
+		} else {
+			spin_lock_bh(&ptp->ptp_lock);
+			ns = timecounter_cyc2time(&ptp->tc, ts);
+			spin_unlock_bh(&ptp->ptp_lock);
+			timestamp.hwtstamp = ns_to_ktime(ns);
+			skb_tstamp_tx(tx_buf->skb, &timestamp);
+		}
+		tx_buf->is_ts_pkt = 0;
+	}
+}
+
 static const struct ptp_clock_info bnxt_ptp_caps = {
 	.owner		= THIS_MODULE,
 	.name		= "bnxt clock",
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
index 8c30b428a428..d38c3500827f 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
@@ -156,6 +156,8 @@ 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_tx_ts_p5(struct bnxt *bp, struct sk_buff *skb);
 int bnxt_get_rx_ts_p5(struct bnxt *bp, u64 *ts, u32 pkt_ts);
+void bnxt_tx_ts_cmp(struct bnxt *bp, struct bnxt_napi *bnapi,
+		    struct tx_ts_cmp *tscmp);
 void bnxt_ptp_rtc_timecounter_init(struct bnxt_ptp_cfg *ptp, u64 ns);
 int bnxt_ptp_init_rtc(struct bnxt *bp, bool phc_cfg);
 int bnxt_ptp_init(struct bnxt *bp, bool phc_cfg);
-- 
2.30.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

[PATCH net-next 05/10] bnxt_en: Add BCM5760X specific PHC registers mapping

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 3785 bytes --]

From: Pavan Chebbi <pavan.chebbi@broadcom.com>

BCM5760X firmware will advertise direct 64-bit PHC registers access
for the driver from BAR0.

Make the necessary changes in handling HWRM_PORT_MAC_PTP_QCFG's
response and PHC register mapping for 5760X chips.

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     | 12 ++++++++----
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  8 ++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 10 +++++++++-
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index e16f50d822c8..93858df547b2 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -9012,7 +9012,7 @@ static int __bnxt_hwrm_ptp_qcfg(struct bnxt *bp)
 	u8 flags;
 	int rc;
 
-	if (bp->hwrm_spec_code < 0x10801 || !BNXT_CHIP_P5(bp)) {
+	if (bp->hwrm_spec_code < 0x10801 || !BNXT_CHIP_P5_PLUS(bp)) {
 		rc = -ENODEV;
 		goto no_ptp;
 	}
@@ -9028,7 +9028,8 @@ static int __bnxt_hwrm_ptp_qcfg(struct bnxt *bp)
 		goto exit;
 
 	flags = resp->flags;
-	if (!(flags & PORT_MAC_PTP_QCFG_RESP_FLAGS_HWRM_ACCESS)) {
+	if (BNXT_CHIP_P5_AND_MINUS(bp) &&
+	    !(flags & PORT_MAC_PTP_QCFG_RESP_FLAGS_HWRM_ACCESS)) {
 		rc = -ENODEV;
 		goto exit;
 	}
@@ -9041,10 +9042,13 @@ static int __bnxt_hwrm_ptp_qcfg(struct bnxt *bp)
 		ptp->bp = bp;
 		bp->ptp_cfg = ptp;
 	}
-	if (flags & PORT_MAC_PTP_QCFG_RESP_FLAGS_PARTIAL_DIRECT_ACCESS_REF_CLOCK) {
+
+	if (flags &
+	    (PORT_MAC_PTP_QCFG_RESP_FLAGS_PARTIAL_DIRECT_ACCESS_REF_CLOCK |
+	     PORT_MAC_PTP_QCFG_RESP_FLAGS_64B_PHC_TIME)) {
 		ptp->refclk_regs[0] = le32_to_cpu(resp->ts_ref_clock_reg_lower);
 		ptp->refclk_regs[1] = le32_to_cpu(resp->ts_ref_clock_reg_upper);
-	} else if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS) {
+	} else if (BNXT_CHIP_P5(bp)) {
 		ptp->refclk_regs[0] = BNXT_TS_REG_TIMESYNC_TS0_LOWER;
 		ptp->refclk_regs[1] = BNXT_TS_REG_TIMESYNC_TS0_UPPER;
 	} else {
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 12d6d17936a9..82b05641953f 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -2263,9 +2263,17 @@ struct bnxt {
 	 (BNXT_CHIP_NUM_58700((bp)->chip_num) &&	\
 	  !BNXT_CHIP_TYPE_NITRO_A0(bp)))
 
+/* Chip class phase 3.x */
+#define BNXT_CHIP_P3(bp)			\
+	(BNXT_CHIP_NUM_57X0X((bp)->chip_num) ||	\
+	 BNXT_CHIP_TYPE_NITRO_A0(bp))
+
 #define BNXT_CHIP_P4_PLUS(bp)			\
 	(BNXT_CHIP_P4(bp) || BNXT_CHIP_P5_PLUS(bp))
 
+#define BNXT_CHIP_P5_AND_MINUS(bp)		\
+	(BNXT_CHIP_P3(bp) || BNXT_CHIP_P4(bp) || BNXT_CHIP_P5(bp))
+
 	struct bnxt_aux_priv	*aux_priv;
 	struct bnxt_en_dev	*edev;
 
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
index 0ca7bc75616b..a3795ef8887d 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
@@ -656,6 +656,14 @@ static int bnxt_map_ptp_regs(struct bnxt *bp)
 				(ptp->refclk_regs[i] & BNXT_GRC_OFFSET_MASK);
 		return 0;
 	}
+	if (bp->flags & BNXT_FLAG_CHIP_P7) {
+		for (i = 0; i < 2; i++) {
+			if (reg_arr[i] & BNXT_GRC_BASE_MASK)
+				return -EINVAL;
+			ptp->refclk_mapped_regs[i] = reg_arr[i];
+		}
+		return 0;
+	}
 	return -ENODEV;
 }
 
@@ -1018,7 +1026,7 @@ int bnxt_ptp_init(struct bnxt *bp, bool phc_cfg)
 	ptp->stats.ts_lost = 0;
 	atomic64_set(&ptp->stats.ts_err, 0);
 
-	if (BNXT_CHIP_P5(bp)) {
+	if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS) {
 		spin_lock_bh(&ptp->ptp_lock);
 		bnxt_refclk_read(bp, NULL, &ptp->current_time);
 		WRITE_ONCE(ptp->old_time, ptp->current_time);
-- 
2.30.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

[PATCH net-next 06/10] bnxt_en: Refactor all PTP TX timestamp fields into a struct

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 7398 bytes --]

From: Pavan Chebbi <pavan.chebbi@broadcom.com>

On the older 5750X (P5) chips, we currently support only 1 TX PTP
packet in-flight waiting for the timestamp.  Refactor the
datastructures to prepare to support up to 4 TX PTP packets.

Combine all fields required for PTP TX timestamp query into one
structure.  An array of this structure will be added in follow-on
patches to support multiple outstanding TX timestamps.

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     | 10 +++--
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 43 ++++++++++---------
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 15 ++++---
 3 files changed, 40 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 93858df547b2..c284aa370c64 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -516,14 +516,18 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
 			lflags |= cpu_to_le32(TX_BD_FLAGS_STAMP);
 			tx_buf->is_ts_pkt = 1;
 		} else if (!skb_is_gso(skb)) {
+			u16 seq_id, hdr_off;
+
 			if (atomic_dec_if_positive(&ptp->tx_avail) < 0) {
 				atomic64_inc(&ptp->stats.ts_err);
 				goto tx_no_ts;
 			}
-			if (!bnxt_ptp_parse(skb, &ptp->tx_seqid,
-					    &ptp->tx_hdr_off)) {
+
+			if (!bnxt_ptp_parse(skb, &seq_id, &hdr_off)) {
 				if (vlan_tag_flags)
-					ptp->tx_hdr_off += VLAN_HLEN;
+					hdr_off += VLAN_HLEN;
+				ptp->txts_req.tx_seqid = seq_id;
+				ptp->txts_req.tx_hdr_off = hdr_off;
 				lflags |= cpu_to_le32(TX_BD_FLAGS_STAMP);
 				tx_buf->is_ts_pkt = 1;
 				skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
index a3795ef8887d..8431cd0ed9e9 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
@@ -123,11 +123,12 @@ static int bnxt_hwrm_port_ts_query(struct bnxt *bp, u32 flags, u64 *ts,
 	req->flags = cpu_to_le32(flags);
 	if ((flags & PORT_TS_QUERY_REQ_FLAGS_PATH) ==
 	    PORT_TS_QUERY_REQ_FLAGS_PATH_TX) {
+		struct bnxt_ptp_tx_req *txts_req = &bp->ptp_cfg->txts_req;
 		u32 tmo_us = txts_tmo * 1000;
 
 		req->enables = cpu_to_le16(BNXT_PTP_QTS_TX_ENABLES);
-		req->ptp_seq_id = cpu_to_le32(bp->ptp_cfg->tx_seqid);
-		req->ptp_hdr_offset = cpu_to_le16(bp->ptp_cfg->tx_hdr_off);
+		req->ptp_seq_id = cpu_to_le32(txts_req->tx_seqid);
+		req->ptp_hdr_offset = cpu_to_le16(txts_req->tx_hdr_off);
 		if (!tmo_us)
 			tmo_us = BNXT_PTP_QTS_TIMEOUT;
 		tmo_us = min(tmo_us, BNXT_PTP_QTS_MAX_TMO_US);
@@ -686,15 +687,17 @@ static void bnxt_stamp_tx_skb(struct bnxt *bp, struct sk_buff *skb)
 {
 	struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
 	struct skb_shared_hwtstamps timestamp;
+	struct bnxt_ptp_tx_req *txts_req;
 	unsigned long now = jiffies;
 	u64 ts = 0, ns = 0;
 	u32 tmo = 0;
 	int rc;
 
-	if (!ptp->txts_pending)
-		ptp->abs_txts_tmo = now + msecs_to_jiffies(ptp->txts_tmo);
-	if (!time_after_eq(now, ptp->abs_txts_tmo))
-		tmo = jiffies_to_msecs(ptp->abs_txts_tmo - now);
+	txts_req = &ptp->txts_req;
+	if (!txts_req->txts_pending)
+		txts_req->abs_txts_tmo = now + msecs_to_jiffies(ptp->txts_tmo);
+	if (!time_after_eq(now, txts_req->abs_txts_tmo))
+		tmo = jiffies_to_msecs(txts_req->abs_txts_tmo - now);
 	rc = bnxt_hwrm_port_ts_query(bp, PORT_TS_QUERY_REQ_FLAGS_PATH_TX, &ts,
 				     tmo);
 	if (!rc) {
@@ -703,11 +706,11 @@ static void bnxt_stamp_tx_skb(struct bnxt *bp, struct sk_buff *skb)
 		ns = timecounter_cyc2time(&ptp->tc, ts);
 		spin_unlock_bh(&ptp->ptp_lock);
 		timestamp.hwtstamp = ns_to_ktime(ns);
-		skb_tstamp_tx(ptp->tx_skb, &timestamp);
+		skb_tstamp_tx(txts_req->tx_skb, &timestamp);
 		ptp->stats.ts_pkts++;
 	} else {
-		if (!time_after_eq(jiffies, ptp->abs_txts_tmo)) {
-			ptp->txts_pending = true;
+		if (!time_after_eq(jiffies, txts_req->abs_txts_tmo)) {
+			txts_req->txts_pending = true;
 			return;
 		}
 		ptp->stats.ts_lost++;
@@ -715,10 +718,10 @@ static void bnxt_stamp_tx_skb(struct bnxt *bp, struct sk_buff *skb)
 				 "TS query for TX timer failed rc = %x\n", rc);
 	}
 
-	dev_kfree_skb_any(ptp->tx_skb);
-	ptp->tx_skb = NULL;
+	dev_kfree_skb_any(txts_req->tx_skb);
+	txts_req->tx_skb = NULL;
 	atomic_inc(&ptp->tx_avail);
-	ptp->txts_pending = false;
+	txts_req->txts_pending = false;
 }
 
 static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
@@ -728,8 +731,8 @@ static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
 	unsigned long now = jiffies;
 	struct bnxt *bp = ptp->bp;
 
-	if (ptp->tx_skb)
-		bnxt_stamp_tx_skb(bp, ptp->tx_skb);
+	if (ptp->txts_req.tx_skb)
+		bnxt_stamp_tx_skb(bp, ptp->txts_req.tx_skb);
 
 	if (!time_after_eq(now, ptp->next_period))
 		return ptp->next_period - now;
@@ -742,7 +745,7 @@ static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
 		spin_unlock_bh(&ptp->ptp_lock);
 		ptp->next_overflow_check = now + BNXT_PHC_OVERFLOW_PERIOD;
 	}
-	if (ptp->txts_pending)
+	if (ptp->txts_req.txts_pending)
 		return 0;
 	return HZ;
 }
@@ -751,11 +754,11 @@ int bnxt_get_tx_ts_p5(struct bnxt *bp, struct sk_buff *skb)
 {
 	struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
 
-	if (ptp->tx_skb) {
+	if (ptp->txts_req.tx_skb) {
 		netdev_err(bp->dev, "deferring skb:one SKB is still outstanding\n");
 		return -EBUSY;
 	}
-	ptp->tx_skb = skb;
+	ptp->txts_req.tx_skb = skb;
 	ptp_schedule_worker(ptp->ptp_clock, 0);
 	return 0;
 }
@@ -1056,9 +1059,9 @@ void bnxt_ptp_clear(struct bnxt *bp)
 	kfree(ptp->ptp_info.pin_config);
 	ptp->ptp_info.pin_config = NULL;
 
-	if (ptp->tx_skb) {
-		dev_kfree_skb_any(ptp->tx_skb);
-		ptp->tx_skb = NULL;
+	if (ptp->txts_req.tx_skb) {
+		dev_kfree_skb_any(ptp->txts_req.tx_skb);
+		ptp->txts_req.tx_skb = NULL;
 	}
 
 	bnxt_unmap_ptp_regs(bp);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
index d38c3500827f..90f1418211e9 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
@@ -85,6 +85,14 @@ struct bnxt_ptp_stats {
 	atomic64_t	ts_err;
 };
 
+struct bnxt_ptp_tx_req {
+	struct sk_buff		*tx_skb;
+	u16			tx_seqid;
+	u16			tx_hdr_off;
+	u8			txts_pending:1;
+	unsigned long		abs_txts_tmo;
+};
+
 struct bnxt_ptp_cfg {
 	struct ptp_clock_info	ptp_info;
 	struct ptp_clock	*ptp_clock;
@@ -93,7 +101,6 @@ struct bnxt_ptp_cfg {
 	struct bnxt_pps		pps_info;
 	/* serialize timecounter access */
 	spinlock_t		ptp_lock;
-	struct sk_buff		*tx_skb;
 	u64			current_time;
 	u64			old_time;
 	unsigned long		next_period;
@@ -102,8 +109,8 @@ struct bnxt_ptp_cfg {
 	/* a 23b shift cyclecounter will overflow in ~36 mins.  Check overflow every 18 mins. */
 	#define BNXT_PHC_OVERFLOW_PERIOD	(18 * 60 * HZ)
 
-	u16			tx_seqid;
-	u16			tx_hdr_off;
+	struct bnxt_ptp_tx_req	txts_req;
+
 	struct bnxt		*bp;
 	atomic_t		tx_avail;
 #define BNXT_MAX_TX_TS	1
@@ -123,14 +130,12 @@ struct bnxt_ptp_cfg {
 					 BNXT_PTP_MSG_PDELAY_REQ |	\
 					 BNXT_PTP_MSG_PDELAY_RESP)
 	u8			tx_tstamp_en:1;
-	u8			txts_pending:1;
 	int			rx_filter;
 	u32			tstamp_filters;
 
 	u32			refclk_regs[2];
 	u32			refclk_mapped_regs[2];
 	u32			txts_tmo;
-	unsigned long		abs_txts_tmo;
 
 	struct bnxt_ptp_stats	stats;
 };
-- 
2.30.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

[PATCH net-next 07/10] bnxt_en: Remove an impossible condition check for PTP TX pending SKB

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 2946 bytes --]

From: Pavan Chebbi <pavan.chebbi@broadcom.com>

In the current 5750X PTP code paths, there is always at most one TX
SKB requested for timestamp and we won't accept another one until we
have retrieved the timestamp or it has timed out.  Remove the
unnecessary check in bnxt_get_tx_ts_p5() for a pending SKB and change
the function to void.

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     | 8 ++------
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 7 +------
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 2 +-
 3 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index c284aa370c64..ed2bbdf6b25f 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -838,12 +838,8 @@ static bool __bnxt_tx_int(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
 		if (unlikely(is_ts_pkt)) {
 			if (BNXT_CHIP_P5(bp)) {
 				/* PTP worker takes ownership of the skb */
-				if (!bnxt_get_tx_ts_p5(bp, skb)) {
-					skb = NULL;
-				} else {
-					atomic64_inc(&bp->ptp_cfg->stats.ts_err);
-					atomic_inc(&bp->ptp_cfg->tx_avail);
-				}
+				bnxt_get_tx_ts_p5(bp, skb);
+				skb = NULL;
 			}
 		}
 
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
index 8431cd0ed9e9..baf191959b13 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
@@ -750,17 +750,12 @@ static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
 	return HZ;
 }
 
-int bnxt_get_tx_ts_p5(struct bnxt *bp, struct sk_buff *skb)
+void bnxt_get_tx_ts_p5(struct bnxt *bp, struct sk_buff *skb)
 {
 	struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
 
-	if (ptp->txts_req.tx_skb) {
-		netdev_err(bp->dev, "deferring skb:one SKB is still outstanding\n");
-		return -EBUSY;
-	}
 	ptp->txts_req.tx_skb = skb;
 	ptp_schedule_worker(ptp->ptp_clock, 0);
-	return 0;
 }
 
 int bnxt_get_rx_ts_p5(struct bnxt *bp, u64 *ts, u32 pkt_ts)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
index 90f1418211e9..ee1709cda47e 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
@@ -159,7 +159,7 @@ int bnxt_ptp_cfg_tstamp_filters(struct bnxt *bp);
 void bnxt_ptp_reapply_pps(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_tx_ts_p5(struct bnxt *bp, struct sk_buff *skb);
+void bnxt_get_tx_ts_p5(struct bnxt *bp, struct sk_buff *skb);
 int bnxt_get_rx_ts_p5(struct bnxt *bp, u64 *ts, u32 pkt_ts);
 void bnxt_tx_ts_cmp(struct bnxt *bp, struct bnxt_napi *bnapi,
 		    struct tx_ts_cmp *tscmp);
-- 
2.30.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

[PATCH net-next 08/10] bnxt_en: Let bnxt_stamp_tx_skb() return error code

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 2420 bytes --]

From: Pavan Chebbi <pavan.chebbi@broadcom.com>

Change the function bnxt_stamp_tx_skb() to return 0 for suceess
or -EAGAIN if the timestamp is still pending in firmware.  The
calling PTP aux worker will reschedule based on the return code.

Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
index baf191959b13..bd1e270307ec 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
@@ -683,7 +683,7 @@ static u64 bnxt_cc_read(const struct cyclecounter *cc)
 	return ns;
 }
 
-static void bnxt_stamp_tx_skb(struct bnxt *bp, struct sk_buff *skb)
+static int bnxt_stamp_tx_skb(struct bnxt *bp, struct sk_buff *skb)
 {
 	struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
 	struct skb_shared_hwtstamps timestamp;
@@ -711,7 +711,7 @@ static void bnxt_stamp_tx_skb(struct bnxt *bp, struct sk_buff *skb)
 	} else {
 		if (!time_after_eq(jiffies, txts_req->abs_txts_tmo)) {
 			txts_req->txts_pending = true;
-			return;
+			return -EAGAIN;
 		}
 		ptp->stats.ts_lost++;
 		netdev_warn_once(bp->dev,
@@ -722,6 +722,8 @@ static void bnxt_stamp_tx_skb(struct bnxt *bp, struct sk_buff *skb)
 	txts_req->tx_skb = NULL;
 	atomic_inc(&ptp->tx_avail);
 	txts_req->txts_pending = false;
+
+	return 0;
 }
 
 static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
@@ -730,12 +732,16 @@ static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
 						ptp_info);
 	unsigned long now = jiffies;
 	struct bnxt *bp = ptp->bp;
+	int rc = 0;
 
 	if (ptp->txts_req.tx_skb)
-		bnxt_stamp_tx_skb(bp, ptp->txts_req.tx_skb);
+		rc = bnxt_stamp_tx_skb(bp, ptp->txts_req.tx_skb);
 
-	if (!time_after_eq(now, ptp->next_period))
+	if (!time_after_eq(now, ptp->next_period)) {
+		if (rc == -EAGAIN)
+			return 0;
 		return ptp->next_period - now;
+	}
 
 	bnxt_ptp_get_current_time(bp);
 	ptp->next_period = now + HZ;
@@ -745,7 +751,7 @@ static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
 		spin_unlock_bh(&ptp->ptp_lock);
 		ptp->next_overflow_check = now + BNXT_PHC_OVERFLOW_PERIOD;
 	}
-	if (ptp->txts_req.txts_pending)
+	if (rc == -EAGAIN)
 		return 0;
 	return HZ;
 }
-- 
2.30.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

[PATCH net-next 09/10] bnxt_en: Increase the max total outstanding PTP TX packets to 4

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 10585 bytes --]

From: Pavan Chebbi <pavan.chebbi@broadcom.com>

Start accepting up to 4 TX TS requests on BCM5750X (P5) chips.
These PTP TX packets will be queued in the ptp->txts_req[] array
waiting for the TX timestamp to complete.  The entries in the
array will be managed by a producer and consumer index.  The
producer index is updated under spinlock since multiple TX rings
can try to send PTP packets at the same time.

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     | 20 +++++--
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  5 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 60 ++++++++++++-------
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 13 ++--
 4 files changed, 68 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index ed2bbdf6b25f..0867861c14bd 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -457,8 +457,8 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	unsigned int length, pad = 0;
 	u32 len, free_size, vlan_tag_flags, cfa_action, flags;
 	struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
-	u16 prod, last_frag;
 	struct pci_dev *pdev = bp->pdev;
+	u16 prod, last_frag, txts_prod;
 	struct bnxt_tx_ring_info *txr;
 	struct bnxt_sw_tx_bd *tx_buf;
 	__le32 lflags = 0;
@@ -526,11 +526,19 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
 			if (!bnxt_ptp_parse(skb, &seq_id, &hdr_off)) {
 				if (vlan_tag_flags)
 					hdr_off += VLAN_HLEN;
-				ptp->txts_req.tx_seqid = seq_id;
-				ptp->txts_req.tx_hdr_off = hdr_off;
 				lflags |= cpu_to_le32(TX_BD_FLAGS_STAMP);
 				tx_buf->is_ts_pkt = 1;
 				skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+
+				spin_lock_bh(&ptp->ptp_tx_lock);
+				txts_prod = ptp->txts_prod;
+				ptp->txts_prod = NEXT_TXTS(txts_prod);
+				spin_unlock_bh(&ptp->ptp_tx_lock);
+
+				ptp->txts_req[txts_prod].tx_seqid = seq_id;
+				ptp->txts_req[txts_prod].tx_hdr_off = hdr_off;
+				tx_buf->txts_prod = txts_prod;
+
 			} else {
 				atomic_inc(&bp->ptp_cfg->tx_avail);
 			}
@@ -770,7 +778,9 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
 tx_kick_pending:
 	if (BNXT_TX_PTP_IS_SET(lflags)) {
 		atomic64_inc(&bp->ptp_cfg->stats.ts_err);
-		atomic_inc(&bp->ptp_cfg->tx_avail);
+		if (!(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
+			/* set SKB to err so PTP worker will clean up */
+			ptp->txts_req[txts_prod].tx_skb = ERR_PTR(-EIO);
 	}
 	if (txr->kick_pending)
 		bnxt_txr_db_kick(bp, txr, txr->tx_prod);
@@ -838,7 +848,7 @@ static bool __bnxt_tx_int(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
 		if (unlikely(is_ts_pkt)) {
 			if (BNXT_CHIP_P5(bp)) {
 				/* PTP worker takes ownership of the skb */
-				bnxt_get_tx_ts_p5(bp, skb);
+				bnxt_get_tx_ts_p5(bp, skb, tx_buf->txts_prod);
 				skb = NULL;
 			}
 		}
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 82b05641953f..e46bd11e52b0 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -878,7 +878,10 @@ struct bnxt_sw_tx_bd {
 	u8			is_push;
 	u8			action;
 	unsigned short		nr_frags;
-	u16			rx_prod;
+	union {
+		u16			rx_prod;
+		u16			txts_prod;
+	};
 };
 
 struct bnxt_sw_rx_bd {
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
index bd1e270307ec..9e93dc8b2b57 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
@@ -110,7 +110,7 @@ static void bnxt_ptp_get_current_time(struct bnxt *bp)
 }
 
 static int bnxt_hwrm_port_ts_query(struct bnxt *bp, u32 flags, u64 *ts,
-				   u32 txts_tmo)
+				   u32 txts_tmo, int slot)
 {
 	struct hwrm_port_ts_query_output *resp;
 	struct hwrm_port_ts_query_input *req;
@@ -123,7 +123,7 @@ static int bnxt_hwrm_port_ts_query(struct bnxt *bp, u32 flags, u64 *ts,
 	req->flags = cpu_to_le32(flags);
 	if ((flags & PORT_TS_QUERY_REQ_FLAGS_PATH) ==
 	    PORT_TS_QUERY_REQ_FLAGS_PATH_TX) {
-		struct bnxt_ptp_tx_req *txts_req = &bp->ptp_cfg->txts_req;
+		struct bnxt_ptp_tx_req *txts_req = &bp->ptp_cfg->txts_req[slot];
 		u32 tmo_us = txts_tmo * 1000;
 
 		req->enables = cpu_to_le16(BNXT_PTP_QTS_TX_ENABLES);
@@ -683,7 +683,7 @@ static u64 bnxt_cc_read(const struct cyclecounter *cc)
 	return ns;
 }
 
-static int bnxt_stamp_tx_skb(struct bnxt *bp, struct sk_buff *skb)
+static int bnxt_stamp_tx_skb(struct bnxt *bp, int slot)
 {
 	struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
 	struct skb_shared_hwtstamps timestamp;
@@ -693,13 +693,13 @@ static int bnxt_stamp_tx_skb(struct bnxt *bp, struct sk_buff *skb)
 	u32 tmo = 0;
 	int rc;
 
-	txts_req = &ptp->txts_req;
-	if (!txts_req->txts_pending)
-		txts_req->abs_txts_tmo = now + msecs_to_jiffies(ptp->txts_tmo);
+	txts_req = &ptp->txts_req[slot];
+	/* make sure bnxt_get_tx_ts_p5() has updated abs_txts_tmo */
+	smp_rmb();
 	if (!time_after_eq(now, txts_req->abs_txts_tmo))
 		tmo = jiffies_to_msecs(txts_req->abs_txts_tmo - now);
 	rc = bnxt_hwrm_port_ts_query(bp, PORT_TS_QUERY_REQ_FLAGS_PATH_TX, &ts,
-				     tmo);
+				     tmo, slot);
 	if (!rc) {
 		memset(&timestamp, 0, sizeof(timestamp));
 		spin_lock_bh(&ptp->ptp_lock);
@@ -709,10 +709,9 @@ static int bnxt_stamp_tx_skb(struct bnxt *bp, struct sk_buff *skb)
 		skb_tstamp_tx(txts_req->tx_skb, &timestamp);
 		ptp->stats.ts_pkts++;
 	} else {
-		if (!time_after_eq(jiffies, txts_req->abs_txts_tmo)) {
-			txts_req->txts_pending = true;
+		if (!time_after_eq(jiffies, txts_req->abs_txts_tmo))
 			return -EAGAIN;
-		}
+
 		ptp->stats.ts_lost++;
 		netdev_warn_once(bp->dev,
 				 "TS query for TX timer failed rc = %x\n", rc);
@@ -720,8 +719,6 @@ static int bnxt_stamp_tx_skb(struct bnxt *bp, struct sk_buff *skb)
 
 	dev_kfree_skb_any(txts_req->tx_skb);
 	txts_req->tx_skb = NULL;
-	atomic_inc(&ptp->tx_avail);
-	txts_req->txts_pending = false;
 
 	return 0;
 }
@@ -732,10 +729,24 @@ static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
 						ptp_info);
 	unsigned long now = jiffies;
 	struct bnxt *bp = ptp->bp;
+	u16 cons = ptp->txts_cons;
+	u8 num_requests;
 	int rc = 0;
 
-	if (ptp->txts_req.tx_skb)
-		rc = bnxt_stamp_tx_skb(bp, ptp->txts_req.tx_skb);
+	num_requests = BNXT_MAX_TX_TS - atomic_read(&ptp->tx_avail);
+	while (num_requests--) {
+		if (IS_ERR(ptp->txts_req[cons].tx_skb))
+			goto next_slot;
+		if (!ptp->txts_req[cons].tx_skb)
+			break;
+		rc = bnxt_stamp_tx_skb(bp, cons);
+		if (rc == -EAGAIN)
+			break;
+next_slot:
+		atomic_inc(&ptp->tx_avail);
+		cons = NEXT_TXTS(cons);
+	}
+	ptp->txts_cons = cons;
 
 	if (!time_after_eq(now, ptp->next_period)) {
 		if (rc == -EAGAIN)
@@ -756,11 +767,16 @@ static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
 	return HZ;
 }
 
-void bnxt_get_tx_ts_p5(struct bnxt *bp, struct sk_buff *skb)
+void bnxt_get_tx_ts_p5(struct bnxt *bp, struct sk_buff *skb, u16 prod)
 {
 	struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
+	struct bnxt_ptp_tx_req *txts_req;
 
-	ptp->txts_req.tx_skb = skb;
+	txts_req = &ptp->txts_req[prod];
+	txts_req->abs_txts_tmo = jiffies + msecs_to_jiffies(ptp->txts_tmo);
+	/* make sure abs_txts_tmo is written first */
+	smp_wmb();
+	txts_req->tx_skb = skb;
 	ptp_schedule_worker(ptp->ptp_clock, 0);
 }
 
@@ -958,7 +974,7 @@ int bnxt_ptp_init_rtc(struct bnxt *bp, bool phc_cfg)
 			return rc;
 	} else {
 		rc = bnxt_hwrm_port_ts_query(bp, PORT_TS_QUERY_REQ_FLAGS_CURRENT_TIME,
-					     &ns, 0);
+					     &ns, 0, 0);
 		if (rc)
 			return rc;
 	}
@@ -1000,6 +1016,7 @@ int bnxt_ptp_init(struct bnxt *bp, bool phc_cfg)
 
 	atomic_set(&ptp->tx_avail, BNXT_MAX_TX_TS);
 	spin_lock_init(&ptp->ptp_lock);
+	spin_lock_init(&ptp->ptp_tx_lock);
 
 	if (BNXT_PTP_USE_RTC(bp)) {
 		bnxt_ptp_timecounter_init(bp, false);
@@ -1049,6 +1066,7 @@ int bnxt_ptp_init(struct bnxt *bp, bool phc_cfg)
 void bnxt_ptp_clear(struct bnxt *bp)
 {
 	struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
+	int i;
 
 	if (!ptp)
 		return;
@@ -1060,9 +1078,11 @@ void bnxt_ptp_clear(struct bnxt *bp)
 	kfree(ptp->ptp_info.pin_config);
 	ptp->ptp_info.pin_config = NULL;
 
-	if (ptp->txts_req.tx_skb) {
-		dev_kfree_skb_any(ptp->txts_req.tx_skb);
-		ptp->txts_req.tx_skb = NULL;
+	for (i = 0; i < BNXT_MAX_TX_TS; i++) {
+		if (ptp->txts_req[i].tx_skb) {
+			dev_kfree_skb_any(ptp->txts_req[i].tx_skb);
+			ptp->txts_req[i].tx_skb = NULL;
+		}
 	}
 
 	bnxt_unmap_ptp_regs(bp);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
index ee1709cda47e..a1910ce86cbb 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
@@ -85,11 +85,13 @@ struct bnxt_ptp_stats {
 	atomic64_t	ts_err;
 };
 
+#define BNXT_MAX_TX_TS		4
+#define NEXT_TXTS(idx)		(((idx) + 1) & (BNXT_MAX_TX_TS - 1))
+
 struct bnxt_ptp_tx_req {
 	struct sk_buff		*tx_skb;
 	u16			tx_seqid;
 	u16			tx_hdr_off;
-	u8			txts_pending:1;
 	unsigned long		abs_txts_tmo;
 };
 
@@ -101,6 +103,8 @@ struct bnxt_ptp_cfg {
 	struct bnxt_pps		pps_info;
 	/* serialize timecounter access */
 	spinlock_t		ptp_lock;
+	/* serialize ts tx request queuing */
+	spinlock_t		ptp_tx_lock;
 	u64			current_time;
 	u64			old_time;
 	unsigned long		next_period;
@@ -109,11 +113,10 @@ struct bnxt_ptp_cfg {
 	/* a 23b shift cyclecounter will overflow in ~36 mins.  Check overflow every 18 mins. */
 	#define BNXT_PHC_OVERFLOW_PERIOD	(18 * 60 * HZ)
 
-	struct bnxt_ptp_tx_req	txts_req;
+	struct bnxt_ptp_tx_req	txts_req[BNXT_MAX_TX_TS];
 
 	struct bnxt		*bp;
 	atomic_t		tx_avail;
-#define BNXT_MAX_TX_TS	1
 	u16			rxctl;
 #define BNXT_PTP_MSG_SYNC			(1 << 0)
 #define BNXT_PTP_MSG_DELAY_REQ			(1 << 1)
@@ -136,6 +139,8 @@ struct bnxt_ptp_cfg {
 	u32			refclk_regs[2];
 	u32			refclk_mapped_regs[2];
 	u32			txts_tmo;
+	u16			txts_prod;
+	u16			txts_cons;
 
 	struct bnxt_ptp_stats	stats;
 };
@@ -159,7 +164,7 @@ int bnxt_ptp_cfg_tstamp_filters(struct bnxt *bp);
 void bnxt_ptp_reapply_pps(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);
-void bnxt_get_tx_ts_p5(struct bnxt *bp, struct sk_buff *skb);
+void bnxt_get_tx_ts_p5(struct bnxt *bp, struct sk_buff *skb, u16 prod);
 int bnxt_get_rx_ts_p5(struct bnxt *bp, u64 *ts, u32 pkt_ts);
 void bnxt_tx_ts_cmp(struct bnxt *bp, struct bnxt_napi *bnapi,
 		    struct tx_ts_cmp *tscmp);
-- 
2.30.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next 09/10] bnxt_en: Increase the max total outstanding PTP TX packets to 4

[Simon Horman]

On Wed, Jun 26, 2024 at 09:43:06AM -0700, Michael Chan wrote:
> From: Pavan Chebbi <pavan.chebbi@broadcom.com>
> 
> Start accepting up to 4 TX TS requests on BCM5750X (P5) chips.
> These PTP TX packets will be queued in the ptp->txts_req[] array
> waiting for the TX timestamp to complete.  The entries in the
> array will be managed by a producer and consumer index.  The
> producer index is updated under spinlock since multiple TX rings
> can try to send PTP packets at the same time.
> 
> Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>

...

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index ed2bbdf6b25f..0867861c14bd 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -457,8 +457,8 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	unsigned int length, pad = 0;
>  	u32 len, free_size, vlan_tag_flags, cfa_action, flags;
>  	struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
> -	u16 prod, last_frag;
>  	struct pci_dev *pdev = bp->pdev;
> +	u16 prod, last_frag, txts_prod;
>  	struct bnxt_tx_ring_info *txr;
>  	struct bnxt_sw_tx_bd *tx_buf;
>  	__le32 lflags = 0;
> @@ -526,11 +526,19 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  			if (!bnxt_ptp_parse(skb, &seq_id, &hdr_off)) {
>  				if (vlan_tag_flags)
>  					hdr_off += VLAN_HLEN;
> -				ptp->txts_req.tx_seqid = seq_id;
> -				ptp->txts_req.tx_hdr_off = hdr_off;
>  				lflags |= cpu_to_le32(TX_BD_FLAGS_STAMP);
>  				tx_buf->is_ts_pkt = 1;
>  				skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> +
> +				spin_lock_bh(&ptp->ptp_tx_lock);
> +				txts_prod = ptp->txts_prod;
> +				ptp->txts_prod = NEXT_TXTS(txts_prod);
> +				spin_unlock_bh(&ptp->ptp_tx_lock);
> +
> +				ptp->txts_req[txts_prod].tx_seqid = seq_id;
> +				ptp->txts_req[txts_prod].tx_hdr_off = hdr_off;
> +				tx_buf->txts_prod = txts_prod;
> +
>  			} else {
>  				atomic_inc(&bp->ptp_cfg->tx_avail);
>  			}
> @@ -770,7 +778,9 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  tx_kick_pending:
>  	if (BNXT_TX_PTP_IS_SET(lflags)) {
>  		atomic64_inc(&bp->ptp_cfg->stats.ts_err);
> -		atomic_inc(&bp->ptp_cfg->tx_avail);
> +		if (!(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
> +			/* set SKB to err so PTP worker will clean up */
> +			ptp->txts_req[txts_prod].tx_skb = ERR_PTR(-EIO);

Hi Michael

Sparse complains that previously it was assumed that ptp could be NULL,
but here it is accessed without checking for that.

Perhaps it can't occur, but my brief check leads me to think it might.

On line 488 there is the following:

	if (unlikely(ipv6_hopopt_jumbo_remove(skb)))
		goto tx_free;

Which will lead to the code in the hunk above.

Then on line 513 there is a check for ptp being NULL:


	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) && ptp &&
	    ptp->tx_tstamp_en) {

And ptp is not set between lines 488 and 513.


Sparse also complains that txts_prod may be used uninitaialised.
This also seems to be a valid concern as it does seem to be the case
on line 488.

>  	}
>  	if (txr->kick_pending)
>  		bnxt_txr_db_kick(bp, txr, txr->tx_prod);

...

Re: [PATCH net-next 09/10] bnxt_en: Increase the max total outstanding PTP TX packets to 4

[Simon Horman]

On Fri, Jun 28, 2024 at 06:03:18PM +0100, Simon Horman wrote:
> On Wed, Jun 26, 2024 at 09:43:06AM -0700, Michael Chan wrote:
> > From: Pavan Chebbi <pavan.chebbi@broadcom.com>
> > 
> > Start accepting up to 4 TX TS requests on BCM5750X (P5) chips.
> > These PTP TX packets will be queued in the ptp->txts_req[] array
> > waiting for the TX timestamp to complete.  The entries in the
> > array will be managed by a producer and consumer index.  The
> > producer index is updated under spinlock since multiple TX rings
> > can try to send PTP packets at the same time.
> > 
> > Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> > Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> 
> ...
> 
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > index ed2bbdf6b25f..0867861c14bd 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > @@ -457,8 +457,8 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
> >  	unsigned int length, pad = 0;
> >  	u32 len, free_size, vlan_tag_flags, cfa_action, flags;
> >  	struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
> > -	u16 prod, last_frag;
> >  	struct pci_dev *pdev = bp->pdev;
> > +	u16 prod, last_frag, txts_prod;
> >  	struct bnxt_tx_ring_info *txr;
> >  	struct bnxt_sw_tx_bd *tx_buf;
> >  	__le32 lflags = 0;
> > @@ -526,11 +526,19 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
> >  			if (!bnxt_ptp_parse(skb, &seq_id, &hdr_off)) {
> >  				if (vlan_tag_flags)
> >  					hdr_off += VLAN_HLEN;
> > -				ptp->txts_req.tx_seqid = seq_id;
> > -				ptp->txts_req.tx_hdr_off = hdr_off;
> >  				lflags |= cpu_to_le32(TX_BD_FLAGS_STAMP);
> >  				tx_buf->is_ts_pkt = 1;
> >  				skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> > +
> > +				spin_lock_bh(&ptp->ptp_tx_lock);
> > +				txts_prod = ptp->txts_prod;
> > +				ptp->txts_prod = NEXT_TXTS(txts_prod);
> > +				spin_unlock_bh(&ptp->ptp_tx_lock);
> > +
> > +				ptp->txts_req[txts_prod].tx_seqid = seq_id;
> > +				ptp->txts_req[txts_prod].tx_hdr_off = hdr_off;
> > +				tx_buf->txts_prod = txts_prod;
> > +
> >  			} else {
> >  				atomic_inc(&bp->ptp_cfg->tx_avail);
> >  			}
> > @@ -770,7 +778,9 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
> >  tx_kick_pending:
> >  	if (BNXT_TX_PTP_IS_SET(lflags)) {
> >  		atomic64_inc(&bp->ptp_cfg->stats.ts_err);
> > -		atomic_inc(&bp->ptp_cfg->tx_avail);
> > +		if (!(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
> > +			/* set SKB to err so PTP worker will clean up */
> > +			ptp->txts_req[txts_prod].tx_skb = ERR_PTR(-EIO);
> 
> Hi Michael
> 
> Sparse complains that previously it was assumed that ptp could be NULL,
> but here it is accessed without checking for that.
> 
> Perhaps it can't occur, but my brief check leads me to think it might.
> 
> On line 488 there is the following:
> 
> 	if (unlikely(ipv6_hopopt_jumbo_remove(skb)))
> 		goto tx_free;
> 
> Which will lead to the code in the hunk above.
> 
> Then on line 513 there is a check for ptp being NULL:
> 
> 
> 	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) && ptp &&
> 	    ptp->tx_tstamp_en) {
> 
> And ptp is not set between lines 488 and 513.
> 
> 
> Sparse also complains that txts_prod may be used uninitaialised.
> This also seems to be a valid concern as it does seem to be the case
> on line 488.
> 
> >  	}
> >  	if (txr->kick_pending)
> >  		bnxt_txr_db_kick(bp, txr, txr->tx_prod);
> 
> ...

Sorry, in my previous email I mentioned Sparse.
But I should have said Smatch.

Re: [PATCH net-next 09/10] bnxt_en: Increase the max total outstanding PTP TX packets to 4

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 4052 bytes --]

On Fri, Jun 28, 2024 at 10:03 AM Simon Horman <horms@kernel.org> wrote:
>
> On Wed, Jun 26, 2024 at 09:43:06AM -0700, Michael Chan wrote:
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > index ed2bbdf6b25f..0867861c14bd 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > @@ -457,8 +457,8 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
> >       unsigned int length, pad = 0;
> >       u32 len, free_size, vlan_tag_flags, cfa_action, flags;
> >       struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
> > -     u16 prod, last_frag;
> >       struct pci_dev *pdev = bp->pdev;
> > +     u16 prod, last_frag, txts_prod;
> >       struct bnxt_tx_ring_info *txr;
> >       struct bnxt_sw_tx_bd *tx_buf;
> >       __le32 lflags = 0;
> > @@ -526,11 +526,19 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
> >                       if (!bnxt_ptp_parse(skb, &seq_id, &hdr_off)) {
> >                               if (vlan_tag_flags)
> >                                       hdr_off += VLAN_HLEN;
> > -                             ptp->txts_req.tx_seqid = seq_id;
> > -                             ptp->txts_req.tx_hdr_off = hdr_off;
> >                               lflags |= cpu_to_le32(TX_BD_FLAGS_STAMP);
> >                               tx_buf->is_ts_pkt = 1;
> >                               skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> > +
> > +                             spin_lock_bh(&ptp->ptp_tx_lock);
> > +                             txts_prod = ptp->txts_prod;
> > +                             ptp->txts_prod = NEXT_TXTS(txts_prod);
> > +                             spin_unlock_bh(&ptp->ptp_tx_lock);
> > +
> > +                             ptp->txts_req[txts_prod].tx_seqid = seq_id;
> > +                             ptp->txts_req[txts_prod].tx_hdr_off = hdr_off;
> > +                             tx_buf->txts_prod = txts_prod;
> > +
> >                       } else {
> >                               atomic_inc(&bp->ptp_cfg->tx_avail);
> >                       }
> > @@ -770,7 +778,9 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
> >  tx_kick_pending:
> >       if (BNXT_TX_PTP_IS_SET(lflags)) {
> >               atomic64_inc(&bp->ptp_cfg->stats.ts_err);
> > -             atomic_inc(&bp->ptp_cfg->tx_avail);
> > +             if (!(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
> > +                     /* set SKB to err so PTP worker will clean up */
> > +                     ptp->txts_req[txts_prod].tx_skb = ERR_PTR(-EIO);
>
> Hi Michael
>
> Sparse complains that previously it was assumed that ptp could be NULL,
> but here it is accessed without checking for that.
>
> Perhaps it can't occur, but my brief check leads me to think it might.

Simon, thanks for the review.  The key is this if statement:

if (BNXT_TX_PTP_IS_SET(lflags))

This if statement is true if the lflags have the TX_BD_FLAGS_STAMP
set.  This flag is set only if ptp is valid because this flag tells
the hardware to take the timestamp.

>
> On line 488 there is the following:
>
>         if (unlikely(ipv6_hopopt_jumbo_remove(skb)))
>                 goto tx_free;
>
> Which will lead to the code in the hunk above.

The lflags will not have the TX_BD_FLAGS_STAMP flag set if we jump from here.

>
> Then on line 513 there is a check for ptp being NULL:
>
>
>         if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) && ptp &&
>             ptp->tx_tstamp_en) {
>
> And ptp is not set between lines 488 and 513.
>
>
> Sparse also complains that txts_prod may be used uninitaialised.
> This also seems to be a valid concern as it does seem to be the case
> on line 488.

Same explanation for txts_prod.  txts_prod will always be set if
lflags has TX_BD_FLAGS_STAMP set and this condition is false:

(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP)

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next 09/10] bnxt_en: Increase the max total outstanding PTP TX packets to 4

[Simon Horman]

On Fri, Jun 28, 2024 at 10:37:19AM -0700, Michael Chan wrote:
> On Fri, Jun 28, 2024 at 10:03 AM Simon Horman <horms@kernel.org> wrote:
> >
> > On Wed, Jun 26, 2024 at 09:43:06AM -0700, Michael Chan wrote:
> > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > > index ed2bbdf6b25f..0867861c14bd 100644
> > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > > @@ -457,8 +457,8 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > >       unsigned int length, pad = 0;
> > >       u32 len, free_size, vlan_tag_flags, cfa_action, flags;
> > >       struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
> > > -     u16 prod, last_frag;
> > >       struct pci_dev *pdev = bp->pdev;
> > > +     u16 prod, last_frag, txts_prod;
> > >       struct bnxt_tx_ring_info *txr;
> > >       struct bnxt_sw_tx_bd *tx_buf;
> > >       __le32 lflags = 0;
> > > @@ -526,11 +526,19 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > >                       if (!bnxt_ptp_parse(skb, &seq_id, &hdr_off)) {
> > >                               if (vlan_tag_flags)
> > >                                       hdr_off += VLAN_HLEN;
> > > -                             ptp->txts_req.tx_seqid = seq_id;
> > > -                             ptp->txts_req.tx_hdr_off = hdr_off;
> > >                               lflags |= cpu_to_le32(TX_BD_FLAGS_STAMP);
> > >                               tx_buf->is_ts_pkt = 1;
> > >                               skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> > > +
> > > +                             spin_lock_bh(&ptp->ptp_tx_lock);
> > > +                             txts_prod = ptp->txts_prod;
> > > +                             ptp->txts_prod = NEXT_TXTS(txts_prod);
> > > +                             spin_unlock_bh(&ptp->ptp_tx_lock);
> > > +
> > > +                             ptp->txts_req[txts_prod].tx_seqid = seq_id;
> > > +                             ptp->txts_req[txts_prod].tx_hdr_off = hdr_off;
> > > +                             tx_buf->txts_prod = txts_prod;
> > > +
> > >                       } else {
> > >                               atomic_inc(&bp->ptp_cfg->tx_avail);
> > >                       }
> > > @@ -770,7 +778,9 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > >  tx_kick_pending:
> > >       if (BNXT_TX_PTP_IS_SET(lflags)) {
> > >               atomic64_inc(&bp->ptp_cfg->stats.ts_err);
> > > -             atomic_inc(&bp->ptp_cfg->tx_avail);
> > > +             if (!(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
> > > +                     /* set SKB to err so PTP worker will clean up */
> > > +                     ptp->txts_req[txts_prod].tx_skb = ERR_PTR(-EIO);
> >
> > Hi Michael
> >
> > Sparse complains that previously it was assumed that ptp could be NULL,
> > but here it is accessed without checking for that.
> >
> > Perhaps it can't occur, but my brief check leads me to think it might.
> 
> Simon, thanks for the review.  The key is this if statement:
> 
> if (BNXT_TX_PTP_IS_SET(lflags))
> 
> This if statement is true if the lflags have the TX_BD_FLAGS_STAMP
> set.  This flag is set only if ptp is valid because this flag tells
> the hardware to take the timestamp.
> 
> >
> > On line 488 there is the following:
> >
> >         if (unlikely(ipv6_hopopt_jumbo_remove(skb)))
> >                 goto tx_free;
> >
> > Which will lead to the code in the hunk above.
> 
> The lflags will not have the TX_BD_FLAGS_STAMP flag set if we jump from here.
> 
> >
> > Then on line 513 there is a check for ptp being NULL:
> >
> >
> >         if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) && ptp &&
> >             ptp->tx_tstamp_en) {
> >
> > And ptp is not set between lines 488 and 513.
> >
> >
> > Sparse also complains that txts_prod may be used uninitaialised.
> > This also seems to be a valid concern as it does seem to be the case
> > on line 488.
> 
> Same explanation for txts_prod.  txts_prod will always be set if
> lflags has TX_BD_FLAGS_STAMP set and this condition is false:
> 
> (bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP)

Hi Michael,

Thanks for your quick response, it is the kind of explanation that
I was looking for, but wasn't sufficiently familiar with the code
to find myself.

[PATCH net-next 10/10] bnxt_en: Remove atomic operations on ptp->tx_avail

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 5653 bytes --]

From: Pavan Chebbi <pavan.chebbi@broadcom.com>

Now that we require the spinlock to protect ptp->txts_prod, change
ptp->tx_avail to non-atomic and protect it under the same spinlock.
Add a new helper function bnxt_ptp_get_txts_prod() to decrement
ptp->tx_avail under spinlock and return the producer.

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     | 20 +++-------------
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 23 +++++++++++++++----
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 10 +++++++-
 3 files changed, 31 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 0867861c14bd..33ba1964168b 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -518,34 +518,20 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		} else if (!skb_is_gso(skb)) {
 			u16 seq_id, hdr_off;
 
-			if (atomic_dec_if_positive(&ptp->tx_avail) < 0) {
-				atomic64_inc(&ptp->stats.ts_err);
-				goto tx_no_ts;
-			}
-
-			if (!bnxt_ptp_parse(skb, &seq_id, &hdr_off)) {
+			if (!bnxt_ptp_parse(skb, &seq_id, &hdr_off) &&
+			    !bnxt_ptp_get_txts_prod(ptp, &txts_prod)) {
 				if (vlan_tag_flags)
 					hdr_off += VLAN_HLEN;
 				lflags |= cpu_to_le32(TX_BD_FLAGS_STAMP);
 				tx_buf->is_ts_pkt = 1;
 				skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 
-				spin_lock_bh(&ptp->ptp_tx_lock);
-				txts_prod = ptp->txts_prod;
-				ptp->txts_prod = NEXT_TXTS(txts_prod);
-				spin_unlock_bh(&ptp->ptp_tx_lock);
-
 				ptp->txts_req[txts_prod].tx_seqid = seq_id;
 				ptp->txts_req[txts_prod].tx_hdr_off = hdr_off;
 				tx_buf->txts_prod = txts_prod;
-
-			} else {
-				atomic_inc(&bp->ptp_cfg->tx_avail);
 			}
 		}
 	}
-
-tx_no_ts:
 	if (unlikely(skb->no_fcs))
 		lflags |= cpu_to_le32(TX_BD_FLAGS_NO_CRC);
 
@@ -12184,7 +12170,7 @@ static int __bnxt_open_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init)
 	if (BNXT_PF(bp))
 		bnxt_vf_reps_open(bp);
 	if (bp->ptp_cfg && !(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
-		atomic_set(&bp->ptp_cfg->tx_avail, BNXT_MAX_TX_TS);
+		WRITE_ONCE(bp->ptp_cfg->tx_avail, BNXT_MAX_TX_TS);
 	bnxt_ptp_init_rtc(bp, true);
 	bnxt_ptp_cfg_tstamp_filters(bp);
 	if (BNXT_SUPPORTS_MULTI_RSS_CTX(bp))
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
index 9e93dc8b2b57..37d42423459c 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
@@ -730,10 +730,10 @@ static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
 	unsigned long now = jiffies;
 	struct bnxt *bp = ptp->bp;
 	u16 cons = ptp->txts_cons;
-	u8 num_requests;
+	u32 num_requests;
 	int rc = 0;
 
-	num_requests = BNXT_MAX_TX_TS - atomic_read(&ptp->tx_avail);
+	num_requests = BNXT_MAX_TX_TS - READ_ONCE(ptp->tx_avail);
 	while (num_requests--) {
 		if (IS_ERR(ptp->txts_req[cons].tx_skb))
 			goto next_slot;
@@ -743,7 +743,7 @@ static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
 		if (rc == -EAGAIN)
 			break;
 next_slot:
-		atomic_inc(&ptp->tx_avail);
+		BNXT_PTP_INC_TX_AVAIL(ptp);
 		cons = NEXT_TXTS(cons);
 	}
 	ptp->txts_cons = cons;
@@ -767,6 +767,21 @@ static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
 	return HZ;
 }
 
+int bnxt_ptp_get_txts_prod(struct bnxt_ptp_cfg *ptp, u16 *prod)
+{
+	spin_lock_bh(&ptp->ptp_tx_lock);
+	if (ptp->tx_avail) {
+		*prod = ptp->txts_prod;
+		ptp->txts_prod = NEXT_TXTS(*prod);
+		ptp->tx_avail--;
+		spin_unlock_bh(&ptp->ptp_tx_lock);
+		return 0;
+	}
+	spin_unlock_bh(&ptp->ptp_tx_lock);
+	atomic64_inc(&ptp->stats.ts_err);
+	return -ENOSPC;
+}
+
 void bnxt_get_tx_ts_p5(struct bnxt *bp, struct sk_buff *skb, u16 prod)
 {
 	struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
@@ -1014,7 +1029,7 @@ int bnxt_ptp_init(struct bnxt *bp, bool phc_cfg)
 
 	bnxt_ptp_free(bp);
 
-	atomic_set(&ptp->tx_avail, BNXT_MAX_TX_TS);
+	WRITE_ONCE(ptp->tx_avail, BNXT_MAX_TX_TS);
 	spin_lock_init(&ptp->ptp_lock);
 	spin_lock_init(&ptp->ptp_tx_lock);
 
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
index a1910ce86cbb..a9a2f9a18c9c 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
@@ -116,7 +116,7 @@ struct bnxt_ptp_cfg {
 	struct bnxt_ptp_tx_req	txts_req[BNXT_MAX_TX_TS];
 
 	struct bnxt		*bp;
-	atomic_t		tx_avail;
+	u32			tx_avail;
 	u16			rxctl;
 #define BNXT_PTP_MSG_SYNC			(1 << 0)
 #define BNXT_PTP_MSG_DELAY_REQ			(1 << 1)
@@ -157,6 +157,13 @@ do {						\
 	((dst) = READ_ONCE(src))
 #endif
 
+#define BNXT_PTP_INC_TX_AVAIL(ptp)		\
+do {						\
+	spin_lock_bh(&(ptp)->ptp_tx_lock);	\
+	(ptp)->tx_avail++;			\
+	spin_unlock_bh(&(ptp)->ptp_tx_lock);	\
+} while (0)
+
 int bnxt_ptp_parse(struct sk_buff *skb, u16 *seq_id, u16 *hdr_off);
 void bnxt_ptp_update_current_time(struct bnxt *bp);
 void bnxt_ptp_pps_event(struct bnxt *bp, u32 data1, u32 data2);
@@ -164,6 +171,7 @@ int bnxt_ptp_cfg_tstamp_filters(struct bnxt *bp);
 void bnxt_ptp_reapply_pps(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_ptp_get_txts_prod(struct bnxt_ptp_cfg *ptp, u16 *prod);
 void bnxt_get_tx_ts_p5(struct bnxt *bp, struct sk_buff *skb, u16 prod);
 int bnxt_get_rx_ts_p5(struct bnxt *bp, u64 *ts, u32 pkt_ts);
 void bnxt_tx_ts_cmp(struct bnxt *bp, struct bnxt_napi *bnapi,
-- 
2.30.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next 10/10] bnxt_en: Remove atomic operations on ptp->tx_avail

[Przemek Kitszel]

On 6/26/24 18:43, Michael Chan wrote:
> From: Pavan Chebbi <pavan.chebbi@broadcom.com>
> 
> Now that we require the spinlock to protect ptp->txts_prod, change
> ptp->tx_avail to non-atomic and protect it under the same spinlock.

Under what condition you will exceed those 4 entries?

> Add a new helper function bnxt_ptp_get_txts_prod() to decrement
> ptp->tx_avail under spinlock and return the producer.
> 
> Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>

overall it is a good series, thanks!

> ---
>   drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 20 +++-------------
>   drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 23 +++++++++++++++----
>   drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 10 +++++++-
>   3 files changed, 31 insertions(+), 22 deletions(-)
> 

Re: [PATCH net-next 10/10] bnxt_en: Remove atomic operations on ptp->tx_avail

[Pavan Chebbi]

[-- Attachment #1: Type: text/plain, Size: 1080 bytes --]

On Thu, Jun 27, 2024 at 3:10 PM Przemek Kitszel
<przemyslaw.kitszel@intel.com> wrote:
>
> On 6/26/24 18:43, Michael Chan wrote:
> > From: Pavan Chebbi <pavan.chebbi@broadcom.com>
> >
> > Now that we require the spinlock to protect ptp->txts_prod, change
> > ptp->tx_avail to non-atomic and protect it under the same spinlock.
>
> Under what condition you will exceed those 4 entries?

Only when an application chooses to do so.

>
> > Add a new helper function bnxt_ptp_get_txts_prod() to decrement
> > ptp->tx_avail under spinlock and return the producer.
> >
> > Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> > Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>
> overall it is a good series, thanks!

Thanks for the review, Przemek!

>
> > ---
> >   drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 20 +++-------------
> >   drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 23 +++++++++++++++----
> >   drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 10 +++++++-
> >   3 files changed, 31 insertions(+), 22 deletions(-)
> >
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next 10/10] bnxt_en: Remove atomic operations on ptp->tx_avail

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 671 bytes --]

On Thu, Jun 27, 2024 at 8:34 AM Pavan Chebbi <pavan.chebbi@broadcom.com> wrote:
>
> On Thu, Jun 27, 2024 at 3:10 PM Przemek Kitszel
> <przemyslaw.kitszel@intel.com> wrote:
> >
> > On 6/26/24 18:43, Michael Chan wrote:
> > > From: Pavan Chebbi <pavan.chebbi@broadcom.com>
> > >
> > > Now that we require the spinlock to protect ptp->txts_prod, change
> > > ptp->tx_avail to non-atomic and protect it under the same spinlock.
> >
> > Under what condition you will exceed those 4 entries?
>
> Only when an application chooses to do so.

The practical scenario is on a Multi-host system sharing the same NIC
with multiple hosts running PTP applications.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]