Re: [PATCH net-next v3 5/9] net: dsa: realtek: rtl8365mb: set and get vlan 4k for family D

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In rtl8365mb_vlan_4k_write()
  (drivers/net/dsa/realtek/rtl8365mb_vlan.c), the family-D branch 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 the CVLAN
(VLAN4k) table entry in rtl8365mb_vlan_4k_read() and
rtl8365mb_vlan_4k_write(). Family C keeps its three-word layout. On
family D the FID is narrowed to 2 bits and the write path forces the
IVL selection bits.

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

[ ... ]

> @@ -244,36 +284,59 @@ 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);
>  

[ ... ]

> +	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.
> +		 */
> +		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: Low]

This doesn't change runtime behavior today, because
rtl8365mb_vlan_4k_port_set() always sets vlan4k.ivl_en = true. However,
the commit message and comments no longer match the code.

The commit message says:

    so that control bit is forced here to match existing driver behavior,
    not a hardware limitation.

Two bits are forced here, though: IVL_EN (bit 3) and SVLAN_CHK_IVL_SVL
(bit 2). Could the commit message mention the second bit and say why 1
is the right value for it?

Family D writes ignore vlan4k->ivl_en, but rtl8365mb_vlan_4k_read()
still reports the field from hardware:

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

So the read path and the write path handle the field differently.

Some comments in the file are also out of date for family D. The
kernel-doc for struct rtl8365mb_vlan4k still says:

 * @ivl_en: enable IVL instead of default SVL

The file header still says VLAN4k entries can choose between IVL and
SVL.

The layout comment above the new size define still says u16[3]:

/* CVLAN (i.e. VLAN4k) table entry layout, u16[3] */
#define RTL8365MB_CVLAN_ENTRY_SIZE			3 /* 48-bits */
#define RTL8365MB_D_CVLAN_ENTRY_SIZE			2 /* 32-bits */

The family D D1 masks are also mixed in with the family C masks under
that comment. Later patches in the series don't update any of these
comments either.

Should these comments be updated to describe the family D layout and
the forced IVL bits?

[ ... ]

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