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

From: Jakub Kicinski
Date: Tue Aug 27 2024 - 17:57:57 EST


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