[PATCH v2 net] net: enetc: Do not configure preemptible TCs if SIs do not support

[PATCH v2 net] net: enetc: Do not configure preemptible TCs if SIs do not support

[Wei Fang]

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 simplified log is as follows.

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.424831] pc : enetc_mm_commit_preemptible_tcs+0x1c4/0x400
[  187.430518] lr : enetc_mm_commit_preemptible_tcs+0x30c/0x400
[  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.606759] ---[ end trace 0000000000000000 ]---

In addition, some PFs also do not support configuring preemptible TCs,
such as eno1 and eno3 on LS1028A. It won't crash like it does for VFs,
but we should prevent these PFs from accessing these unimplemented
registers.

Fixes: 827145392a4a ("net: enetc: only commit preemptible TCs to hardware when MM TX is active")
Signed-off-by: Wei Fang <wei.fang@nxp.com>
Suggested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1 Link: https://lore.kernel.org/imx/20241030082117.1172634-1-wei.fang@nxp.com/
v2 changes:
1. Change the title and refine the commit message
2. Only set ENETC_SI_F_QBU bit for PFs which support Qbu
3. Prevent all SIs which not support Qbu from configuring preemptible
TCs
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index c09370eab319..59d4ca52dc21 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 (!(priv->si->hw_features & ENETC_SI_F_QBU))
+		return;
+
 	priv->preemptible_tcs = preemptible_tcs;
 	enetc_mm_commit_preemptible_tcs(priv);
 }
@@ -1752,7 +1755,12 @@ void enetc_get_si_caps(struct enetc_si *si)
 	if (val & ENETC_SIPCAPR0_QBV)
 		si->hw_features |= ENETC_SI_F_QBV;
 
-	if (val & ENETC_SIPCAPR0_QBU)
+	/* Although the SIPCAPR0 of VF indicates that VF supports Qbu,
+	 * only PF can access the related registers to configure Qbu.
+	 * Therefore, ENETC_SI_F_QBU is set only for PFs which support
+	 * this feature.
+	 */
+	if (val & ENETC_SIPCAPR0_QBU && enetc_si_is_pf(si))
 		si->hw_features |= ENETC_SI_F_QBU;
 
 	if (val & ENETC_SIPCAPR0_PSFP)
-- 
2.34.1

Re: [PATCH v2 net] net: enetc: Do not configure preemptible TCs if SIs do not support

[Vladimir Oltean]

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

On Mon, Nov 04, 2024 at 01:43:09PM +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 simplified log is as follows.
> 
> 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.424831] pc : enetc_mm_commit_preemptible_tcs+0x1c4/0x400
> [  187.430518] lr : enetc_mm_commit_preemptible_tcs+0x30c/0x400
> [  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.606759] ---[ end trace 0000000000000000 ]---
> 
> In addition, some PFs also do not support configuring preemptible TCs,
> such as eno1 and eno3 on LS1028A. It won't crash like it does for VFs,
> but we should prevent these PFs from accessing these unimplemented
> registers.
> 
> Fixes: 827145392a4a ("net: enetc: only commit preemptible TCs to hardware when MM TX is active")
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> Suggested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> v1 Link: https://lore.kernel.org/imx/20241030082117.1172634-1-wei.fang@nxp.com/
> v2 changes:
> 1. Change the title and refine the commit message
> 2. Only set ENETC_SI_F_QBU bit for PFs which support Qbu
> 3. Prevent all SIs which not support Qbu from configuring preemptible
> TCs
> ---
>  drivers/net/ethernet/freescale/enetc/enetc.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index c09370eab319..59d4ca52dc21 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 (!(priv->si->hw_features & ENETC_SI_F_QBU))
> +		return;
> +
>  	priv->preemptible_tcs = preemptible_tcs;
>  	enetc_mm_commit_preemptible_tcs(priv);
>  }
> @@ -1752,7 +1755,12 @@ void enetc_get_si_caps(struct enetc_si *si)
>  	if (val & ENETC_SIPCAPR0_QBV)
>  		si->hw_features |= ENETC_SI_F_QBV;
>  
> -	if (val & ENETC_SIPCAPR0_QBU)
> +	/* Although the SIPCAPR0 of VF indicates that VF supports Qbu,
> +	 * only PF can access the related registers to configure Qbu.
> +	 * Therefore, ENETC_SI_F_QBU is set only for PFs which support
> +	 * this feature.
> +	 */
> +	if (val & ENETC_SIPCAPR0_QBU && enetc_si_is_pf(si))
>  		si->hw_features |= ENETC_SI_F_QBU;
>  
>  	if (val & ENETC_SIPCAPR0_PSFP)
> -- 
> 2.34.1
>

