| Message ID | 20241030082117.1172634-1-wei.fang@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: enetc: prevent VF from configuring preemptiable TCs | expand |
802.1Q spells 'preemptible' using 'i', 802.3 spells it using 'a', you decided to spell it using both :) FWIW, the kernel uses 'preemptible' because 802.1Q is the authoritative standard for this feature, 802.3 just references it. On Wed, Oct 30, 2024 at 04:21:17PM +0800, Wei Fang wrote: > Both ENETC PF and VF drivers share enetc_setup_tc_mqprio() to configure > MQPRIO. And enetc_setup_tc_mqprio() calls enetc_change_preemptible_tcs() > to configure preemptible TCs. However, only PF is able to configure > preemptible TCs. Because only PF has related registers, while VF does not > have these registers. So for VF, its hw->port pointer is NULL. Therefore, > VF will access an invalid pointer when accessing a non-existent register, > which will cause a call trace issue. The call trace log is shown below. > > root@ls1028ardb:~# tc qdisc add dev eno0vf0 parent root handle 100: \ > mqprio num_tc 4 map 0 0 1 1 2 2 3 3 queues 1@0 1@1 1@2 1@3 hw 1 > [ 187.290775] Unable to handle kernel paging request at virtual address 0000000000001f00 > [ 187.298760] Mem abort info: > [ 187.301607] ESR = 0x0000000096000004 > [ 187.305373] EC = 0x25: DABT (current EL), IL = 32 bits > [ 187.310714] SET = 0, FnV = 0 > [ 187.313778] EA = 0, S1PTW = 0 > [ 187.316923] FSC = 0x04: level 0 translation fault > [ 187.321818] Data abort info: > [ 187.324701] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000 > [ 187.330207] CM = 0, WnR = 0, TnD = 0, TagAccess = 0 > [ 187.335278] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 > [ 187.340610] user pgtable: 4k pages, 48-bit VAs, pgdp=0000002084b71000 > [ 187.347076] [0000000000001f00] pgd=0000000000000000, p4d=0000000000000000 > [ 187.353900] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP > [ 187.360188] Modules linked in: xt_conntrack xt_addrtype cfg80211 rfkill snd_soc_hdmi_codec fsl_jr_uio caam_jr caamkeyblob_desc caamhash_desc caamalg_descp > [ 187.406320] CPU: 0 PID: 1117 Comm: tc Not tainted 6.6.52-gfbb48d8e2ddb #1 > [ 187.413131] Hardware name: LS1028A RDB Board (DT) > [ 187.417846] pstate: 40000005 (nZcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 187.424831] pc : enetc_mm_commit_preemptible_tcs+0x1c4/0x400 > [ 187.430518] lr : enetc_mm_commit_preemptible_tcs+0x30c/0x400 > [ 187.436195] sp : ffff800084a4b400 > [ 187.439515] x29: ffff800084a4b400 x28: 0000000000000004 x27: ffff6a58c5229808 > [ 187.446679] x26: 0000000080000203 x25: ffff800085218600 x24: 0000000000000004 > [ 187.453842] x23: ffff6a58c42e4a00 x22: ffff6a58c5229808 x21: ffff6a58c42e4980 > [ 187.461004] x20: ffff6a58c5229800 x19: 0000000000001f00 x18: 0000000000000001 > [ 187.468167] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 > [ 187.475328] x14: 00000000000003f8 x13: 0000000000000400 x12: 0000000000065feb > [ 187.482491] x11: 0000000000000000 x10: ffff6a593f6f9918 x9 : 0000000000000000 > [ 187.489653] x8 : ffff800084a4b668 x7 : 0000000000000003 x6 : 0000000000000001 > [ 187.496815] x5 : 0000000000001f00 x4 : 0000000000000000 x3 : 0000000000000000 > [ 187.503977] x2 : 0000000000000000 x1 : 0000000000000200 x0 : ffffd2fbd8497460 > [ 187.511140] Call trace: > [ 187.513588] enetc_mm_commit_preemptible_tcs+0x1c4/0x400 > [ 187.518918] enetc_setup_tc_mqprio+0x180/0x214 > [ 187.523374] enetc_vf_setup_tc+0x1c/0x30 > [ 187.527306] mqprio_enable_offload+0x144/0x178 > [ 187.531766] mqprio_init+0x3ec/0x668 > [ 187.535351] qdisc_create+0x15c/0x488 > [ 187.539023] tc_modify_qdisc+0x398/0x73c > [ 187.542958] rtnetlink_rcv_msg+0x128/0x378 > [ 187.547064] netlink_rcv_skb+0x60/0x130 > [ 187.550910] rtnetlink_rcv+0x18/0x24 > [ 187.554492] netlink_unicast+0x300/0x36c > [ 187.558425] netlink_sendmsg+0x1a8/0x420 > [ 187.562358] ____sys_sendmsg+0x214/0x26c > [ 187.566292] ___sys_sendmsg+0xb0/0x108 > [ 187.570050] __sys_sendmsg+0x88/0xe8 > [ 187.573634] __arm64_sys_sendmsg+0x24/0x30 > [ 187.577742] invoke_syscall+0x48/0x114 > [ 187.581503] el0_svc_common.constprop.0+0x40/0xe0 > [ 187.586222] do_el0_svc+0x1c/0x28 > [ 187.589544] el0_svc+0x40/0xe4 > [ 187.592607] el0t_64_sync_handler+0x120/0x12c > [ 187.596976] el0t_64_sync+0x190/0x194 > [ 187.600648] Code: d65f03c0 d283e005 8b050273 14000050 (b9400273) > [ 187.606759] ---[ end trace 0000000000000000 ]--- Please be more succint with the stack trace. Nobody cares about more than this: Unable to handle kernel paging request at virtual address 0000000000001f00 Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP Hardware name: LS1028A RDB Board (DT) pc : enetc_mm_commit_preemptible_tcs+0x1c4/0x400 lr : enetc_mm_commit_preemptible_tcs+0x30c/0x400 Call trace: enetc_mm_commit_preemptible_tcs+0x1c4/0x400 enetc_setup_tc_mqprio+0x180/0x214 enetc_vf_setup_tc+0x1c/0x30 mqprio_enable_offload+0x144/0x178 mqprio_init+0x3ec/0x668 qdisc_create+0x15c/0x488 tc_modify_qdisc+0x398/0x73c rtnetlink_rcv_msg+0x128/0x378 netlink_rcv_skb+0x60/0x130 > > Fixes: 827145392a4a ("net: enetc: only commit preemptible TCs to hardware when MM TX is active") > Signed-off-by: Wei Fang <wei.fang@nxp.com> > --- > drivers/net/ethernet/freescale/enetc/enetc.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c > index c09370eab319..c9f7b7b4445f 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > @@ -28,6 +28,9 @@ EXPORT_SYMBOL_GPL(enetc_port_mac_wr); > static void enetc_change_preemptible_tcs(struct enetc_ndev_priv *priv, > u8 preemptible_tcs) > { > + if (!enetc_si_is_pf(priv->si)) > + return; > + Actually please do this instead: if (!(si->hw_features & ENETC_SI_F_QBU)) We also shouldn't do anything here for those PFs which do not support frame preemption (eno1, eno3 on LS1028A). It won't crash like it does for VFs, but we should still avoid accessing registers which aren't implemented. The ethtool mm ops are protected using this test, but I didn't realize that tc has its own separate entry point which needs it too. > priv->preemptible_tcs = preemptible_tcs; > enetc_mm_commit_preemptible_tcs(priv); > } > -- > 2.34.1 >
> 802.1Q spells 'preemptible' using 'i', 802.3 spells it using 'a', you > decided to spell it using both :) > My bad, I'll fix the typo > FWIW, the kernel uses 'preemptible' because 802.1Q is the authoritative > standard for this feature, 802.3 just references it. > > On Wed, Oct 30, 2024 at 04:21:17PM +0800, Wei Fang wrote: > > Both ENETC PF and VF drivers share enetc_setup_tc_mqprio() to configure > > MQPRIO. And enetc_setup_tc_mqprio() calls > enetc_change_preemptible_tcs() > > to configure preemptible TCs. However, only PF is able to configure > > preemptible TCs. Because only PF has related registers, while VF does not > > have these registers. So for VF, its hw->port pointer is NULL. Therefore, > > VF will access an invalid pointer when accessing a non-existent register, > > which will cause a call trace issue. The call trace log is shown below. > > > > root@ls1028ardb:~# tc qdisc add dev eno0vf0 parent root handle 100: \ > > mqprio num_tc 4 map 0 0 1 1 2 2 3 3 queues 1@0 1@1 1@2 1@3 hw 1 > > [ 187.290775] Unable to handle kernel paging request at virtual address > 0000000000001f00 > > [ 187.298760] Mem abort info: > > [ 187.301607] ESR = 0x0000000096000004 > > [ 187.305373] EC = 0x25: DABT (current EL), IL = 32 bits > > [ 187.310714] SET = 0, FnV = 0 > > [ 187.313778] EA = 0, S1PTW = 0 > > [ 187.316923] FSC = 0x04: level 0 translation fault > > [ 187.321818] Data abort info: > > [ 187.324701] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000 > > [ 187.330207] CM = 0, WnR = 0, TnD = 0, TagAccess = 0 > > [ 187.335278] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 > > [ 187.340610] user pgtable: 4k pages, 48-bit VAs, > pgdp=0000002084b71000 > > [ 187.347076] [0000000000001f00] pgd=0000000000000000, > p4d=0000000000000000 > > [ 187.353900] Internal error: Oops: 0000000096000004 [#1] PREEMPT > SMP > > [ 187.360188] Modules linked in: xt_conntrack xt_addrtype cfg80211 rfkill > snd_soc_hdmi_codec fsl_jr_uio caam_jr caamkeyblob_desc caamhash_desc > caamalg_descp > > [ 187.406320] CPU: 0 PID: 1117 Comm: tc Not tainted > 6.6.52-gfbb48d8e2ddb #1 > > [ 187.413131] Hardware name: LS1028A RDB Board (DT) > > [ 187.417846] pstate: 40000005 (nZcv daif -PAN -UAO -TCO -DIT -SSBS > BTYPE=--) > > [ 187.424831] pc : enetc_mm_commit_preemptible_tcs+0x1c4/0x400 > > [ 187.430518] lr : enetc_mm_commit_preemptible_tcs+0x30c/0x400 > > [ 187.436195] sp : ffff800084a4b400 > > [ 187.439515] x29: ffff800084a4b400 x28: 0000000000000004 x27: > ffff6a58c5229808 > > [ 187.446679] x26: 0000000080000203 x25: ffff800085218600 x24: > 0000000000000004 > > [ 187.453842] x23: ffff6a58c42e4a00 x22: ffff6a58c5229808 x21: > ffff6a58c42e4980 > > [ 187.461004] x20: ffff6a58c5229800 x19: 0000000000001f00 x18: > 0000000000000001 > > [ 187.468167] x17: 0000000000000000 x16: 0000000000000000 x15: > 0000000000000000 > > [ 187.475328] x14: 00000000000003f8 x13: 0000000000000400 x12: > 0000000000065feb > > [ 187.482491] x11: 0000000000000000 x10: ffff6a593f6f9918 x9 : > 0000000000000000 > > [ 187.489653] x8 : ffff800084a4b668 x7 : 0000000000000003 x6 : > 0000000000000001 > > [ 187.496815] x5 : 0000000000001f00 x4 : 0000000000000000 x3 : > 0000000000000000 > > [ 187.503977] x2 : 0000000000000000 x1 : 0000000000000200 x0 : > ffffd2fbd8497460 > > [ 187.511140] Call trace: > > [ 187.513588] enetc_mm_commit_preemptible_tcs+0x1c4/0x400 > > [ 187.518918] enetc_setup_tc_mqprio+0x180/0x214 > > [ 187.523374] enetc_vf_setup_tc+0x1c/0x30 > > [ 187.527306] mqprio_enable_offload+0x144/0x178 > > [ 187.531766] mqprio_init+0x3ec/0x668 > > [ 187.535351] qdisc_create+0x15c/0x488 > > [ 187.539023] tc_modify_qdisc+0x398/0x73c > > [ 187.542958] rtnetlink_rcv_msg+0x128/0x378 > > [ 187.547064] netlink_rcv_skb+0x60/0x130 > > [ 187.550910] rtnetlink_rcv+0x18/0x24 > > [ 187.554492] netlink_unicast+0x300/0x36c > > [ 187.558425] netlink_sendmsg+0x1a8/0x420 > > [ 187.562358] ____sys_sendmsg+0x214/0x26c > > [ 187.566292] ___sys_sendmsg+0xb0/0x108 > > [ 187.570050] __sys_sendmsg+0x88/0xe8 > > [ 187.573634] __arm64_sys_sendmsg+0x24/0x30 > > [ 187.577742] invoke_syscall+0x48/0x114 > > [ 187.581503] el0_svc_common.constprop.0+0x40/0xe0 > > [ 187.586222] do_el0_svc+0x1c/0x28 > > [ 187.589544] el0_svc+0x40/0xe4 > > [ 187.592607] el0t_64_sync_handler+0x120/0x12c > > [ 187.596976] el0t_64_sync+0x190/0x194 > > [ 187.600648] Code: d65f03c0 d283e005 8b050273 14000050 > (b9400273) > > [ 187.606759] ---[ end trace 0000000000000000 ]--- > > Please be more succint with the stack trace. Nobody cares about more > than this: Okay, thanks > > Unable to handle kernel paging request at virtual address 0000000000001f00 > Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP > Hardware name: LS1028A RDB Board (DT) > pc : enetc_mm_commit_preemptible_tcs+0x1c4/0x400 > lr : enetc_mm_commit_preemptible_tcs+0x30c/0x400 > Call trace: > enetc_mm_commit_preemptible_tcs+0x1c4/0x400 > enetc_setup_tc_mqprio+0x180/0x214 > enetc_vf_setup_tc+0x1c/0x30 > mqprio_enable_offload+0x144/0x178 > mqprio_init+0x3ec/0x668 > qdisc_create+0x15c/0x488 > tc_modify_qdisc+0x398/0x73c > rtnetlink_rcv_msg+0x128/0x378 > netlink_rcv_skb+0x60/0x130 > > > > > Fixes: 827145392a4a ("net: enetc: only commit preemptible TCs to > hardware when MM TX is active") > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > --- > > drivers/net/ethernet/freescale/enetc/enetc.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c > b/drivers/net/ethernet/freescale/enetc/enetc.c > > index c09370eab319..c9f7b7b4445f 100644 > > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > > @@ -28,6 +28,9 @@ EXPORT_SYMBOL_GPL(enetc_port_mac_wr); > > static void enetc_change_preemptible_tcs(struct enetc_ndev_priv *priv, > > u8 preemptible_tcs) > > { > > + if (!enetc_si_is_pf(priv->si)) > > + return; > > + > > Actually please do this instead: > > if (!(si->hw_features & ENETC_SI_F_QBU)) > > We also shouldn't do anything here for those PFs which do not support frame > preemption (eno1, eno3 on LS1028A). It won't crash like it does for VFs, > but we should still avoid accessing registers which aren't implemented. > The ethtool mm ops are protected using this test, but I didn't realize > that tc has its own separate entry point which needs it too. > Yeah, I agree with you, I will use your solution, thanks. > > priv->preemptible_tcs = preemptible_tcs; > > enetc_mm_commit_preemptible_tcs(priv); > > } > > -- > > 2.34.1 > >
> > 802.1Q spells 'preemptible' using 'i', 802.3 spells it using 'a', you > > decided to spell it using both :) > > > > My bad, I'll fix the typo > > > FWIW, the kernel uses 'preemptible' because 802.1Q is the > > authoritative standard for this feature, 802.3 just references it. > > > > On Wed, Oct 30, 2024 at 04:21:17PM +0800, Wei Fang wrote: > > > Both ENETC PF and VF drivers share enetc_setup_tc_mqprio() to > > > configure MQPRIO. And enetc_setup_tc_mqprio() calls > > enetc_change_preemptible_tcs() > > > to configure preemptible TCs. However, only PF is able to configure > > > preemptible TCs. Because only PF has related registers, while VF > > > does not have these registers. So for VF, its hw->port pointer is > > > NULL. Therefore, VF will access an invalid pointer when accessing a > > > non-existent register, which will cause a call trace issue. The call trace log is > shown below. > > > > > > root@ls1028ardb:~# tc qdisc add dev eno0vf0 parent root handle 100: > > > \ mqprio num_tc 4 map 0 0 1 1 2 2 3 3 queues 1@0 1@1 1@2 1@3 hw 1 [ > > > 187.290775] Unable to handle kernel paging request at virtual > > > address > > 0000000000001f00 > > > [ 187.298760] Mem abort info: > > > [ 187.301607] ESR = 0x0000000096000004 > > > [ 187.305373] EC = 0x25: DABT (current EL), IL = 32 bits > > > [ 187.310714] SET = 0, FnV = 0 > > > [ 187.313778] EA = 0, S1PTW = 0 > > > [ 187.316923] FSC = 0x04: level 0 translation fault > > > [ 187.321818] Data abort info: > > > [ 187.324701] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000 > > > [ 187.330207] CM = 0, WnR = 0, TnD = 0, TagAccess = 0 > > > [ 187.335278] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 > > > [ 187.340610] user pgtable: 4k pages, 48-bit VAs, > > pgdp=0000002084b71000 > > > [ 187.347076] [0000000000001f00] pgd=0000000000000000, > > p4d=0000000000000000 > > > [ 187.353900] Internal error: Oops: 0000000096000004 [#1] PREEMPT > > SMP > > > [ 187.360188] Modules linked in: xt_conntrack xt_addrtype cfg80211 > > > rfkill > > snd_soc_hdmi_codec fsl_jr_uio caam_jr caamkeyblob_desc caamhash_desc > > caamalg_descp > > > [ 187.406320] CPU: 0 PID: 1117 Comm: tc Not tainted > > 6.6.52-gfbb48d8e2ddb #1 > > > [ 187.413131] Hardware name: LS1028A RDB Board (DT) [ 187.417846] > > > pstate: 40000005 (nZcv daif -PAN -UAO -TCO -DIT -SSBS > > BTYPE=--) > > > [ 187.424831] pc : enetc_mm_commit_preemptible_tcs+0x1c4/0x400 > > > [ 187.430518] lr : enetc_mm_commit_preemptible_tcs+0x30c/0x400 > > > [ 187.436195] sp : ffff800084a4b400 [ 187.439515] x29: > > > ffff800084a4b400 x28: 0000000000000004 x27: > > ffff6a58c5229808 > > > [ 187.446679] x26: 0000000080000203 x25: ffff800085218600 x24: > > 0000000000000004 > > > [ 187.453842] x23: ffff6a58c42e4a00 x22: ffff6a58c5229808 x21: > > ffff6a58c42e4980 > > > [ 187.461004] x20: ffff6a58c5229800 x19: 0000000000001f00 x18: > > 0000000000000001 > > > [ 187.468167] x17: 0000000000000000 x16: 0000000000000000 x15: > > 0000000000000000 > > > [ 187.475328] x14: 00000000000003f8 x13: 0000000000000400 x12: > > 0000000000065feb > > > [ 187.482491] x11: 0000000000000000 x10: ffff6a593f6f9918 x9 : > > 0000000000000000 > > > [ 187.489653] x8 : ffff800084a4b668 x7 : 0000000000000003 x6 : > > 0000000000000001 > > > [ 187.496815] x5 : 0000000000001f00 x4 : 0000000000000000 x3 : > > 0000000000000000 > > > [ 187.503977] x2 : 0000000000000000 x1 : 0000000000000200 x0 : > > ffffd2fbd8497460 > > > [ 187.511140] Call trace: > > > [ 187.513588] enetc_mm_commit_preemptible_tcs+0x1c4/0x400 > > > [ 187.518918] enetc_setup_tc_mqprio+0x180/0x214 [ 187.523374] > > > enetc_vf_setup_tc+0x1c/0x30 [ 187.527306] > > > mqprio_enable_offload+0x144/0x178 [ 187.531766] > > > mqprio_init+0x3ec/0x668 [ 187.535351] qdisc_create+0x15c/0x488 [ > > > 187.539023] tc_modify_qdisc+0x398/0x73c [ 187.542958] > > > rtnetlink_rcv_msg+0x128/0x378 [ 187.547064] > > > netlink_rcv_skb+0x60/0x130 [ 187.550910] rtnetlink_rcv+0x18/0x24 [ > > > 187.554492] netlink_unicast+0x300/0x36c [ 187.558425] > > > netlink_sendmsg+0x1a8/0x420 [ 187.562358] > > > ____sys_sendmsg+0x214/0x26c [ 187.566292] > > > ___sys_sendmsg+0xb0/0x108 [ 187.570050] __sys_sendmsg+0x88/0xe8 > [ > > > 187.573634] __arm64_sys_sendmsg+0x24/0x30 [ 187.577742] > > > invoke_syscall+0x48/0x114 [ 187.581503] > > > el0_svc_common.constprop.0+0x40/0xe0 > > > [ 187.586222] do_el0_svc+0x1c/0x28 [ 187.589544] > > > el0_svc+0x40/0xe4 [ 187.592607] el0t_64_sync_handler+0x120/0x12c [ > > > 187.596976] el0t_64_sync+0x190/0x194 [ 187.600648] Code: d65f03c0 > > > d283e005 8b050273 14000050 > > (b9400273) > > > [ 187.606759] ---[ end trace 0000000000000000 ]--- > > > > Please be more succint with the stack trace. Nobody cares about more > > than this: > > Okay, thanks > > > > > Unable to handle kernel paging request at virtual address > > 0000000000001f00 Internal error: Oops: 0000000096000004 [#1] PREEMPT > > SMP Hardware name: LS1028A RDB Board (DT) pc : > > enetc_mm_commit_preemptible_tcs+0x1c4/0x400 > > lr : enetc_mm_commit_preemptible_tcs+0x30c/0x400 > > Call trace: > > enetc_mm_commit_preemptible_tcs+0x1c4/0x400 > > enetc_setup_tc_mqprio+0x180/0x214 > > enetc_vf_setup_tc+0x1c/0x30 > > mqprio_enable_offload+0x144/0x178 > > mqprio_init+0x3ec/0x668 > > qdisc_create+0x15c/0x488 > > tc_modify_qdisc+0x398/0x73c > > rtnetlink_rcv_msg+0x128/0x378 > > netlink_rcv_skb+0x60/0x130 > > > > > > > > Fixes: 827145392a4a ("net: enetc: only commit preemptible TCs to > > hardware when MM TX is active") > > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > > --- > > > drivers/net/ethernet/freescale/enetc/enetc.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c > > b/drivers/net/ethernet/freescale/enetc/enetc.c > > > index c09370eab319..c9f7b7b4445f 100644 > > > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > > > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > > > @@ -28,6 +28,9 @@ EXPORT_SYMBOL_GPL(enetc_port_mac_wr); > > > static void enetc_change_preemptible_tcs(struct enetc_ndev_priv *priv, > > > u8 preemptible_tcs) > > > { > > > + if (!enetc_si_is_pf(priv->si)) > > > + return; > > > + > > > > Actually please do this instead: > > > > if (!(si->hw_features & ENETC_SI_F_QBU)) > > Actually, VFs of eno0 have ENETC_SI_F_QBU bit set. So we should use the following check instead. if (!enetc_si_is_pf(si) || !(si->hw_features & ENETC_SI_F_QBU)) Or we only set ENETC_SI_F_QBU bit for PF in enetc_get_si_caps() if the PF supports 802.1 Qbu. > > We also shouldn't do anything here for those PFs which do not support > > frame preemption (eno1, eno3 on LS1028A). It won't crash like it does > > for VFs, but we should still avoid accessing registers which aren't implemented. > > The ethtool mm ops are protected using this test, but I didn't realize > > that tc has its own separate entry point which needs it too. > > > > Yeah, I agree with you, I will use your solution, thanks. > > > > priv->preemptible_tcs = preemptible_tcs; > > > enetc_mm_commit_preemptible_tcs(priv); > > > } > > > -- > > > 2.34.1 > > >
On Thu, Oct 31, 2024 at 05:46:47AM +0200, Wei Fang wrote: > > > Actually please do this instead: > > > > > > if (!(si->hw_features & ENETC_SI_F_QBU)) > > > > > Actually, VFs of eno0 have ENETC_SI_F_QBU bit set. So we should use the > following check instead. > > if (!enetc_si_is_pf(si) || !(si->hw_features & ENETC_SI_F_QBU)) > > Or we only set ENETC_SI_F_QBU bit for PF in enetc_get_si_caps() if the PF > supports 802.1 Qbu. This one is weird. I don't know why the ENETC would push a capability in the SI port capability register 0 for the VSI, if the VSI doesn't have access to the port registers in the first place. Let me ask internally, so we could figure out what's the best thing to do.
On Fri, Nov 01, 2024 at 01:55:19PM +0200, Vladimir Oltean wrote: > On Thu, Oct 31, 2024 at 05:46:47AM +0200, Wei Fang wrote: > > > > Actually please do this instead: > > > > > > > > if (!(si->hw_features & ENETC_SI_F_QBU)) > > > > > > > > Actually, VFs of eno0 have ENETC_SI_F_QBU bit set. So we should use the > > following check instead. > > > > if (!enetc_si_is_pf(si) || !(si->hw_features & ENETC_SI_F_QBU)) > > > > Or we only set ENETC_SI_F_QBU bit for PF in enetc_get_si_caps() if the PF > > supports 802.1 Qbu. > > This one is weird. I don't know why the ENETC would push a capability in > the SI port capability register 0 for the VSI, if the VSI doesn't have > access to the port registers in the first place. Let me ask internally, > so we could figure out what's the best thing to do. Let's mask the ENETC_SI_F_QBU feature for VSIs in enetc_get_si_caps(). Though we should do the same with ENETC_SIPCAPR0_QBV and ENETC_SIPCAPR0_PSFP.
> -----Original Message----- > From: Vladimir Oltean <vladimir.oltean@nxp.com> > Sent: 2024年11月1日 22:03 > To: Wei Fang <wei.fang@nxp.com> > Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com; andrew+netdev@lunn.ch; Claudiu Manoil > <claudiu.manoil@nxp.com>; netdev@vger.kernel.org; > linux-kernel@vger.kernel.org; imx@lists.linux.dev > Subject: Re: [PATCH net] net: enetc: prevent VF from configuring preemptiable > TCs > > On Fri, Nov 01, 2024 at 01:55:19PM +0200, Vladimir Oltean wrote: > > On Thu, Oct 31, 2024 at 05:46:47AM +0200, Wei Fang wrote: > > > > > Actually please do this instead: > > > > > > > > > > if (!(si->hw_features & ENETC_SI_F_QBU)) > > > > > > > > > > > Actually, VFs of eno0 have ENETC_SI_F_QBU bit set. So we should use the > > > following check instead. > > > > > > if (!enetc_si_is_pf(si) || !(si->hw_features & ENETC_SI_F_QBU)) > > > > > > Or we only set ENETC_SI_F_QBU bit for PF in enetc_get_si_caps() if the PF > > > supports 802.1 Qbu. > > > > This one is weird. I don't know why the ENETC would push a capability in > > the SI port capability register 0 for the VSI, if the VSI doesn't have > > access to the port registers in the first place. Let me ask internally, > > so we could figure out what's the best thing to do. > > Let's mask the ENETC_SI_F_QBU feature for VSIs in enetc_get_si_caps(). > Though we should do the same with ENETC_SIPCAPR0_QBV and > ENETC_SIPCAPR0_PSFP. Yes, I agree with you, for QBV and PSFP, I think we'd better to use a separate patch to clear these two bits for VFs, and the target of the patch will be net-next tree, because the related interfaces of these two features are not accessed by VFs, so there are no notable and visible issues at present.
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c index c09370eab319..c9f7b7b4445f 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.c +++ b/drivers/net/ethernet/freescale/enetc/enetc.c @@ -28,6 +28,9 @@ EXPORT_SYMBOL_GPL(enetc_port_mac_wr); static void enetc_change_preemptible_tcs(struct enetc_ndev_priv *priv, u8 preemptible_tcs) { + if (!enetc_si_is_pf(priv->si)) + return; + priv->preemptible_tcs = preemptible_tcs; enetc_mm_commit_preemptible_tcs(priv); }
Both ENETC PF and VF drivers share enetc_setup_tc_mqprio() to configure MQPRIO. And enetc_setup_tc_mqprio() calls enetc_change_preemptible_tcs() to configure preemptible TCs. However, only PF is able to configure preemptible TCs. Because only PF has related registers, while VF does not have these registers. So for VF, its hw->port pointer is NULL. Therefore, VF will access an invalid pointer when accessing a non-existent register, which will cause a call trace issue. The call trace log is shown below. root@ls1028ardb:~# tc qdisc add dev eno0vf0 parent root handle 100: \ mqprio num_tc 4 map 0 0 1 1 2 2 3 3 queues 1@0 1@1 1@2 1@3 hw 1 [ 187.290775] Unable to handle kernel paging request at virtual address 0000000000001f00 [ 187.298760] Mem abort info: [ 187.301607] ESR = 0x0000000096000004 [ 187.305373] EC = 0x25: DABT (current EL), IL = 32 bits [ 187.310714] SET = 0, FnV = 0 [ 187.313778] EA = 0, S1PTW = 0 [ 187.316923] FSC = 0x04: level 0 translation fault [ 187.321818] Data abort info: [ 187.324701] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000 [ 187.330207] CM = 0, WnR = 0, TnD = 0, TagAccess = 0 [ 187.335278] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [ 187.340610] user pgtable: 4k pages, 48-bit VAs, pgdp=0000002084b71000 [ 187.347076] [0000000000001f00] pgd=0000000000000000, p4d=0000000000000000 [ 187.353900] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP [ 187.360188] Modules linked in: xt_conntrack xt_addrtype cfg80211 rfkill snd_soc_hdmi_codec fsl_jr_uio caam_jr caamkeyblob_desc caamhash_desc caamalg_descp [ 187.406320] CPU: 0 PID: 1117 Comm: tc Not tainted 6.6.52-gfbb48d8e2ddb #1 [ 187.413131] Hardware name: LS1028A RDB Board (DT) [ 187.417846] pstate: 40000005 (nZcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 187.424831] pc : enetc_mm_commit_preemptible_tcs+0x1c4/0x400 [ 187.430518] lr : enetc_mm_commit_preemptible_tcs+0x30c/0x400 [ 187.436195] sp : ffff800084a4b400 [ 187.439515] x29: ffff800084a4b400 x28: 0000000000000004 x27: ffff6a58c5229808 [ 187.446679] x26: 0000000080000203 x25: ffff800085218600 x24: 0000000000000004 [ 187.453842] x23: ffff6a58c42e4a00 x22: ffff6a58c5229808 x21: ffff6a58c42e4980 [ 187.461004] x20: ffff6a58c5229800 x19: 0000000000001f00 x18: 0000000000000001 [ 187.468167] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 [ 187.475328] x14: 00000000000003f8 x13: 0000000000000400 x12: 0000000000065feb [ 187.482491] x11: 0000000000000000 x10: ffff6a593f6f9918 x9 : 0000000000000000 [ 187.489653] x8 : ffff800084a4b668 x7 : 0000000000000003 x6 : 0000000000000001 [ 187.496815] x5 : 0000000000001f00 x4 : 0000000000000000 x3 : 0000000000000000 [ 187.503977] x2 : 0000000000000000 x1 : 0000000000000200 x0 : ffffd2fbd8497460 [ 187.511140] Call trace: [ 187.513588] enetc_mm_commit_preemptible_tcs+0x1c4/0x400 [ 187.518918] enetc_setup_tc_mqprio+0x180/0x214 [ 187.523374] enetc_vf_setup_tc+0x1c/0x30 [ 187.527306] mqprio_enable_offload+0x144/0x178 [ 187.531766] mqprio_init+0x3ec/0x668 [ 187.535351] qdisc_create+0x15c/0x488 [ 187.539023] tc_modify_qdisc+0x398/0x73c [ 187.542958] rtnetlink_rcv_msg+0x128/0x378 [ 187.547064] netlink_rcv_skb+0x60/0x130 [ 187.550910] rtnetlink_rcv+0x18/0x24 [ 187.554492] netlink_unicast+0x300/0x36c [ 187.558425] netlink_sendmsg+0x1a8/0x420 [ 187.562358] ____sys_sendmsg+0x214/0x26c [ 187.566292] ___sys_sendmsg+0xb0/0x108 [ 187.570050] __sys_sendmsg+0x88/0xe8 [ 187.573634] __arm64_sys_sendmsg+0x24/0x30 [ 187.577742] invoke_syscall+0x48/0x114 [ 187.581503] el0_svc_common.constprop.0+0x40/0xe0 [ 187.586222] do_el0_svc+0x1c/0x28 [ 187.589544] el0_svc+0x40/0xe4 [ 187.592607] el0t_64_sync_handler+0x120/0x12c [ 187.596976] el0t_64_sync+0x190/0x194 [ 187.600648] Code: d65f03c0 d283e005 8b050273 14000050 (b9400273) [ 187.606759] ---[ end trace 0000000000000000 ]--- Fixes: 827145392a4a ("net: enetc: only commit preemptible TCs to hardware when MM TX is active") Signed-off-by: Wei Fang <wei.fang@nxp.com> --- drivers/net/ethernet/freescale/enetc/enetc.c | 3 +++ 1 file changed, 3 insertions(+)