Message-ID: <Z0S3btCU0raWZIUc@lizhi-Precision-Tower-5810>
Date: Mon, 25 Nov 2024 12:44:14 -0500
From: Frank Li <Frank.li@....com>
To: Wei Fang <wei.fang@....com>
Cc: claudiu.manoil@....com, vladimir.oltean@....com, xiaoning.wang@....com,
	andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, imx@...ts.linux.dev
Subject: Re: [PATCH v3 net 1/2] net: enetc: read TSN capabilities from port
 register, not SI

On Mon, Nov 25, 2024 at 05:07:18PM +0800, Wei Fang wrote:
> From: Vladimir Oltean <vladimir.oltean@....com>
>
> 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@...028ardb:~# 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@....com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>

Reviewed-by: Frank Li <Frank.Li@....com>

> ---
> v3: new patch.
> ---
>  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 c47b4a743d93..203862ec1114 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);
> @@ -416,6 +433,8 @@ static void enetc_port_si_configure(struct enetc_si *si)
>  	int num_rings, i;
>  	u32 val;
>
> +	enetc_port_get_caps(si);
> +
>  	val = enetc_port_rd(hw, ENETC_PCAPR0);
>  	num_rings = min(ENETC_PCAPR0_RXBDR(val), ENETC_PCAPR0_TXBDR(val));
>
> --
> 2.34.1
>

Powered by blists - more mailing lists