Re: [net-next PATCH v5 5/8] net: dsa: realtek: rtl8365mb: add VLAN 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 VLAN support

This commit adds VLAN support to the Realtek RTL8365MB switch driver.
It implements a dynamic allocation scheme to synchronize port membership
across the exhaustive 4K VLAN table and a smaller 32-entry VLANMC table
used primarily for PVID assignment.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c
[ ... ]
> @@ -1210,6 +1262,197 @@ static void rtl8365mb_port_stp_state_set(struct dsa_switch *ds, int port,
>  			   val << RTL8365MB_MSTI_CTRL_PORT_STATE_OFFSET(port));
>  }
>  
> +static int rtl8365mb_port_set_transparent(struct realtek_priv *priv,
> +					  int igr_port, int egr_port,
> +					  bool enable)
> +{
[ ... ]
> +	return regmap_update_bits(priv->map,
> +			RTL8365MB_VLAN_EGRESS_TRANSPARENT_REG(egr_port),
> +			BIT(igr_port), enable ? BIT(igr_port) : 0);
> +}
[ ... ]
> +static int rtl8365mb_port_vlan_filtering(struct dsa_switch *ds, int port,
> +					 bool vlan_filtering,
> +					 struct netlink_ext_ack *extack)
> +{
[ ... ]
> +	/* However, we also enable/disable egress filtering because the switch
> +	 * still consider the egress interface VLAN membership to forward the
> +	 * traffic. We enable/disable that check disabling/enabling transparent
> +	 * VLAN between the ingress port and all other available ports.
> +	 */
> +	dsa_switch_for_each_available_port(dp, ds) {
> +		/* port isolation will still keep traffic inside the bridge */
> +		ret = rtl8365mb_port_set_transparent(priv, port, dp->index,
> +						     !vlan_filtering);
> +		if (ret)
> +			goto undo_transparent;
> +	}

This configures the hardware to bypass egress filtering for traffic
originating from port and destined to dp->index. Does this miss the
inverse operation for traffic destined to port?

When a VLAN-aware port (or the CPU port) sends traffic to this VLAN-unaware
port, the switch evaluates the egress port's VLAN membership. Since the
VLAN-unaware port is generally not a member of the frame's VLAN, will the
switch incorrectly drop the packet?

[ ... ]
> +static int rtl8365mb_port_vlan_add(struct dsa_switch *ds, int port,
> +				   const struct switchdev_obj_port_vlan *vlan,
> +				   struct netlink_ext_ack *extack)
> +{
[ ... ]
> +	/* add port to vlan4k. It knows nothing about PVID */
> +	ret = rtl8365mb_vlan_4k_port_add(ds, port, vlan, extack);
> +	if (ret)
> +		return ret;
> +
> +	/* Set PVID if needed */
> +	if (pvid) {
> +		ret = rtl8365mb_vlan_pvid_port_add(ds, port, vlan, extack);
> +		if (ret)
> +			goto undo_vlan_4k;
> +	} else {
> +		/* or try to unset it if not */
> +		ret = rtl8365mb_vlan_pvid_port_del(ds, port, vlan);
> +		if (ret)
> +			goto undo_vlan_4k;
> +	}
> +
> +	return 0;
> +
> +undo_vlan_4k:
> +	(void)rtl8365mb_vlan_4k_port_del(ds, port, vlan);
> +	return ret;
> +}

What happens to the port's hardware state if rtl8365mb_vlan_pvid_port_add()
fails (for example, the 32-entry VLAN MC table is exhausted, returning -E2BIG)?

Looking at rtl8365mb_vlan_pvid_port_add() below, it unconditionally removes the
existing PVID via rtl8365mb_vlan_pvid_port_del() before attempting to allocate
the new one. If the allocation fails, the error path aborts without restoring
the old PVID.

Furthermore, the error path then jumps to undo_vlan_4k and blindly calls
rtl8365mb_vlan_4k_port_del(). Could this permanently strip the port of its
previous PVID and disconnect it from its previously configured VLAN, leaving
hardware state desynchronized from the software bridge?

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_vlan.c b/drivers/net/dsa/realtek/rtl8365mb_vlan.c
> --- a/drivers/net/dsa/realtek/rtl8365mb_vlan.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_vlan.c
[ ... ]
> +static int
> +rtl8365mb_vlan_4k_port_set(struct dsa_switch *ds, int port,
> +			   const struct switchdev_obj_port_vlan *vlan,
> +			   struct netlink_ext_ack *extack,
> +			       bool include)
> +{
> +	struct realtek_priv *priv = ds->priv;
> +	struct rtl8365mb_vlan4k vlan4k = {0};
> +	int ret;
[ ... ]
> +	ret = rtl8365mb_vlan_4k_read(priv, vlan->vid, &vlan4k);
> +	if (ret) {
> +		RTL_VLAN_ERR("Failed to read VLAN 4k table");
> +		return ret;
> +	}
> +
> +	if (include)
> +		vlan4k.member |= BIT(port);
> +	else
> +		vlan4k.member &= ~BIT(port);
[ ... ]
> +	ret = rtl8365mb_vlan_4k_write(priv, &vlan4k);

Since the switch uses read-modify-write operations to update the global
VLAN 4K and VLAN MC tables, could this cause a Time-of-Check to Time-of-Use
race?

While the underlying priv->map_lock protects individual read or write queries,
it is released between them. If concurrent switchdev operations are triggered
on different ports for the same VLAN ID, could one update silently overwrite
and revert the other?

[ ... ]
> +static int rtl8365mb_vlan_mc_find(struct realtek_priv *priv, u16 vid,
> +				  u8 *index, u8 *first_free)
> +{
[ ... ]
> +	for (vlanmc_idx = 1; vlanmc_idx <= RTL8365MB_VLAN_MCMAX; vlanmc_idx++) {
> +		/* just read the 4th word, where the evid is */
> +		ret = regmap_read(priv->map,
> +				  RTL8365MB_VLAN_MC_REG(vlanmc_idx) + 3,
> +				  &vlan_entry_d3);
> +		if (ret)
> +			return ret;
> +
> +		evid = FIELD_GET(RTL8365MB_VLAN_MC_D3_EVID_MASK, vlan_entry_d3);
> +
> +		if (evid == vid) {
> +			*index = vlanmc_idx;
> +			return 0;
> +		}
> +
> +		if (evid == 0x0 && *first_free < 1)
> +			*first_free = vlanmc_idx;
> +	}
> +	return 0;
> +}

Does treating evid == 0x0 as an empty slot risk corrupting PVID assignments if
a user configures VID 0 as a valid PVID?

Linux supports configuring VID 0 (for priority-tagged frames) on user ports.
If a port sets VID 0 as its PVID, it will be assigned a hardware slot where
evid = 0.

When a subsequent port requests a different PVID, the allocator might see
evid == 0, assume the slot is free, and overwrite it. Should the allocator
verify the member mask instead to determine if a slot is genuinely empty?

[ ... ]
> +int rtl8365mb_vlan_pvid_port_del(struct dsa_switch *ds, int port,
> +				 const struct switchdev_obj_port_vlan *vlan)
> +{
[ ... ]
> +	/* Changing accept frame is what really removes PVID */
> +	accepted_frame = RTL8365MB_FRAME_TYPE_TAGGED_ONLY;
> +	ret = rtl8365mb_vlan_port_set_framefilter(priv, port, accepted_frame);

When a PVID is removed, the driver sets the port's frame filter to
RTL8365MB_FRAME_TYPE_TAGGED_ONLY to drop untagged frames.

If the port later transitions to a VLAN-unaware bridge or standalone mode,
rtl8365mb_port_vlan_filtering(ds, port, false, ...) is called.

Since that function disables ingress filtering but leaves the frame filter
untouched, will the port continue to silently drop all untagged traffic at the
MAC level, breaking standard network connectivity?