Re: [net-next PATCH v5 7/8] net: dsa: realtek: rtl8365mb: add FDB support

[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.
---
net: dsa: realtek: rtl8365mb: add FDB support

This patch implements FDB and MDB management for the RTL8365MB switch
family, adds an l2_lock mutex to serialize L2 table access, and wires
up port_fdb_*, port_mdb_*, and port_fast_age DSA ops. It also sets the
assisted_learning_on_cpu_port and fdb_isolation switch flags.

A few questions inline below.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_l2.c b/drivers/net/dsa/realtek/rtl8365mb_l2.c
> new file mode 100644
> index 000000000000..6d99fb59d3c8
> --- /dev/null
> +++ b/drivers/net/dsa/realtek/rtl8365mb_l2.c

[ ... ]

> +int rtl8365mb_l2_add_uc(struct realtek_priv *priv, int port,
> +			const unsigned char mac_addr[static ETH_ALEN],
> +			u16 efid, u16 vid)
> +{
> +	u16 data[RTL8365MB_L2_ENTRY_SIZE] = { 0 };
> +	struct rtl8365mb_l2_uc uc = { 0 };
> +	u16 addr;
> +	int ret;
> +
> +	memcpy(uc.key.mac_addr, mac_addr, ETH_ALEN);
> +	uc.key.efid = efid;
> +	uc.key.fid = 0;
> +	uc.key.ivl = true;
> +	uc.key.vid = vid;
> +
> +	uc.port = port;
> +	/* do not let HW decrease age */
> +	uc.is_static = true;
> +	/* age greater than 0 adds/updates entries */
> +	uc.age = 1;
> +	rtl8365mb_l2_uc_to_data(&uc, data);
> +
> +	/* add the new entry or update an existing one */
> +	ret = rtl8365mb_table_query(priv, RTL8365MB_TABLE_L2,
> +				    RTL8365MB_TABLE_OP_WRITE, &addr,
> +				    0, 0,
> +				    data, RTL8365MB_L2_ENTRY_SIZE);
> +	/* addr will hold the table index, but it is not used here */
> +	if (ret == -ENOENT) {
> +		/* -ENOENT means the just added entry was not found (and @addr
> +		 * does not hold the table index. Although any error will be
> +		 * treated equally by the caller, assume that the missing entry
> +		 * means the table is full (tested in real HW).
> +		 */
> +		return -ENOSPC;
> +	}
> +	return ret;
> +}

[Low]
Is the -ENOENT check here ever reached for an OP_WRITE?

Looking at rtl8365mb_table_query() in rtl8365mb_table.c, the L2 status
handling explicitly maps a miss for OP_WRITE to -ENOSPC and only returns
-ENOENT for reads:

	if (!hit) {
		if (op == RTL8365MB_TABLE_OP_WRITE)
			ret = -ENOSPC;
		else
			ret = -ENOENT;
		goto out;
	}

If that mapping is the contract, the conditional and the comment
("-ENOENT means the just added entry was not found") look like dead
code. Would it be clearer to either drop the check entirely, or rewrite
it as if (ret == -ENOSPC) so the intent matches the table layer?

[ ... ]

> +int rtl8365mb_l2_flush(struct realtek_priv *priv, int port, u16 vid)
> +{
> +	int mode = vid ? RTL8365MB_L2_FLUSH_CTRL2_MODE_PORT_VID :
> +			 RTL8365MB_L2_FLUSH_CTRL2_MODE_PORT;
> +	u32 val, mask;
> +	int ret;
[ ... ]
> +	if (port < 8) {
> +		val = FIELD_PREP(RTL8365MB_L2_FLUSH_PORT_MSK_MSK,
> +				 BIT(port) & 0xFF);
> +		ret = regmap_write(priv->map_nolock,
> +				   RTL8365MB_L2_FLUSH_PORT_REG, val);
> +		if (ret)
> +			goto out;
> +
> +		mask = FIELD_PREP(RTL8365MB_L2_FLUSH_PORT_BUSY_MSK,
> +				  BIT(port) & 0xFF);
> +		ret = regmap_read_poll_timeout(priv->map_nolock,
> +					       RTL8365MB_L2_FLUSH_PORT_REG,
> +					       val, !(val & mask), 10, 100);
> +		if (ret)
> +			goto out;
> +	} else {
> +		val = FIELD_PREP(RTL8365MB_L2_FLUSH_PORT_EXT_MSK_MSK,
> +				 BIT(port) >> 8);
> +		ret = regmap_write(priv->map_nolock,
> +				   RTL8365MB_L2_FLUSH_PORT_EXT_REG, val);
> +		if (ret)
> +			goto out;
> +
> +		mask = FIELD_PREP(RTL8365MB_L2_FLUSH_PORT_EXT_BUSY_MSK,
> +				  BIT(port) >> 8);
> +		ret = regmap_read_poll_timeout(priv->map_nolock,
> +					       RTL8365MB_L2_FLUSH_PORT_EXT_REG,
> +					       val, !(val & mask), 10, 100);
> +		if (ret)
> +			goto out;
> +	}

[Medium]
Is the 100us timeout on regmap_read_poll_timeout() long enough for a
port flush to complete?

The arguments are sleep_us=10, timeout_us=100, giving roughly 100us
total to walk a LUT that, per RTL8365MB_LEARN_LIMIT_MAX, holds up to
2112 entries. The same driver's rtl8365mb_table_poll_busy() uses 10000
(10 ms) for L2 table operations.

If the hardware has not finished within 100us, the function returns
-ETIMEDOUT and rtl83xx_port_fast_age() only logs the error and
continues, leaving dynamic entries that an STP transition expected to
be flushed. Was 100us intentional, or should this match the 10 ms used
elsewhere in the driver?

[ ... ]

> +int rtl8365mb_l2_add_mc(struct realtek_priv *priv, int port,
> +			const unsigned char mac_addr[static ETH_ALEN],
> +			u16 vid)
> +{
> +	u16 data[RTL8365MB_L2_ENTRY_SIZE] = { 0 };
> +	struct rtl8365mb_l2_mc mc = { 0 };
> +	u16 addr;
> +	int ret;
> +
> +	memcpy(mc.key.mac_addr, mac_addr, ETH_ALEN);
> +	mc.key.vid = vid;
> +	mc.key.ivl = true;
> +	/* Already set the port and is_static, although not used in OP_READ,
> +	 * data will be ready for OP_WRITE if it is a new entry.
> +	 */
> +	mc.member |= BIT(port);
> +	mc.is_static = 1;
> +	rtl8365mb_l2_mc_to_data(&mc, data);
> +
> +	/* First look for an existing entry (to get existing port members) */
> +	ret = rtl8365mb_table_query(priv, RTL8365MB_TABLE_L2,
> +				    RTL8365MB_TABLE_OP_READ, &addr,
> +				    RTL8365MB_TABLE_L2_METHOD_MAC, 0,
> +				    data, RTL8365MB_L2_ENTRY_SIZE);
> +	if (!ret) {
> +		/* There is already an entry... */
> +		rtl8365mb_l2_data_to_mc(data, &mc);
> +		dev_dbg(priv->dev, "l2_add_mc: found existing addr=%d member=0x%x igmpidx=0x%x static=%d\n",
> +			addr, mc.member, mc.igmpidx, mc.is_static);
> +		/* the port must be added as a member */
> +		mc.member |= BIT(port);
> +		rtl8365mb_l2_mc_to_data(&mc, data);

[Medium]
Can this path silently demote a bridge mdb entry to non-static?

When a matching entry already exists, rtl8365mb_l2_data_to_mc() parses
every field from hardware into mc, including is_static:

	mc->is_static = FIELD_GET(RTL8365MB_L2_MC_D5_STATIC_MSK, data[5]);

After that, only mc.member is updated before the OP_WRITE. If the
existing hardware entry has is_static = 0 (for instance, an entry left
behind by l2_del_mc that did not reclaim the slot, or an entry created
by IGMP snooping), the rewritten entry stays non-static, so the
hardware can age it out even though the bridge requested a static MDB
entry.

Would it make sense to force mc.is_static = 1 (and also clear the
non-key fields you do not want to inherit) before the OP_WRITE on the
hit path?

> +	} else if (ret == -ENOENT) {
[ ... ]
> +	} else {
> +		return ret;
> +	}
> +
> +	/* add the new entry or update an existing one */
> +	ret = rtl8365mb_table_query(priv, RTL8365MB_TABLE_L2,
> +				    RTL8365MB_TABLE_OP_WRITE, &addr,
> +				    0, 0,
> +				    data, RTL8365MB_L2_ENTRY_SIZE);
> +	/* addr will hold the table index, but it is not used here */
> +	if (ret == -ENOENT) {
> +		/* -ENOENT means the just added entry was not found (and @addr
> +		 * does not hold the table index. Although any error will be
> +		 * treated equally by the caller, assume that the missing entry
> +		 * means the table is full (tested in real HW).
> +		 */
> +		return -ENOSPC;
> +	}
> +
> +	return ret;
> +}

[Low]
Same question as in rtl8365mb_l2_add_uc(): the table layer already maps
an OP_WRITE miss to -ENOSPC, so the -ENOENT branch and its comment
appear to be dead code here too. Would if (ret == -ENOSPC) better match
the contract of rtl8365mb_table_query()?

> +int rtl8365mb_l2_del_mc(struct realtek_priv *priv, int port,
> +			const unsigned char mac_addr[static ETH_ALEN],
> +			u16 vid)
> +{
[ ... ]
> +	rtl8365mb_l2_data_to_mc(data, &mc);
> +	dev_dbg(priv->dev, "l2_del_mc: found existing addr=%d member=0x%x igmpidx=0x%x static=%d\n",
> +		addr, mc.member, mc.igmpidx, mc.is_static);
> +	/* the port must be removed as a member */
> +	mc.member &= ~BIT(port);
> +	if (!mc.member) {
> +		/* With no members, zero all non-key fields to delete the
> +		 * entry. However, setting is_static = 0 might be enough to
> +		 * signal the HW to release the slot.
> +		 */
> +		mc.is_static = 0;
> +		mc.igmpidx = 0;
> +		mc.priority = 0;
> +		mc.fwd_pri = 0;
> +		mc.igmp_asic = 0;
> +	}
> +	rtl8365mb_l2_mc_to_data(&mc, data);
> +
> +	/* update the existing entry. */
> +	ret = rtl8365mb_table_query(priv, RTL8365MB_TABLE_L2,
> +				    RTL8365MB_TABLE_OP_WRITE, &addr,
> +				    0, 0,
> +				    data, RTL8365MB_L2_ENTRY_SIZE);
> +	return ret;
> +}

[Medium]
Does this path actually reclaim the L2 slot when the last member is
removed?

The unicast delete in rtl8365mb_l2_del_uc() uses age = 0, which the
in-tree comment describes as the documented delete signal. The
multicast path here only zeros is_static and the other non-key fields,
and the comment itself notes uncertainty:

	/* With no members, zero all non-key fields to delete the
	 * entry. However, setting is_static = 0 might be enough to
	 * signal the HW to release the slot.
	 */

If the hardware does not auto-reclaim a row when is_static becomes 0
with an empty member mask, the LUT (RTL8365MB_LEARN_LIMIT_MAX = 2112)
will accumulate dead rows for previously used MAC+VID multicast keys
and eventually fail subsequent adds with -ENOSPC.

Has this been confirmed against the vendor documentation, and is there
a more explicit delete primitive analogous to age = 0 for the multicast
case?