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

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?

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

[Jens Emil Schulz Ostergaard]

On Tue, 2026-05-05 at 18:46 -0700, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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)?
> 

I will update the commit message.

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

I will remove VLAN_HSR_PRP as it is not used at the moment.


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

I will use -EINVAL instead of -EBUSY in both add and del.

> > +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?