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

[Jakub Kicinski]

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?