Re: [PATCH net 2/2] net: dsa: realtek: rtl8365mb: keep the neutral PVID in VLAN-unaware bridges

["Alvin Šipraga" <alvin.sipraga@analog.com>]

On Wed, Nov 04, 2026 at 09:25:14AM +0100, Stijn Cleynhens wrote:
> The switch looks up its forwarding database by the VLAN that it
> classifies a frame to, and untagged frames are classified to the port
> PVID. With configure_vlan_while_not_filtering, the bridge PVID is
> programmed into the port also while the bridge is VLAN-unaware, so
> untagged frames of such a bridge are looked up in VLAN 1. DSA however
> installs the FDB and MDB entries of a VLAN-unaware bridge with VID 0,
> so these entries never match a lookup.
> 
> This breaks assisted learning on the CPU port: when a station that was
> first learned behind a user port moves behind the CPU port (e.g. a
> wireless client roaming between access points), the static entry that
> DSA installs for it on the CPU port does not override the learned one,
> and unicast to the station is dropped as a hairpin until the learned
> entry ages out. Clients that need a DHCP reply in that window do not
> get one.
> 
> Keep the PVID requested by the bridge per port and only program it
> while the port filters VLANs. Otherwise keep the neutral PVID (VLANMC
> index 0, VID 0) that standalone ports already use,

Makes sense.

> so that untagged frames are classified to the VID of the bridge
> entries.

This part threw me off a bit. What do you mean by "VID of the bridge
entries"? If the neutral PVID is programmed on the port of a
VLAN-unaware bridge, I would expect all untagged frames to be classified
VID=0. And with IVL this should be classified local to the bridge. Is
that what you want to say, or am I missing something?

> Commit the PVID
> again when VLAN filtering is toggled.
> 
> This only covers untagged frames. Tagged frames in a VLAN-unaware bridge
> are still classified by their tag, so a tagged station that roams keeps
> hitting the stale learned entry until it ages out, as before. Fixing
> that would need the switch to ignore the C-tag on ingress of these
> ports, which is left for a separate change.
> 
> Fixes: 336e3e4a1ab3 ("net: dsa: realtek: rtl8365mb: add FDB support")
> Signed-off-by: Stijn Cleynhens <stijncleynhens@gmail.com>
> ---
>  drivers/net/dsa/realtek/rtl8365mb_main.c | 139 +++++++++++++++--------
>  drivers/net/dsa/realtek/rtl8365mb_vlan.c |   9 +-
>  drivers/net/dsa/realtek/rtl8365mb_vlan.h |   3 +-
>  3 files changed, 101 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index b90e46c16..a7db267bc 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c
> @@ -838,6 +838,8 @@ struct rtl8365mb_cpu {
>   *         access via rtl8365mb_get_stats64
>   * @stats_lock: protect the stats structure during read/update
>   * @mib_work: delayed work for polling MIB counters
> + * @bridge_pvid: PVID requested by the bridge, 0 if none. Only programmed
> + *               while the port filters VLANs. Protected by vlan_lock.
>   */
>  struct rtl8365mb_port {
>  	struct realtek_priv *priv;
> @@ -845,6 +847,7 @@ struct rtl8365mb_port {
>  	struct rtnl_link_stats64 stats;
>  	spinlock_t stats_lock;
>  	struct delayed_work mib_work;
> +	u16 bridge_pvid;
>  };
>  
>  /**
> @@ -2004,6 +2007,41 @@ rtl8365mb_port_set_vlan_egress_mode(struct realtek_priv *priv, int port,
>  			RTL8365MB_PORT_MISC_CFG_VLAN_EGRESS_MODE_MASK, val);
>  }
>  
> +/* Program the PVID that the port should use for untagged frames: the PVID of
> + * the bridge while the port filters VLANs, and the neutral PVID (VID 0)
> + * otherwise. A VLAN-unaware bridge, like a standalone port, gets its FDB and
> + * MDB entries with VID 0 from DSA. As the switch looks up its forwarding
> + * database by the VLAN that it classifies a frame to, a bridge PVID would make
> + * those entries miss every lookup.
> + */
> +static int rtl8365mb_port_commit_pvid(struct dsa_switch *ds, int port,
> +				      bool vlan_filtering,
> +				      struct netlink_ext_ack *extack)
> +{
> +	struct realtek_priv *priv = ds->priv;
> +	struct rtl8365mb *mb = priv->chip_data;
> +	u16 hw_pvid, pvid = 0;

Please separate the variables on their own lines.

> +	int ret;
> +
> +	lockdep_assert_held(&priv->vlan_lock);
> +
> +	if (vlan_filtering)
> +		pvid = mb->ports[port].bridge_pvid;

I think an else { pvid = 0; } would be more explicit than setting it
upstairs, but it's a nitpick.

> +
> +	ret = rtl8365mb_vlan_port_get_pvid(priv, port, &hw_pvid);
> +	if (ret)
> +		return ret;
> +
> +	if (pvid == hw_pvid)
> +		return 0;
> +
> +	if (pvid)
> +		return rtl8365mb_vlan_pvid_port_set(ds, port, pvid, extack);
> +
> +	return rtl8365mb_vlan_pvid_port_clear(ds, port, hw_pvid,
> +					      vlan_filtering);

Can we unify _set and _clear somehow? The code gets quite confusing with
the two variants, and it seems now that with the vlan_filtering
argument, you can unify the logic.

In general the patch makes sense, I'm just straining a bit to follow the
logic reliably. If not something like the above, can you give a thought
as to how it might otherwise be simplified?

Kind regards,
Alvin