[PATCH net-next] bnxt_en: replace PTP spinlock with seqlock

[PATCH net-next] bnxt_en: replace PTP spinlock with seqlock

[Vadim Fedorenko]

We can see high contention on ptp_lock while doing RX timestamping
on high packet rates over several queues. Spinlock is not effecient
to protect timecounter for RX timestamps when reads are the most
usual operations and writes are only occasional. It's better to use
seqlock in such cases.

Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 30 +++-----
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 76 +++++++++----------
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 26 +++++--
 3 files changed, 67 insertions(+), 65 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 6e422e24750a..0c1a52681822 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -2255,9 +2255,7 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
 			if (!bnxt_get_rx_ts_p5(bp, &ts, cmpl_ts)) {
 				struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
 
-				spin_lock_bh(&ptp->ptp_lock);
-				ns = timecounter_cyc2time(&ptp->tc, ts);
-				spin_unlock_bh(&ptp->ptp_lock);
+				ns = bnxt_timecounter_cyc2time(ptp, ts);
 				memset(skb_hwtstamps(skb), 0,
 				       sizeof(*skb_hwtstamps(skb)));
 				skb_hwtstamps(skb)->hwtstamp = ns_to_ktime(ns);
@@ -2757,17 +2755,18 @@ static int bnxt_async_event_process(struct bnxt *bp,
 		case ASYNC_EVENT_CMPL_PHC_UPDATE_EVENT_DATA1_FLAGS_PHC_RTC_UPDATE:
 			if (BNXT_PTP_USE_RTC(bp)) {
 				struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
+				unsigned long flags;
 				u64 ns;
 
 				if (!ptp)
 					goto async_event_process_exit;
 
-				spin_lock_bh(&ptp->ptp_lock);
+				write_seqlock_irqsave(&ptp->ptp_lock, flags);
 				bnxt_ptp_update_current_time(bp);
 				ns = (((u64)BNXT_EVENT_PHC_RTC_UPDATE(data1) <<
 				       BNXT_PHC_BITS) | ptp->current_time);
 				bnxt_ptp_rtc_timecounter_init(ptp, ns);
-				spin_unlock_bh(&ptp->ptp_lock);
+				write_sequnlock_irqrestore(&ptp->ptp_lock, flags);
 			}
 			break;
 		}
@@ -13493,13 +13492,9 @@ static void bnxt_force_fw_reset(struct bnxt *bp)
 	    test_bit(BNXT_STATE_IN_FW_RESET, &bp->state))
 		return;
 
