diff mbox series

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

Message ID 20230210172822.12960-1-richard@routerhints.com (mailing list archive)
State New, archived
Headers show
Series [net] net: dsa: mt7530: fix CPU flooding and do not set CPU association | expand

Commit Message

Arınç ÜNAL Feb. 10, 2023, 5:28 p.m. UTC 
From: Richard van Schagen <richard@routerhints.com>

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.

Since CPU->port is forced via the DSA tag, connecting CPU to all user ports
of the switch breaks communication over VLAN tagged frames. 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.

[ arinc.unal@arinc9.com: Wrote subject and changelog ]

Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Signed-off-by: Richard van Schagen <richard@routerhints.com>
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

Comments

Vladimir Oltean Feb. 10, 2023, 6:44 p.m. UTC | #1 
Hi,

On Fri, Feb 10, 2023 at 08:28:23PM +0300, arinc9.unal@gmail.com wrote:
> From: Richard van Schagen <richard@routerhints.com>
> 
> 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.

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.

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

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

> 
> [ arinc.unal@arinc9.com: Wrote subject and changelog ]
> 
> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Signed-off-by: Richard van Schagen <richard@routerhints.com>
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>

Missing Fixes: tags for patches sent to "net". Multiple problems =>
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
>
Richard van Schagen Feb. 11, 2023, 8:52 p.m. UTC | #2 
> On 10 Feb 2023, at 19:44, Vladimir Oltean <olteanv@gmail.com> wrote:
> 
> Hi,
> 
> On Fri, Feb 10, 2023 at 08:28:23PM +0300, arinc9.unal@gmail.com wrote:
>> From: Richard van Schagen <richard@routerhints.com>
>> 
>> 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@arinc9.com: Wrote subject and changelog ]
>> 
>> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> Signed-off-by: Richard van Schagen <richard@routerhints.com>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> 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
Vladimir Oltean Feb. 13, 2023, 1:36 p.m. UTC | #3 
On Sat, Feb 11, 2023 at 08:52:20PM +0000, Richard van Schagen wrote:
> > 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?

"that" meaning what? A device tree specifying that there are two CPU
ports, used with a kernel that only sets up the first CPU port?
Why would that be the same as a wrong DTS? What's wrong about it?

FYI, there exists an Arm certification program called SystemReady IR,
where the goal is for one firmware image (U-Boot with bootefi) to boot a
number of different embedded Linux or BSD distributions, having different
kernel versions. With this boot flow, currently there is no concept to
get a device tree from the OS, so using EFI_FDT_USE_INTERNAL, U-Boot
will provide its own runtime device tree to the booted OS.

In U-Boot there are efforts for several SoCs to periodically sync up
their device trees with the latest Linux device trees, in order for the
most complete hardware description to be available to all booted
distributions. U-Boot drivers are also expected to work with the same
device trees that Linux uses.

The work of these people is made unnecessarily harder by mentalities
like this, that there's such a thing as a "wrong DTS with a kernel".
Generally, the default expectation is that at least for a time window,
kernels do something sensible when given device trees newer than them
(forward compatibility). This has always been a guideline for device
tree usage, and with SystemReady IR, it makes it possible for one
firmware image to support distros having different kernel versions.

The current DSA device tree binding implementation (in the framework)
has always been careful to ignore other CPU ports present in the device
tree and just use the first one, until it gained support for changing
the default assignment.

Drivers which have forward or backward compatibility bugs can reasonably
have those bugs fixed, those fixes included into the stable LTS kernels,
and from there, integrated into distributions.

Now, if you are certain that Mediatek SoCs are not used in this
particular way, and there is some strong reason to not make an effort to
preserve forward compatibility, that is entirely a separate discussion.

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

And one would expect this information to be accurately described in the
device tree. Is there any reason to not trust the correctness of the
device tree?

> Not sure how we could prevent that?

Prevent what? Flooding to an unused CPU port?
diff mbox series

Patch

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);
+
 	/* Enable and reset MIB counters */
 	mt7530_mib_reset(ds);