Re: [PATCH net-next v3 6/9] net: dsa: realtek: rtl8365mb: use raw VID for PVID on family D

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] This patch adds a family D branch to
  rtl8365mb_vlan_port_get_pvid()…

--

net: dsa: realtek: rtl8365mb: use raw VID for PVID on family D

On family D chips, this commit reads the PVID as a raw 12-bit VID from the
per-port register at 0x0700 + port. Family C keeps using the VLAN MC table
index. It also adds direct-VID variants of the PVID set and clear paths
that skip the MC table on family D.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_vlan.c b/drivers/net/dsa/realtek/rtl8365mb_vlan.c
> index 2c4c5baf93f10..ccfa764b98125 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_vlan.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_vlan.c

[ ... ]

> @@ -742,11 +751,25 @@ int rtl8365mb_vlan_port_get_pvid(struct realtek_priv *priv, int port, u16 *pvid)
>  	u8 vlanmc_idx;
>  	int ret;
>  
> -	ret = rtl8365mb_vlan_get_pvid_mc(priv, port, &vlanmc_idx, &vlanmc);
> -	if (ret)
> -		return ret;
> +	if (rtl8365mb_get_family(priv) == RTL8365MB_FAMILY_D) {
> +		u32 data;
> +
> +		ret = regmap_read(priv->map,
> +				  RTL8365MB_D_VLAN_PVID_CTRL_REG(port),
> +				  &data);
> +		if (ret)
> +			return ret;
> +
> +		*pvid = data & RTL8365MB_D_VLAN_PVID_CTRL_MASK;

[Severity: Medium]
Can this report a PVID that the bridge never configured?

The kernel-doc for rtl8365mb_vlan_port_get_pvid() still says:

 * Returns the port PVID if defined or 0 if not.

On family C that holds. An unconfigured port has VLANMC index 0, and
rtl8365mb_vlan_get_pvid_mc() memsets the entry, so evid is 0. On family D
this branch returns whatever the register holds.

rtl8365mb_vlan_setup() only calls
rtl8365mb_port_vlan_filtering(ds, dp->index, false, NULL) for each port
and then sets RTL8365MB_VLAN_CTRL_EN_MASK. Nothing there writes 0 to the
family D PVID registers. The only code that writes 0 is
rtl8365mb_vlan_pvid_port_clear_direct(), and it only does so when the
bridge clears a VID that matches the register.

After rtl8365mb_reset_chip(), the register holds its hardware default or
a value loaded from EEPROM. If that value is non-zero, two callers act on
it.

In rtl8365mb_port_vlan_filtering() with vlan_filtering set:

	if (pvid_vid)
		accepted_frame = RTL8365MB_FRAME_TYPE_ANY_FRAME;

Here, a VLAN-filtering port with no bridge PVID (for example a bridge
with default_pvid 0) would accept untagged frames into the stale VID.

In the undo_set_pvid path of rtl8365mb_port_vlan_add():

	if (pvid_vid)
		(void)rtl8365mb_vlan_pvid_port_set(ds, port, pvid_vid,
						   NULL);

On error, this path would program the stale VID as the PVID.

I couldn't confirm the reset value of these registers from the tree, so
this depends on the hardware or EEPROM default. The family D get_pvid
branch and rtl8365mb_vlan_setup() also look unchanged at the end of the
series.

Should rtl8365mb_vlan_setup() clear the family D PVID registers to 0? That
would match the family C convention of VLANMC index 0 as the neutral
PVID.

> +	} else {
> +		ret = rtl8365mb_vlan_get_pvid_mc(priv, port, &vlanmc_idx,
> +						 &vlanmc);
> +		if (ret)
> +			return ret;
> +
> +		*pvid = vlanmc.evid;
> +	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/e84d76ee-03df-49b0-8c9a-b289dfae8728%40yahoo.com