Re: [net-next PATCH v3 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.
---
Subject: net: dsa: realtek: rtl8365mb: add VLAN support

This patch adds VLAN support for the rtl8365mb switch family, using the
4096-entry VLAN4K table as the source of truth for membership and
dynamically allocating entries from the 32-entry VLAN MC table to back
per-port PVID assignments. Standalone ports use VLAN-transparent mode
combined with port isolation, and filtering is toggled per-port via
ingress filtering, transparent mode and the egress tag-mode register.

[Low]
Regarding this portion of the commit message:

    Once a member in a bridge, the port isolation is expanded to include
    the bridge members. When that bridge enables VLAN filtering, the VLAN
    transparent feature is disabled, letting the switch filter based on
    VLAN setup.

This patch does not add port_bridge_join or port_bridge_leave callbacks
and does not adjust the isolation masks when bridge membership changes;
that functionality appears to be introduced by the following patch in the
series (net: dsa: realtek: rtl8365mb: add port_bridge_{join,leave}).

Would it be clearer to describe the end state as applying to the full
series, or to move this paragraph to the patch that actually implements
the bridge-driven isolation adjustment?

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index 7ddb82fcd026..b37aa847a9aa 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c

[ ... ]

> +static int rtl8365mb_port_vlan_filtering(struct dsa_switch *ds, int port,
> +					 bool vlan_filtering,
> +					 struct netlink_ext_ack *extack)
> +{
> +	enum rtl8365mb_vlan_egress_mode mode;
> +	struct realtek_priv *priv = ds->priv;
> +	struct dsa_port *dp;
> +	int ret;
> +
> +	dev_dbg(priv->dev, "port %d: %s VLAN filtering\n", port,
> +		vlan_filtering ? "enable" : "disable");
> +
> +	/* When vlan filter is enable/disabled in a bridge, this function is
> +	 * called for all member ports. We need to enable/disable ingress
> +	 * VLAN membership check.
> +	 */
> +	ret = rtl8365mb_port_set_ingress_filtering(priv, port, vlan_filtering);
> +	if (ret)
> +		return ret;
> +
> +	/* 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)
> +			return ret;
> +	}
> +
> +	/* When VLAN filtering is disabled, preserve frames exactly as received.
> +	 * Otherwise, the VLAN egress pipeline may still alter tag state
> +	 * according to VLAN membership and untag configuration.
> +	 */
> +	if (vlan_filtering)
> +		mode = RTL8365MB_VLAN_EGRESS_MODE_ORIGINAL;
> +	else
> +		mode = RTL8365MB_VLAN_EGRESS_MODE_REAL_KEEP;
> +
> +	return rtl8365mb_port_set_vlan_egress_mode(priv, port, mode);
> +}

[Low]
If any of the regmap writes here fails, is the hardware left in a
partially-updated state?

rtl8365mb_port_set_ingress_filtering() is already committed before the
loop runs, and inside the loop each rtl8365mb_port_set_transparent() call
flips a transparent bit on a different egress register. A mid-loop failure
returns the error but leaves earlier ports with transparent mode toggled
and also skips the trailing rtl8365mb_port_set_vlan_egress_mode() write.

Would it be worth reverting already-applied changes on error, so that a
returned failure corresponds to an unchanged port configuration rather
than a port split between the old and new vlan_filtering state?

[ ... ]

> @@ -2035,6 +2266,20 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
>  	if (ret)
>  		goto out_teardown_irq;
>  
> +	ds->configure_vlan_while_not_filtering = true;
> +
> +	/* Set up VLAN */
> +	ret = rtl8365mb_vlan_setup(ds);
> +	if (ret)
> +		goto out_teardown_irq;
> +
> +	/* Set maximum packet length to 1536 bytes */
> +	ret = regmap_update_bits(priv->map, RTL8365MB_CFG0_MAX_LEN_REG,
> +				 RTL8365MB_CFG0_MAX_LEN_MASK,
> +				 FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK, 1536));
> +	if (ret)
> +		goto out_teardown_irq;
> +

[Medium]
Is this RTL8365MB_CFG0_MAX_LEN_REG write intended to be part of this VLAN
patch?

