Message ID | 20191026180427.14039-3-olteanv@gmail.com |
---|---|
State | Accepted |
Delegated to: | David Miller |
Headers | show |
Series | VLAN fixes for Ocelot switch | expand |
On 10/26/2019 11:04 AM, Vladimir Oltean wrote: > The switch driver keeps a "vid" variable per port, which signifies _the_ > VLAN ID that is stripped on that port's egress (aka the native VLAN on a > trunk port). > > That is the way the hardware is designed (mostly). The port->vid is > programmed into REW:PORT:PORT_VLAN_CFG:PORT_VID and the rewriter is told > to send all traffic as tagged except the one having port->vid. > > There exists a possibility of finer-grained egress untagging decisions: > using the VCAP IS1 engine, one rule can be added to match every > VLAN-tagged frame whose VLAN should be untagged, and set POP_CNT=1 as > action. However, the IS1 can hold at most 512 entries, and the VLANs are > in the order of 6 * 4096. > > So the code is fine for now. But this sequence of commands: > > $ bridge vlan add dev swp0 vid 1 pvid untagged > $ bridge vlan add dev swp0 vid 2 untagged > > makes untagged and pvid-tagged traffic be sent out of swp0 as tagged > with VID 1, despite user's request. > > Prevent that from happening. The user should temporarily remove the > existing untagged VLAN (1 in this case), add it back as tagged, and then > add the new untagged VLAN (2 in this case).> > Cc: Antoine Tenart <antoine.tenart@bootlin.com> > Cc: Alexandre Belloni <alexandre.belloni@bootlin.com> > Fixes: 7142529f1688 ("net: mscc: ocelot: add VLAN filtering") > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> [snip] > + if (untagged && port->vid != vid) { > + if (port->vid) { > + dev_err(ocelot->dev, > + "Port already has a native VLAN: %d\n", > + port->vid); This sounds like a extended netlink ack candidate for improving user experience, but this should do for now.
On Sat, 26 Oct 2019 at 23:34, Florian Fainelli <f.fainelli@gmail.com> wrote: > > > > On 10/26/2019 11:04 AM, Vladimir Oltean wrote: > > The switch driver keeps a "vid" variable per port, which signifies _the_ > > VLAN ID that is stripped on that port's egress (aka the native VLAN on a > > trunk port). > > > > That is the way the hardware is designed (mostly). The port->vid is > > programmed into REW:PORT:PORT_VLAN_CFG:PORT_VID and the rewriter is told > > to send all traffic as tagged except the one having port->vid. > > > > There exists a possibility of finer-grained egress untagging decisions: > > using the VCAP IS1 engine, one rule can be added to match every > > VLAN-tagged frame whose VLAN should be untagged, and set POP_CNT=1 as > > action. However, the IS1 can hold at most 512 entries, and the VLANs are > > in the order of 6 * 4096. > > > > So the code is fine for now. But this sequence of commands: > > > > $ bridge vlan add dev swp0 vid 1 pvid untagged > > $ bridge vlan add dev swp0 vid 2 untagged > > > > makes untagged and pvid-tagged traffic be sent out of swp0 as tagged > > with VID 1, despite user's request. > > > > Prevent that from happening. The user should temporarily remove the > > existing untagged VLAN (1 in this case), add it back as tagged, and then > > add the new untagged VLAN (2 in this case).> > > Cc: Antoine Tenart <antoine.tenart@bootlin.com> > > Cc: Alexandre Belloni <alexandre.belloni@bootlin.com> > > Fixes: 7142529f1688 ("net: mscc: ocelot: add VLAN filtering") > > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > Thanks, Florian. > [snip] > > > + if (untagged && port->vid != vid) { > > + if (port->vid) { > > + dev_err(ocelot->dev, > > + "Port already has a native VLAN: %d\n", > > + port->vid); > > This sounds like a extended netlink ack candidate for improving user > experience, but this should do for now. > -- > Florian I know what you're saying. I wanted to drag in minimal dependencies for the fix. The driver is going to see major rework anyway soon (will gain a DSA front-end), hence the reason why I copied the DSA people to the fixes. Having extack propagate to more drivers is always welcome, and DSA would be a good start to see that being implemented. Regards, -Vladimir
On 26/10/2019 21:04:27+0300, Vladimir Oltean wrote: > The switch driver keeps a "vid" variable per port, which signifies _the_ > VLAN ID that is stripped on that port's egress (aka the native VLAN on a > trunk port). > > That is the way the hardware is designed (mostly). The port->vid is > programmed into REW:PORT:PORT_VLAN_CFG:PORT_VID and the rewriter is told > to send all traffic as tagged except the one having port->vid. > > There exists a possibility of finer-grained egress untagging decisions: > using the VCAP IS1 engine, one rule can be added to match every > VLAN-tagged frame whose VLAN should be untagged, and set POP_CNT=1 as > action. However, the IS1 can hold at most 512 entries, and the VLANs are > in the order of 6 * 4096. > > So the code is fine for now. But this sequence of commands: > > $ bridge vlan add dev swp0 vid 1 pvid untagged > $ bridge vlan add dev swp0 vid 2 untagged > > makes untagged and pvid-tagged traffic be sent out of swp0 as tagged > with VID 1, despite user's request. > > Prevent that from happening. The user should temporarily remove the > existing untagged VLAN (1 in this case), add it back as tagged, and then > add the new untagged VLAN (2 in this case). > > Cc: Antoine Tenart <antoine.tenart@bootlin.com> > Cc: Alexandre Belloni <alexandre.belloni@bootlin.com> > Fixes: 7142529f1688 ("net: mscc: ocelot: add VLAN filtering") > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > --- > drivers/net/ethernet/mscc/ocelot.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c > index 552252331e55..18d7ba033d05 100644 > --- a/drivers/net/ethernet/mscc/ocelot.c > +++ b/drivers/net/ethernet/mscc/ocelot.c > @@ -262,8 +262,15 @@ static int ocelot_vlan_vid_add(struct net_device *dev, u16 vid, bool pvid, > port->pvid = vid; > > /* Untagged egress vlan clasification */ > - if (untagged) > + if (untagged && port->vid != vid) { > + if (port->vid) { > + dev_err(ocelot->dev, > + "Port already has a native VLAN: %d\n", > + port->vid); > + return -EBUSY; > + } > port->vid = vid; > + } > > ocelot_vlan_port_apply(ocelot, port); > > -- > 2.17.1 >
diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c index 552252331e55..18d7ba033d05 100644 --- a/drivers/net/ethernet/mscc/ocelot.c +++ b/drivers/net/ethernet/mscc/ocelot.c @@ -262,8 +262,15 @@ static int ocelot_vlan_vid_add(struct net_device *dev, u16 vid, bool pvid, port->pvid = vid; /* Untagged egress vlan clasification */ - if (untagged) + if (untagged && port->vid != vid) { + if (port->vid) { + dev_err(ocelot->dev, + "Port already has a native VLAN: %d\n", + port->vid); + return -EBUSY; + } port->vid = vid; + } ocelot_vlan_port_apply(ocelot, port);
The switch driver keeps a "vid" variable per port, which signifies _the_ VLAN ID that is stripped on that port's egress (aka the native VLAN on a trunk port). That is the way the hardware is designed (mostly). The port->vid is programmed into REW:PORT:PORT_VLAN_CFG:PORT_VID and the rewriter is told to send all traffic as tagged except the one having port->vid. There exists a possibility of finer-grained egress untagging decisions: using the VCAP IS1 engine, one rule can be added to match every VLAN-tagged frame whose VLAN should be untagged, and set POP_CNT=1 as action. However, the IS1 can hold at most 512 entries, and the VLANs are in the order of 6 * 4096. So the code is fine for now. But this sequence of commands: $ bridge vlan add dev swp0 vid 1 pvid untagged $ bridge vlan add dev swp0 vid 2 untagged makes untagged and pvid-tagged traffic be sent out of swp0 as tagged with VID 1, despite user's request. Prevent that from happening. The user should temporarily remove the existing untagged VLAN (1 in this case), add it back as tagged, and then add the new untagged VLAN (2 in this case). Cc: Antoine Tenart <antoine.tenart@bootlin.com> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com> Fixes: 7142529f1688 ("net: mscc: ocelot: add VLAN filtering") Signed-off-by: Vladimir Oltean <olteanv@gmail.com> --- drivers/net/ethernet/mscc/ocelot.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)