Re: [RFC PATCH v3 net-next 22/24] net: dsa: add support for bridge TX forwarding offload

[Vladimir Oltean <olteanv@gmail.com>]

On Mon, Jul 12, 2021 at 06:21:40PM +0300, Vladimir Oltean wrote:
> +static bool dsa_port_bridge_tx_fwd_prepare(struct dsa_port *dp,
> +					   struct net_device *bridge_dev)
> +{
> +	struct dsa_switch *ds = dp->ds;
> +	struct dsa_switch_tree *dst;
> +	int bridge_num, err;
> +
> +	dst = ds->dst;
> +
> +	bridge_num = dsa_tree_find_bridge_num(dst, bridge_dev);
> +	if (bridge_num < 0) {
> +		/* First port that offloads TX forwarding for this bridge */
> +		int bridge_num;

Stupid bug: "bridge_num" is redeclared here as a different variable with
a different scope, shadowing the one from dsa_port_bridge_tx_fwd_prepare().

> +
> +		bridge_num = find_first_zero_bit(&dst->fwd_offloading_bridges,
> +						 DSA_MAX_NUM_OFFLOADING_BRIDGES);
> +		if (bridge_num >= ds->num_fwd_offloading_bridges)
> +			return false;
> +
> +		set_bit(bridge_num, &dst->fwd_offloading_bridges);
> +	}
> +
> +	dp->bridge_num = bridge_num;

and then here, the scope from the "if" block is lost, so the bridge_num
variable is still -1. So dp->bridge_num remains -1.

Deleting the "int bridge_num" declaration from the "if" block fixes the
issue. This got introduced between v2 and v3.

> +
> +	/* Notify the driver */
> +	err = dsa_port_bridge_fwd_offload_add(dp, bridge_dev, bridge_num);
> +	if (err) {
> +		dsa_port_bridge_tx_fwd_unprepare(dp, bridge_dev);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>  int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br,
>  			 struct netlink_ext_ack *extack)
>  {
> @@ -241,6 +310,7 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br,
>  	};
>  	struct net_device *dev = dp->slave;
>  	struct net_device *brport_dev;
> +	bool tx_fwd_offload;
>  	int err;
>  
>  	/* Here the interface is already bridged. Reflect the current
> @@ -254,7 +324,10 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br,
>  	if (err)
>  		goto out_rollback;
>  
> -	err = switchdev_bridge_port_offload(brport_dev, dev, dp, false, extack);
> +	tx_fwd_offload = dsa_port_bridge_tx_fwd_prepare(dp, br);
> +
> +	err = switchdev_bridge_port_offload(brport_dev, dev, dp, tx_fwd_offload,
> +					    extack);
>  	if (err)
>  		goto out_rollback_unbridge;
>  
> @@ -279,6 +352,8 @@ int dsa_port_pre_bridge_leave(struct dsa_port *dp, struct net_device *br,
>  	struct net_device *brport_dev = dsa_port_to_bridge_port(dp);
>  	struct net_device *dev = dp->slave;
>  
> +	dsa_port_bridge_tx_fwd_prepare(dp, br);

We are in the pre_bridge_leave path, this should have been _unprepare.

> +
>  	return switchdev_bridge_port_unoffload(brport_dev, dev, dp, extack);
>  }

The patches should work otherwise, if somebody wants to test they should
make these changes.

There are also some more changes that need to be made to mlxsw to
properly handle the unoffload of bridge ports that are LAG devices and
VLAN devices. I have them in my tree now. When net-next reopens I will
first send a series of 7 refactoring patches for dpaa2-switch, mlxsw and
prestera, and then the TX data plane offload in DSA as a 15-patch series.