rtl8365mb_setup() already calls rtl8365mb_port_change_mtu(ds,
cpu->trap_port, ETH_DATA_LEN) earlier, which programs the same global
RTL8365MB_CFG0_MAX_LEN_REG with frame_size derived from
ETH_DATA_LEN + VLAN_ETH_HLEN + ETH_FCS_LEN (1522). This new hunk then
unconditionally overwrites that with 1536.

Neither the subject nor the commit message mention any MTU or
frame-length change. Would it make sense to split this MAX_LEN_REG
adjustment into its own patch with a rationale for the 1536 value, rather
than carrying it as an unrelated side-effect of VLAN support?

[ ... ]

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

[ ... ]

> +static int rtl8365mb_vlan_mc_read(struct realtek_priv *priv, u32 index,
> +				  struct rtl8365mb_vlanmc *vlanmc)
> +{
> +	u16 data[RTL8365MB_VLAN_MC_ENTRY_SIZE];
> +	int ret;
> +
> +	ret = regmap_bulk_read(priv->map, RTL8365MB_VLAN_MC_REG(index), &data,
> +			       RTL8365MB_VLAN_MC_ENTRY_SIZE);
> +	if (ret)
> +		return ret;
> +
> +	vlanmc->member = FIELD_GET(RTL8365MB_VLAN_MC_D0_MBR_MASK, data[0]);
> +	vlanmc->fid = FIELD_GET(RTL8365MB_VLAN_MC_D1_FID_MASK, data[1]);
> +	vlanmc->priority = FIELD_GET(RTL8365MB_VLAN_MC_D2_VBPRI_MASK, data[2]);
> +	vlanmc->evid = FIELD_GET(RTL8365MB_VLAN_MC_D3_EVID_MASK, data[3]);
> +
> +	return 0;
> +}
> +
> +static int rtl8365mb_vlan_mc_write(struct realtek_priv *priv, u32 index,
> +				   const struct rtl8365mb_vlanmc *vlanmc)
> +{
> +	u16 data[4] = { 0 };
> +	int ret;
> +
> +	data[0] |= FIELD_PREP(RTL8365MB_VLAN_MC_D0_MBR_MASK, vlanmc->member);
> +	data[1] |= FIELD_PREP(RTL8365MB_VLAN_MC_D1_FID_MASK, vlanmc->fid);
> +	data[2] |= FIELD_PREP(RTL8365MB_VLAN_MC_D2_METERIDX_MASK,
> +			      vlanmc->meteridx);
> +	data[2] |= FIELD_PREP(RTL8365MB_VLAN_MC_D2_ENVLANPOL_MASK,
> +			      vlanmc->policing_en);
> +	data[2] |=
> +		FIELD_PREP(RTL8365MB_VLAN_MC_D2_VBPRI_MASK, vlanmc->priority);
> +	data[2] |= FIELD_PREP(RTL8365MB_VLAN_MC_D2_VBPEN_MASK,
> +			      vlanmc->priority_en);
> +	data[3] |= FIELD_PREP(RTL8365MB_VLAN_MC_D3_EVID_MASK, vlanmc->evid);
> +
> +	ret = regmap_bulk_write(priv->map, RTL8365MB_VLAN_MC_REG(index), &data,
> +				RTL8365MB_VLAN_MC_ENTRY_SIZE);
> +
> +	return ret;
> +}

[Medium]
Are rtl8365mb_vlan_mc_read() and rtl8365mb_vlan_mc_write() intended to be
symmetric?

rtl8365mb_vlan_mc_read() only extracts member, fid, priority and evid from
the hardware entry, while rtl8365mb_vlan_mc_write() writes member, fid,
meteridx, policing_en (ENVLANPOL), priority, priority_en (VBPEN) and
evid.

In rtl8365mb_vlan_mc_port_set() the pattern is to declare
struct rtl8365mb_vlanmc vlanmc = {0}, call rtl8365mb_vlan_mc_read() when
an entry exists, toggle only vlanmc.member, and write it back. Because
priority_en, policing_en and meteridx are never populated by the read,
the subsequent write unconditionally FIELD_PREPs them as zero.

