* [PATCH net v4] bnxt_en: improve TX timestamping FIFO configuration
@ 2025-04-24 12:55 Vadim Fedorenko
2025-04-26 2:00 ` patchwork-bot+netdevbpf
2025-04-30 12:59 ` Taehee Yoo
0 siblings, 2 replies; 6+ messages in thread
From: Vadim Fedorenko @ 2025-04-24 12:55 UTC (permalink / raw)
To: Vadim Fedorenko, Michael Chan, Pavan Chebbi, Jakub Kicinski
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")
Reviewed-by: Michael Chan <michael.chan@broadcom.com>
Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
---
v3 -> v4:
* actually remove leftover unused variable in bnxt_ptp_clear()
(v3 was not committed before preparing unfortunately)
v2 -> v3:
* remove leftover unused variable in bnxt_ptp_clear()
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 | 29 ++++++++++++++-----
drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 1 +
3 files changed, 25 insertions(+), 10 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..0669d43472f5 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);
@@ -1105,7 +1126,6 @@ int bnxt_ptp_init(struct bnxt *bp)
void bnxt_ptp_clear(struct bnxt *bp)
{
struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
- int i;
if (!ptp)
return;
@@ -1117,12 +1137,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] 6+ messages in thread
* Re: [PATCH net v4] bnxt_en: improve TX timestamping FIFO configuration
2025-04-24 12:55 [PATCH net v4] bnxt_en: improve TX timestamping FIFO configuration Vadim Fedorenko
@ 2025-04-26 2:00 ` patchwork-bot+netdevbpf
2025-04-30 12:59 ` Taehee Yoo
1 sibling, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2025-04-26 2:00 UTC (permalink / raw)
To: Vadim Fedorenko
Cc: vadim.fedorenko, michael.chan, pavan.chebbi, kuba, richardcochran,
netdev
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Thu, 24 Apr 2025 05:55:47 -0700 you 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.
>
> [...]
Here is the summary with links:
- [net,v4] bnxt_en: improve TX timestamping FIFO configuration
https://git.kernel.org/netdev/net/c/8f7ae5a85137
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v4] bnxt_en: improve TX timestamping FIFO configuration
2025-04-24 12:55 [PATCH net v4] bnxt_en: improve TX timestamping FIFO configuration Vadim Fedorenko
2025-04-26 2:00 ` patchwork-bot+netdevbpf
@ 2025-04-30 12:59 ` Taehee Yoo
2025-04-30 14:55 ` Vadim Fedorenko
1 sibling, 1 reply; 6+ messages in thread
From: Taehee Yoo @ 2025-04-30 12:59 UTC (permalink / raw)
To: Vadim Fedorenko
Cc: Vadim Fedorenko, Michael Chan, Pavan Chebbi, Jakub Kicinski,
Richard Cochran, netdev
On Thu, Apr 24, 2025 at 10:11 PM Vadim Fedorenko <vadfed@meta.com> wrote:
>
Hi Vadim,
Thanks for this work!
> 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")
> Reviewed-by: Michael Chan <michael.chan@broadcom.com>
> Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
> ---
> v3 -> v4:
> * actually remove leftover unused variable in bnxt_ptp_clear()
> (v3 was not committed before preparing unfortunately)
> v2 -> v3:
> * remove leftover unused variable in bnxt_ptp_clear()
> 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 | 29 ++++++++++++++-----
> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 1 +
> 3 files changed, 25 insertions(+), 10 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..0669d43472f5 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);
> @@ -1105,7 +1126,6 @@ int bnxt_ptp_init(struct bnxt *bp)
> void bnxt_ptp_clear(struct bnxt *bp)
> {
> struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
> - int i;
>
> if (!ptp)
> return;
> @@ -1117,12 +1137,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
>
>
I’ve encountered a kernel panic that I think is related to this patch.
Could you please investigate it?
Reproducer:
ip link set $interface up
modprobe -rv bnxt_en
Splat looks like:
Oops: general protection fault, probably for non-canonical address
0xdffffc00000000fd:I
KASAN: null-ptr-deref in range [0x00000000000007e8-0x00000000000007ef]
CPU: 2 UID: 0 PID: 1963 Comm: modprobe Not tainted 6.15.0-rc3+ #5
PREEMPT(undef) 78b5b
RIP: 0010:__kthread_cancel_work_sync (/kernel/kthread.c:1476)
Code: 00 48 b8 00 00 00 00 00 fc ff df 41 57 4c 8d 7f 18 41 56 4c 89
fa 41 55 48 c1 ea4
Code starting with the faulting instruction
===========================================
0: 00 48 b8 add %cl,-0x48(%rax)
3: 00 00 add %al,(%rax)
5: 00 00 add %al,(%rax)
7: 00 fc add %bh,%ah
9: ff (bad)
a: df 41 57 filds 0x57(%rcx)
d: 4c 8d 7f 18 lea 0x18(%rdi),%r15
11: 41 56 push %r14
13: 4c 89 fa mov %r15,%rdx
16: 41 55 push %r13
18: 48 rex.W
19: c1 .byte 0xc1
1a: a4 movsb %ds:(%rsi),%es:(%rdi)
RSP: 0018:ffff888111857608 EFLAGS: 00010292
RAX: dffffc0000000000 RBX: 00000000000007d0 RCX: ffff8881330ece8e
RDX: 00000000000000fd RSI: 0000000000000001 RDI: 00000000000007d0
RBP: ffff888198ad8e00 R08: 0000000000000001 R09: ffff888198b800d8
R10: ffff888198b8019f R11: 0000000000000000 R12: ffff888198ad9008
R13: 0000000000000001 R14: ffff888198ad8e88 R15: 00000000000007e8
FS: 00007f831f921080(0000) GS:ffff888888405000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00005655646882e0 CR3: 000000014c52e000 CR4: 00000000007506f0
PKRU: 55555554
Call Trace:
<TASK>
bnxt_ptp_free_txts_skbs
(/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:807) bnxt_en
__bnxt_close_nic.constprop.0
(/drivers/net/ethernet/broadcom/bnxt/bnxt.c:3513
/drivers/net/ethernet/broadcom/bnxt/bnxt.c:3523
/drivers/net/ethernet/broadcom/bnxt/bnxt.c:12965) bnxt_en
? __lock_acquire (/kernel/locking/lockdep.c:5246)
? __pfx___bnxt_close_nic.constprop.0
(/drivers/net/ethernet/broadcom/bnxt/bnxt.c:12940) bnxt_en
bnxt_close_nic (/drivers/net/ethernet/broadcom/bnxt/bnxt.c:12980) bnxt_en
? do_raw_spin_trylock (/./arch/x86/include/asm/atomic.h:107
/./include/linux/atomic/atomic-arch-fallback.h:2170
/./include/linux/atomic/atomic-instrumented.h:1302
/./include/asm-generic/qspinlock.h:97
/kernel/locking/spinlock_debug.c:123)
? __pfx_bnxt_close_nic
(/drivers/net/ethernet/broadcom/bnxt/bnxt.c:12980) bnxt_en
? __local_bh_enable_ip (/./arch/x86/include/asm/irqflags.h:42
/./arch/x86/include/asm/irqflags.h:119 /kernel/softirq.c:412)
? lockdep_hardirqs_on (/./arch/x86/include/generated/asm/syscalls_64.h:316)
? dev_deactivate_many (/net/sched/sch_generic.c:1325
/net/sched/sch_generic.c:1383)
? __local_bh_enable_ip (/./arch/x86/include/asm/irqflags.h:42
/./arch/x86/include/asm/irqflags.h:119 /kernel/softirq.c:412)
bnxt_close (/drivers/net/ethernet/broadcom/bnxt/bnxt.c:12215
/drivers/net/ethernet/broadcom/bnxt/bnxt.c:13015) bnxt_en
? __pfx_bnxt_close (/drivers/net/ethernet/broadcom/bnxt/bnxt.c:13011) bnxt_en
? notifier_call_chain (/kernel/notifier.c:85)
__dev_close_many (/net/core/dev.c:1702)
? __pfx___dev_close_many (/net/core/dev.c:1663)
? __pfx___mutex_lock (/arch/x86/entry/syscall_32.c:46)
dev_close_many (/net/core/dev.c:1729)
? __pfx_dev_close_many (/net/core/dev.c:1719)
unregister_netdevice_many_notify (/net/core/dev.c:11946)
? rcu_is_watching (/./include/linux/context_tracking.h:128
/kernel/rcu/tree.c:736)
? __mutex_lock (/arch/x86/entry/syscall_32.c:46)
? __pfx_unregister_netdevice_many_notify (/net/core/dev.c:11909)
? rtnl_net_dev_lock (/net/core/dev.c:2093)
? __pfx___mutex_lock (/arch/x86/entry/syscall_32.c:46)
unregister_netdevice_queue (/net/core/dev.c:11891)
? __pfx_unregister_netdevice_queue (/net/core/dev.c:11880)
? rtnl_net_dev_lock (/./include/linux/rcupdate.h:331
/./include/linux/rcupdate.h:841 /net/core/dev.c:2084)
? rtnl_net_dev_lock (/net/core/dev.c:2093)
unregister_netdev (/./include/net/net_namespace.h:409
/./include/linux/netdevice.h:2708 /net/core/dev.c:2104
/net/core/dev.c:12065)
bnxt_remove_one (/drivers/net/ethernet/broadcom/bnxt/bnxt.c:16012) bnxt_en
pci_device_remove (/drivers/pci/pci-driver.c:474)
device_release_driver_internal (/drivers/base/dd.c:1275 /drivers/base/dd.c:1296)
driver_detach (/drivers/base/dd.c:1360)
bus_remove_driver (/drivers/base/bus.c:748)
pci_unregister_driver (/./include/linux/spinlock.h:351
/drivers/pci/pci-driver.c:85 /drivers/pci/pci-driver.c:1465)
bnxt_exit (/drivers/net/ethernet/broadcom/bnxt/bnxt.c:1588) bnxt_en
__do_sys_delete_module.constprop.0 (/kernel/module/main.c:781)
? __pfx___do_sys_delete_module.constprop.0 (/kernel/module/main.c:724)
? __pfx_rseq_syscall (/kernel/rseq.c:458)
? ksys_write (/fs/read_write.c:736)
? __pfx_ksys_write (/fs/read_write.c:726)
? rcu_is_watching (/./include/linux/context_tracking.h:128
/kernel/rcu/tree.c:736)
? do_syscall_64 (/./include/trace/events/initcall.h:10)
do_syscall_64 (/./include/trace/events/initcall.h:10)
entry_SYSCALL_64_after_hwframe (/./include/trace/events/initcall.h:27)
RIP: 0033:0x7f831f12ac9b
Code: 73 01 c3 48 8b 0d 7d 81 0d 00 f7 d8 64 89 01 48 83 c8 ff c3 66
2e 0f 1f 84 00 008
Code starting with the faulting instruction
===========================================
0: 73 01 jae 0x3
2: c3 ret
3: 48 8b 0d 7d 81 0d 00 mov 0xd817d(%rip),%rcx # 0xd8187
a: f7 d8 neg %eax
c: 64 89 01 mov %eax,%fs:(%rcx)
f: 48 83 c8 ff or $0xffffffffffffffff,%rax
13: c3 ret
14: 66 data16
15: 2e cs
16: 0f .byte 0xf
17: 1f (bad)
18: 84 00 test %al,(%rax)
1a: 08 .byte 0x8
RSP: 002b:00007ffdfb651448 EFLAGS: 00000206 ORIG_RAX: 00000000000000b0
RAX: ffffffffffffffda RBX: 000055fb93ae5fa0 RCX: 00007f831f12ac9b
RDX: 0000000000000000 RSI: 0000000000000800 RDI: 000055fb93ae6008
RBP: 00007ffdfb651470 R08: 0000000000000073 R09: 0000000000000000
R10: 00000000ffffffff R11: 0000000000000206 R12: 0000000000000000
R13: 00007ffdfb6514a0 R14: 0000000000000000 R15: 0000000000000000
</TASK>
Modules linked in: xt_nat xt_tcpudp veth xt_conntrack nft_chain_nat
xt_MASQUERADE nf_c]
---[ end trace 0000000000000000 ]---
RIP: 0010:__kthread_cancel_work_sync (/kernel/kthread.c:1476)
Code: 00 48 b8 00 00 00 00 00 fc ff df 41 57 4c 8d 7f 18 41 56 4c 89
fa 41 55 48 c1 ea4
Code starting with the faulting instruction
===========================================
0: 00 48 b8 add %cl,-0x48(%rax)
3: 00 00 add %al,(%rax)
5: 00 00 add %al,(%rax)
7: 00 fc add %bh,%ah
9: ff (bad)
a: df 41 57 filds 0x57(%rcx)
d: 4c 8d 7f 18 lea 0x18(%rdi),%r15
11: 41 56 push %r14
13: 4c 89 fa mov %r15,%rdx
16: 41 55 push %r13
18: 48 rex.W
19: c1 .byte 0xc1
1a: a4 movsb %ds:(%rsi),%es:(%rdi)
RSP: 0018:ffff888111857608 EFLAGS: 00010292
RAX: dffffc0000000000 RBX: 00000000000007d0 RCX: ffff8881330ece8e
RDX: 00000000000000fd RSI: 0000000000000001 RDI: 00000000000007d0
RBP: ffff888198ad8e00 R08: 0000000000000001 R09: ffff888198b800d8
R10: ffff888198b8019f R11: 0000000000000000 R12: ffff888198ad9008
R13: 0000000000000001 R14: ffff888198ad8e88 R15: 00000000000007e8
FS: 00007f831f921080(0000) GS:ffff888888405000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00005655646882e0 CR3: 000000014c52e000 CR4: 00000000007506f0
PKRU: 55555554
Kernel panic - not syncing: Fatal exception
Kernel Offset: 0x6000000 from 0xffffffff81000000 (relocation range:
0xffffffff80000000)
Thanks a lot!
Taehee Yoo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v4] bnxt_en: improve TX timestamping FIFO configuration
2025-04-30 12:59 ` Taehee Yoo
@ 2025-04-30 14:55 ` Vadim Fedorenko
2025-04-30 16:43 ` Taehee Yoo
0 siblings, 1 reply; 6+ messages in thread
From: Vadim Fedorenko @ 2025-04-30 14:55 UTC (permalink / raw)
To: Taehee Yoo
Cc: Michael Chan, Pavan Chebbi, Jakub Kicinski, Richard Cochran,
netdev
On 30/04/2025 13:59, Taehee Yoo wrote:
> On Thu, Apr 24, 2025 at 10:11 PM Vadim Fedorenko <vadfed@meta.com> wrote:
>>
>
> Hi Vadim,
> Thanks for this work!
>
>> 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")
>> Reviewed-by: Michael Chan <michael.chan@broadcom.com>
>> Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
>> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
>> ---
>> v3 -> v4:
>> * actually remove leftover unused variable in bnxt_ptp_clear()
>> (v3 was not committed before preparing unfortunately)
>> v2 -> v3:
>> * remove leftover unused variable in bnxt_ptp_clear()
>> 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 | 29 ++++++++++++++-----
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 1 +
>> 3 files changed, 25 insertions(+), 10 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..0669d43472f5 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);
>> @@ -1105,7 +1126,6 @@ int bnxt_ptp_init(struct bnxt *bp)
>> void bnxt_ptp_clear(struct bnxt *bp)
>> {
>> struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
>> - int i;
>>
>> if (!ptp)
>> return;
>> @@ -1117,12 +1137,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
>>
>>
>
> I’ve encountered a kernel panic that I think is related to this patch.
> Could you please investigate it?
>
> Reproducer:
> ip link set $interface up
> modprobe -rv bnxt_en
>
Hi Taehee!
Yeah, looks like there are some issues on the remove path.
Could you please test the diff which may fix the problem:
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 78e496b0ec26..86a5de44b6f3 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -16006,8 +16006,8 @@ static void bnxt_remove_one(struct pci_dev *pdev)
bnxt_rdma_aux_device_del(bp);
- bnxt_ptp_clear(bp);
unregister_netdev(dev);
+ bnxt_ptp_clear(bp);
bnxt_rdma_aux_device_uninit(bp);
> Splat looks like:
> Oops: general protection fault, probably for non-canonical address
> 0xdffffc00000000fd:I
> KASAN: null-ptr-deref in range [0x00000000000007e8-0x00000000000007ef]
> CPU: 2 UID: 0 PID: 1963 Comm: modprobe Not tainted 6.15.0-rc3+ #5
> PREEMPT(undef) 78b5b
> RIP: 0010:__kthread_cancel_work_sync (/kernel/kthread.c:1476)
> Code: 00 48 b8 00 00 00 00 00 fc ff df 41 57 4c 8d 7f 18 41 56 4c 89
> fa 41 55 48 c1 ea4
>
> Code starting with the faulting instruction
> ===========================================
> 0: 00 48 b8 add %cl,-0x48(%rax)
> 3: 00 00 add %al,(%rax)
> 5: 00 00 add %al,(%rax)
> 7: 00 fc add %bh,%ah
> 9: ff (bad)
> a: df 41 57 filds 0x57(%rcx)
> d: 4c 8d 7f 18 lea 0x18(%rdi),%r15
> 11: 41 56 push %r14
> 13: 4c 89 fa mov %r15,%rdx
> 16: 41 55 push %r13
> 18: 48 rex.W
> 19: c1 .byte 0xc1
> 1a: a4 movsb %ds:(%rsi),%es:(%rdi)
> RSP: 0018:ffff888111857608 EFLAGS: 00010292
> RAX: dffffc0000000000 RBX: 00000000000007d0 RCX: ffff8881330ece8e
> RDX: 00000000000000fd RSI: 0000000000000001 RDI: 00000000000007d0
> RBP: ffff888198ad8e00 R08: 0000000000000001 R09: ffff888198b800d8
> R10: ffff888198b8019f R11: 0000000000000000 R12: ffff888198ad9008
> R13: 0000000000000001 R14: ffff888198ad8e88 R15: 00000000000007e8
> FS: 00007f831f921080(0000) GS:ffff888888405000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00005655646882e0 CR3: 000000014c52e000 CR4: 00000000007506f0
> PKRU: 55555554
> Call Trace:
> <TASK>
> bnxt_ptp_free_txts_skbs
> (/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c:807) bnxt_en
> __bnxt_close_nic.constprop.0
> (/drivers/net/ethernet/broadcom/bnxt/bnxt.c:3513
> /drivers/net/ethernet/broadcom/bnxt/bnxt.c:3523
> /drivers/net/ethernet/broadcom/bnxt/bnxt.c:12965) bnxt_en
> ? __lock_acquire (/kernel/locking/lockdep.c:5246)
> ? __pfx___bnxt_close_nic.constprop.0
> (/drivers/net/ethernet/broadcom/bnxt/bnxt.c:12940) bnxt_en
> bnxt_close_nic (/drivers/net/ethernet/broadcom/bnxt/bnxt.c:12980) bnxt_en
> ? do_raw_spin_trylock (/./arch/x86/include/asm/atomic.h:107
> /./include/linux/atomic/atomic-arch-fallback.h:2170
> /./include/linux/atomic/atomic-instrumented.h:1302
> /./include/asm-generic/qspinlock.h:97
> /kernel/locking/spinlock_debug.c:123)
> ? __pfx_bnxt_close_nic
> (/drivers/net/ethernet/broadcom/bnxt/bnxt.c:12980) bnxt_en
> ? __local_bh_enable_ip (/./arch/x86/include/asm/irqflags.h:42
> /./arch/x86/include/asm/irqflags.h:119 /kernel/softirq.c:412)
> ? lockdep_hardirqs_on (/./arch/x86/include/generated/asm/syscalls_64.h:316)
> ? dev_deactivate_many (/net/sched/sch_generic.c:1325
> /net/sched/sch_generic.c:1383)
> ? __local_bh_enable_ip (/./arch/x86/include/asm/irqflags.h:42
> /./arch/x86/include/asm/irqflags.h:119 /kernel/softirq.c:412)
> bnxt_close (/drivers/net/ethernet/broadcom/bnxt/bnxt.c:12215
> /drivers/net/ethernet/broadcom/bnxt/bnxt.c:13015) bnxt_en
> ? __pfx_bnxt_close (/drivers/net/ethernet/broadcom/bnxt/bnxt.c:13011) bnxt_en
> ? notifier_call_chain (/kernel/notifier.c:85)
> __dev_close_many (/net/core/dev.c:1702)
> ? __pfx___dev_close_many (/net/core/dev.c:1663)
> ? __pfx___mutex_lock (/arch/x86/entry/syscall_32.c:46)
> dev_close_many (/net/core/dev.c:1729)
> ? __pfx_dev_close_many (/net/core/dev.c:1719)
> unregister_netdevice_many_notify (/net/core/dev.c:11946)
> ? rcu_is_watching (/./include/linux/context_tracking.h:128
> /kernel/rcu/tree.c:736)
> ? __mutex_lock (/arch/x86/entry/syscall_32.c:46)
> ? __pfx_unregister_netdevice_many_notify (/net/core/dev.c:11909)
> ? rtnl_net_dev_lock (/net/core/dev.c:2093)
> ? __pfx___mutex_lock (/arch/x86/entry/syscall_32.c:46)
> unregister_netdevice_queue (/net/core/dev.c:11891)
> ? __pfx_unregister_netdevice_queue (/net/core/dev.c:11880)
> ? rtnl_net_dev_lock (/./include/linux/rcupdate.h:331
> /./include/linux/rcupdate.h:841 /net/core/dev.c:2084)
> ? rtnl_net_dev_lock (/net/core/dev.c:2093)
> unregister_netdev (/./include/net/net_namespace.h:409
> /./include/linux/netdevice.h:2708 /net/core/dev.c:2104
> /net/core/dev.c:12065)
> bnxt_remove_one (/drivers/net/ethernet/broadcom/bnxt/bnxt.c:16012) bnxt_en
> pci_device_remove (/drivers/pci/pci-driver.c:474)
> device_release_driver_internal (/drivers/base/dd.c:1275 /drivers/base/dd.c:1296)
> driver_detach (/drivers/base/dd.c:1360)
> bus_remove_driver (/drivers/base/bus.c:748)
> pci_unregister_driver (/./include/linux/spinlock.h:351
> /drivers/pci/pci-driver.c:85 /drivers/pci/pci-driver.c:1465)
> bnxt_exit (/drivers/net/ethernet/broadcom/bnxt/bnxt.c:1588) bnxt_en
> __do_sys_delete_module.constprop.0 (/kernel/module/main.c:781)
> ? __pfx___do_sys_delete_module.constprop.0 (/kernel/module/main.c:724)
> ? __pfx_rseq_syscall (/kernel/rseq.c:458)
> ? ksys_write (/fs/read_write.c:736)
> ? __pfx_ksys_write (/fs/read_write.c:726)
> ? rcu_is_watching (/./include/linux/context_tracking.h:128
> /kernel/rcu/tree.c:736)
> ? do_syscall_64 (/./include/trace/events/initcall.h:10)
> do_syscall_64 (/./include/trace/events/initcall.h:10)
> entry_SYSCALL_64_after_hwframe (/./include/trace/events/initcall.h:27)
> RIP: 0033:0x7f831f12ac9b
> Code: 73 01 c3 48 8b 0d 7d 81 0d 00 f7 d8 64 89 01 48 83 c8 ff c3 66
> 2e 0f 1f 84 00 008
>
> Code starting with the faulting instruction
> ===========================================
> 0: 73 01 jae 0x3
> 2: c3 ret
> 3: 48 8b 0d 7d 81 0d 00 mov 0xd817d(%rip),%rcx # 0xd8187
> a: f7 d8 neg %eax
> c: 64 89 01 mov %eax,%fs:(%rcx)
> f: 48 83 c8 ff or $0xffffffffffffffff,%rax
> 13: c3 ret
> 14: 66 data16
> 15: 2e cs
> 16: 0f .byte 0xf
> 17: 1f (bad)
> 18: 84 00 test %al,(%rax)
> 1a: 08 .byte 0x8
> RSP: 002b:00007ffdfb651448 EFLAGS: 00000206 ORIG_RAX: 00000000000000b0
> RAX: ffffffffffffffda RBX: 000055fb93ae5fa0 RCX: 00007f831f12ac9b
> RDX: 0000000000000000 RSI: 0000000000000800 RDI: 000055fb93ae6008
> RBP: 00007ffdfb651470 R08: 0000000000000073 R09: 0000000000000000
> R10: 00000000ffffffff R11: 0000000000000206 R12: 0000000000000000
> R13: 00007ffdfb6514a0 R14: 0000000000000000 R15: 0000000000000000
> </TASK>
> Modules linked in: xt_nat xt_tcpudp veth xt_conntrack nft_chain_nat
> xt_MASQUERADE nf_c]
> ---[ end trace 0000000000000000 ]---
> RIP: 0010:__kthread_cancel_work_sync (/kernel/kthread.c:1476)
> Code: 00 48 b8 00 00 00 00 00 fc ff df 41 57 4c 8d 7f 18 41 56 4c 89
> fa 41 55 48 c1 ea4
>
> Code starting with the faulting instruction
> ===========================================
> 0: 00 48 b8 add %cl,-0x48(%rax)
> 3: 00 00 add %al,(%rax)
> 5: 00 00 add %al,(%rax)
> 7: 00 fc add %bh,%ah
> 9: ff (bad)
> a: df 41 57 filds 0x57(%rcx)
> d: 4c 8d 7f 18 lea 0x18(%rdi),%r15
> 11: 41 56 push %r14
> 13: 4c 89 fa mov %r15,%rdx
> 16: 41 55 push %r13
> 18: 48 rex.W
> 19: c1 .byte 0xc1
> 1a: a4 movsb %ds:(%rsi),%es:(%rdi)
> RSP: 0018:ffff888111857608 EFLAGS: 00010292
> RAX: dffffc0000000000 RBX: 00000000000007d0 RCX: ffff8881330ece8e
> RDX: 00000000000000fd RSI: 0000000000000001 RDI: 00000000000007d0
> RBP: ffff888198ad8e00 R08: 0000000000000001 R09: ffff888198b800d8
> R10: ffff888198b8019f R11: 0000000000000000 R12: ffff888198ad9008
> R13: 0000000000000001 R14: ffff888198ad8e88 R15: 00000000000007e8
> FS: 00007f831f921080(0000) GS:ffff888888405000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00005655646882e0 CR3: 000000014c52e000 CR4: 00000000007506f0
> PKRU: 55555554
> Kernel panic - not syncing: Fatal exception
> Kernel Offset: 0x6000000 from 0xffffffff81000000 (relocation range:
> 0xffffffff80000000)
>
> Thanks a lot!
> Taehee Yoo
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net v4] bnxt_en: improve TX timestamping FIFO configuration
2025-04-30 14:55 ` Vadim Fedorenko
@ 2025-04-30 16:43 ` Taehee Yoo
2025-04-30 16:47 ` Vadim Fedorenko
0 siblings, 1 reply; 6+ messages in thread
From: Taehee Yoo @ 2025-04-30 16:43 UTC (permalink / raw)
To: Vadim Fedorenko
Cc: Michael Chan, Pavan Chebbi, Jakub Kicinski, Richard Cochran,
netdev
On Wed, Apr 30, 2025 at 11:55 PM Vadim Fedorenko
<vadim.fedorenko@linux.dev> wrote:
>
> On 30/04/2025 13:59, Taehee Yoo wrote:
> > On Thu, Apr 24, 2025 at 10:11 PM Vadim Fedorenko <vadfed@meta.com> wrote:
> >>
> >
> > Hi Vadim,
> > Thanks for this work!
> >
> >> 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")
> >> Reviewed-by: Michael Chan <michael.chan@broadcom.com>
> >> Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> >> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
> >> ---
> >> v3 -> v4:
> >> * actually remove leftover unused variable in bnxt_ptp_clear()
> >> (v3 was not committed before preparing unfortunately)
> >> v2 -> v3:
> >> * remove leftover unused variable in bnxt_ptp_clear()
> >> 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 | 29 ++++++++++++++-----
> >> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 1 +
> >> 3 files changed, 25 insertions(+), 10 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..0669d43472f5 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);
> >> @@ -1105,7 +1126,6 @@ int bnxt_ptp_init(struct bnxt *bp)
> >> void bnxt_ptp_clear(struct bnxt *bp)
> >> {
> >> struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
> >> - int i;
> >>
> >> if (!ptp)
> >> return;
> >> @@ -1117,12 +1137,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
> >>
> >>
> >
> > I’ve encountered a kernel panic that I think is related to this patch.
> > Could you please investigate it?
> >
> > Reproducer:
> > ip link set $interface up
> > modprobe -rv bnxt_en
> >
>
> Hi Taehee!
>
> Yeah, looks like there are some issues on the remove path.
> Could you please test the diff which may fix the problem:
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 78e496b0ec26..86a5de44b6f3 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -16006,8 +16006,8 @@ static void bnxt_remove_one(struct pci_dev *pdev)
>
> bnxt_rdma_aux_device_del(bp);
>
> - bnxt_ptp_clear(bp);
> unregister_netdev(dev);
> + bnxt_ptp_clear(bp);
>
> bnxt_rdma_aux_device_uninit(bp);
>
Thanks! It seems to fix the issue.
The above reproducer can't reproduce a kernel panic.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v4] bnxt_en: improve TX timestamping FIFO configuration
2025-04-30 16:43 ` Taehee Yoo
@ 2025-04-30 16:47 ` Vadim Fedorenko
0 siblings, 0 replies; 6+ messages in thread
From: Vadim Fedorenko @ 2025-04-30 16:47 UTC (permalink / raw)
To: Taehee Yoo
Cc: Michael Chan, Pavan Chebbi, Jakub Kicinski, Richard Cochran,
netdev
On 30/04/2025 17:43, Taehee Yoo wrote:
> On Wed, Apr 30, 2025 at 11:55 PM Vadim Fedorenko
> <vadim.fedorenko@linux.dev> wrote:
>>
>> On 30/04/2025 13:59, Taehee Yoo wrote:
>>> On Thu, Apr 24, 2025 at 10:11 PM Vadim Fedorenko <vadfed@meta.com> wrote:
>>>>
>>>
>>> Hi Vadim,
>>> Thanks for this work!
>>>
>>>> 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")
>>>> Reviewed-by: Michael Chan <michael.chan@broadcom.com>
>>>> Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
>>>> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
>>>> ---
>>>> v3 -> v4:
>>>> * actually remove leftover unused variable in bnxt_ptp_clear()
>>>> (v3 was not committed before preparing unfortunately)
>>>> v2 -> v3:
>>>> * remove leftover unused variable in bnxt_ptp_clear()
>>>> 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 | 29 ++++++++++++++-----
>>>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 1 +
>>>> 3 files changed, 25 insertions(+), 10 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..0669d43472f5 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);
>>>> @@ -1105,7 +1126,6 @@ int bnxt_ptp_init(struct bnxt *bp)
>>>> void bnxt_ptp_clear(struct bnxt *bp)
>>>> {
>>>> struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
>>>> - int i;
>>>>
>>>> if (!ptp)
>>>> return;
>>>> @@ -1117,12 +1137,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
>>>>
>>>>
>>>
>>> I’ve encountered a kernel panic that I think is related to this patch.
>>> Could you please investigate it?
>>>
>>> Reproducer:
>>> ip link set $interface up
>>> modprobe -rv bnxt_en
>>>
>>
>> Hi Taehee!
>>
>> Yeah, looks like there are some issues on the remove path.
>> Could you please test the diff which may fix the problem:
>>
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> index 78e496b0ec26..86a5de44b6f3 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> @@ -16006,8 +16006,8 @@ static void bnxt_remove_one(struct pci_dev *pdev)
>>
>> bnxt_rdma_aux_device_del(bp);
>>
>> - bnxt_ptp_clear(bp);
>> unregister_netdev(dev);
>> + bnxt_ptp_clear(bp);
>>
>> bnxt_rdma_aux_device_uninit(bp);
>>
>
> Thanks! It seems to fix the issue.
> The above reproducer can't reproduce a kernel panic.
Great, thanks, I'll post a proper patch soon
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-04-30 16:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-24 12:55 [PATCH net v4] bnxt_en: improve TX timestamping FIFO configuration Vadim Fedorenko
2025-04-26 2:00 ` patchwork-bot+netdevbpf
2025-04-30 12:59 ` Taehee Yoo
2025-04-30 14:55 ` Vadim Fedorenko
2025-04-30 16:43 ` Taehee Yoo
2025-04-30 16:47 ` Vadim Fedorenko
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).