diff mbox series

[net-next,3/3] net: dsa: qca8k: limit user ports access to the first CPU port on setup

Message ID 20230724033058.16795-3-ansuelsmth@gmail.com
State New
Headers
Series [net-next,1/3] net: dsa: tag_qca: return early if dev is not found |

Commit Message

Christian Marangi July 24, 2023, 3:30 a.m. UTC 
  In preparation for multi-CPU support, set CPU port LOOKUP MEMBER outside
the port loop and setup the LOOKUP MEMBER mask for user ports only to
the first CPU port.

This is to handle flooding condition where every CPU port is set as
target and prevent packet duplication for unknown frames from user ports.

Secondary CPU port LOOKUP MEMBER mask will be setup later when
port_change_master will be implemented.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Comments

Simon Horman July 26, 2023, 8:19 a.m. UTC | #1 
On Mon, Jul 24, 2023 at 05:30:58AM +0200, Christian Marangi wrote:
> In preparation for multi-CPU support, set CPU port LOOKUP MEMBER outside
> the port loop and setup the LOOKUP MEMBER mask for user ports only to
> the first CPU port.
> 
> This is to handle flooding condition where every CPU port is set as
> target and prevent packet duplication for unknown frames from user ports.
> 
> Secondary CPU port LOOKUP MEMBER mask will be setup later when
> port_change_master will be implemented.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>
Vladimir Oltean July 26, 2023, 1:18 p.m. UTC | #2 
On Mon, Jul 24, 2023 at 05:30:58AM +0200, Christian Marangi wrote:
> In preparation for multi-CPU support, set CPU port LOOKUP MEMBER outside
> the port loop and setup the LOOKUP MEMBER mask for user ports only to
> the first CPU port.
> 
> This is to handle flooding condition where every CPU port is set as
> target and prevent packet duplication for unknown frames from user ports.
> 
> Secondary CPU port LOOKUP MEMBER mask will be setup later when
> port_change_master will be implemented.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---

This is kinda "net.git" material, in the sense that it fixes the current
driver behavior with device trees from the future, right?
Florian Fainelli July 26, 2023, 10:21 p.m. UTC | #3 
On 7/23/23 20:30, Christian Marangi wrote:
> In preparation for multi-CPU support, set CPU port LOOKUP MEMBER outside
> the port loop and setup the LOOKUP MEMBER mask for user ports only to
> the first CPU port.
> 
> This is to handle flooding condition where every CPU port is set as
> target and prevent packet duplication for unknown frames from user ports.
> 
> Secondary CPU port LOOKUP MEMBER mask will be setup later when
> port_change_master will be implemented.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Christian Marangi July 27, 2023, 7:10 p.m. UTC | #4 
On Wed, Jul 26, 2023 at 04:18:51PM +0300, Vladimir Oltean wrote:
> On Mon, Jul 24, 2023 at 05:30:58AM +0200, Christian Marangi wrote:
> > In preparation for multi-CPU support, set CPU port LOOKUP MEMBER outside
> > the port loop and setup the LOOKUP MEMBER mask for user ports only to
> > the first CPU port.
> > 
> > This is to handle flooding condition where every CPU port is set as
> > target and prevent packet duplication for unknown frames from user ports.
> > 
> > Secondary CPU port LOOKUP MEMBER mask will be setup later when
> > port_change_master will be implemented.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> 
> This is kinda "net.git" material, in the sense that it fixes the current
> driver behavior with device trees from the future, right?

This is not strictly a fix. The secondary CPU (if defined) doesn't have
flood enabled so the switch won't forward packet. It's more of a
cleanup/preparation from my point of view. What do you think?
Vladimir Oltean July 27, 2023, 9:14 p.m. UTC | #5 
On Thu, Jul 27, 2023 at 09:10:56PM +0200, Christian Marangi wrote:
> On Wed, Jul 26, 2023 at 04:18:51PM +0300, Vladimir Oltean wrote:
> > On Mon, Jul 24, 2023 at 05:30:58AM +0200, Christian Marangi wrote:
> > > In preparation for multi-CPU support, set CPU port LOOKUP MEMBER outside
> > > the port loop and setup the LOOKUP MEMBER mask for user ports only to
> > > the first CPU port.
> > > 
> > > This is to handle flooding condition where every CPU port is set as
> > > target and prevent packet duplication for unknown frames from user ports.
> > > 
> > > Secondary CPU port LOOKUP MEMBER mask will be setup later when
> > > port_change_master will be implemented.
> > > 
> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > ---
> > 
> > This is kinda "net.git" material, in the sense that it fixes the current
> > driver behavior with device trees from the future, right?
> 
> This is not strictly a fix. The secondary CPU (if defined) doesn't have
> flood enabled so the switch won't forward packet. It's more of a
> cleanup/preparation from my point of view. What do you think?
> 
> -- 
> 	Ansuel

Ah, ok, if packets don't reach the second CPU port anyway then it's fine.
Vladimir Oltean July 27, 2023, 9:16 p.m. UTC | #6 
On Mon, Jul 24, 2023 at 05:30:58AM +0200, Christian Marangi wrote:
> In preparation for multi-CPU support, set CPU port LOOKUP MEMBER outside
> the port loop and setup the LOOKUP MEMBER mask for user ports only to
> the first CPU port.
> 
> This is to handle flooding condition where every CPU port is set as
> target and prevent packet duplication for unknown frames from user ports.
> 
> Secondary CPU port LOOKUP MEMBER mask will be setup later when
> port_change_master will be implemented.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

>  drivers/net/dsa/qca/qca8k-8xxx.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index 31552853fdd4..6286a64a2fe3 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -1850,18 +1850,16 @@ qca8k_setup(struct dsa_switch *ds)
>  	if (ret)
>  		return ret;
>  
> +	/* CPU port gets connected to all user ports of the switch */
> +	ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(cpu_port),
> +			QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
> +	if (ret)
> +		return ret;
> +
>  	/* Setup connection between CPU port & user ports
>  	 * Configure specific switch configuration for ports
>  	 */
>  	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
> -		/* CPU port gets connected to all user ports of the switch */
> -		if (dsa_is_cpu_port(ds, i)) {
> -			ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
> -					QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
> -			if (ret)
> -				return ret;
> -		}
> -
>  		/* Individual user ports get connected to CPU port only */
>  		if (dsa_is_user_port(ds, i)) {
>  			ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),

FWIW, the remaining loop can be rewritten (in a separate patch) using
dsa_switch_for_each_user_port(), which is actually an operation of lower
complexity compared to "for" + "dsa_is_user_port".

> -- 
> 2.40.1
>
diff mbox series

Patch

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 31552853fdd4..6286a64a2fe3 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -1850,18 +1850,16 @@  qca8k_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
+	/* CPU port gets connected to all user ports of the switch */
+	ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(cpu_port),
+			QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
+	if (ret)
+		return ret;
+
 	/* Setup connection between CPU port & user ports
 	 * Configure specific switch configuration for ports
 	 */
 	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
-		/* CPU port gets connected to all user ports of the switch */
-		if (dsa_is_cpu_port(ds, i)) {
-			ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
-					QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
-			if (ret)
-				return ret;
-		}
-
 		/* Individual user ports get connected to CPU port only */
 		if (dsa_is_user_port(ds, i)) {
 			ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),