[PATCH net] bnxt_en: improve TX timestamping FIFO configuration

[PATCH net] bnxt_en: improve TX timestamping FIFO configuration

[Vadim Fedorenko]

Reconfiguration of netdev may trigger close/open procedure which can
break FIFO status by adjusting the amount of empty slots for TX
timestamps. But it is not really needed because timestamps for the
packets sent over the wire still can be retrieved. On the other side,
during netdev close procedure any skbs waiting for TX timestamps can be
leaked because there is no cleaning procedure called. Free skbs waiting
for TX timestamps when closing netdev.

Fixes: 8aa2a79e9b95 ("bnxt_en: Increase the max total outstanding PTP TX packets to 4")
Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     |  4 ++--
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 23 +++++++++++++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h |  1 +
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index c8e3468eee61..45d178586316 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -3517,6 +3517,8 @@ static void bnxt_free_skbs(struct bnxt *bp)
 {
 	bnxt_free_tx_skbs(bp);
 	bnxt_free_rx_skbs(bp);
+	if (bp->ptp_cfg && !(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
+		bnxt_ptp_free_txts_skbs(bp->ptp_cfg);
 }
 
 static void bnxt_init_ctx_mem(struct bnxt_ctx_mem_type *ctxm, void *p, int len)
@@ -12797,8 +12799,6 @@ 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 && !(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
-		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 2d4e19b96ee7..39dc4f1f651a 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
@@ -794,6 +794,29 @@ static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
 	return HZ;
 }
 
+void bnxt_ptp_free_txts_skbs(struct bnxt_ptp_cfg *ptp)
+{
+	struct bnxt_ptp_tx_req *txts_req;
+	u16 cons = ptp->txts_cons;
+
+	/* make sure ptp aux worker finished with
+	 * possible BNXT_STATE_OPEN set
+	 */
+	ptp_cancel_worker_sync(ptp->ptp_clock);
+
+	spin_lock_bh(&ptp->ptp_tx_lock);
+	ptp->tx_avail = BNXT_MAX_TX_TS;
+	while (cons != ptp->txts_prod) {
+		txts_req = &ptp->txts_req[cons];
+		if (!IS_ERR_OR_NULL(txts_req->tx_skb))
+			dev_kfree_skb_any(txts_req->tx_skb);
+		cons = NEXT_TXTS(cons);
+	}
+	ptp->txts_cons = cons;
+	spin_unlock_bh(&ptp->ptp_tx_lock);
+	ptp_schedule_worker(ptp->ptp_clock, 0);
+}
+
 int bnxt_ptp_get_txts_prod(struct bnxt_ptp_cfg *ptp, u16 *prod)
 {
 	spin_lock_bh(&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 a95f05e9c579..0481161d26ef 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
@@ -162,6 +162,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_ptp_free_txts_skbs(struct bnxt_ptp_cfg *ptp);
 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);
-- 
2.47.1

Re: [PATCH net] bnxt_en: improve TX timestamping FIFO configuration

[Michael Chan]

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

On Wed, Apr 16, 2025 at 8:01 AM Vadim Fedorenko <vadfed@meta.com> wrote:
>
> Reconfiguration of netdev may trigger close/open procedure which can
> break FIFO status by adjusting the amount of empty slots for TX
> timestamps. But it is not really needed because timestamps for the
> packets sent over the wire still can be retrieved. On the other side,
> during netdev close procedure any skbs waiting for TX timestamps can be
> leaked because there is no cleaning procedure called. Free skbs waiting
> for TX timestamps when closing netdev.
>
> Fixes: 8aa2a79e9b95 ("bnxt_en: Increase the max total outstanding PTP TX packets to 4")
> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c     |  4 ++--
>  drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 23 +++++++++++++++++++
>  drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h |  1 +
>  3 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index c8e3468eee61..45d178586316 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -3517,6 +3517,8 @@ static void bnxt_free_skbs(struct bnxt *bp)
>  {
>         bnxt_free_tx_skbs(bp);
>         bnxt_free_rx_skbs(bp);
> +       if (bp->ptp_cfg && !(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
> +               bnxt_ptp_free_txts_skbs(bp->ptp_cfg);

Since these are TX SKBs, it's slightly more logical if we put this in
bnxt_free_tx_skbs().

>  }
>
>  static void bnxt_init_ctx_mem(struct bnxt_ctx_mem_type *ctxm, void *p, int len)

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> index 2d4e19b96ee7..39dc4f1f651a 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> @@ -794,6 +794,29 @@ static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
>         return HZ;
>  }
>
> +void bnxt_ptp_free_txts_skbs(struct bnxt_ptp_cfg *ptp)
> +{
> +       struct bnxt_ptp_tx_req *txts_req;
> +       u16 cons = ptp->txts_cons;
> +
> +       /* make sure ptp aux worker finished with
> +        * possible BNXT_STATE_OPEN set
> +        */
> +       ptp_cancel_worker_sync(ptp->ptp_clock);
> +
> +       spin_lock_bh(&ptp->ptp_tx_lock);

I think the spinlock is not needed because bnxt_tx_disable() should
have been called already in the close path.

> +       ptp->tx_avail = BNXT_MAX_TX_TS;
> +       while (cons != ptp->txts_prod) {
> +               txts_req = &ptp->txts_req[cons];
> +               if (!IS_ERR_OR_NULL(txts_req->tx_skb))
> +                       dev_kfree_skb_any(txts_req->tx_skb);
> +               cons = NEXT_TXTS(cons);

I think we can remove the similar code we have in bnxt_ptp_clear().
We should always go through this path before bnxt_ptp_clear().

Thanks for the patch.

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

Re: [PATCH net] bnxt_en: improve TX timestamping FIFO configuration

[Vadim Fedorenko]

On 16/04/2025 18:03, Michael Chan wrote:
> On Wed, Apr 16, 2025 at 8:01 AM Vadim Fedorenko <vadfed@meta.com> wrote:
>>
>> Reconfiguration of netdev may trigger close/open procedure which can
>> break FIFO status by adjusting the amount of empty slots for TX
>> timestamps. But it is not really needed because timestamps for the
>> packets sent over the wire still can be retrieved. On the other side,
>> during netdev close procedure any skbs waiting for TX timestamps can be
>> leaked because there is no cleaning procedure called. Free skbs waiting
>> for TX timestamps when closing netdev.
>>
>> Fixes: 8aa2a79e9b95 ("bnxt_en: Increase the max total outstanding PTP TX packets to 4")
>> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
>> ---
>>   drivers/net/ethernet/broadcom/bnxt/bnxt.c     |  4 ++--
>>   drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 23 +++++++++++++++++++
>>   drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h |  1 +
>>   3 files changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> index c8e3468eee61..45d178586316 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> @@ -3517,6 +3517,8 @@ static void bnxt_free_skbs(struct bnxt *bp)
>>   {
>>          bnxt_free_tx_skbs(bp);
>>          bnxt_free_rx_skbs(bp);
>> +       if (bp->ptp_cfg && !(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
>> +               bnxt_ptp_free_txts_skbs(bp->ptp_cfg);
> 
> Since these are TX SKBs, it's slightly more logical if we put this in
> bnxt_free_tx_skbs().

Do you mean to move this chunk to bnxt_free_tx_skbs() ?
I put it here because the driver has 3 different FIFOs to keep SKBs,
and it's logical to move PTP FIFO free function out of TX part..
But have no strong opinion.

>>   }
>>
>>   static void bnxt_init_ctx_mem(struct bnxt_ctx_mem_type *ctxm, void *p, int len)
> 
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> index 2d4e19b96ee7..39dc4f1f651a 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> @@ -794,6 +794,29 @@ static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
>>          return HZ;
>>   }
>>
>> +void bnxt_ptp_free_txts_skbs(struct bnxt_ptp_cfg *ptp)
>> +{
>> +       struct bnxt_ptp_tx_req *txts_req;
>> +       u16 cons = ptp->txts_cons;
>> +
>> +       /* make sure ptp aux worker finished with
>> +        * possible BNXT_STATE_OPEN set
>> +        */
>> +       ptp_cancel_worker_sync(ptp->ptp_clock);
>> +
>> +       spin_lock_bh(&ptp->ptp_tx_lock);
> 
> I think the spinlock is not needed because bnxt_tx_disable() should
> have been called already in the close path.

Hmm ... ok, yeah, TX won't be woken up at that moment. I'll remove it.

> 
>> +       ptp->tx_avail = BNXT_MAX_TX_TS;
>> +       while (cons != ptp->txts_prod) {
>> +               txts_req = &ptp->txts_req[cons];
>> +               if (!IS_ERR_OR_NULL(txts_req->tx_skb))
>> +                       dev_kfree_skb_any(txts_req->tx_skb);
>> +               cons = NEXT_TXTS(cons);
> 
> I think we can remove the similar code we have in bnxt_ptp_clear().
> We should always go through this path before bnxt_ptp_clear().

The difference with bnxt_ptp_clear() code is that this one clears SKBs
waiting in the queue according to consumer/producer pointers while
bnxt_ptp_clear() iterates over all slots, a bit more on safe side. 
Should I adjust this part to check all slots before removing
bnxt_ptp_clear() FIFO manipulations?

I believe in the normal way of things there should be no need to iterate
over all slots, but maybe you think of some conditions when we have to
check all slots?

> 
> Thanks for the patch.

Re: [PATCH net] bnxt_en: improve TX timestamping FIFO configuration

[Michael Chan]

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

On Wed, Apr 16, 2025 at 10:35 AM Vadim Fedorenko
<vadim.fedorenko@linux.dev> wrote:
>
> On 16/04/2025 18:03, Michael Chan wrote:
> > On Wed, Apr 16, 2025 at 8:01 AM Vadim Fedorenko <vadfed@meta.com> wrote:
> >> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> >> index c8e3468eee61..45d178586316 100644
> >> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> >> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> >> @@ -3517,6 +3517,8 @@ static void bnxt_free_skbs(struct bnxt *bp)
> >>   {
> >>          bnxt_free_tx_skbs(bp);
> >>          bnxt_free_rx_skbs(bp);
> >> +       if (bp->ptp_cfg && !(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
> >> +               bnxt_ptp_free_txts_skbs(bp->ptp_cfg);
> >
> > Since these are TX SKBs, it's slightly more logical if we put this in
> > bnxt_free_tx_skbs().
>
> Do you mean to move this chunk to bnxt_free_tx_skbs() ?

Yes, move these 2 lines to the end of bnxt_free_tx_skbs().  I think it
just makes a little more sense since these are transmit SKBs.

> I put it here because the driver has 3 different FIFOs to keep SKBs,
> and it's logical to move PTP FIFO free function out of TX part..
> But have no strong opinion.
>

> >> +       ptp->tx_avail = BNXT_MAX_TX_TS;
> >> +       while (cons != ptp->txts_prod) {
> >> +               txts_req = &ptp->txts_req[cons];
> >> +               if (!IS_ERR_OR_NULL(txts_req->tx_skb))
> >> +                       dev_kfree_skb_any(txts_req->tx_skb);
> >> +               cons = NEXT_TXTS(cons);
> >
> > I think we can remove the similar code we have in bnxt_ptp_clear().
> > We should always go through this path before bnxt_ptp_clear().
>
> The difference with bnxt_ptp_clear() code is that this one clears SKBs
> waiting in the queue according to consumer/producer pointers while
> bnxt_ptp_clear() iterates over all slots, a bit more on safe side.
> Should I adjust this part to check all slots before removing
> bnxt_ptp_clear() FIFO manipulations?
>
> I believe in the normal way of things there should be no need to iterate
> over all slots, but maybe you think of some conditions when we have to
> check all slots?

We generally iterate over all ring indices in the cleanup code.  For
the PTP slots, I think it is not necessary.  Any valid SKBs should be
between the consumer and producer indices.  So, either way is fine
with me.  There are only 4 slots after all.  Thanks.

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