netdev - Re: [PATCH net-next v2 11/13] net: dsa: realtek: rtl8365mb: use DSA CPU port

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20211219221913.c7hxslrvkj6cyrle@skbuf>
Date:   Mon, 20 Dec 2021 00:19:13 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Luiz Angelo Daros de Luca <luizluca@...il.com>
Cc:     netdev@...r.kernel.org, linus.walleij@...aro.org, andrew@...n.ch,
        vivien.didelot@...il.com, f.fainelli@...il.com,
        alsi@...g-olufsen.dk, arinc.unal@...nc9.com
Subject: Re: [PATCH net-next v2 11/13] net: dsa: realtek: rtl8365mb: use DSA
 CPU port

On Sat, Dec 18, 2021 at 05:14:23AM -0300, Luiz Angelo Daros de Luca wrote:
> Instead of a fixed CPU port, assume that DSA is correct.
> 
> Tested-by: Arınç ÜNAL <arinc.unal@...nc9.com>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@...il.com>
> ---

I don't necessarily see the value added by this change. Since (I think)
only a single port can be a CPU port, a sanity check seems to be missing
that the CPU port in the device tree is the expected one. This seems to
be missing with or without your patch. You are unnaturally splitting the
initialization of a data structure between rtl8365mb_setup() and
rtl8365mb_detect(). Maybe what you should do is keep everything in
rtl8365mb_detect() where it is right now, and check in rtl8365mb_setup
that the cpu_dp->index is equal to priv->cpu_port?

>  drivers/net/dsa/realtek/rtl8365mb.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index a8f44538a87a..b79a4639b283 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -103,14 +103,13 @@
>  
>  /* Chip-specific data and limits */
>  #define RTL8365MB_CHIP_ID_8365MB_VC		0x6367
> -#define RTL8365MB_CPU_PORT_NUM_8365MB_VC	6
>  #define RTL8365MB_LEARN_LIMIT_MAX_8365MB_VC	2112
>  
>  /* Family-specific data and limits */
>  #define RTL8365MB_PHYADDRMAX	7
>  #define RTL8365MB_NUM_PHYREGS	32
>  #define RTL8365MB_PHYREGMAX	(RTL8365MB_NUM_PHYREGS - 1)
> -#define RTL8365MB_MAX_NUM_PORTS	(RTL8365MB_CPU_PORT_NUM_8365MB_VC + 1)
> +#define RTL8365MB_MAX_NUM_PORTS  7
>  
>  /* Chip identification registers */
>  #define RTL8365MB_CHIP_ID_REG		0x1300
> @@ -1827,9 +1826,18 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
>  		dev_info(priv->dev, "no interrupt support\n");
>  
>  	/* Configure CPU tagging */
> -	ret = rtl8365mb_cpu_config(priv);
> -	if (ret)
> -		goto out_teardown_irq;
> +	for (i = 0; i < priv->num_ports; i++) {
> +		if (!(dsa_is_cpu_port(priv->ds, i)))
> +			continue;

dsa_switch_for_each_cpu_port(cpu_dp, ds)

> +		priv->cpu_port = i;
> +		mb->cpu.mask = BIT(priv->cpu_port);
> +		mb->cpu.trap_port = priv->cpu_port;
> +		ret = rtl8365mb_cpu_config(priv);
> +		if (ret)
> +			goto out_teardown_irq;
> +
> +		break;
> +	}
>  
>  	/* Configure ports */
>  	for (i = 0; i < priv->num_ports; i++) {
> @@ -1960,8 +1968,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
>  			 "found an RTL8365MB-VC switch (ver=0x%04x)\n",
>  			 chip_ver);
>  
> -		priv->cpu_port = RTL8365MB_CPU_PORT_NUM_8365MB_VC;
> -		priv->num_ports = priv->cpu_port + 1;
> +		priv->num_ports = RTL8365MB_MAX_NUM_PORTS;
>  
>  		mb->priv = priv;
>  		mb->chip_id = chip_id;
> @@ -1972,8 +1979,6 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
>  		mb->jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc);
>  
>  		mb->cpu.enable = 1;
> -		mb->cpu.mask = BIT(priv->cpu_port);
> -		mb->cpu.trap_port = priv->cpu_port;
>  		mb->cpu.insert = RTL8365MB_CPU_INSERT_TO_ALL;
>  		mb->cpu.position = RTL8365MB_CPU_POS_AFTER_SA;
>  		mb->cpu.rx_length = RTL8365MB_CPU_RXLEN_64BYTES;
> -- 
> 2.34.0
>