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

[Ioana Ciornei]

On Fri, Jun 05, 2026 at 07:10:56PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> dpaa2-switch: offload port objects on an upper bond device
> 
> This patch adds support for offloading port objects, VLANs and MDBs,
> added on upper bond devices. It introduces switchdev_handle_*() replication
> helpers for port objects and a refcount mechanism for handling MDBs in bond
> devices.
> 
> > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> > index fadebf6a758d1..9aba5f348abc0 100644
> > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> > @@ -2215,14 +2231,17 @@ static int dpaa2_switch_port_mdb_del(struct net_device *netdev,
> >  				     const struct switchdev_obj_port_mdb *mdb)
> >  {
> >  	struct ethsw_port_priv *port_priv = netdev_priv(netdev);
> > +	struct dpaa2_switch_lag *lag;
> >  	int err;
> >  
> >  	if (!dpaa2_switch_port_lookup_address(netdev, 0, mdb->addr))
> >  		return -ENOENT;
> 
> [Severity: High]
> Does returning an error here abort the switchdev_handle_port_obj_del()
> replication loop for the remaining bond slaves?
> 
> This patch uses switchdev_handle_port_obj_del() to replicate object
> deletions across all slave devices. If a driver callback returns an error
> other than -EOPNOTSUPP on one of the slaves, the replication loop aborts,
> skipping all remaining slaves.
> 
> For example, dpaa2_switch_port_mdb_del() returns -ENOENT if the MAC is not
> found, and dpaa2_switch_port_vlans_del() propagates hardware deletion errors.
> 
> Will this leave the remaining slave ports with stale VLANs and MDBs fully
> active in software and hardware, causing silent data leaks and inconsistent
> state?

I don't see how I could fix this at driver level without a change in
switchdev_handle_port_obj_del() or the callback prototype for del_cb.

> 
> [ ... ]
> 
> > @@ -3128,37 +3154,23 @@ static int dpaa2_switch_port_event(struct notifier_block *nb,
> >  	}
> >  }
> >  
> > -static int dpaa2_switch_port_obj_event(unsigned long event,
> [ ... ]
> >  static int dpaa2_switch_port_blocking_event(struct notifier_block *nb,
> >  					    unsigned long event, void *ptr)
> >  {
> >  	struct net_device *dev = switchdev_notifier_info_to_dev(ptr);
> > +	int err;
> >  
> >  	switch (event) {
> >  	case SWITCHDEV_PORT_OBJ_ADD:
> > +		err = switchdev_handle_port_obj_add(dev, ptr,
> > +						    dpaa2_switch_port_dev_check,
> > +						    dpaa2_switch_port_obj_add);
> > +		return notifier_from_errno(err);
> 
> [Severity: High]
> Can this code leak memory and hardware MDBs when a slave leaves a bond?
> 
> When switchdev_handle_port_obj_add() replicates MDB additions across all
> bond slaves, it increments the a->refcount in lag->fdbs for each slave. 
> 
> If a slave leaves the bond (e.g., via ip link set eth1 nomaster), shouldn't
> dpaa2_switch_port_bond_leave() be updated to decrement the slave's share
> of the MDB refcounts?
> 
> Without this, when the bond is later removed from the bridge,
> SWITCHDEV_PORT_OBJ_DEL replicates only over the remaining slaves. The MDB
> refcounts will not reach zero because the decrements from the departed
> slaves are permanently lost. This would leak struct dpaa2_mac_addr memory
> and leave the hardware MDB permanently programmed on the LAG.

No, the concern is not valid. Please look at the
nbp_switchdev_unsync_objs() function called from
switchdev_bridge_port_unoffload() -> call_switchdev_blocking_notifiers()
-> br_switchdev_blocking_event() -> br_switchdev_port_unoffload().