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: <464bd9da-16ab-793d-972d-dff8967bdc50@bang-olufsen.dk>
Date:   Sun, 19 Dec 2021 23:19:03 +0000
From:   Alvin Šipraga <ALSI@...g-olufsen.dk>
To:     Vladimir Oltean <olteanv@...il.com>,
        Luiz Angelo Daros de Luca <luizluca@...il.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "arinc.unal@...nc9.com" <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next v2 11/13] net: dsa: realtek: rtl8365mb: use DSA
 CPU port

On 12/19/21 23:19, Vladimir Oltean wrote:
> 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?

I'm quite sure the switch family does actually support multiple CPU 
ports. If you have a cascaded switch, CPU-tagged frames may pass between 
the external ports of the switches. Any port can be configured to parse 
CPU-tagged frames. And the CPU port configuration register allows for a 
mask of EXT ports to be configured for trapping.

However, this change requires a more thorough explanation of what it is 
trying to achieve. I already see that Vladimir is confused. The control 
flow also looks rather strange.

If I am to guess what deficiency you are trying to address, it's that 
the driver assumes that the CPU port is the EXT port; since there is 
only one possible EXT port, this is hardcoded with 
RTL8365MB_CPU_PORT_NUM_8365MB_VC = 6. But now your new switch actually 
has _two_ EXT ports. Which of those is the CPU port is configured by 
setting the realtek,ext-int property in the device tree node of the CPU 
port. But that means that the CPU port cannot be hardcoded. So you want 
to get this information from DSA instead.

Similar to my comment to another patch in your series, I think it's 
worth making the driver support multiple EXT interfaces. Then it should 
be clearer in the series why you are making these changes.

Please consider also Vladimir's point about unnaturally splitting code. 
I can see it elsewhere in the series a little bit too. It is nice to 
keep the structure of the driver consistent - at least while it is still 
young and innocent. :-)

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