Re: [PATCH net-next v3 10/12] dpaa2-switch: offload port objects on an upper bond device
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-06-06 02:11:04
Also in:
lkml
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.
quoted hunk ↗ jump to hunk
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@@ -2096,30 +2096,46 @@ static int dpaa2_switch_port_mdb_add(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; /* Check if address is already set on this port */ if (dpaa2_switch_port_lookup_address(netdev, 0, mdb->addr)) return -EEXIST;
[Severity: High] Does this incorrectly block valid MDB additions for the same MAC address but different VLAN IDs? Looking at dpaa2_switch_port_lookup_address(), it checks the VLAN-unaware netdev->mc list. If an MDB with the same MAC but different VID is added, it returns -EEXIST. Later, if the user attempts to delete the un-added MDB, dpaa2_switch_port_mdb_del() finds the MAC from the first MDB, skips hardware removal due to VID mismatch, and successfully calls dev_mc_del(). When the legitimate MDB is later deleted, the lookup fails with -ENOENT, skipping dpaa2_switch_lag_fdb_del() and permanently leaking the dynamically allocated struct dpaa2_mac_addr and the hardware FDB entry. [ ... ]
quoted hunk ↗ jump to hunk
@@ -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? [ ... ]
quoted hunk ↗ jump to hunk
@@ -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.