Re: [PATCH net-next v5 5/9] net: dsa: realtek: rtl8365mb: set and get vlan 4k for 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] On family D, rtl8365mb_vlan_4k_write()
  (drivers/net/dsa/realtek/rtl8365mb_vlan.c) sets…

--

net: dsa: realtek: rtl8365mb: set and get vlan 4k for family D

This commit gives family D its own two-word pack/unpack for CVLAN (VLAN 4K)
table entries, instead of reusing the three-word family C layout. It uses
a narrower 2-bit FID and drops the priority/meter fields. On write, it
forces both the IVL_EN and SVLAN_CHK_IVL_SVL bits to 1.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_vlan.c b/drivers/net/dsa/realtek/rtl8365mb_vlan.c
> index 8d49ffadce926..c2cfdf194aa9e 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_vlan.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_vlan.c

[ ... ]

> @@ -244,36 +290,61 @@ static int rtl8365mb_vlan_4k_write(struct realtek_priv *priv,
>  	val = FIELD_GET(RTL8365MB_CVLAN_UNTAG_LO_MASK, vlan4k->untag);
>  	data[0] |= FIELD_PREP(RTL8365MB_CVLAN_ENTRY_D0_UNTAG_MASK, val);
>
> -	data[1] |= FIELD_PREP(RTL8365MB_CVLAN_ENTRY_D1_FID_MASK, vlan4k->fid);

[ ... ]

> +	if (is_d) {
> +		/* The chip supports both IVL and SVL, but the caller (see
> +		 * rtl8365mb_vlan_4k_port_set()) never requests SVL, so both
> +		 * IVL/SVL selector bits are forced here rather than threaded
> +		 * through from vlan4k->ivl_en, which family C does honor.
> +		 * The vendor CVLAN API leaves SVLAN_CHK_IVL_SVL at 0, see the
> +		 * commit message.
> +		 */
> +		data[1] |= FIELD_PREP(RTL8365MB_D_CVLAN_ENTRY_D1_IVL_EN_MASK,
> +				      1) |
> +			   FIELD_PREP(RTL8365MB_D_CVLAN_ENTRY_D1_SVLAN_CHK_IVL_SVL_MASK,
> +				      1);

[Severity: Medium]
Should SVLAN_CHK_IVL_SVL be written as 1 in rtl8365mb_vlan_4k_write() on
family D?

The commit message says:

    The vendor RTL8367D CVLAN API leaves SVLAN_CHK_IVL_SVL
    at 0 and only sets it from the SVLAN member API; the value 1 has not been
    verified against the hardware with the bit cleared.

The reason given for forcing both bits is that the only caller always asks
for IVL:

drivers/net/dsa/realtek/rtl8365mb_vlan.c:rtl8365mb_vlan_4k_port_set() {
    ...
	vlan4k.ivl_en = true; /* always use Independent VLAN Learning */
    ...
}

That explains IVL_EN = 1, but does it explain the SVLAN bit? The vendor ties
that bit to SVLAN membership. This driver never configures SVLAN, and
RTL8365MB_D_CVLAN_ENTRY_D1_SVLAN_CHK_IVL_SVL_MASK is the only SVLAN-related
symbol under drivers/net/dsa/realtek/.

Even so, every bridge VLAN add or delete on a family D port sets the bit to
1:

rtl8365mb_vlan_4k_port_add()/rtl8365mb_vlan_4k_port_del()
  -> rtl8365mb_vlan_4k_port_set()
    -> rtl8365mb_vlan_4k_write()

This also covers entries whose member set becomes empty after the last
port is removed.

rtl8365mb_vlan_4k_read() never decodes this bit, so the read-modify-write
in rtl8365mb_vlan_4k_port_set() can't preserve whatever value the hardware
had.

If the bit has any effect, for example on how S-tagged traffic follows this
CVLAN entry's IVL/SVL setting, could learning or forwarding for matching
VIDs differ from what the vendor code produces?

Would it be safer to follow the vendor CVLAN path here? That would mean
writing SVLAN_CHK_IVL_SVL as 0 and deriving IVL_EN from vlan4k->ivl_en.

A related point about IVL_EN: on family D, rtl8365mb_vlan_4k_write()
ignores vlan4k->ivl_en, but rtl8365mb_vlan_4k_read() decodes it:

> +		vlan4k->ivl_en =
> +			FIELD_GET(RTL8365MB_D_CVLAN_ENTRY_D1_IVL_EN_MASK,
> +				  data[1]);

That asymmetry only does no harm because the single caller sets
ivl_en = true.

The code is unchanged at the end of the series, so this behavior ships with
the RTL8367S-VB enablement in "net: dsa: realtek: rtl8365mb: add support
for RTL8367S-VB".

> +		data[1] |= FIELD_PREP(RTL8365MB_D_CVLAN_ENTRY_D1_FID_MASK,
> +				      vlan4k->fid);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/aa5fa12c-e43e-4e5e-9f77-c6eaf477abc4%40yahoo.com