Would it be safer to unpack VBPEN, ENVLANPOL and METERIDX in
rtl8365mb_vlan_mc_read() as well, so that read/modify/write preserves
those bits? The rtl8365mb_vlan_4k_read()/rtl8365mb_vlan_4k_write() pair
already handles all fields symmetrically.

[ ... ]

> +static int rtl8365mb_vlan_mc_find(struct realtek_priv *priv, u16 vid,
> +				  u8 *index, u8 *first_free)
> +{
> +	u32 vlan_entry_d3;
> +	u8 vlanmc_idx;
> +	u16 evid;
> +	int ret;
> +
> +	*index = 0;
> +	*first_free = 0;
> +
> +	/* look for existing entry or an empty one */
> +	/* vlanmc index 0 is reserved as a neutral PVID value for standalone
> +	 * ports. Traffic reaches the CPU via VLAN transparent mode.
> +	 **/
> +	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;
> +}

[Low]
Does this function correctly disambiguate "VLAN 0 already programmed"
from "this slot is empty"?

Both conditions use evid == 0 as their signal:

    if (evid == vid) {
        *index = vlanmc_idx;
        return 0;
    }

    if (evid == 0x0 && *first_free < 1)
        *first_free = vlanmc_idx;

When called with vid == 0, the first empty slot encountered satisfies
evid == vid and is returned via *index as if it were a pre-existing
match. rtl8365mb_vlan_mc_port_set() then treats it as an allocated entry
and performs a read-modify-write that silently claims the slot.

The driver's entry points only reject vid > RTL8365MB_MAX_MC_VID; vid ==
0 is not explicitly guarded. Would adding a "used" bit per MC slot (or an
explicit vid == 0 check at the add/del entry points) remove this
ambiguity?

[ ... ]

> +int rtl8365mb_vlan_pvid_port_add(struct dsa_switch *ds, int port,
> +				 const struct switchdev_obj_port_vlan *vlan,
> +				 struct netlink_ext_ack *extack)
> +{
> +	bool pvid = !!(vlan->flags & BRIDGE_VLAN_INFO_PVID);
> +	enum rtl8365mb_frame_ingress accepted_frame;
> +	struct realtek_priv *priv = ds->priv;
> +	u8 _unused_first_free_idx;
> +	u8 vlanmc_idx;
> +	int ret;
> +
> +	if (!pvid)
> +		return 0;
> +
> +	/* Find or allocate a new vlan MC and add port to members,
> +	 * although members are not checked by the HW in vlan MC.
> +	 */
> +	ret = rtl8365mb_vlan_mc_port_set(ds, port, vlan, extack, true);
> +	if (ret)
> +		return ret;
> +
> +	/* look for existing entry */
> +	ret = rtl8365mb_vlan_mc_find(priv, vlan->vid, &vlanmc_idx,
> +				     &_unused_first_free_idx);
> +	if (ret) {
> +		RTL_VLAN_ERR("Failed to find a VLAN MC table index");
> +		goto undo_vlan_mc_port_set;
> +	}
> +
> +	if (!vlanmc_idx) {
> +		RTL_VLAN_ERR("VLAN should already exist in VLAN MC");
> +		goto undo_vlan_mc_port_set;
> +	}

[Medium]
Does this error path return success where it intended to return an
error?

At the !vlanmc_idx branch, ret is 0 (rtl8365mb_vlan_mc_find() just
succeeded), and the undo label ends with:

    undo_vlan_mc_port_set:
        (void)rtl8365mb_vlan_mc_port_set(ds, port, vlan, NULL, false);
        return ret;

So rtl8365mb_vlan_pvid_port_add() would return 0 after logging "VLAN
should already exist in VLAN MC" and rolling back the MC allocation.
rtl8365mb_port_vlan_add() then treats the operation as successful and
leaves the VLAN4K membership in place, hiding the inconsistency from the
DSA core.

Should this branch set ret to something like -EIO (or -ENOENT) before
the goto, so the rollback is paired with a returned error?

> +
> +	ret = rtl8365mb_vlan_port_set_pvid(priv, port, vlanmc_idx);
> +	if (ret) {
> +		RTL_VLAN_ERR("Failed to set port PVID");
> +		goto undo_vlan_mc_port_set;
> +	}

[ ... ]