As per internal discussions, the correct fix would be to read these
hw_features from the ENETC_PCAPR0 register rather than ENETC_SIPCAPR0,
and have this code exclusively in the PF driver.

I'm expecting a new change which moves the capability detection which is
similar to what I have attached here, and then your patch will only contain
the snippet from enetc_change_preemptible_tcs() which you've already posted.

pw-bot: changes-requested

[-- Attachment #2: 0001-net-enetc-read-TSN-capabilities-from-port-register-n.patch --]
[-- Type: text/x-diff, Size: 5483 bytes --]

From a83143233c9c3fa9c5437624ad50c9b5c46bb2ec Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Wed, 6 Nov 2024 15:17:13 +0200
Subject: [PATCH] net: enetc: read TSN capabilities from port register, not SI

Configuring TSN (Qbv, Qbu, PSFP) capabilities requires access to port
registers, which are available to the PSI but not the VSI.

Yet, the SI port capability register 0 (PSICAPR0), exposed to both PSIs
and VSIs, presents the same capabilities to the VF as to the PF, thus
leading the VF driver into thinking it can configure these features.

In the case of ENETC_SI_F_QBU, having it set in the VF leads to a crash:

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.424831] pc : enetc_mm_commit_preemptible_tcs+0x1c4/0x400
[  187.430518] lr : enetc_mm_commit_preemptible_tcs+0x30c/0x400
[  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.606759] ---[ end trace 0000000000000000 ]---

while the other TSN features in the VF are harmless, because the
net_device_ops used for the VF driver do not expose entry points for
these other features.

These capability bits are in the process of being defeatured from the SI
registers. We should read them from the port capability register, where
they are also present, and which is naturally only exposed to the PF.

The change to blame (relevant for stable backports) is the one where
this started being a problem, aka when the kernel started to crash due
to the wrong capability seen by the VF driver.

Fixes: 827145392a4a ("net: enetc: only commit preemptible TCs to hardware when MM TX is active")
Reported-by: Wei Fang <wei.fang@nxp.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc.c  |  9 ---------
 .../net/ethernet/freescale/enetc/enetc_hw.h   |  6 +++---
 .../net/ethernet/freescale/enetc/enetc_pf.c   | 19 +++++++++++++++++++
 3 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 35634c516e26..bece220535a1 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -1756,15 +1756,6 @@ void enetc_get_si_caps(struct enetc_si *si)
 		rss = enetc_rd(hw, ENETC_SIRSSCAPR);
 		si->num_rss = ENETC_SIRSSCAPR_GET_NUM_RSS(rss);
 	}
-
-	if (val & ENETC_SIPCAPR0_QBV)
-		si->hw_features |= ENETC_SI_F_QBV;
-
-	if (val & ENETC_SIPCAPR0_QBU)
-		si->hw_features |= ENETC_SI_F_QBU;
-
-	if (val & ENETC_SIPCAPR0_PSFP)
-		si->hw_features |= ENETC_SI_F_PSFP;
 }
 EXPORT_SYMBOL_GPL(enetc_get_si_caps);
 
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
index 7c3285584f8a..55ba949230ff 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
@@ -23,10 +23,7 @@
 #define ENETC_SICTR0	0x18
 #define ENETC_SICTR1	0x1c
 #define ENETC_SIPCAPR0	0x20
-#define ENETC_SIPCAPR0_PSFP	BIT(9)
 #define ENETC_SIPCAPR0_RSS	BIT(8)
-#define ENETC_SIPCAPR0_QBV	BIT(4)
-#define ENETC_SIPCAPR0_QBU	BIT(3)
 #define ENETC_SIPCAPR0_RFS	BIT(2)
 #define ENETC_SIPCAPR1	0x24
 #define ENETC_SITGTGR	0x30
@@ -194,6 +191,9 @@ enum enetc_bdr_type {TX, RX};
 #define ENETC_PCAPR0		0x0900
 #define ENETC_PCAPR0_RXBDR(val)	((val) >> 24)
 #define ENETC_PCAPR0_TXBDR(val)	(((val) >> 16) & 0xff)
+#define ENETC_PCAPR0_PSFP	BIT(9)
+#define ENETC_PCAPR0_QBV	BIT(4)
+#define ENETC_PCAPR0_QBU	BIT(3)
 #define ENETC_PCAPR1		0x0904
 #define ENETC_PSICFGR0(n)	(0x0940 + (n) * 0xc)  /* n = SI index */
 #define ENETC_PSICFGR0_SET_TXBDR(val)	((val) & 0xff)
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
index a76ce41eb197..61e2df3e047f 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
@@ -409,6 +409,23 @@ static void enetc_port_assign_rfs_entries(struct enetc_si *si)
 	enetc_port_wr(hw, ENETC_PRFSMR, ENETC_PRFSMR_RFSE);
 }
 
