Re: [PATCH net v3 3/8] net: dsa: b53: clear stale state when creating an ARL entry

[Florian Fainelli <florian.fainelli@broadcom.com>]

On 10/31/26 14:56, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] In b53_arl_op() (drivers/net/dsa/b53/b53_common.c), deleting a
>    {MAC,VID} that is missing from the ARL now returns -ENOENT.
> - [Medium] Deleting an ARL entry that is not in hardware now fails with
>    -ENOENT, including the -ENOSPC lookup result, which used to return 0…
> 
> --
> 
> net: dsa: b53: clear stale state when creating an ARL entry
> 
> When an ARL lookup misses, the patch now clears the b53_arl_entry before
> building a new one. This stops a multicast add from ORing the port into an
> unrelated bin's bitmap. A delete of an absent entry now returns -ENOENT
> without writing to hardware, and that includes the -ENOSPC lookup result,
> which used to return 0.
> 
>> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
>> index 559d4b360c828..c38e8c5838bd8 100644
>> --- a/drivers/net/dsa/b53/b53_common.c
>> +++ b/drivers/net/dsa/b53/b53_common.c
>> @@ -1990,11 +1990,15 @@ static int b53_arl_op(struct b53_device *dev, int op, int port,
>>   	case -ENOSPC:
>>   		dev_dbg(dev->dev, "{%pM,%.4d} no space left in ARL\n",
>>   			addr, vid);
>> -		return is_valid ? ret : 0;
>> +		return is_valid ? ret : -ENOENT;
>>   	case -ENOENT:
>> +		if (!is_valid)
>> +			return ret;
> 
> [Severity: Medium]
> Can returning -ENOENT on delete leave DSA's host address tracking out of
> sync with the ARL?
> 
> b53_fdb_del() and b53_mdb_del() pass this value back to DSA. On CPU/DSA
> ports, when the last reference drops, dsa_port_do_fdb_del() in
> net/dsa/switch.c treats any driver error as "still in hardware":
> 
> 	err = ds->ops->port_fdb_del(ds, port, addr, vid, db);
> 	trace_dsa_fdb_del_hw(dp, addr, vid, &db, err);
> 	if (err) {
> 		refcount_set(&a->refcount, 1);
> 		goto out;
> 	}
> 
> 	list_del(&a->list);
> 	kfree(a);
> 
> dsa_port_do_mdb_del() handles errors the same way. The struct dsa_mac_addr
> then stays on dp->fdbs or dp->mdbs with refcount 1.
> 
> A later add of the same {addr, vid, db} finds that record in
> dsa_port_do_fdb_add() or dsa_port_do_mdb_add() and only calls
> refcount_inc(). port_fdb_add or port_mdb_add is never called, so the
> hardware entry is not reinstalled. The record is only freed by the leftover
> sweep in dsa_port_teardown().
> 
> How can the hardware lose an entry that DSA still tracks? b53 doesn't set
> ds->fdb_isolation, so dsa_port_bridge_host_fdb_add/del() force
> db.bridge.num = 0, and all bridges share one ARL namespace.
> 
> A unicast ARL entry holds a single port. A user-port static FDB for the
> same MAC/VID, for example in another bridge, overwrites the CPU port's host
> entry. Deleting that user-port FDB invalidates the entry, and the later
> host delete then misses.
> 
> There may be a second path, though I have not confirmed it.
> bcm_sf2_sw_resume() calls bcm_sf2_sw_rst() and then ds->ops->setup(), and
> does not replay FDB/MDB entries. If that reset clears the ARL, every later
> host delete would miss.
> 
> Before this patch, the 0 return let DSA free its record, so the next add
> reprogrammed the switch. Would returning 0 for a delete of an absent entry
> fix the stale-bin problem without this side effect?
> 
> The commit message mentions the return value change, but not its effect on
> the DSA core refcounting. With the Fixes: tag this will also go to stable.
> Later patches in the series don't touch b53_arl_op(), b53_fdb_del() or
> b53_mdb_del().
> 
> [Severity: Medium]
> On BCM5325 and BCM5365, does this make every MDB delete fail?
> 
> b53_mdb_add() returns before it touches the ARL:
> 
> 	if (is5325(priv) || is5365(priv))
> 		return -EOPNOTSUPP;
> 
> b53_mdb_del() has no matching check:
> 
> 	ret = b53_arl_op(priv, 0, port, mdb->addr, mdb->vid, false);
> 	mutex_unlock(&priv->arl_mutex);
> 	if (ret)
> 		dev_err(ds->dev, "failed to delete MDB entry\n");
> 
> The bridge calls switchdev_port_obj_del() on RTM_DELMDB whether or not the
> add was offloaded:
> 
> br_switchdev_mdb_notify()
>    switchdev_port_obj_del()
>      dsa_user_port_obj_del()
>        dsa_port_mdb_del()
>          dsa_port_do_mdb_del()
>            b53_mdb_del()
>              b53_arl_op() -> -ENOENT
> 
> So every group leave or expiry on a bridged 5325/5365 port with snooping
> enabled would log "failed to delete MDB entry". It would also log a second
> message from switchdev_port_obj_del_deferred(), which only ignores
> -EOPNOTSUPP:
> 
> 	err = switchdev_port_obj_del_now(dev, obj);
> 	if (err && err != -EOPNOTSUPP)
> 		switchdev_obj_id_to_helpful_msg(dev, obj->id, err, false);
> 
> On other chips, when a user-port FDB or MDB add fails with -ENOSPC, the
> bridge delete that follows now fails too. For FDBs,
> dsa_user_switchdev_event_work() then logs "port %d failed to delete %pM
> vid %d from fdb".
> 
> Should b53_mdb_del() get the same 5325/5365 guard as b53_mdb_add()? Or
> should a missing entry on delete be treated as success? b53_arl_op() and
> b53_mdb_del() are still the same at the end of the series.

Agreed. I’ll respin this patch such that deleting an absent entry 
returns success for both lookup-miss cases, while retaining the early 
return without writing hardware. This avoids retaining stale DSA 
host-address references and errors for entries that were never 
offloaded. I’ll update the commit message accordingly.

pw-bot: cr
-- 
Florian