Date:   Fri, 26 Nov 2021 03:59:36 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Oleksij Rempel <o.rempel@...gutronix.de>
Cc:     Woojung Huh <woojung.huh@...rochip.com>,
        UNGLinuxDriver@...rochip.com, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, kernel@...gutronix.de,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net v2 1/1] net: dsa: microchip: implement multi-bridge
 support

On Wed, Nov 24, 2021 at 02:31:43PM +0100, Oleksij Rempel wrote:
> Current driver version is able to handle only one bridge at time.
> Configuring two bridges on two different ports would end up shorting this
> bridges by HW. To reproduce it:
> 
> 	ip l a name br0 type bridge
> 	ip l a name br1 type bridge
> 	ip l s dev br0 up
> 	ip l s dev br1 up
> 	ip l s lan1 master br0
> 	ip l s dev lan1 up
> 	ip l s lan2 master br1
> 	ip l s dev lan2 up
> 
> 	Ping on lan1 and get response on lan2, which should not happen.
> 
> This happened, because current driver version is storing one global "Port VLAN
> Membership" and applying it to all ports which are members of any
> bridge.
> To solve this issue, we need to handle each port separately.
> 
> This patch is dropping the global port member storage and calculating
> membership dynamically depending on STP state and bridge participation.
> 
> Note: STP support was broken before this patch and should be fixed
> separately.
> 
> Fixes: c2e866911e25 ("net: dsa: microchip: break KSZ9477 DSA driver into two files")
> Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>
> ---
>  drivers/net/dsa/microchip/ksz8795.c    | 53 ++++-----------------
>  drivers/net/dsa/microchip/ksz9477.c    | 64 ++++----------------------
>  drivers/net/dsa/microchip/ksz_common.c | 50 +++++++++++---------
>  drivers/net/dsa/microchip/ksz_common.h |  1 -
>  4 files changed, 45 insertions(+), 123 deletions(-)

Generally I like this patch, and the diffstat says it all, the existing
logic is a huge mess that isn't even thought out properly.

