* [PATCH net v2] bnxt_en: improve TX timestamping FIFO configuration
@ 2025-04-17 16:01 Vadim Fedorenko
2025-04-17 17:01 ` Michael Chan
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Vadim Fedorenko @ 2025-04-17 16:01 UTC (permalink / raw)
To: Michael Chan, Pavan Chebbi, Jakub Kicinski, Vadim Fedorenko
Cc: Richard Cochran, netdev
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>
---
v1 -> v2:
* move clearing of TS skbs to bnxt_free_tx_skbs
* remove spinlock as no TX is possible after bnxt_tx_disable()
* remove extra FIFO clearing in bnxt_ptp_clear()
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 5 ++--
drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 28 ++++++++++++++-----
drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 1 +
3 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index c8e3468eee61..2c8e2c19d854 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -3414,6 +3414,9 @@ static void bnxt_free_tx_skbs(struct bnxt *bp)
bnxt_free_one_tx_ring_skbs(bp, txr, i);
}
+
+ if (bp->ptp_cfg && !(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
+ bnxt_ptp_free_txts_skbs(bp->ptp_cfg);
}
static void bnxt_free_one_rx_ring(struct bnxt *bp, struct bnxt_rx_ring_info *rxr)
@@ -12797,8 +12800,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..197893220070 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
@@ -794,6 +794,27 @@ 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);
+
+ 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;
+ 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);
@@ -1117,12 +1138,5 @@ void bnxt_ptp_clear(struct bnxt *bp)
kfree(ptp->ptp_info.pin_config);
ptp->ptp_info.pin_config = 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 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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net v2] bnxt_en: improve TX timestamping FIFO configuration
2025-04-17 16:01 [PATCH net v2] bnxt_en: improve TX timestamping FIFO configuration Vadim Fedorenko
@ 2025-04-17 17:01 ` Michael Chan
2025-04-21 9:38 ` Pavan Chebbi
2025-04-23 1:51 ` Jakub Kicinski
2 siblings, 0 replies; 5+ messages in thread
From: Michael Chan @ 2025-04-17 17:01 UTC (permalink / raw)
To: Vadim Fedorenko
Cc: Pavan Chebbi, Jakub Kicinski, Vadim Fedorenko, Richard Cochran,
netdev
[-- Attachment #1: Type: text/plain, Size: 961 bytes --]
On Thu, Apr 17, 2025 at 9: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>
> ---
> v1 -> v2:
> * move clearing of TS skbs to bnxt_free_tx_skbs
> * remove spinlock as no TX is possible after bnxt_tx_disable()
> * remove extra FIFO clearing in bnxt_ptp_clear()
> ---
Thanks.
Reviewed-by: Michael Chan <michael.chan@broadcom.com>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4196 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net v2] bnxt_en: improve TX timestamping FIFO configuration
2025-04-17 16:01 [PATCH net v2] bnxt_en: improve TX timestamping FIFO configuration Vadim Fedorenko
2025-04-17 17:01 ` Michael Chan
@ 2025-04-21 9:38 ` Pavan Chebbi
2025-04-21 17:35 ` Michael Chan
2025-04-23 1:51 ` Jakub Kicinski
2 siblings, 1 reply; 5+ messages in thread
From: Pavan Chebbi @ 2025-04-21 9:38 UTC (permalink / raw)
To: Vadim Fedorenko
Cc: Michael Chan, Jakub Kicinski, Vadim Fedorenko, Richard Cochran,
netdev
[-- Attachment #1: Type: text/plain, Size: 3532 bytes --]
On Thu, Apr 17, 2025 at 9:31 PM 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>
> ---
> v1 -> v2:
> * move clearing of TS skbs to bnxt_free_tx_skbs
> * remove spinlock as no TX is possible after bnxt_tx_disable()
> * remove extra FIFO clearing in bnxt_ptp_clear()
> ---
> drivers/net/ethernet/broadcom/bnxt/bnxt.c | 5 ++--
> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 28 ++++++++++++++-----
> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 1 +
> 3 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index c8e3468eee61..2c8e2c19d854 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -3414,6 +3414,9 @@ static void bnxt_free_tx_skbs(struct bnxt *bp)
>
> bnxt_free_one_tx_ring_skbs(bp, txr, i);
> }
> +
> + if (bp->ptp_cfg && !(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
> + bnxt_ptp_free_txts_skbs(bp->ptp_cfg);
> }
>
> static void bnxt_free_one_rx_ring(struct bnxt *bp, struct bnxt_rx_ring_info *rxr)
> @@ -12797,8 +12800,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..197893220070 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> @@ -794,6 +794,27 @@ 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);
> +
> + 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);
For completeness, should we not set txts_req->tx_skb = NULL here, just
like we did in bnxt_ptp_clear which is now gone.
> + cons = NEXT_TXTS(cons);
> + }
> + ptp->txts_cons = cons;
> + ptp_schedule_worker(ptp->ptp_clock, 0);
> +}
> +
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4196 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net v2] bnxt_en: improve TX timestamping FIFO configuration
2025-04-21 9:38 ` Pavan Chebbi
@ 2025-04-21 17:35 ` Michael Chan
0 siblings, 0 replies; 5+ messages in thread
From: Michael Chan @ 2025-04-21 17:35 UTC (permalink / raw)
To: Pavan Chebbi
Cc: Vadim Fedorenko, Jakub Kicinski, Vadim Fedorenko, Richard Cochran,
netdev
On Mon, Apr 21, 2025 at 2:39 AM Pavan Chebbi <pavan.chebbi@broadcom.com> wrote:
>
> On Thu, Apr 17, 2025 at 9:31 PM Vadim Fedorenko <vadfed@meta.com> wrote:
> >
> > +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);
> > +
> > + 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);
>
> For completeness, should we not set txts_req->tx_skb = NULL here, just
> like we did in bnxt_ptp_clear which is now gone.
I agree it is better to set it to NULL so we don't keep a dangling
pointer in the array. But I think it is not strictly necessary
because only the entries between cons and prod are valid. This loop
will advance cons to prod.
>
> > + cons = NEXT_TXTS(cons);
> > + }
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net v2] bnxt_en: improve TX timestamping FIFO configuration
2025-04-17 16:01 [PATCH net v2] bnxt_en: improve TX timestamping FIFO configuration Vadim Fedorenko
2025-04-17 17:01 ` Michael Chan
2025-04-21 9:38 ` Pavan Chebbi
@ 2025-04-23 1:51 ` Jakub Kicinski
2 siblings, 0 replies; 5+ messages in thread
From: Jakub Kicinski @ 2025-04-23 1:51 UTC (permalink / raw)
To: Vadim Fedorenko
Cc: Michael Chan, Pavan Chebbi, Vadim Fedorenko, Richard Cochran,
netdev
On Thu, 17 Apr 2025 09:01:41 -0700 Vadim Fedorenko wrote:
> @@ -1117,12 +1138,5 @@ void bnxt_ptp_clear(struct bnxt *bp)
> kfree(ptp->ptp_info.pin_config);
> ptp->ptp_info.pin_config = 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);
drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:1129:6: warning: unused variable 'i' [-Wunused-variable]
1129 | int i;
| ^
--
pw-bot: cr
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-04-23 1:51 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-17 16:01 [PATCH net v2] bnxt_en: improve TX timestamping FIFO configuration Vadim Fedorenko
2025-04-17 17:01 ` Michael Chan
2025-04-21 9:38 ` Pavan Chebbi
2025-04-21 17:35 ` Michael Chan
2025-04-23 1:51 ` Jakub Kicinski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).