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

[Jakub Kicinski <kuba@kernel.org>]

Lots of small coding issues here..

On Tue, 27 Aug 2024 19:08:55 +0800 Wan Junjie wrote:
> +	/* Default vlan, use port's fdb id directly*/

add space at the end of the comment

> @@ -126,16 +135,28 @@ static void dpaa2_switch_fdb_get_flood_cfg(struct ethsw_core *ethsw, u16 fdb_id,
>  					   struct dpsw_egress_flood_cfg *cfg)
>  {
>  	int i = 0, j;

no need to init i any more

> +	u16 vid = 4096;

reorder the variable declarations, they should be sorted longest to
shortest

>  	memset(cfg, 0, sizeof(*cfg));
>  
> +	for (i = 0; i < ethsw->sw_attr.max_fdbs; i++) {
> +		if (ethsw->fdbs[i].fdb_id == fdb_id) {
> +			vid = ethsw->fdbs[i].vid;
> +			break;
> +		}
> +	}

> @@ -155,7 +176,7 @@ static void dpaa2_switch_fdb_get_flood_cfg(struct ethsw_core *ethsw, u16 fdb_id,
>  static int dpaa2_switch_fdb_set_egress_flood(struct ethsw_core *ethsw, u16 fdb_id)
>  {
>  	struct dpsw_egress_flood_cfg flood_cfg;
> -	int err;
> +	int err, i;

unused

>  	/* Setup broadcast flooding domain */
>  	dpaa2_switch_fdb_get_flood_cfg(ethsw, fdb_id, DPSW_BROADCAST, &flood_cfg);

> +	err = dpaa2_switch_fdb_set_egress_flood(ethsw, vcfg.fdb_id);
> +	if (err)
> +		return err;
> +
>  	return 0;

	return dpaa2_switch_fdb_set_egress_flood(ethsw, vcfg.fdb_id);


> +	/* mark fdb as unsued for this vlan */
> +	for (i = 0; i < ethsw->sw_attr.max_fdbs; i++) {
> +		fdb = ethsw->fdbs;
> +		if (fdb[i].vid == vid) {
> +			fdb[i].in_use = false;
> +		}

no need for {} in case of the 'if'

>  static void dpaa2_switch_port_fast_age(struct ethsw_port_priv *port_priv)
>  {
> -	dpaa2_switch_fdb_iterate(port_priv,
> -				 dpaa2_switch_fdb_entry_fast_age, NULL);
> +	u16 vid;
> +
> +	for (vid = 0; vid <= VLAN_VID_MASK; vid++) {
> +		if (port_priv->vlans[vid] & ETHSW_VLAN_MEMBER) {
> +			dpaa2_switch_fdb_iterate(port_priv,
> +						 dpaa2_switch_fdb_entry_fast_age, NULL);
> +		}

same here

> +	}
>  }
>  
>  static int dpaa2_switch_port_vlan_add(struct net_device *netdev, __be16 proto,
> @@ -1670,10 +1752,24 @@ static int dpaa2_switch_port_attr_stp_state_set(struct net_device *netdev,
>  	return err;
>  }
>  
> +static int dpaa2_switch_port_flood_vlan(struct net_device *vdev, int vid, void *arg)
> +{
> +	struct ethsw_port_priv *port_priv = netdev_priv(arg);
> +	struct ethsw_core *ethsw = port_priv->ethsw_data;
> +
> +	if (!vdev)
> +		return -ENODEV;
> +
> +	return dpaa2_switch_fdb_set_egress_flood(ethsw,
> +						  dpaa2_switch_port_get_fdb_id(port_priv, vid));

save the return value of dpaa2_switch_port_get_fdb_id(port_priv, vid)
to a temp variable, avoid long lines

> +}


> @@ -1681,6 +1777,12 @@ static int dpaa2_switch_port_flood(struct ethsw_port_priv *port_priv,
>  	if (flags.mask & BR_FLOOD)
>  		port_priv->ucast_flood = !!(flags.val & BR_FLOOD);
>  
> +	/* Recreate the egress flood domain of every vlan domain */
> +	err = vlan_for_each(netdev, dpaa2_switch_port_flood_vlan, netdev);
> +	if (err)
> +		netdev_err(netdev, "Unable to restore vlan flood err (%d)\n", err);
> +		return err;

and here you're missing brackets :S

>  	return dpaa2_switch_fdb_set_egress_flood(ethsw, port_priv->fdb->fdb_id);
>  }
>  
-- 
pw-bot: cr