+static void enetc_port_get_caps(struct enetc_si *si)
+{
+	struct enetc_hw *hw = &si->hw;
+	u32 val;
+
+	val = enetc_port_rd(hw, ENETC_PCAPR0);
+
+	if (val & ENETC_PCAPR0_QBV)
+		si->hw_features |= ENETC_SI_F_QBV;
+
+	if (val & ENETC_PCAPR0_QBU)
+		si->hw_features |= ENETC_SI_F_QBU;
+
+	if (val & ENETC_PCAPR0_PSFP)
+		si->hw_features |= ENETC_SI_F_PSFP;
+}
+
 static void enetc_port_si_configure(struct enetc_si *si)
 {
 	struct enetc_pf *pf = enetc_si_priv(si);
@@ -1026,6 +1043,8 @@ static int enetc_pf_probe(struct pci_dev *pdev,
 	pf->total_vfs = pci_sriov_get_totalvfs(pdev);
 	pf->ops = &enetc_pf_ops;
 
+	enetc_port_get_caps(pf->si);
+
 	err = enetc_setup_mac_addresses(node, pf);
 	if (err)
 		goto err_setup_mac_addresses;
-- 
2.43.0

RE: [PATCH v2 net] net: enetc: Do not configure preemptible TCs if SIs do not support

[Wei Fang]

> -----Original Message-----
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Sent: 2024年11月6日 22:50
> 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 v2 net] net: enetc: Do not configure preemptible TCs if SIs
> do not support
> 
> On Mon, Nov 04, 2024 at 01:43:09PM +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 simplified log is as follows.
> >
> > 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.424831] pc :
> > enetc_mm_commit_preemptible_tcs+0x1c4/0x400
> > [  187.430518] lr : enetc_mm_commit_preemptible_tcs+0x30c/0x400
> > [  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.606759] ---[ end trace
> > 0000000000000000 ]---
> >
> > In addition, some PFs also do not support configuring preemptible TCs,
> > such as eno1 and eno3 on LS1028A. It won't crash like it does for VFs,
> > but we should prevent these PFs from accessing these unimplemented
> > registers.
> >
> > Fixes: 827145392a4a ("net: enetc: only commit preemptible TCs to
> > hardware when MM TX is active")
> > Signed-off-by: Wei Fang <wei.fang@nxp.com>
> > Suggested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> > v1 Link:
> >
> https://lore.kernel.org/imx/20241030082117.1172634-1-wei.fang@nxp.com/
> > v2 changes:
> > 1. Change the title and refine the commit message 2. Only set
> > ENETC_SI_F_QBU bit for PFs which support Qbu 3. Prevent all SIs which
> > not support Qbu from configuring preemptible TCs
> > ---
> >  drivers/net/ethernet/freescale/enetc/enetc.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c
> > b/drivers/net/ethernet/freescale/enetc/enetc.c
> > index c09370eab319..59d4ca52dc21 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 (!(priv->si->hw_features & ENETC_SI_F_QBU))
> > +		return;
> > +
> >  	priv->preemptible_tcs = preemptible_tcs;
> >  	enetc_mm_commit_preemptible_tcs(priv);
> >  }
> > @@ -1752,7 +1755,12 @@ void enetc_get_si_caps(struct enetc_si *si)
> >  	if (val & ENETC_SIPCAPR0_QBV)
> >  		si->hw_features |= ENETC_SI_F_QBV;
> >
> > -	if (val & ENETC_SIPCAPR0_QBU)
> > +	/* Although the SIPCAPR0 of VF indicates that VF supports Qbu,
> > +	 * only PF can access the related registers to configure Qbu.
> > +	 * Therefore, ENETC_SI_F_QBU is set only for PFs which support
> > +	 * this feature.
> > +	 */
> > +	if (val & ENETC_SIPCAPR0_QBU && enetc_si_is_pf(si))
> >  		si->hw_features |= ENETC_SI_F_QBU;
> >
> >  	if (val & ENETC_SIPCAPR0_PSFP)
> > --
> > 2.34.1
> >
> 
> As per internal discussions, the correct fix would be to read these
> hw_features from the ENETC_PCAPR0 register rather than ENETC_SIPCAPR0,
> and have this code exclusively in the PF driver.
> 
> I'm expecting a new change which moves the capability detection which is
> similar to what I have attached here, and then your patch will only contain
> the snippet from enetc_change_preemptible_tcs() which you've already
> posted.
> 

OK, I will update the third version later, probably after the recent i.MX95
patches are merged into the net tree, because I am worried that it may
cause a merge conflict.

> pw-bot: changes-requested