> 
> diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
> index 43fc3087aeb3..b5f6dff70c89 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -1008,51 +1008,27 @@ static void ksz8_cfg_port_member(struct ksz_device *dev, int port, u8 member)
>  static void ksz8_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
>  {
>  	struct ksz_device *dev = ds->priv;
> -	int forward = dev->member;
>  	struct ksz_port *p;
> -	int member = -1;
>  	u8 data;
>  
> -	p = &dev->ports[port];
> -
>  	ksz_pread8(dev, port, P_STP_CTRL, &data);
>  	data &= ~(PORT_TX_ENABLE | PORT_RX_ENABLE | PORT_LEARN_DISABLE);
>  
>  	switch (state) {
>  	case BR_STATE_DISABLED:
>  		data |= PORT_LEARN_DISABLE;
> -		if (port < dev->phy_port_cnt)
> -			member = 0;
>  		break;
>  	case BR_STATE_LISTENING:
>  		data |= (PORT_RX_ENABLE | PORT_LEARN_DISABLE);
> -		if (port < dev->phy_port_cnt &&
> -		    p->stp_state == BR_STATE_DISABLED)
> -			member = dev->host_mask | p->vid_member;
>  		break;
>  	case BR_STATE_LEARNING:
>  		data |= PORT_RX_ENABLE;
>  		break;
>  	case BR_STATE_FORWARDING:
>  		data |= (PORT_TX_ENABLE | PORT_RX_ENABLE);
> -
> -		/* This function is also used internally. */
> -		if (port == dev->cpu_port)
> -			break;
> -
> -		/* Port is a member of a bridge. */
> -		if (dev->br_member & BIT(port)) {
> -			dev->member |= BIT(port);
> -			member = dev->member;
> -		} else {
> -			member = dev->host_mask | p->vid_member;
> -		}
>  		break;
>  	case BR_STATE_BLOCKING:
>  		data |= PORT_LEARN_DISABLE;
> -		if (port < dev->phy_port_cnt &&
> -		    p->stp_state == BR_STATE_DISABLED)
> -			member = dev->host_mask | p->vid_member;
>  		break;
>  	default:
>  		dev_err(ds->dev, "invalid STP state: %d\n", state);
> @@ -1060,22 +1036,11 @@ static void ksz8_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
>  	}
>  
>  	ksz_pwrite8(dev, port, P_STP_CTRL, data);
> +
> +	p = &dev->ports[port];
>  	p->stp_state = state;
> -	/* Port membership may share register with STP state. */
> -	if (member >= 0 && member != p->member)
> -		ksz8_cfg_port_member(dev, port, (u8)member);
> -
> -	/* Check if forwarding needs to be updated. */
> -	if (state != BR_STATE_FORWARDING) {
> -		if (dev->br_member & BIT(port))
> -			dev->member &= ~BIT(port);
> -	}
>  
> -	/* When topology has changed the function ksz_update_port_member
> -	 * should be called to modify port forwarding behavior.
> -	 */
> -	if (forward != dev->member)
> -		ksz_update_port_member(dev, port);
> +	ksz_update_port_member(dev, port);
>  }
>  
>  static void ksz8_flush_dyn_mac_table(struct ksz_device *dev, int port)
> @@ -1341,7 +1306,7 @@ static void ksz8795_cpu_interface_select(struct ksz_device *dev, int port)
>  
>  static void ksz8_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  {
> -	struct ksz_port *p = &dev->ports[port];
> +	struct dsa_switch *ds = dev->ds;
>  	struct ksz8 *ksz8 = dev->priv;
>  	const u32 *masks;
>  	u8 member;
> @@ -1364,14 +1329,15 @@ static void ksz8_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  	/* enable 802.1p priority */
>  	ksz_port_cfg(dev, port, P_PRIO_CTRL, PORT_802_1P_ENABLE, true);
>  
> -	if (cpu_port) {
> +	if (dsa_is_cpu_port(ds, port)) {

Why did you make this change? The "cpu_port" argument is no longer used now.

>  		if (!ksz_is_ksz88x3(dev))
>  			ksz8795_cpu_interface_select(dev, port);
>  
> -		member = dev->port_mask;
> +		member = dsa_user_ports(ds);
>  	} else {
> -		member = dev->host_mask | p->vid_member;
> +		member = BIT(dsa_upstream_port(ds, port));
>  	}
> +
>  	ksz8_cfg_port_member(dev, port, member);
>  }
>  
> @@ -1392,11 +1358,9 @@ static void ksz8_config_cpu_port(struct dsa_switch *ds)
>  	ksz_cfg(dev, regs[S_TAIL_TAG_CTRL], masks[SW_TAIL_TAG_ENABLE], true);
>  
>  	p = &dev->ports[dev->cpu_port];
> -	p->vid_member = dev->port_mask;
>  	p->on = 1;
>  
>  	ksz8_port_setup(dev, dev->cpu_port, true);
> -	dev->member = dev->host_mask;
>  
>  	for (i = 0; i < dev->phy_port_cnt; i++) {
>  		p = &dev->ports[i];
> @@ -1404,7 +1368,6 @@ static void ksz8_config_cpu_port(struct dsa_switch *ds)
>  		/* Initialize to non-zero so that ksz_cfg_port_member() will
>  		 * be called.
>  		 */

These comments seem out of date, and the ksz_cfg_port_member() function
does not exist either. Can you please update them?

> -		p->vid_member = BIT(i);
>  		p->member = dev->port_mask;
>  		ksz8_port_stp_state_set(ds, i, BR_STATE_DISABLED);
>  
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index 854e25f43fa7..3e7df6c0dc72 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -400,8 +400,6 @@ static void ksz9477_port_stp_state_set(struct dsa_switch *ds, int port,
>  	struct ksz_device *dev = ds->priv;
>  	struct ksz_port *p = &dev->ports[port];
>  	u8 data;
> -	int member = -1;
> -	int forward = dev->member;
>  
>  	ksz_pread8(dev, port, P_STP_CTRL, &data);
>  	data &= ~(PORT_TX_ENABLE | PORT_RX_ENABLE | PORT_LEARN_DISABLE);
> @@ -409,40 +407,18 @@ static void ksz9477_port_stp_state_set(struct dsa_switch *ds, int port,
>  	switch (state) {
>  	case BR_STATE_DISABLED:
>  		data |= PORT_LEARN_DISABLE;
> -		if (port != dev->cpu_port)
> -			member = 0;
>  		break;
>  	case BR_STATE_LISTENING:
>  		data |= (PORT_RX_ENABLE | PORT_LEARN_DISABLE);
> -		if (port != dev->cpu_port &&
> -		    p->stp_state == BR_STATE_DISABLED)
> -			member = dev->host_mask | p->vid_member;
>  		break;
>  	case BR_STATE_LEARNING:
>  		data |= PORT_RX_ENABLE;
>  		break;
>  	case BR_STATE_FORWARDING:
>  		data |= (PORT_TX_ENABLE | PORT_RX_ENABLE);
> -
> -		/* This function is also used internally. */
> -		if (port == dev->cpu_port)
> -			break;
> -
> -		member = dev->host_mask | p->vid_member;
> -		mutex_lock(&dev->dev_mutex);
> -
> -		/* Port is a member of a bridge. */
> -		if (dev->br_member & (1 << port)) {
> -			dev->member |= (1 << port);
> -			member = dev->member;
> -		}
> -		mutex_unlock(&dev->dev_mutex);
>  		break;
>  	case BR_STATE_BLOCKING:
>  		data |= PORT_LEARN_DISABLE;
> -		if (port != dev->cpu_port &&
> -		    p->stp_state == BR_STATE_DISABLED)
> -			member = dev->host_mask | p->vid_member;
>  		break;
>  	default:
>  		dev_err(ds->dev, "invalid STP state: %d\n", state);
> @@ -451,23 +427,8 @@ static void ksz9477_port_stp_state_set(struct dsa_switch *ds, int port,
>  
>  	ksz_pwrite8(dev, port, P_STP_CTRL, data);
>  	p->stp_state = state;
> -	mutex_lock(&dev->dev_mutex);
> -	/* Port membership may share register with STP state. */
> -	if (member >= 0 && member != p->member)
> -		ksz9477_cfg_port_member(dev, port, (u8)member);
> -
> -	/* Check if forwarding needs to be updated. */
> -	if (state != BR_STATE_FORWARDING) {
> -		if (dev->br_member & (1 << port))
> -			dev->member &= ~(1 << port);
> -	}
>  
> -	/* When topology has changed the function ksz_update_port_member
> -	 * should be called to modify port forwarding behavior.
> -	 */
> -	if (forward != dev->member)
> -		ksz_update_port_member(dev, port);
> -	mutex_unlock(&dev->dev_mutex);
> +	ksz_update_port_member(dev, port);
>  }
>  
>  static void ksz9477_flush_dyn_mac_table(struct ksz_device *dev, int port)
> @@ -1168,10 +1129,10 @@ static void ksz9477_phy_errata_setup(struct ksz_device *dev, int port)
>  
>  static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  {
> -	u8 data8;
> -	u8 member;
> -	u16 data16;
>  	struct ksz_port *p = &dev->ports[port];
> +	struct dsa_switch *ds = dev->ds;
> +	u8 data8, member;
> +	u16 data16;
>  
>  	/* enable tag tail for host port */
>  	if (cpu_port)
> @@ -1250,12 +1211,12 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  		ksz_pwrite8(dev, port, REG_PORT_XMII_CTRL_1, data8);
>  		p->phydev.duplex = 1;
>  	}
> -	mutex_lock(&dev->dev_mutex);
> -	if (cpu_port)
> -		member = dev->port_mask;
> +
> +	if (dsa_is_cpu_port(ds, port))
> +		member = dsa_user_ports(ds);
>  	else
> -		member = dev->host_mask | p->vid_member;
> -	mutex_unlock(&dev->dev_mutex);
> +		member = BIT(dsa_upstream_port(ds, port));
> +
>  	ksz9477_cfg_port_member(dev, port, member);
>  
>  	/* clear pending interrupts */
> @@ -1276,8 +1237,6 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds)
>  			const char *prev_mode;
>  
>  			dev->cpu_port = i;
> -			dev->host_mask = (1 << dev->cpu_port);
> -			dev->port_mask |= dev->host_mask;
>  			p = &dev->ports[i];
>  
>  			/* Read from XMII register to determine host port
> @@ -1312,13 +1271,10 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds)
>  
>  			/* enable cpu port */
>  			ksz9477_port_setup(dev, i, true);
> -			p->vid_member = dev->port_mask;
>  			p->on = 1;
>  		}
>  	}
>  
> -	dev->member = dev->host_mask;
> -
>  	for (i = 0; i < dev->port_cnt; i++) {
>  		if (i == dev->cpu_port)
>  			continue;
> @@ -1327,8 +1283,6 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds)
>  		/* Initialize to non-zero so that ksz_cfg_port_member() will
>  		 * be called.
>  		 */
> -		p->vid_member = (1 << i);
> -		p->member = dev->port_mask;
>  		ksz9477_port_stp_state_set(ds, i, BR_STATE_DISABLED);
>  		p->on = 1;
>  		if (i < dev->phy_port_cnt)
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 7c2968a639eb..39583920d6e9 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -22,21 +22,40 @@
>  
>  void ksz_update_port_member(struct ksz_device *dev, int port)
>  {
> -	struct ksz_port *p;
> +	struct ksz_port *p = &dev->ports[port];
> +	struct dsa_switch *ds = dev->ds;
> +	const struct dsa_port *dp;

Reverse Christmas notation.

> +	u8 port_member = 0, cpu_port;
>  	int i;
>  
> -	for (i = 0; i < dev->port_cnt; i++) {
> -		if (i == port || i == dev->cpu_port)
> +	if (!dsa_is_user_port(ds, port))
> +		return;
> +
> +	dp = dsa_to_port(ds, port);
> +	cpu_port = BIT(dsa_upstream_port(ds, port));
> +
> +	for (i = 0; i < ds->num_ports; i++) {
> +		const struct dsa_port *other_dp = dsa_to_port(ds, i);
> +		struct ksz_port *other_p = &dev->ports[i];
> +		u8 val = 0;
> +
> +		if (!dsa_is_user_port(ds, i))
>  			continue;
> -		p = &dev->ports[i];
> -		if (!(dev->member & (1 << i)))
> +		if (port == i)
> +			continue;
> +		if (!dp->bridge_dev || dp->bridge_dev != other_dp->bridge_dev)
>  			continue;
>  
> -		/* Port is a member of the bridge and is forwarding. */
> -		if (p->stp_state == BR_STATE_FORWARDING &&
> -		    p->member != dev->member)
> -			dev->dev_ops->cfg_port_member(dev, i, dev->member);
> +		if (other_p->stp_state == BR_STATE_FORWARDING &&
> +		    p->stp_state == BR_STATE_FORWARDING) {
> +			val |= BIT(port);
> +			port_member |= BIT(i);
> +		}
> +
> +		dev->dev_ops->cfg_port_member(dev, i, val | cpu_port);
>  	}
> +
> +	dev->dev_ops->cfg_port_member(dev, port, port_member | cpu_port);
>  }
>  EXPORT_SYMBOL_GPL(ksz_update_port_member);
>  
> @@ -175,12 +194,6 @@ EXPORT_SYMBOL_GPL(ksz_get_ethtool_stats);
>  int ksz_port_bridge_join(struct dsa_switch *ds, int port,
>  			 struct net_device *br)
>  {
> -	struct ksz_device *dev = ds->priv;
> -
> -	mutex_lock(&dev->dev_mutex);
> -	dev->br_member |= (1 << port);
> -	mutex_unlock(&dev->dev_mutex);
> -
>  	/* port_stp_state_set() will be called after to put the port in
>  	 * appropriate state so there is no need to do anything.
>  	 */
> @@ -192,13 +205,6 @@ EXPORT_SYMBOL_GPL(ksz_port_bridge_join);
>  void ksz_port_bridge_leave(struct dsa_switch *ds, int port,
>  			   struct net_device *br)
>  {
> -	struct ksz_device *dev = ds->priv;
> -
> -	mutex_lock(&dev->dev_mutex);
> -	dev->br_member &= ~(1 << port);
> -	dev->member &= ~(1 << port);
> -	mutex_unlock(&dev->dev_mutex);
> -
>  	/* port_stp_state_set() will be called after to put the port in
>  	 * forwarding state so there is no need to do anything.
>  	 */
> diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
> index 1597c63988b4..18c9b2e34cd1 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -26,7 +26,6 @@ struct ksz_port_mib {
>  
>  struct ksz_port {
>  	u16 member;
> -	u16 vid_member;

Can you also delete the remaining writes to p->member from ksz8795.c and
ksz9477.c? It appears that it is no longer used now.

>  	bool remove_tag;		/* Remove Tag flag set, for ksz8795 only */
>  	int stp_state;
>  	struct phy_device phydev;
> -- 
> 2.30.2
> 

Powered by blists - more mailing lists