-	if (ptp) {
-		spin_lock_bh(&ptp->ptp_lock);
-		set_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
-		spin_unlock_bh(&ptp->ptp_lock);
-	} else {
-		set_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
-	}
+	set_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
+	/* Make sure TS reader can see RESET bit set */
+	smp_mb__after_atomic();
 	bnxt_fw_reset_close(bp);
 	wait_dsecs = fw_health->master_func_wait_dsecs;
 	if (fw_health->primary) {
@@ -13560,13 +13555,10 @@ void bnxt_fw_reset(struct bnxt *bp)
 		struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
 		int n = 0, tmo;
 
-		if (ptp) {
-			spin_lock_bh(&ptp->ptp_lock);
-			set_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
-			spin_unlock_bh(&ptp->ptp_lock);
-		} else {
-			set_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
-		}
+		set_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
+		/* Make sure TS reader can see RESET bit set */
+		smp_mb__after_atomic();
+
 		if (bp->pf.active_vfs &&
 		    !test_bit(BNXT_STATE_FW_FATAL_COND, &bp->state))
 			n = bnxt_get_registered_vfs(bp);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
index 37d42423459c..ee4287519c50 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
@@ -62,23 +62,25 @@ static int bnxt_ptp_settime(struct ptp_clock_info *ptp_info,
 	struct bnxt_ptp_cfg *ptp = container_of(ptp_info, struct bnxt_ptp_cfg,
 						ptp_info);
 	u64 ns = timespec64_to_ns(ts);
+	unsigned long flags;
 
 	if (BNXT_PTP_USE_RTC(ptp->bp))
 		return bnxt_ptp_cfg_settime(ptp->bp, ns);
 
-	spin_lock_bh(&ptp->ptp_lock);
+	write_seqlock_irqsave(&ptp->ptp_lock, flags);
 	timecounter_init(&ptp->tc, &ptp->cc, ns);
-	spin_unlock_bh(&ptp->ptp_lock);
+	write_sequnlock_irqrestore(&ptp->ptp_lock, flags);
 	return 0;
 }
 
-/* Caller holds ptp_lock */
 static int bnxt_refclk_read(struct bnxt *bp, struct ptp_system_timestamp *sts,
 			    u64 *ns)
 {
 	struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
 	u32 high_before, high_now, low;
 
+	/* Make sure the RESET bit is set */
+	smp_mb__before_atomic();
 	if (test_bit(BNXT_STATE_IN_FW_RESET, &bp->state))
 		return -EIO;
 
@@ -100,13 +102,14 @@ static int bnxt_refclk_read(struct bnxt *bp, struct ptp_system_timestamp *sts,
 static void bnxt_ptp_get_current_time(struct bnxt *bp)
 {
 	struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
+	unsigned long flags;
 
 	if (!ptp)
 		return;
-	spin_lock_bh(&ptp->ptp_lock);
+	write_seqlock_irqsave(&ptp->ptp_lock, flags);
 	WRITE_ONCE(ptp->old_time, ptp->current_time);
 	bnxt_refclk_read(bp, NULL, &ptp->current_time);
-	spin_unlock_bh(&ptp->ptp_lock);
+	write_sequnlock_irqrestore(&ptp->ptp_lock, flags);
 }
 
 static int bnxt_hwrm_port_ts_query(struct bnxt *bp, u32 flags, u64 *ts,
@@ -152,20 +155,16 @@ static int bnxt_ptp_gettimex(struct ptp_clock_info *ptp_info,
 	u64 ns, cycles;
 	int rc;
 
-	spin_lock_bh(&ptp->ptp_lock);
 	rc = bnxt_refclk_read(ptp->bp, sts, &cycles);
-	if (rc) {
-		spin_unlock_bh(&ptp->ptp_lock);
+	if (rc)
 		return rc;
-	}
-	ns = timecounter_cyc2time(&ptp->tc, cycles);
-	spin_unlock_bh(&ptp->ptp_lock);
+
+	ns = bnxt_timecounter_cyc2time(ptp, cycles);
 	*ts = ns_to_timespec64(ns);
 
 	return 0;
 }
 
-/* Caller holds ptp_lock */
 void bnxt_ptp_update_current_time(struct bnxt *bp)
 {
 	struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
@@ -177,6 +176,7 @@ void bnxt_ptp_update_current_time(struct bnxt *bp)
 static int bnxt_ptp_adjphc(struct bnxt_ptp_cfg *ptp, s64 delta)
 {
 	struct hwrm_port_mac_cfg_input *req;
+	unsigned long flags;
 	int rc;
 
 	rc = hwrm_req_init(ptp->bp, req, HWRM_PORT_MAC_CFG);
@@ -190,9 +190,9 @@ static int bnxt_ptp_adjphc(struct bnxt_ptp_cfg *ptp, s64 delta)
 	if (rc) {
 		netdev_err(ptp->bp->dev, "ptp adjphc failed. rc = %x\n", rc);
 	} else {
-		spin_lock_bh(&ptp->ptp_lock);
+		write_seqlock_irqsave(&ptp->ptp_lock, flags);
 		bnxt_ptp_update_current_time(ptp->bp);
-		spin_unlock_bh(&ptp->ptp_lock);
+		write_sequnlock_irqrestore(&ptp->ptp_lock, flags);
 	}
 
 	return rc;
@@ -202,13 +202,14 @@ static int bnxt_ptp_adjtime(struct ptp_clock_info *ptp_info, s64 delta)
 {
 	struct bnxt_ptp_cfg *ptp = container_of(ptp_info, struct bnxt_ptp_cfg,
 						ptp_info);
+	unsigned long flags;
 
 	if (BNXT_PTP_USE_RTC(ptp->bp))
 		return bnxt_ptp_adjphc(ptp, delta);
 
-	spin_lock_bh(&ptp->ptp_lock);
+	write_seqlock_irqsave(&ptp->ptp_lock, flags);
 	timecounter_adjtime(&ptp->tc, delta);
-	spin_unlock_bh(&ptp->ptp_lock);
+	write_sequnlock_irqrestore(&ptp->ptp_lock, flags);
 	return 0;
 }
 
@@ -236,14 +237,15 @@ static int bnxt_ptp_adjfine(struct ptp_clock_info *ptp_info, long scaled_ppm)
 	struct bnxt_ptp_cfg *ptp = container_of(ptp_info, struct bnxt_ptp_cfg,
 						ptp_info);
 	struct bnxt *bp = ptp->bp;
+	unsigned long flags;
 
 	if (!BNXT_MH(bp))
 		return bnxt_ptp_adjfine_rtc(bp, scaled_ppm);
 
-	spin_lock_bh(&ptp->ptp_lock);
+	write_seqlock_irqsave(&ptp->ptp_lock, flags);
 	timecounter_read(&ptp->tc);
 	ptp->cc.mult = adjust_by_scaled_ppm(ptp->cmult, scaled_ppm);
-	spin_unlock_bh(&ptp->ptp_lock);
+	write_sequnlock_irqrestore(&ptp->ptp_lock, flags);
 	return 0;
 }
 
@@ -254,9 +256,7 @@ void bnxt_ptp_pps_event(struct bnxt *bp, u32 data1, u32 data2)
 	u64 ns, pps_ts;
 
 	pps_ts = EVENT_PPS_TS(data2, data1);
-	spin_lock_bh(&ptp->ptp_lock);
-	ns = timecounter_cyc2time(&ptp->tc, pps_ts);
-	spin_unlock_bh(&ptp->ptp_lock);
+	ns = bnxt_timecounter_cyc2time(ptp, pps_ts);
 
 	switch (EVENT_DATA2_PPS_EVENT_TYPE(data2)) {
 	case ASYNC_EVENT_CMPL_PPS_TIMESTAMP_EVENT_DATA2_EVENT_TYPE_INTERNAL:
@@ -395,14 +395,11 @@ static int bnxt_get_target_cycles(struct bnxt_ptp_cfg *ptp, u64 target_ns,
 	u64 nsec_now, nsec_delta;
 	int rc;
 
-	spin_lock_bh(&ptp->ptp_lock);
 	rc = bnxt_refclk_read(ptp->bp, NULL, &cycles_now);
-	if (rc) {
-		spin_unlock_bh(&ptp->ptp_lock);
+	if (rc)
 		return rc;
-	}
-	nsec_now = timecounter_cyc2time(&ptp->tc, cycles_now);
-	spin_unlock_bh(&ptp->ptp_lock);
+
+	nsec_now = bnxt_timecounter_cyc2time(ptp, cycles_now);
 
 	nsec_delta = target_ns - nsec_now;
 	*cycles_delta = div64_u64(nsec_delta << ptp->cc.shift, ptp->cc.mult);
@@ -702,9 +699,7 @@ static int bnxt_stamp_tx_skb(struct bnxt *bp, int slot)
 				     tmo, slot);
 	if (!rc) {
 		memset(&timestamp, 0, sizeof(timestamp));
-		spin_lock_bh(&ptp->ptp_lock);
-		ns = timecounter_cyc2time(&ptp->tc, ts);
-		spin_unlock_bh(&ptp->ptp_lock);
+		ns = bnxt_timecounter_cyc2time(ptp, ts);
 		timestamp.hwtstamp = ns_to_ktime(ns);
 		skb_tstamp_tx(txts_req->tx_skb, &timestamp);
 		ptp->stats.ts_pkts++;
@@ -730,6 +725,7 @@ 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;
+	unsigned long flags;
 	u32 num_requests;
 	int rc = 0;
 
@@ -757,9 +753,9 @@ static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
 	bnxt_ptp_get_current_time(bp);
 	ptp->next_period = now + HZ;
 	if (time_after_eq(now, ptp->next_overflow_check)) {
-		spin_lock_bh(&ptp->ptp_lock);
+		write_seqlock_irqsave(&ptp->ptp_lock, flags);
 		timecounter_read(&ptp->tc);
-		spin_unlock_bh(&ptp->ptp_lock);
+		write_sequnlock_irqrestore(&ptp->ptp_lock, flags);
 		ptp->next_overflow_check = now + BNXT_PHC_OVERFLOW_PERIOD;
 	}
 	if (rc == -EAGAIN)
@@ -833,9 +829,7 @@ void bnxt_tx_ts_cmp(struct bnxt *bp, struct bnxt_napi *bnapi,
 				   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);
+			ns = bnxt_timecounter_cyc2time(ptp, ts);
 			timestamp.hwtstamp = ns_to_ktime(ns);
 			skb_tstamp_tx(tx_buf->skb, &timestamp);
 		}
@@ -975,6 +969,7 @@ void bnxt_ptp_rtc_timecounter_init(struct bnxt_ptp_cfg *ptp, u64 ns)
 int bnxt_ptp_init_rtc(struct bnxt *bp, bool phc_cfg)
 {
 	struct timespec64 tsp;
+	unsigned long flags;
 	u64 ns;
 	int rc;
 
@@ -993,9 +988,9 @@ int bnxt_ptp_init_rtc(struct bnxt *bp, bool phc_cfg)
 		if (rc)
 			return rc;
 	}
-	spin_lock_bh(&bp->ptp_cfg->ptp_lock);
+	write_seqlock_irqsave(&bp->ptp_cfg->ptp_lock, flags);
 	bnxt_ptp_rtc_timecounter_init(bp->ptp_cfg, ns);
-	spin_unlock_bh(&bp->ptp_cfg->ptp_lock);
+	write_sequnlock_irqrestore(&bp->ptp_cfg->ptp_lock, flags);
 
 	return 0;
 }
@@ -1015,6 +1010,7 @@ static void bnxt_ptp_free(struct bnxt *bp)
 int bnxt_ptp_init(struct bnxt *bp, bool phc_cfg)
 {
 	struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
+	unsigned long flags;
 	int rc;
 
 	if (!ptp)
@@ -1030,7 +1026,7 @@ int bnxt_ptp_init(struct bnxt *bp, bool phc_cfg)
 	bnxt_ptp_free(bp);
 
 	WRITE_ONCE(ptp->tx_avail, BNXT_MAX_TX_TS);
-	spin_lock_init(&ptp->ptp_lock);
+	seqlock_init(&ptp->ptp_lock);
 	spin_lock_init(&ptp->ptp_tx_lock);
 
 	if (BNXT_PTP_USE_RTC(bp)) {
@@ -1063,10 +1059,10 @@ int bnxt_ptp_init(struct bnxt *bp, bool phc_cfg)
 	atomic64_set(&ptp->stats.ts_err, 0);
 
 	if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS) {
-		spin_lock_bh(&ptp->ptp_lock);
+		write_seqlock_irqsave(&ptp->ptp_lock, flags);
 		bnxt_refclk_read(bp, NULL, &ptp->current_time);
 		WRITE_ONCE(ptp->old_time, ptp->current_time);
-		spin_unlock_bh(&ptp->ptp_lock);
+		write_sequnlock_irqrestore(&ptp->ptp_lock, flags);
 		ptp_schedule_worker(ptp->ptp_clock, 0);
 	}
 	ptp->txts_tmo = BNXT_PTP_DFLT_TX_TMO;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
index a9a2f9a18c9c..103eff803a3b 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
@@ -102,7 +102,7 @@ struct bnxt_ptp_cfg {
 	struct timecounter	tc;
 	struct bnxt_pps		pps_info;
 	/* serialize timecounter access */
-	spinlock_t		ptp_lock;
+	seqlock_t		ptp_lock;
 	/* serialize ts tx request queuing */
 	spinlock_t		ptp_tx_lock;
 	u64			current_time;
@@ -146,11 +146,13 @@ struct bnxt_ptp_cfg {
 };
 
 #if BITS_PER_LONG == 32
-#define BNXT_READ_TIME64(ptp, dst, src)		\
-do {						\
-	spin_lock_bh(&(ptp)->ptp_lock);		\
-	(dst) = (src);				\
-	spin_unlock_bh(&(ptp)->ptp_lock);	\
+#define BNXT_READ_TIME64(ptp, dst, src)			\
+do {							\
+	unsigned int seq;				\
+	do {						\
+		seq = read_seqbegin(&(ptp)->ptp_lock);	\
+		(dst) = (src);				\
+	while (read_seqretry(&(ptp)->ptp_lock, seq);	\
 } while (0)
 #else
 #define BNXT_READ_TIME64(ptp, dst, src)		\
@@ -180,4 +182,16 @@ 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);
 void bnxt_ptp_clear(struct bnxt *bp);
+static inline u64 bnxt_timecounter_cyc2time(struct bnxt_ptp_cfg *ptp, u64 ts)
+{
+	unsigned int seq;
+	u64 ns;
+
+	do {
+		seq = read_seqbegin(&ptp->ptp_lock);
+		ns = timecounter_cyc2time(&ptp->tc, ts);
+	} while (read_seqretry(&ptp->ptp_lock, seq));
+
+	return ns;
+}
 #endif
-- 
2.43.5

Re: [PATCH net-next] bnxt_en: replace PTP spinlock with seqlock

[Jakub Kicinski]

On Mon, 14 Oct 2024 16:29:47 -0700 Vadim Fedorenko wrote:
> -	spin_lock_bh(&ptp->ptp_lock);
> +	write_seqlock_irqsave(&ptp->ptp_lock, flags);
>  	timecounter_adjtime(&ptp->tc, delta);
> -	spin_unlock_bh(&ptp->ptp_lock);
> +	write_sequnlock_irqrestore(&ptp->ptp_lock, flags);

I think when you adjtime / adjfine (IOW on all the write path) you still
need the spin lock. But in addition also the seq lock. And then the
read path can take just the seq lock.

This will also remove any uncertainty about the bit ops.

Re: [PATCH net-next] bnxt_en: replace PTP spinlock with seqlock

[Vadim Fedorenko]

On 15/10/2024 00:35, Jakub Kicinski wrote:
> On Mon, 14 Oct 2024 16:29:47 -0700 Vadim Fedorenko wrote:
>> -	spin_lock_bh(&ptp->ptp_lock);
>> +	write_seqlock_irqsave(&ptp->ptp_lock, flags);
>>   	timecounter_adjtime(&ptp->tc, delta);
>> -	spin_unlock_bh(&ptp->ptp_lock);
>> +	write_sequnlock_irqrestore(&ptp->ptp_lock, flags);
> 
> I think when you adjtime / adjfine (IOW on all the write path) you still
> need the spin lock. But in addition also the seq lock. And then the
> read path can take just the seq lock.

I think there is a spinlock in seqlock_t which is used to prevent
multiple writers.

> This will also remove any uncertainty about the bit ops.

Should I use read_seqlock_excl_bh()/write_seqlock_bh() for the bit ops
then?

Re: [PATCH net-next] bnxt_en: replace PTP spinlock with seqlock

[Jakub Kicinski]

On Tue, 15 Oct 2024 11:25:00 +0100 Vadim Fedorenko wrote:
> > I think when you adjtime / adjfine (IOW on all the write path) you still
> > need the spin lock. But in addition also the seq lock. And then the
> > read path can take just the seq lock.  
> 
> I think there is a spinlock in seqlock_t which is used to prevent
> multiple writers.

My bad.

> > This will also remove any uncertainty about the bit ops.  
> 
> Should I use read_seqlock_excl_bh()/write_seqlock_bh() for the bit ops
> then?

Yup, modulo what Micheal said in the other leg of the thread, but SGTM.

Re: [PATCH net-next] bnxt_en: replace PTP spinlock with seqlock

[Michael Chan]

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

On Mon, Oct 14, 2024 at 4:29 PM Vadim Fedorenko <vadfed@meta.com> wrote:
>
> We can see high contention on ptp_lock while doing RX timestamping
> on high packet rates over several queues. Spinlock is not effecient
> to protect timecounter for RX timestamps when reads are the most
> usual operations and writes are only occasional. It's better to use
> seqlock in such cases.
>
> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>

> -/* Caller holds ptp_lock */
>  static int bnxt_refclk_read(struct bnxt *bp, struct ptp_system_timestamp *sts,
>                             u64 *ns)
>  {
>         struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
>         u32 high_before, high_now, low;
>
> +       /* Make sure the RESET bit is set */
> +       smp_mb__before_atomic();

This may not be sufficient.  MMIO read of any register (clock register
in this case) can hang the chip if it is undergoing reset.

>         if (test_bit(BNXT_STATE_IN_FW_RESET, &bp->state))
>                 return -EIO;

We could have missed the flag and got here while the chip is about to be reset.

I will review the patch in more detail tomorrow.  Thanks.

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

Re: [PATCH net-next] bnxt_en: replace PTP spinlock with seqlock

[Vadim Fedorenko]

On 15/10/2024 07:20, Michael Chan wrote:
> On Mon, Oct 14, 2024 at 4:29 PM Vadim Fedorenko <vadfed@meta.com> wrote:
>>
>> We can see high contention on ptp_lock while doing RX timestamping
>> on high packet rates over several queues. Spinlock is not effecient
>> to protect timecounter for RX timestamps when reads are the most
>> usual operations and writes are only occasional. It's better to use
>> seqlock in such cases.
>>
>> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
> 
>> -/* Caller holds ptp_lock */
>>   static int bnxt_refclk_read(struct bnxt *bp, struct ptp_system_timestamp *sts,
>>                              u64 *ns)
>>   {
>>          struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
>>          u32 high_before, high_now, low;
>>
>> +       /* Make sure the RESET bit is set */
>> +       smp_mb__before_atomic();
> 
> This may not be sufficient.  MMIO read of any register (clock register
> in this case) can hang the chip if it is undergoing reset.
> 
>>          if (test_bit(BNXT_STATE_IN_FW_RESET, &bp->state))
>>                  return -EIO;
> 
> We could have missed the flag and got here while the chip is about to be reset.
> 
> I will review the patch in more detail tomorrow.  Thanks.


Ok, so we have to serialize bnxt_refclk_read() and FW RESETS, but don't
block readers of ptp, especially on RX hot path. So it looks like
read_seqcount_excl_bh() can help us with it.

Re: [PATCH net-next] bnxt_en: replace PTP spinlock with seqlock

[kernel test robot]

Hi Vadim,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Vadim-Fedorenko/bnxt_en-replace-PTP-spinlock-with-seqlock/20241015-073207
base:   net-next/main
patch link:    https://lore.kernel.org/r/20241014232947.4059941-1-vadfed%40meta.com
patch subject: [PATCH net-next] bnxt_en: replace PTP spinlock with seqlock
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20241018/202410182038.mpo0UaRH-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241018/202410182038.mpo0UaRH-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410182038.mpo0UaRH-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   drivers/net/ethernet/broadcom/bnxt/bnxt.c: In function 'bnxt_force_fw_reset':
>> drivers/net/ethernet/broadcom/bnxt/bnxt.c:13488:30: warning: unused variable 'ptp' [-Wunused-variable]
   13488 |         struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
         |                              ^~~
   drivers/net/ethernet/broadcom/bnxt/bnxt.c: In function 'bnxt_fw_reset':
   drivers/net/ethernet/broadcom/bnxt/bnxt.c:13555:38: warning: unused variable 'ptp' [-Wunused-variable]
   13555 |                 struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
         |                                      ^~~
--
   In file included from drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:21:
   drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c: In function 'bnxt_get_rx_ts_p5':
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h:155:52: error: expected ')' before ';' token
     155 |         while (read_seqretry(&(ptp)->ptp_lock, seq);    \
         |               ~                                    ^
   drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:802:9: note: in expansion of macro 'BNXT_READ_TIME64'
     802 |         BNXT_READ_TIME64(ptp, time, ptp->old_time);
         |         ^~~~~~~~~~~~~~~~
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h:156:1: error: expected expression before '}' token
     156 | } while (0)
         | ^
   drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:802:9: note: in expansion of macro 'BNXT_READ_TIME64'
     802 |         BNXT_READ_TIME64(ptp, time, ptp->old_time);
         |         ^~~~~~~~~~~~~~~~
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:810:1: error: expected 'while' before 'void'
     810 | void bnxt_tx_ts_cmp(struct bnxt *bp, struct bnxt_napi *bnapi,
         | ^~~~
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:857:12: error: invalid storage class for function 'bnxt_ptp_verify'
     857 | static int bnxt_ptp_verify(struct ptp_clock_info *ptp_info, unsigned int pin,
         |            ^~~~~~~~~~~~~~~
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:870:12: error: invalid storage class for function 'bnxt_ptp_pps_init'
     870 | static int bnxt_ptp_pps_init(struct bnxt *bp)
         |            ^~~~~~~~~~~~~~~~~
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:931:13: error: invalid storage class for function 'bnxt_pps_config_ok'
     931 | static bool bnxt_pps_config_ok(struct bnxt *bp)
         |             ^~~~~~~~~~~~~~~~~~
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:938:13: error: invalid storage class for function 'bnxt_ptp_timecounter_init'
     938 | static void bnxt_ptp_timecounter_init(struct bnxt *bp, bool init_tc)
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:998:13: error: invalid storage class for function 'bnxt_ptp_free'
     998 | static void bnxt_ptp_free(struct bnxt *bp)
         |             ^~~~~~~~~~~~~
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:1100:1: error: expected declaration or statement at end of input
    1100 | }
         | ^
   drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c: At top level:
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:1077:6: warning: 'bnxt_ptp_clear' defined but not used [-Wunused-function]
    1077 | void bnxt_ptp_clear(struct bnxt *bp)
         |      ^~~~~~~~~~~~~~
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:1010:5: warning: 'bnxt_ptp_init' defined but not used [-Wunused-function]
    1010 | int bnxt_ptp_init(struct bnxt *bp, bool phc_cfg)
         |     ^~~~~~~~~~~~~

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for GET_FREE_REGION
   Depends on [n]: SPARSEMEM [=n]
   Selected by [m]:
   - RESOURCE_KUNIT_TEST [=m] && RUNTIME_TESTING_MENU [=y] && KUNIT [=m]


vim +155 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h

   147	
   148	#if BITS_PER_LONG == 32
   149	#define BNXT_READ_TIME64(ptp, dst, src)			\
   150	do {							\
   151		unsigned int seq;				\
   152		do {						\
   153			seq = read_seqbegin(&(ptp)->ptp_lock);	\
   154			(dst) = (src);				\
 > 155		while (read_seqretry(&(ptp)->ptp_lock, seq);	\
 > 156	} while (0)
   157	#else
   158	#define BNXT_READ_TIME64(ptp, dst, src)		\
   159		((dst) = READ_ONCE(src))
   160	#endif
   161	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki