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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Subject: 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_mdb.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_mdb.c
> new file mode 100644
> index 000000000000..4cf92887a4eb
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_mdb.c
> @@ -0,0 +1,391 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2026 Microchip Technology Inc.
> + */
> +
> +#include "lan9645x_main.h"

[ ... ]

> +static void lan9645x_mdb_encode_mac(unsigned char *mac,
> +				    struct lan9645x_mdb_entry *mdb_entry,
> +				    enum macaccess_entry_type type)
> +{
> +	ether_addr_copy(mac, mdb_entry->mac);
> +
> +	/* The HW encodes the portmask in the high bits of the mac for ip
> +	 * multicast entries, to save on the limited PGID resources.
> +	 *
> +	 * IPv4 Multicast DMAC: 0x01005Exxxxxx
> +	 * IPv6 Multicast DMAC: 0x3333xxxxxxxx
> +	 *
> +	 * which gives us 24 or 16 bits to encode the portmask.
> +	 */
> +	if (type == ENTRYTYPE_MACV4) {
> +		mac[0] = 0;
> +		mac[1] = mdb_entry->ports >> 8;
> +		mac[2] = mdb_entry->ports & 0xff;
> +	} else if (type == ENTRYTYPE_MACV6) {
> +		mac[0] = mdb_entry->ports >> 8;
> +		mac[1] = mdb_entry->ports & 0xff;
> +	}
> +}

Does this code need to mask out BIT(CPU_PORT) from mdb_entry->ports?

For L2 multicast, the CPU port is explicitly stripped from the PGID portmask
to avoid conflicting instructions, since CPU_PORT forwarding is handled by the
cpu_copy flag. Here, the IP multicast portmask is encoded directly into the
MAC address using the full mdb_entry->ports. Could this cause hardware to
receive conflicting instructions (forwarding to port 9 and copying to the CPU
extraction queue), leading to duplicate multicast packets for the CPU?

[ ... ]

> +static struct lan9645x_pgid_entry *
> +lan9645x_mdb_pgid_entry_create(struct lan9645x *lan9645x, u16 ports)
> +{
> +	struct lan9645x_pgid_entry *pgid_entry = NULL;
> +	int index;
> +
> +	for (index = PGID_GP_START; index < PGID_GP_END; index++) {

Is it safe to start the search at PGID_GP_START (defined as CPU_PORT)?

Hardware uses ANA_PGID(DEST_IDX) to forward unicast traffic, and MAC table
entries for the CPU have DEST_IDX = 9. When the first L2 multicast group is
added, it appears this loop will allocate PGID index 9, overwriting the
unicast portmask for the CPU port. Could this cause all unicast traffic
destined for the CPU to be flooded to the external physical ports in the
multicast group instead?

> +		bool used = false;
> +
> +		list_for_each_entry(pgid_entry, &lan9645x->pgid_entries, list) {
> +			if (pgid_entry->index == index) {
> +				used = true;
> +				break;
> +			}
> +		}
> +
> +		if (!used)
> +			return lan9645x_pgid_entry_alloc(lan9645x, index,
> +							 ports);
> +	}
> +
> +	return ERR_PTR(-ENOSPC);
> +}

[ ... ]

