Re: [PATCH net-next v4 2/8] net: dsa: motorcomm: Drop VID 0 VLAN table programming

[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] The commit message says the VID 0 entry is never looked up,
  but the driver itself can make VID 0 the classification VID, so the…

--

net: dsa: motorcomm: Drop VID 0 VLAN table programming

This commit removes the write of VLAN table entry 0 in
yt921x_chip_setup_dsa(). That write set LEARN_DIS and all port members
with an empty untag mask. The commit message says the hardware classifies
priority-tagged frames to the port's PVID before the VLAN table lookup, so
the VID 0 entry is never looked up.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 693564aeaec14..47f06945b8c4e 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c

[ ... ]

> @@ -2321,15 +2320,6 @@ static int yt921x_chip_setup_dsa(struct yt921x_priv *priv)
>  	if (res)
>  		return res;
>  
> -	/* Tagged VID 0 should be treated as untagged, which confuses the
> -	 * hardware a lot
> -	 */
> -	ctrls[0] = YT921X_VLAN_CTRLa_LEARN_DIS | YT921X_VLAN_CTRLa_PORTS_M;
> -	ctrls[1] = 0;
> -	res = yt921x_reg64_write(priv, YT921X_VLANn_CTRL(0), ctrls);
> -	if (res)
> -		return res;
> -

[Severity: Medium]
Is entry 0 really unreachable when the port's PVID is 0? The commit message
says:

  the hardware classifies them to the port's PVID before the VLAN table
  lookup, just like untagged frames (measured on YT9215S), so the VID 0
  entry is never looked up.

The driver itself programs a hardware PVID of 0 on VLAN-filtering ports.
In yt921x_vlan_filtering(), br_vlan_get_pvid() gives 0 when the bridge port
has no PVID:

	else
		br_vlan_get_pvid(bdev, &pvid);
	res = yt921x_port_set_pvid(priv, port, pvid);
	...
	if (vlan_filtering && !pvid)
		ctrl |= YT921X_PORT_VLAN_CTRL1_CVLAN_DROP_UNTAGGED;

yt921x_pvid_clear() does the same after the PVID VLAN is deleted, for
example via yt921x_dsa_port_vlan_del():

	res = yt921x_port_set_pvid(priv, port,
				   vlan_filtering ? 0 : YT921X_VID_UNWARE);

In both cases the only extra guard is CVLAN_DROP_UNTAGGED. Does that bit
also drop priority-tagged frames? If it does not, a priority-tagged frame
on such a port would be classified to PVID 0 and look up entry 0.

After this patch, entry 0 keeps its reset contents. The 8021q layer can
also change it at runtime by adding VID 0 automatically:

dsa_user_vlan_rx_add_vid(vid = 0)
  dsa_port_vlan_add() / dsa_port_host_vlan_add()
    yt921x_dsa_port_vlan_add()
      yt921x_vlan_add()

yt921x_vlan_add() sets only the port, CPU port and untag bits, never
LEARN_DIS:

	masks[0] = YT921X_VLAN_CTRLa_PORTn(port) |
		   YT921X_VLAN_CTRLa_PORTS(priv->cpu_ports_mask);
	ctrls[0] = masks[0];
	...
	return yt921x_reg64_update_bits(priv, YT921X_VLANn_CTRL(vid),
					masks, ctrls);

No other code in the driver sets YT921X_VLAN_CTRLa_LEARN_DIS. Could the
switch now learn source MACs and forward in hardware in VID 0 on a
filtering port with no PVID? The Linux bridge would drop those frames.

The measurement in the commit message also covers only YT9215S. The rest
of the series does not restore a LEARN_DIS write to YT921X_VLANn_CTRL(0),
and PVID 0 is still programmed in yt921x_vlan_filtering() and
yt921x_pvid_clear().

Should the commit message cover the PVID 0 case and the loss of LEARN_DIS
on VID 0? Or should the VID 0 programming be kept?

>  	return 0;
>  }
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107121631.1040612-1-mmyangfl%40gmail.com