Re: [PATCH net] net: dsa: mt7530: fix CPU flooding and do not set CPU association

[Richard van Schagen]

> On 10 Feb 2023, at 19:44, Vladimir Oltean <olteanv@xxxxxxxxx> wrote:
> 
> Hi,
> 
> On Fri, Feb 10, 2023 at 08:28:23PM +0300, arinc9.unal@xxxxxxxxx wrote:
>> From: Richard van Schagen <richard@xxxxxxxxxxxxxxx>
>> 
>> The original code only enables flooding on CPU port, on port 6, since
>> that's the last one set up. In doing so, it removes flooding on port 5,
>> which made so that, in order to communicate properly over port 5, a frame
>> had to be sent from a user port to the DSA master. Fix this.
> 
> Separate patch for this. I don't understand the correlation with the
> other part below.

Will do, and there isn’t. Flooding became evident now we can use it at 2nd CPU.

> FWIW, the problem can also be solved similar to 8d5f7954b7c8 ("net: dsa:
> felix: break at first CPU port during init and teardown"), and both CPU
> ports could be added to the flooding mask only as part of the "multiple
> CPU ports" feature. When a multiple CPU ports device tree is used with a
> kernel capable of a single CPU port, your patch enables flooding towards
> the second CPU port, which will never be used (or up). Not sure if you
> want that.

So basically that means the wrong DTS with a kernel? Isn’t that similar to the wrong DTS 
for a device? Port 5 / GMAC1 can be used as <ethphy0>, <ethphy4>, external phy on port 5.
e.g. SPF port on port 5, or used as second CPU port. Not sure how we could prevent that?

For now I would like to keep the logic as-is: iterate over all dsa-ports and when its a CPU port
Enable flooding on that port. Not used (or up) is the default even when using multiple dsa cpus.
For this we added the .change_master. No flooding will take place until we actually connect a user
port to the cpu. 

>> 
>> Since CPU->port is forced via the DSA tag, connecting CPU to all user ports
>> of the switch breaks communication over VLAN tagged frames.
> 
> Here, I understand almost nothing from this phrase.
> 
> "CPU->port" means "association between user port and CPU port"?
> 
> You're saying that association is forced through the DSA tag? Details?
> Who or what is the DSA tag? Or are you saying that packets transmitted
> by tag_mtk.c are always sent as control plane, and will reach an egress
> port regardless of the port matrix of the CPU port?
> 

When using either a “Force Port” in the DMA descriptor (which works always but we don’t use) OR
when we are forcing a Port in the Special Tag (provided that’s enabled) we always bypass any restriction
between the CPU and the User port. 

> "Connecting CPU to all user ports" means assigning PCR_MATRIX(dsa_user_ports())
> to the port matrix of the CPU port, yes? Why would that break communication
> for VLAN-tagged traffic (and what is the source and destination of that
> traffic)?
> 
>> Therefore, remove the code that sets CPU assocation.
>> This way, the CPU reverts to not being connected to any port as soon
>> as ".port_enable" is called.
> 
> Partly to blame may be the poor phrasing here. AFAICS, the port matrix
> of the CPU port remains 0 throughout the lifetime of the driver. Why
> mention ".port_enable"? That handles the user -> CPU direction, not the
> CPU -> user direction.
> 
In .port_enable we also get to “enable” any CPU port. Until now we didn’t
just return 0 like other drivers. Instead on empty priv->port[cpu].pm was used.
So we connect ALL users ports to the CPU when we call cpu_enable, and clear
that when .port_enable is called. So remove both. Don’t connect anything during
Cpu enable and add a check for user-port.

>> 
>> [ arinc.unal@xxxxxxxxxx: Wrote subject and changelog ]
>> 
>> Tested-by: Arınç ÜNAL <arinc.unal@xxxxxxxxxx>
>> Signed-off-by: Richard van Schagen <richard@xxxxxxxxxxxxxxx>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@xxxxxxxxxx>
> 
> Missing Fixes: tags for patches sent to "net". Multiple problems =>
> multiple patches.
> 

Will separate the fixes to multiple patches.
 
>> ---
>> drivers/net/dsa/mt7530.c | 17 ++++++++---------
>> 1 file changed, 8 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 3a15015bc409..b5ad4b4fc00c 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -997,6 +997,7 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
>> {
>> struct mt7530_priv *priv = ds->priv;
>> int ret;
>> + u32 val;
>> 
>> /* Setup max capability of CPU port at first */
>> if (priv->info->cpu_port_config) {
>> @@ -1009,20 +1010,15 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
>> mt7530_write(priv, MT7530_PVC_P(port),
>>      PORT_SPEC_TAG);
>> 
>> - /* Disable flooding by default */
>> - mt7530_rmw(priv, MT7530_MFC, BC_FFP_MASK | UNM_FFP_MASK | UNU_FFP_MASK,
>> -    BC_FFP(BIT(port)) | UNM_FFP(BIT(port)) | UNU_FFP(BIT(port)));
>> + /* Enable flooding on CPU */
>> + val = mt7530_read(priv, MT7530_MFC);
>> + val |= BC_FFP(BIT(port)) | UNM_FFP(BIT(port)) | UNU_FFP(BIT(port));
>> + mt7530_write(priv, MT7530_MFC, val);
>> 
>> /* Set CPU port number */
>> if (priv->id == ID_MT7621)
>> mt7530_rmw(priv, MT7530_MFC, CPU_MASK, CPU_EN | CPU_PORT(port));
>> 
>> - /* CPU port gets connected to all user ports of
>> -  * the switch.
>> -  */
>> - mt7530_write(priv, MT7530_PCR_P(port),
>> -      PCR_MATRIX(dsa_user_ports(priv->ds)));
>> -
>> /* Set to fallback mode for independent VLAN learning */
>> mt7530_rmw(priv, MT7530_PCR_P(port), PCR_PORT_VLAN_MASK,
>>    MT7530_PORT_FALLBACK_MODE);
>> @@ -2204,6 +2200,9 @@ mt7530_setup(struct dsa_switch *ds)
>> 
>> priv->p6_interface = PHY_INTERFACE_MODE_NA;
>> 
>> + /* Disable flooding by default */
>> + mt7530_rmw(priv, MT7530_MFC, BC_FFP_MASK | UNM_FFP_MASK | UNU_FFP_MASK, 0);
>> +
> 
> Shouldn't mt7531_setup() have this too?
> 
>> /* Enable and reset MIB counters */
>> mt7530_mib_reset(ds);
>> 
>> -- 
>> 2.37.2