> +static int __lan9645x_mdb_add(struct lan9645x *lan9645x, int chip_port,
> +			      const unsigned char addr[ETH_ALEN], u16 vid,
> +			      enum macaccess_entry_type type)
> +{
> +	struct lan9645x_pgid_entry *old_pgid, *new_pgid;
> +	struct lan9645x_mdb_entry *mdb_entry;
> +	unsigned char mac[ETH_ALEN];
> +	int err, pgid_index;
> +	bool cpu_copy;
> +
> +	mdb_entry = lan9645x_mdb_entry_lookup(lan9645x, addr, vid);
> +	if (!mdb_entry) {
> +		mdb_entry = lan9645x_mdb_entry_alloc(lan9645x, addr, vid);
> +		if (IS_ERR(mdb_entry))
> +			return PTR_ERR(mdb_entry);
> +	}
> +
> +	if (mdb_entry->ports & BIT(chip_port))
> +		return 0;
> +
> +	mdb_entry->ports |= BIT(chip_port);
> +
> +	/* Encode mac for IP mc */
> +	lan9645x_mdb_encode_mac(mac, mdb_entry, type);
> +
> +	/* Update PGID ptr for non-IP entries (L2 multicast) */
> +	old_pgid = mdb_entry->pgid;
> +	new_pgid = lan9645x_mdb_pgid_entry_get(lan9645x, mdb_entry, type);
> +	if (IS_ERR(new_pgid)) {
> +		/* Out of PGIDs or mem. Continue forwarding to old port
> +		 * group, or remove if fresh mdb_entry.
> +		 */
> +		mdb_entry->ports &= ~BIT(chip_port);
> +		if (!mdb_entry->ports)
> +			lan9645x_mdb_entry_dealloc(lan9645x, mdb_entry);
> +
> +		return PTR_ERR(new_pgid);
> +	}
> +	mdb_entry->pgid = new_pgid;
> +
> +	cpu_copy = !!(mdb_entry->ports & BIT(CPU_PORT));
> +	pgid_index = lan9645x_mdb_pgid_index(mdb_entry, type);
> +
> +	/* Make sure to write on top of existing entry, so we do not disrupt
> +	 * flowing traffic.
> +	 */
> +	err = lan9645x_mact_learn_cpu_copy(lan9645x, pgid_index, mac,
> +					   mdb_entry->vid, type, cpu_copy);
> +	lan9645x_pgid_entry_put(lan9645x, old_pgid);
> +	return err;
> +}

What happens if lan9645x_mact_learn_cpu_copy() fails and returns an error?

The software MDB state (mdb_entry->ports and mdb_entry->pgid) is updated and
the old PGID reference is dropped via lan9645x_pgid_entry_put() before
verifying if the hardware MAC table update succeeds.

If the hardware update fails, the software state isn't rolled back, leaving
them out of sync. Furthermore, the dropped old_pgid reference could allow the
index to be reallocated to a new multicast group while the hardware still
points to the old PGID index, potentially causing silent traffic cross-talk.

> +
> +static int __lan9645x_mdb_del(struct lan9645x *lan9645x, int chip_port,
> +			      const unsigned char addr[ETH_ALEN], u16 vid,
> +			      enum macaccess_entry_type type)
> +{
> +	struct lan9645x_pgid_entry *old_pgid, *new_pgid;
> +	struct lan9645x_mdb_entry *mdb_entry;
> +	unsigned char mac[ETH_ALEN];
> +	int err, pgid_index;
> +	bool cpu_copy;
> +
> +	mdb_entry = lan9645x_mdb_entry_lookup(lan9645x, addr, vid);
> +	if (!mdb_entry)
> +		return -ENOENT;
> +
> +	if (!(mdb_entry->ports & BIT(chip_port)))
> +		return 0;
> +
> +	mdb_entry->ports &= ~BIT(chip_port);
> +
> +	/* Encode mac for IP mc */
> +	lan9645x_mdb_encode_mac(mac, mdb_entry, type);
> +
> +	/* Update PGID ptr for non-IP entries (L2 multicast) */
> +	old_pgid = mdb_entry->pgid;
> +	new_pgid = lan9645x_mdb_pgid_entry_get(lan9645x, mdb_entry, type);
> +	if (IS_ERR(new_pgid)) {
> +		/* Continue forwarding to old port group. */
> +		mdb_entry->ports |= BIT(chip_port);
> +		return PTR_ERR(new_pgid);
> +	}
> +	mdb_entry->pgid = new_pgid;
> +
> +	if (!mdb_entry->ports) {
> +		lan9645x_mact_forget(lan9645x, mac, mdb_entry->vid, type);
> +		lan9645x_pgid_entry_put(lan9645x, old_pgid);
> +		lan9645x_mdb_entry_dealloc(lan9645x, mdb_entry);
> +		return 0;
> +	}

Similar to the addition path above, if lan9645x_mact_forget() fails, its
return value is ignored, and the software MDB entry and PGID are freed
anyway. This could also lead to hardware/software state desynchronization.

> +
> +	cpu_copy = !!(mdb_entry->ports & BIT(CPU_PORT));
> +	pgid_index = lan9645x_mdb_pgid_index(mdb_entry, type);
> +
> +	err = lan9645x_mact_learn_cpu_copy(lan9645x, pgid_index, mac,
> +					   mdb_entry->vid, type, cpu_copy);
> +	lan9645x_pgid_entry_put(lan9645x, old_pgid);
> +	return err;
> +}