Re: [PATCH v2] dpaa2-switch: fix flooding domain among multiple vlans

[Simon Horman <horms@kernel.org>]

On Mon, Sep 02, 2024 at 09:50:51AM +0800, Wan Junjie wrote:
> Currently, dpaa2 switch only cares dst mac and egress interface
> in FDB. And all ports with different vlans share the same FDB.
> 
> This will make things messed up when one device connected to
> dpaa2 switch via two interfaces. Ports get two different vlans
> assigned. These two ports will race for a same dst mac entry
> since multiple vlans share one FDB.
> 
> FDB below may not show up at the same time.
> 02:00:77:88:99:aa dev swp0 self
> 02:00:77:88:99:aa dev swp1 self
> But in fact, for rules on the bridge, they should be:
> 02:00:77:88:99:aa dev swp0 vlan 10 master br0
> 02:00:77:88:99:aa dev swp1 vlan 20 master br0
> 
> This patch address this by borrowing unused form ports' FDB
> when ports join bridge. And append offload flag to hardware
> offloaded rules so we can tell them from those on bridges.
> 
> Signed-off-by: Wan Junjie <junjie.wan@inceptio.ai>

Hi Wan Junjie,

Some minor feedback from my side.

...

> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> index a293b08f36d4..217c68bb0faa 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> @@ -25,8 +25,17 @@
>  
>  #define DEFAULT_VLAN_ID			1
>  
> -static u16 dpaa2_switch_port_get_fdb_id(struct ethsw_port_priv *port_priv)
> +static u16 dpaa2_switch_port_get_fdb_id(struct ethsw_port_priv *port_priv, u16 vid)

This, and several other lines in this patch, could be trivially
line wrapped in order for them to be <= 80 columns wide, as is
still preferred in Networking code.

This and a number of other minor problems are flagged by:
./scripts/checkpatch.pl --strict --codespell --max-line-length=80

>  {
> +	struct ethsw_core *ethsw = port_priv->ethsw_data;
> +	int i;
> +
> +	if (port_priv->fdb->bridge_dev) {
> +		for (i = 0; i < ethsw->sw_attr.max_fdbs; i++)
> +			if (ethsw->fdbs[i].vid == vid)
> +				return ethsw->fdbs[i].fdb_id;
> +	}
> +	/* Default vlan, use port's fdb id directly */
>  	return port_priv->fdb->fdb_id;
>  }
>  

...

> @@ -191,10 +212,38 @@ static void *dpaa2_iova_to_virt(struct iommu_domain *domain,
>  static int dpaa2_switch_add_vlan(struct ethsw_port_priv *port_priv, u16 vid)
>  {
>  	struct ethsw_core *ethsw = port_priv->ethsw_data;
> +	struct net_device *netdev = port_priv->netdev;
> +	struct dpsw_fdb_cfg fdb_cfg = {0};
>  	struct dpsw_vlan_cfg vcfg = {0};
> +	struct dpaa2_switch_fdb *fdb;
> +	u16 fdb_id;
>  	int err;
>  
> -	vcfg.fdb_id = dpaa2_switch_port_get_fdb_id(port_priv);
> +	/* If ports are under a bridge, then
> +	 * every VLAN domain should use a different fdb.
> +	 * If ports are standalone, and
> +	 * vid is 1 this should reuse the allocated port fdb.
> +	 */
> +	if (port_priv->fdb->bridge_dev) {
> +		fdb = dpaa2_switch_fdb_get_unused(ethsw);
> +		if (!fdb) {
> +			/* if not available, create a new fdb */
> +			err = dpsw_fdb_add(ethsw->mc_io, 0, ethsw->dpsw_handle,
> +					   &fdb_id, &fdb_cfg);
> +			if (err) {
> +				netdev_err(netdev, "dpsw_fdb_add err %d\n", err);
> +				return err;
> +			}

fdb is still NULL here. Based on my reading of dpaa2_switch_port_init()
I think you need the following. Possibly also with an error check.

			fdb = dpaa2_switch_fdb_get_unused(ethsw);

Flagged by Smatch.

> +			fdb->fdb_id = fdb_id;
> +		}
> +		fdb->vid = vid;
> +		fdb->in_use = true;
> +		fdb->bridge_dev = NULL;
> +		vcfg.fdb_id = fdb->fdb_id;
> +	} else {
> +		/* Standalone, port's private fdb shared */
> +		vcfg.fdb_id = dpaa2_switch_port_get_fdb_id(port_priv, vid);
> +	}
>  	err = dpsw_vlan_add(ethsw->mc_io, 0,
>  			    ethsw->dpsw_handle, vid, &vcfg);
>  	if (err) {

...

-- 
pw-bot: cr