Message-ID: <20230912222302.jxolk3t74vbgr35s@skbuf>
Date: Wed, 13 Sep 2023 01:23:02 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Pawel Dembicki <paweldembicki@...il.com>
Cc: netdev@...r.kernel.org, Dan Carpenter <dan.carpenter@...aro.org>,
	Simon Horman <simon.horman@...igine.com>,
	Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Russell King <linux@...linux.org.uk>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v3 8/8] net: dsa: vsc73xx: Add bridge support

On Tue, Sep 12, 2023 at 02:22:02PM +0200, Pawel Dembicki wrote:
> This patch adds bridge support for vsc73xx driver.
> It introduce two functions for port_bridge_join and
> vsc73xx_port_bridge_leave handling.
> 
> Those functions implement forwarding adjust and use
> dsa_tag_8021q_bridge_* api for adjust VLAN configuration.
> 
> Signed-off-by: Pawel Dembicki <paweldembicki@...il.com>
> ---
> v3:
>   - All vlan commits was reworked
>   - move VLAN_AWR and VLAN_DBLAWR to port setup in other commit
>   - drop vlan table upgrade
> v2:
>   - no changes done
> 
>  drivers/net/dsa/vitesse-vsc73xx-core.c | 72 ++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> index bf903502bac1..9d0367c2c52c 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> @@ -600,6 +600,9 @@ static int vsc73xx_setup(struct dsa_switch *ds)
>  
>  	dev_info(vsc->dev, "set up the switch\n");
>  
> +	ds->untag_bridge_pvid = true;
> +	ds->max_num_bridges = 7;

Can you please refactor this into DSA_TAG_8021Q_MAX_NUM_BRIDGES and use
it in sja1105 too?

> +
>  	/* Issue RESET */
>  	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
>  		      VSC73XX_GLORESET_MASTER_RESET);
> @@ -1456,6 +1459,73 @@ static int vsc73xx_tag_8021q_vlan_del(struct dsa_switch *ds, int port, u16 vid)
>  	return vsc73xx_update_vlan_table(vsc, port, vid, 0);
>  }
>  
> +static void vsc73xx_update_forwarding_map(struct vsc73xx *vsc)
> +{
> +	int i;
> +
> +	for (i = 0; i < vsc->ds->num_ports; i++) {
> +		u32 val;
> +
> +		vsc73xx_read(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +			     VSC73XX_SRCMASKS + i, &val);
> +		/* update only if port is in forwarding state */
> +		if (val & VSC73XX_SRCMASKS_PORTS_MASK)
> +			vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +					    VSC73XX_SRCMASKS + i,
> +					    VSC73XX_SRCMASKS_PORTS_MASK,
> +					    vsc->forward_map[i]);
> +	}
> +}

I suspect you'll have to rethink this. If you look at del_nbp() and dsa_port_bridge_leave(),
you'll see it goes through a few phases. First the bridge calls br_stp_disable_port(p),
then netdev_upper_dev_unlink(dev, br->dev) which invokes dsa_port_bridge_leave().
So at this stage, the port is in BR_STATE_DISABLED and ds->ops->port_stp_state_set()
duly notifies the driver of that.

Then, ds->ops->port_bridge_leave() gets called, and then, ds->ops->port_stp_state_set()
again, for the standalone port, in BR_STATE_FORWARDING.

So actually, the last to take effect upon the forwarding map is port_stp_state_set(),
not port_bridge_leave().

I suspect you can remove the vsc73xx_update_forwarding_map() and just
rely on the STP implementation, and fix that while you're at it.

On the other switch ports except the one on which the STP state is changing,
you can look at dp->stp_state. There needs to be an "administrative" forwarding
mask (determined by port_bridge_join and port_bridge_leave), and an "operational"
one (determined by STP states).

> +
> +static int vsc73xx_port_bridge_join(struct dsa_switch *ds, int port,
> +				    struct dsa_bridge bridge,
> +				    bool *tx_fwd_offload,
> +				    struct netlink_ext_ack *extack)
> +{
> +	struct vsc73xx *vsc = ds->priv;
> +	int i;
> +
> +	*tx_fwd_offload = true;
> +
> +	for (i = 0; i < ds->num_ports; i++) {
> +		/* Add this port to the forwarding matrix of the
> +		 * other ports in the same bridge, and viceversa.
> +		 */
> +		if (!dsa_is_user_port(ds, i))
> +			continue;

	dsa_switch_for_each_user_port(other_dp, ds) please

it is a lower-complexity iteration over the port list, which should be
preferred over a for loop + any dsa_to_port() wrapper like dsa_is_user_port().

> +
> +		if (i == port)
> +			continue;
> +
> +		if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge))

and "other_dp" here

> +			continue;
> +
> +		vsc->forward_map[port] |= VSC73XX_SRCMASKS_PORTS_MASK & BIT(i);
> +		vsc->forward_map[i] |= VSC73XX_SRCMASKS_PORTS_MASK & BIT(port);
> +	}
> +	vsc73xx_update_forwarding_map(vsc);
> +
> +	return dsa_tag_8021q_bridge_join(ds, port, bridge);
> +}
> +
> +static void vsc73xx_port_bridge_leave(struct dsa_switch *ds, int port,
> +				      struct dsa_bridge bridge)
> +{
> +	struct vsc73xx *vsc = ds->priv;
> +	int i;
> +
> +	/* configure forward map to CPU <-> port only */
> +	for (i = 0; i < vsc->ds->num_ports; i++) {
> +		if (i == CPU_PORT)
> +			continue;
> +		vsc->forward_map[i] &= VSC73XX_SRCMASKS_PORTS_MASK & ~BIT(port);
> +	}
> +	vsc->forward_map[port] = VSC73XX_SRCMASKS_PORTS_MASK & BIT(CPU_PORT);
> +
> +	vsc73xx_update_forwarding_map(vsc);
> +	dsa_tag_8021q_bridge_leave(ds, port, bridge);
> +}
> +
>  static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
>  {
>  	struct vsc73xx *vsc = ds->priv;
> @@ -1557,6 +1627,8 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
>  	.get_sset_count = vsc73xx_get_sset_count,
>  	.port_enable = vsc73xx_port_enable,
>  	.port_disable = vsc73xx_port_disable,
> +	.port_bridge_join = vsc73xx_port_bridge_join,
> +	.port_bridge_leave = vsc73xx_port_bridge_leave,
>  	.port_change_mtu = vsc73xx_change_mtu,
>  	.port_max_mtu = vsc73xx_get_max_mtu,
>  	.port_stp_state_set = vsc73xx_port_stp_state_set,
> -- 
> 2.34.1
> 

Powered by blists - more mailing lists