Re: [PATCH net-next 10/13] dpaa2-switch: offload port objects on an upper bond device

[Ioana Ciornei]

On Wed, May 06, 2026 at 06:15:37PM +0300, Ioana Ciornei wrote:
> This patch adds support for offloading port objects, VLANs and MDBs,
> added on upper bond devices.
> 
> First of all, the use of the switchdev_handle_*() replication helpers
> is introduced for the SWITCHDEV_PORT_OBJ_ADD/SWITCHDEV_PORT_OBJ_DEL
> events. With this change, setting up the 'port_obj_info->handled = true'
> is not needed anymore since it's now handled by the new helpers.
> 
> In the DPAA2 architecture, there is no difference in adding a FDB or MDB
> which points towards a LAG port. Unlike other architectures, we do not
> need to populate all the possible destinations which are under the LAG,
> we only have to specify a single queueing destination (QDID) which
> represents the LAG. This all means that handling of MDBs in bond devices
> needs to have refcount mechanism as with the FDBs.
> This mechanism is triggered by calling the dpaa2_switch_lag_fdb_add() /
> dpaa2_switch_lag_fdb_del() functions which were added in the previous
> patch.
> 
> Signed-off-by: Ioana Ciornei <ioana.ciornei@xxxxxxx>
> ---
>  .../ethernet/freescale/dpaa2/dpaa2-switch.c   | 66 ++++++++++---------
>  1 file changed, 36 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> index cce0af47ca07..17a7a64064b5 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> @@ -2060,7 +2060,11 @@ static int dpaa2_switch_port_mdb_add(struct net_device *netdev,
>  	if (dpaa2_switch_port_lookup_address(netdev, 0, mdb->addr))
>  		return -EEXIST;
>  
> -	err = dpaa2_switch_port_fdb_add(port_priv, mdb->addr);
> +	if (port_priv->lag)
> +		err = dpaa2_switch_lag_fdb_add(port_priv->lag, mdb->addr,
> +					       mdb->vid);
> +	else
> +		err = dpaa2_switch_port_fdb_add(port_priv, mdb->addr);
>  	if (err)
>  		return err;

Sashiko notes:

	If dev_mc_add() fails further down in this function, the
	rollback path unconditionally calls
	dpaa2_switch_port_fdb_del_mc():

	err = dev_mc_add(netdev, mdb->addr);
	if (err) {
		netdev_err(netdev, "dev_mc_add err %d\n", err);
		dpaa2_switch_port_fdb_del_mc(port_priv, mdb->addr);
	}

	Does this bypass dpaa2_switch_lag_fdb_del() for LAG MDBs?
	It looks like this could leak the dpaa2_mac_addr structure
	allocated during dpaa2_switch_lag_fdb_add() and leave the
	software lag->fdbs list out of sync with the hardware FDB state.

Yes, the cleanup needs to also take into account if the port is under a
LAG or not. Will fix it.

(...)

> @@ -2178,9 +2190,10 @@ static int dpaa2_switch_port_mdb_del(struct net_device *netdev,
>  	if (!dpaa2_switch_port_lookup_address(netdev, 0, mdb->addr))
>  		return -ENOENT;
>  
> -	err = dpaa2_switch_port_fdb_del(port_priv, mdb->addr);
> -	if (err)
> -		return err;
> +	if (port_priv->lag)
> +		dpaa2_switch_lag_fdb_del(port_priv->lag, mdb->addr, mdb->vid);
> +	else
> +		dpaa2_switch_port_fdb_del(port_priv, mdb->addr);

Sashiko notes:

	Is it intentional to drop the error check for hardware FDB
	deletion here?

	Previously, the function returned early if
	dpaa2_switch_port_fdb_del() returned an error. By silently
	dropping the return value, the driver proceeds to remove the MAC
	from the device's multicast list anyway, which might mask
	hardware desynchronization regressions from the caller.

Yes, it was intentional. I think there is no point propagating such an
error on the remove path.