Re: [PATCH net-next v2 6/9] net: dsa: lan9645x: add vlan support

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: lan9645x: add vlan support

Add support for vlanaware bridge. We reserve vid 4095 for standalone
mode, to implement fdb-isolation. A vlan-unaware bridge uses vid 0.

> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> index 599e589c4ec3..046c95a72242 100644
> --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
[ ... ]
> @@ -535,11 +536,54 @@ static void lan9645x_port_bridge_leave(struct dsa_switch *ds, int port,
[ ... ]
> +static int lan9645x_port_vlan_filtering(struct dsa_switch *ds, int port,
> +					bool enabled,
> +					struct netlink_ext_ack *extack)
> +{
> +	struct lan9645x *lan9645x = ds->priv;
> +	struct lan9645x_port *p;
> +
> +	p = lan9645x_to_port(lan9645x, port);
> +	p->vlan_aware = enabled;
> +	lan9645x_vlan_port_apply(p);

In other places, the driver avoids calling egress functions on the npi
port because it conflicts with cpu port header extraction. Since this
unconditionally applies the vlan configuration to both ingress and egress,
could this overwrite the npi port's configuration when the state is
synchronized?

> +
> +	return 0;
> +}
> +
> +static int lan9645x_port_vlan_add(struct dsa_switch *ds, int port,
[ ... ]
> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_vlan.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_vlan.c
> new file mode 100644
> index 000000000000..0459932a626c
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_vlan.c
[ ... ]
> +static void lan9645x_vlan_hw_wr(struct lan9645x *lan9645x, u16 vid)
> +{
> +	struct lan9645x_vlan *v = &lan9645x->vlans[vid];
> +	bool cpu_dis = !(v->portmask & BIT(CPU_PORT));
> +	u32 val;
> +
> +	val = ANA_VLANTIDX_VLAN_PGID_CPU_DIS_SET(cpu_dis) |
> +	      ANA_VLANTIDX_V_INDEX_SET(vid) |
> +	      ANA_VLANTIDX_VLAN_SEC_FWD_ENA_SET(v->s_fwd_ena) |
> +	      ANA_VLANTIDX_VLAN_FLOOD_DIS_SET(v->fld_dis) |
> +	      ANA_VLANTIDX_VLAN_PRIV_VLAN_SET(v->prv_vlan) |
> +	      ANA_VLANTIDX_VLAN_LEARN_DISABLED_SET(v->lrn_dis) |
> +	      ANA_VLANTIDX_VLAN_MIRROR_SET(v->mir) |
> +	      ANA_VLANTIDX_VLAN_SRC_CHK_SET(v->src_chk);
> +
> +	lan_wr(val, lan9645x, ANA_VLANTIDX);
> +	lan_wr(v->portmask, lan9645x, ANA_VLAN_PORT_MASK);
> +	lan_wr(VLANACCESS_CMD_WRITE, lan9645x, ANA_VLANACCESS);

Are these registers written with raw values rather than using their respective
set macros? This circumvents the register abstraction and might risk failures
if the hardware layout changes.

> +
> +	if (lan9645x_vlan_wait_for_completion(lan9645x))
> +		dev_err(lan9645x->dev, "Vlan set mask failed\n");
> +}
[ ... ]
> +static void
> +lan9645x_vlan_port_apply_egress(struct lan9645x_port *p,
> +				struct lan9645x_vlan_port_info *info)
> +{
[ ... ]
> +	lan_rmw(REW_TAG_CFG_TAG_TPID_CFG_SET(3) |
> +		REW_TAG_CFG_TAG_CFG_SET(tag_cfg),
> +		REW_TAG_CFG_TAG_TPID_CFG |
> +		REW_TAG_CFG_TAG_CFG,
> +		lan9645x, REW_TAG_CFG(p->chip_port));
> +
> +	lan_rmw(REW_PORT_VLAN_CFG_PORT_TPID_SET(ETH_P_8021AD) |
> +		REW_PORT_VLAN_CFG_PORT_VID_SET(port_vid),
> +		REW_PORT_VLAN_CFG_PORT_TPID |
> +		REW_PORT_VLAN_CFG_PORT_VID,
> +		lan9645x, REW_PORT_VLAN_CFG(p->chip_port));
> +}

Setting the port tpid to 0x88a8 causes untagged ingress frames to egress trunk
ports with 802.1ad tags. Could this break interoperability with standard
networks that expect 0x8100 tags?

> +static void lan9645x_vlan_port_apply_ingress(struct lan9645x_port *p)
> +{
[ ... ]
> +	/* Drop frames with multicast source address */
> +	val = ANA_DROP_CFG_DROP_MC_SMAC_ENA_SET(1);
> +	if (p->vlan_aware && !pvid)
> +		/* If port is vlan-aware and tagged, drop untagged and priority
> +		 * tagged frames.
> +		 */
> +		val |= ANA_DROP_CFG_DROP_UNTAGGED_ENA_SET(1) |
> +		       ANA_DROP_CFG_DROP_PRIO_S_TAGGED_ENA_SET(1) |
> +		       ANA_DROP_CFG_DROP_PRIO_C_TAGGED_ENA_SET(1);
> +
> +	lan_wr(val, lan9645x, ANA_DROP_CFG(p->chip_port));
> +}

Does this overwrite the entire 32-bit register and silently clear other
initialized bitfields? Would it be safer to use a read-modify-write here
instead?

[ ... ]
> +static int lan9645x_vlan_cpu_add(struct lan9645x_port *p, u16 vid, bool pvid,
> +				 bool untagged)
> +{
> +	struct lan9645x_vlan *v;
> +
> +	v = lan9645x_vlan_port_modify(p, vid, pvid, untagged);

Is it intentional that this bypasses the bounds check on the vlan id?

> +	v->portmask |= BIT(CPU_PORT) | BIT(p->chip_port);
> +	lan9645x_vlan_hw_wr(p->lan9645x, vid);
> +	lan9645x_vlan_port_apply_ingress(p);
> +
> +	return 0;
> +}
> +
> +int lan9645x_vlan_port_add_vlan(struct lan9645x_port *p, u16 vid, bool pvid,
> +				bool untagged, struct netlink_ext_ack *extack)
> +{
[ ... ]
> +void lan9645x_vlan_port_del_vlan(struct lan9645x_port *p, u16 vid)
> +{
> +	struct lan9645x *lan9645x = p->lan9645x;
> +	struct lan9645x_vlan *v;
> +
> +	if (!vid)
> +		return;
> +
> +	v = lan9645x_vlan_port_modify(p, vid, false, false);

There is a check for the maximum vlan id when adding a vlan, but no bounds
check when deleting one. Could userspace use this to delete the reserved
vid 4095 and break standalone forwarding?

> +	v->portmask &= ~BIT(p->chip_port);
> +
> +	if (p->chip_port == lan9645x->npi)
> +		v->portmask &= ~BIT(CPU_PORT);
> +
> +	lan9645x_vlan_hw_wr(lan9645x, vid);
[ ... ]
> +void lan9645x_vlan_init(struct lan9645x *lan9645x)
> +{
[ ... ]
> +	for (vid = 1; vid < VLAN_N_VID; vid++)
> +		lan9645x_vlan_hw_wr(lan9645x, vid);

If the indirect table access times out, the write function prints an error
but does not return a status code. Will this cause the initialization loop
to stall for the full timeout duration 4095 consecutive times if the
hardware is unresponsive?

> +
> +	/* Set all the ports + cpu to be part of HOST_PVID and UNAWARE_PVID */
> +	lan9645x->vlans[HOST_PVID].portmask = all_ports;