diff mbox series

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

Message ID 20240626164307.219568-11-michael.chan@broadcom.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series bnxt_en: PTP updates for net-next | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 842 this patch: 842
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/build_clang success Errors and warnings before: 849 this patch: 849
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 852 this patch: 852
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 121 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 2 this patch: 2
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-06-27--03-00 (tests: 665)

Commit Message

Michael Chan June 26, 2024, 4:43 p.m. UTC 
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(-)

Comments

Przemek Kitszel June 27, 2024, 9:40 a.m. UTC | #1 
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(-)
>
Pavan Chebbi June 27, 2024, 3:34 p.m. UTC | #2 
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(-)
> >
>
Michael Chan June 27, 2024, 3:43 p.m. UTC | #3 
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.
diff mbox series

Patch

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,