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:

quoted

Both ENETC PF and VF drivers share enetc_setup_tc_mqprio() to configure
MQPRIO. And enetc_setup_tc_mqprio() calls

enetc_change_preemptible_tcs()

quoted

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

quoted

[  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

quoted

[  187.347076] [0000000000001f00] pgd=0000000000000000,

p4d=0000000000000000

quoted

[  187.353900] Internal error: Oops: 0000000096000004 [#1] PREEMPT

SMP

quoted

[  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

quoted

[  187.406320] CPU: 0 PID: 1117 Comm: tc Not tainted

6.6.52-gfbb48d8e2ddb #1

quoted

[  187.413131] Hardware name: LS1028A RDB Board (DT)
[  187.417846] pstate: 40000005 (nZcv daif -PAN -UAO -TCO -DIT -SSBS

BTYPE=--)

quoted

[  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

quoted

[  187.446679] x26: 0000000080000203 x25: ffff800085218600 x24:

0000000000000004

quoted

[  187.453842] x23: ffff6a58c42e4a00 x22: ffff6a58c5229808 x21:

ffff6a58c42e4980

quoted

[  187.461004] x20: ffff6a58c5229800 x19: 0000000000001f00 x18:

0000000000000001

quoted

[  187.468167] x17: 0000000000000000 x16: 0000000000000000 x15:

0000000000000000

quoted

[  187.475328] x14: 00000000000003f8 x13: 0000000000000400 x12:

0000000000065feb

quoted

[  187.482491] x11: 0000000000000000 x10: ffff6a593f6f9918 x9 :

0000000000000000

quoted

[  187.489653] x8 : ffff800084a4b668 x7 : 0000000000000003 x6 :

0000000000000001

quoted

[  187.496815] x5 : 0000000000001f00 x4 : 0000000000000000 x3 :

0000000000000000

quoted

[  187.503977] x2 : 0000000000000000 x1 : 0000000000000200 x0 :

ffffd2fbd8497460

quoted

[  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)

quoted

[  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

quoted

Fixes: 827145392a4a ("net: enetc: only commit preemptible TCs to

hardware when MM TX is active")

quoted

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

quoted

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.

quoted

 	priv->preemptible_tcs = preemptible_tcs;
 	enetc_mm_commit_preemptible_tcs(priv);
 }
--
2.34.1