Re: [PATCH net-next v11 8/9] net: dsa: lan9645x: add mdb management

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Duplicate CPU packet delivery on IP multicast MDB matches
- [Low] Inverted teardown sequence between MAC and MDB initialization
--

--- Patch [8]: [PATCH net-next v11 8/9] net: dsa: lan9645x: add mdb management ---
commit c8677da78fd630673d16e31ac732c8854e853dbe
Author: Jens Emil Schulz Østergaard <[email protected]>

net: dsa: lan9645x: add mdb management

Add support for dsa mdb callbacks. L2 multicast and IP multicast is
handled differently. IP multicast stores the port group mask inline in
the mac table. L2 multicast points to a PGID index, which encodes the
port group mask.

> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> index af158d4461aee..fc604edb309b9 100644
> --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> @@ -70,6 +70,7 @@ static void lan9645x_teardown(struct dsa_switch *ds)
>  	destroy_workqueue(lan9645x->owq);
>  	lan9645x_npi_port_deinit(lan9645x, lan9645x->npi);
>  	lan9645x_mac_deinit(lan9645x);
> +	lan9645x_mdb_deinit(lan9645x);

[Severity: Low]
In lan9645x_setup(), lan9645x_mac_init() is called before lan9645x_mdb_init().
However, lan9645x_teardown() and the err_mac path destroy these in the forward
order (lan9645x_mac_deinit() followed by lan9645x_mdb_deinit()).

Since lan9645x_mac_deinit() destroys mact_lock, could calling it before
lan9645x_mdb_deinit() cause a use-after-destroy if MDB cleanup ever needs to
take mact_lock?

[ ... ]

> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_mdb.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_mdb.c
> new file mode 100644
> index 0000000000000..ea37bb660273c
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_mdb.c
[ ... ]
> +static int lan9645x_mdb_update_dest(struct lan9645x *lan9645x,
> +				    struct lan9645x_mdb_entry *mdb_entry,
> +				    enum macaccess_entry_type type,
> +				    struct lan9645x_pgid_entry *new_pgid,
> +				    u16 new_ports)
> +{
> +	unsigned char mac[ETH_ALEN] __aligned(2);
> +	struct lan9645x_pgid_entry *old_pgid;
> +	int err, pgid_index;
> +	bool cpu_copy;
> +
> +	old_pgid = mdb_entry->pgid;
> +	lan9645x_mdb_encode_mac(mac, mdb_entry->mac, new_ports, type);
> +	cpu_copy = !!(new_ports & BIT(lan9645x->num_phys_ports));

[Severity: Medium]
For IP multicast entries, the port mask is stored inline in the high bytes of
the MAC address via lan9645x_mdb_encode_mac().

Because new_ports includes the CPU port bit, the hardware will forward the
packet to the CPU port according to the inline mask. The code also sets the
MAC_CPU_COPY bit via cpu_copy.

If the hardware acts on both the inline port mask and the MAC_CPU_COPY bit,
will this result in duplicate packets being delivered to the CPU?

Should the CPU port bit be stripped from the new_ports mask before encoding it
into the MAC address, similarly to how it is stripped from L2 multicast PGID
entries in lan9645x_mdb_pgid_entry_get()?

> +	pgid_index = PGID_INDEX(new_pgid);
> +
> +	/* For IP multicast, the hardware lookup uses the DMAC
> +	 * (01:00:5E:.. / 33:33:..) as the (mac, vid) key, not the encoded mac.
> +	 * Therefore, this CMD_LEARN will atomically rewrite the existing
> +	 * hardware entry. We intentionally do not do a forget before learn
> +	 * sequence, as that would not be atomic, and leave a forwarding gap.
> +	 */
> +	err = lan9645x_mact_learn_cpu_copy(lan9645x, pgid_index, mac,
> +					   mdb_entry->vid, type, cpu_copy);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-dsa_lan9645x_switch_driver_base-v11-0-007ebc983a0a@microchip.com?part=8