Re: [PATCH net-next v2 3/8] net: dsa: realtek: rtl8365mb: get pvid 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] At this commit only rtl8365mb_vlan_port_get_pvid() is converted
  to the family D layout.

--

net: dsa: realtek: rtl8365mb: get pvid for family D

On family D chips, this commit changes rtl8365mb_vlan_port_get_pvid() to
read the PVID directly from a per-port register at 0x0700 + port, taking
bits [11:0]. Family C keeps the old lookup through the VLAN MC table
index.

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

[ ... ]

> @@ -679,11 +685,22 @@ 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);

[Severity: Low]
At this commit, do the family D reader and writers use different register
layouts?

The new branch reads 0x0700 + port and treats bits [11:0] as a VID. The
writers are not changed, so on family D they still use the family C
layout. rtl8365mb_vlan_port_set_pvid() writes a 5-bit MC index into
0x0700 + (port >> 1):

drivers/net/dsa/realtek/rtl8365mb_vlan.c:rtl8365mb_vlan_port_set_pvid() {
    ...
	val = vlanmc_idx << RTL8365MB_VLAN_PVID_CTRL_PORT_MCIDX_OFFSET(port);
	ret = regmap_update_bits(priv->map,
				 RTL8365MB_VLAN_PVID_CTRL_REG(port),
				 RTL8365MB_VLAN_PVID_CTRL_PORT_MCIDX_MASK(port),
				 val);
    ...
}

rtl8365mb_vlan_pvid_port_set() and rtl8365mb_vlan_pvid_port_clear() also
still read the old state through rtl8365mb_vlan_get_pvid_mc(). So on
family D at this commit, reading the PVID back after
rtl8365mb_vlan_pvid_port_set() would not return the VID that was set.
That affects callers such as rtl8365mb_port_vlan_filtering() in
rtl8365mb_main.c.

This is resolved later in the series. The commit "net: dsa: realtek:
rtl8365mb: set/clear pvid for family D" adds family D dispatches at the
top of rtl8365mb_vlan_pvid_port_set() and rtl8365mb_vlan_pvid_port_clear().
Those dispatches go to rtl8365mb_vlan_pvid_port_set_direct() and
rtl8365mb_vlan_pvid_port_clear_direct(), which use the same
RTL8365MB_D_VLAN_PVID_CTRL_REG(port) / GENMASK(11, 0) layout as this
getter.

Also, the family D chip (ID 0x6642) only starts probing with "net: dsa:
realtek: rtl8365mb: detect RTL8367S-VB" later in the series. So no
existing setup is affected.

Could the getter and the set/clear conversion be merged into one commit,
or reordered? That would keep the family D read and write paths
consistent at every point in the series.

> +		if (ret)
> +			return ret;
> +
> +		*pvid = data & RTL8365MB_D_VLAN_PVID_CTRL_MASK;
> +	} else {

[ ... ]

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