diff mbox series

[net-next,v3,8/8] net: dsa: vsc73xx: Add bridge support

Message ID 20230912122201.3752918-9-paweldembicki@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: dsa: vsc73xx: Make vsc73xx usable | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1341 this patch: 1341
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 1364 this patch: 1364
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1364 this patch: 1364
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 90 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Pawel Dembicki Sept. 12, 2023, 12:22 p.m. UTC 
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@gmail.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(+)

Comments

Vladimir Oltean Sept. 12, 2023, 10:23 p.m. UTC | #1 
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@gmail.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
>
diff mbox series

Patch

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;
+
 	/* 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]);
+	}
+}
+
+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;
+
+		if (i == port)
+			continue;
+
+		if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge))
+			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,