Re: [PATCH net-next v2 6/8] net: dsa: realtek: rtl8365mb: set/clear pvid for family D

[netdev-bot+sashiko@kernel.org]

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] The commit message and the new kernel-doc are inaccurate in two
  places.
- [Low] Several comments are stale after this patch.

--

net: dsa: realtek: rtl8365mb: set/clear pvid for family D

This patch adds a direct-VID path to rtl8365mb_vlan_pvid_port_set() and
rtl8365mb_vlan_pvid_port_clear() for RTL8365MB_FAMILY_D chips. On those
chips, the PVID is written as a raw 12-bit VID to a per-port register, and
the VLAN MC table is skipped entirely.

> The RTL8367S-VB (family D) has no working VLAN member-config (MC)
> table in hardware. rtl8365mb_vlan_port_get_pvid() was already fixed
> to read PVID directly from its dedicated per-port register, but
> rtl8365mb_vlan_pvid_port_set() and rtl8365mb_vlan_pvid_port_clear()
> still went through the MC-index allocation path meant for family C,
> which on family D hardware writes into the wrong register/field
> (colliding with the neighbouring port's real PVID register) and reads
> back a VLAN MC table that does not exist on this silicon.

[Severity: Low]
Is "the neighbouring port's real PVID register" accurate for all ports?

The family-C macros map port p like this, with a 5-bit field at bit offset
((_p) & 1) << 3:

#define RTL8365MB_VLAN_PVID_CTRL_REG(_p) \
		(RTL8365MB_VLAN_PVID_CTRL_BASE + ((_p) >> 1))

On family D, 0x0700 + q is port q's own 12-bit VID register:

#define RTL8365MB_D_VLAN_PVID_CTRL_REG(port) \
	(RTL8365MB_D_VLAN_PVID_CTRL_BASE + (port))

So a family-C write for port p lands in the VID field of port (p >> 1).
Port 0 corrupts its own VID. Only ports 1 and 2 hit a neighbour (ports 0
and 1). Port 3 hits port 1, ports 4 and 5 hit port 2, and ports 6 and 7
hit port 3.

Could the commit message say that port p's write lands in port
(p >> 1)'s PVID register?

Separately, the new kernel-doc for rtl8365mb_vlan_pvid_port_set_direct()
says:

 * the rollback behavior of the family-C implementation above.

The family-C rtl8365mb_vlan_pvid_port_set() is defined below this helper,
though, not above it.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_vlan.c b/drivers/net/dsa/realtek/rtl8365mb_vlan.c
> index 0466e8cbc4123..da517b1aae208 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_vlan.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_vlan.c
> @@ -810,6 +810,64 @@ rtl8365mb_vlan_port_set_framefilter(struct realtek_priv *priv,

[ ... ]

>  /*
>   * rtl8365mb_vlan_pvid_port_set() - Configure a port's PVID and associated
>   * VLANMC entry
> @@ -839,6 +897,13 @@ int rtl8365mb_vlan_pvid_port_set(struct dsa_switch *ds, int port, u16 vid,
>  
>  	lockdep_assert_held(&priv->vlan_lock);
>  
> +	/* This chip family has no VLAN MC table - PVID is a raw VID in a
> +	 * dedicated per-port register, and there is no separate membership
> +	 * table entry to allocate/track.
> +	 */
> +	if (rtl8365mb_get_family(priv) == RTL8365MB_FAMILY_D)
> +		return rtl8365mb_vlan_pvid_port_set_direct(priv, port, vid);
> +

[Severity: Low]
With this early return in place, should the kernel-doc for
rtl8365mb_vlan_pvid_port_set() be updated too? It still says:

 * rtl8365mb_vlan_pvid_port_set() - Configure a port's PVID and associated
 * VLANMC entry
 ...
 * Allocates or reuses a hardware VLANMC entry to map the given port to its new
 * PVID.

On RTL8365MB_FAMILY_D, rtl8365mb_vlan_pvid_port_set_direct() writes a raw
VID to RTL8365MB_D_VLAN_PVID_CTRL_REG(port) and never touches the VLANMC
table.

The same applies to rtl8365mb_vlan_pvid_port_clear(). Its kernel-doc still
says:

 * Resets the target port's hardware PVID allocation to 0. Cleans up and frees
 * the associated VLANMC entry if no other ports are referencing it.

The new family D branch calls rtl8365mb_vlan_pvid_port_clear_direct()
instead, which does no VLANMC management.

The file header comment also still says, without naming a family:

 * programming of a port-based VLAN ID (PVID). The PVID is programmed on a
 * per-port basis via register field, which refers to a specific VLAN membership
 * configuration via an index 0~31.
 ...
 * VLANMC index 0 is reserved as a neutral PVID, used for standalone ports.

The struct rtl8365mb_vlanmc kernel-doc also still lists "PVID
configuration" as one of the remaining uses of the VLAN membership
configuration database.

Could these comments mention the family D raw-VID path? The "above"
reference in the rtl8365mb_vlan_pvid_port_set_direct() kernel-doc, noted
earlier, is the same kind of stale comment.

>  	/* Read the old PVID exclusively to undo in case of error */
>  	ret = rtl8365mb_vlan_get_pvid_mc(priv, port, &prev_vlanmc_idx,
>  					 &prev_vlanmc);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/28382f3f-03c1-4606-9b11-86f118abeafe%40yahoo.com