diff mbox series

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

Message ID 20241104054309.1388433-1-wei.fang@nxp.com (mailing list archive)
State New
Headers show
Series [v2,net] net: enetc: Do not configure preemptible TCs if SIs do not support | expand

Commit Message

Wei Fang Nov. 4, 2024, 5:43 a.m. UTC 
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(-)

Comments

Vladimir Oltean Nov. 6, 2024, 2:49 p.m. UTC | #1 
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
Wei Fang Nov. 7, 2024, 1:36 a.m. UTC | #2 
> -----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
diff mbox series

Patch

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)