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

Jakub Kicinski
2026-05-06T01:46:14Z

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

Adds VLAN-aware bridge support to the lan9645x DSA driver, wiring up
.port_vlan_filtering, .port_vlan_add and .port_vlan_del, along with a
new lan9645x_vlan.c that programs the hardware VLAN table and per-port
ingress/egress tag handling. Reserves PVIDs for standalone and unaware
modes to implement FDB isolation.

> 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.

Does the commit message accurately describe the reserved VID range?
Looking at drivers/net/dsa/microchip/lan9645x/lan9645x_main.h:

#define UNAWARE_PVID 0
#define HOST_PVID 4095
#define VLAN_HSR_PRP 4094
#define VLAN_MAX (VLAN_HSR_PRP - 1)

VLAN_MAX ends up at 4093, and both the add and delete paths reject any
vid > VLAN_MAX, with an extack that says "VLAN range 4094-4095
reserved.". So in practice both 4094 and 4095 are removed from the
user-visible VID space, not only 4095 as the commit log states. Could
the message be updated to reflect that, and to clarify that "A
vlan-unaware bridge uses vid 0" refers to an internal UNAWARE_PVID
rather than to a VID provisioned by the bridge core (the driver
silently returns 0 for vid == 0)?

> 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..854a2dde1ecf
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_vlan.c

[ ... ]

> +int lan9645x_vlan_port_add_vlan(struct lan9645x_port *p, u16 vid, bool pvid,
> + bool untagged, struct netlink_ext_ack *extack)
> +{
> + struct lan9645x *lan9645x = p->lan9645x;
> + struct lan9645x_vlan_port_info info;
> + struct lan9645x_vlan old_vlan;
> + struct lan9645x_vlan *v;
> + u16 old_pvid;
> +
> + /* Kernel VLAN core adds vid 0, which collides with our UNAWARE_PVID.
> + * We handle priority tagged frames by other means.
> + */
> + if (!vid)
> + return 0;
> +
> + if (vid > VLAN_MAX) {
> + NL_SET_ERR_MSG_MOD(extack, "VLAN range 4094-4095 reserved.");
> + return -EBUSY;
> + }

Is reserving VID 4094 for HSR/PRP appropriate at this point? Grepping
the series shows VLAN_HSR_PRP has no consumer beyond its use in the
definition of VLAN_MAX, and no HSR/PRP code is present in-tree for
this driver.

The net effect is that 4094, a valid 802.1Q VID, is permanently
carved out of the user-visible VLAN space on first merge. Would it be
better to only reserve 4095 now, and introduce the 4094 reservation
together with the HSR/PRP support that actually consumes it?

On the errno choice: -EBUSY conventionally implies a transient
condition that might succeed on retry, but this rejection is
permanent for any VID above VLAN_MAX. Would -EOPNOTSUPP (or -EINVAL,
for a structurally out-of-range VID) be a better fit here, so that
userspace tooling does not interpret the failure as retryable?

> +int lan9645x_vlan_port_del_vlan(struct lan9645x_port *p, u16 vid)
> +{
> + struct lan9645x *lan9645x = p->lan9645x;
> + struct lan9645x_vlan *v;
> +
> + if (!vid)
> + return 0;
> +
> + if (vid > VLAN_MAX)
> + return -EBUSY;

Same question for the delete path: should this be -EOPNOTSUPP (or
similar) rather than -EBUSY for a permanently reserved VID?