* [PATCH/RFC net-next] rocker: by default accept untagged packets
@ 2015-05-26 0:55 Simon Horman
2015-05-26 7:28 ` Scott Feldman
0 siblings, 1 reply; 6+ messages in thread
From: Simon Horman @ 2015-05-26 0:55 UTC (permalink / raw)
To: Scott Feldman, Jiri Pirko, David Miller; +Cc: netdev, Simon Horman
This will occur anyway if the 8021q module is loaded as it will
call rocker_port_vlan_rx_add_vid for vlan 0. This code is here
to handle the case where the 8021q module is not loaded.
This patch also handles the case where the 8021q is unloaded
removing all VLANs from all ports.
This change should not affect bridging, although the rules are
harmlessly installed anyway. This is in keeping with the behaviour
for VLANs when the 8021q modules is loaded.
To aid implementation of the above provide a helper
and use it to replace some existing code.
Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
drivers/net/ethernet/rocker/rocker.c | 51 +++++++++++++++++++++++++++---------
1 file changed, 39 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index 36f7edfc3c7a..bc00e0abd8b6 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -3720,6 +3720,19 @@ static int rocker_port_router_mac(struct rocker_port *rocker_port,
return err;
}
+static int rocker_port_vlan_rx_vid(struct rocker_port *rocker_port,
+ enum switchdev_trans trans, int flag,
+ u16 vid)
+{
+ int err;
+
+ err = rocker_port_vlan(rocker_port, trans, flag, vid);
+ if (err)
+ return err;
+
+ return rocker_port_router_mac(rocker_port, trans, flag, htons(vid));
+}
+
static int rocker_port_fwding(struct rocker_port *rocker_port,
enum switchdev_trans trans)
{
@@ -4009,6 +4022,16 @@ static int rocker_port_open(struct net_device *dev)
goto err_request_rx_irq;
}
+ /* By default accept untagged vlan packets.
+ *
+ * This will occur anyway if the 8021q module is loaded as it will
+ * call rocker_port_vlan_rx_add_vid for vlan 0. This code is here
+ * to handle the case where the 8021q module is not loaded.
+ */
+ err = rocker_port_vlan_rx_vid(rocker_port, SWITCHDEV_TRANS_NONE, 0, 0);
+ if (err)
+ goto err_fwd_enable;
+
err = rocker_port_fwd_enable(rocker_port, SWITCHDEV_TRANS_NONE);
if (err)
goto err_fwd_enable;
@@ -4187,29 +4210,33 @@ static int rocker_port_vlan_rx_add_vid(struct net_device *dev,
__be16 proto, u16 vid)
{
struct rocker_port *rocker_port = netdev_priv(dev);
- int err;
-
- err = rocker_port_vlan(rocker_port, SWITCHDEV_TRANS_NONE, 0, vid);
- if (err)
- return err;
- return rocker_port_router_mac(rocker_port, SWITCHDEV_TRANS_NONE,
- 0, htons(vid));
+ return rocker_port_vlan_rx_vid(rocker_port, SWITCHDEV_TRANS_NONE,
+ 0, vid);
}
static int rocker_port_vlan_rx_kill_vid(struct net_device *dev,
__be16 proto, u16 vid)
{
struct rocker_port *rocker_port = netdev_priv(dev);
- int err;
+ int err, i;
- err = rocker_port_router_mac(rocker_port, SWITCHDEV_TRANS_NONE,
- ROCKER_OP_FLAG_REMOVE, htons(vid));
+ err = rocker_port_vlan_rx_vid(rocker_port, SWITCHDEV_TRANS_NONE,
+ ROCKER_OP_FLAG_REMOVE, vid);
if (err)
return err;
- return rocker_port_vlan(rocker_port, SWITCHDEV_TRANS_NONE,
- ROCKER_OP_FLAG_REMOVE, vid);
+ /* If no vlans are set then the last one has been removed;
+ * restore the default behaviour of accepting untagged packets.
+ *
+ * This may occur if the 8021q module is unloaded.
+ */
+ for (i = 0; i < ROCKER_VLAN_BITMAP_LEN; i++)
+ if (rocker_port->vlan_bitmap[i])
+ return 0;
+ return rocker_port_vlan_rx_vid(rocker_port, SWITCHDEV_TRANS_NONE,
+ 0, 0);
+
}
static int rocker_port_get_phys_port_name(struct net_device *dev,
--
2.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH/RFC net-next] rocker: by default accept untagged packets
2015-05-26 0:55 [PATCH/RFC net-next] rocker: by default accept untagged packets Simon Horman
@ 2015-05-26 7:28 ` Scott Feldman
2015-05-26 9:04 ` Scott Feldman
0 siblings, 1 reply; 6+ messages in thread
From: Scott Feldman @ 2015-05-26 7:28 UTC (permalink / raw)
To: Simon Horman; +Cc: Jiri Pirko, David Miller, Netdev
On Mon, May 25, 2015 at 5:55 PM, Simon Horman
<simon.horman@netronome.com> wrote:
> This will occur anyway if the 8021q module is loaded as it will
> call rocker_port_vlan_rx_add_vid for vlan 0. This code is here
> to handle the case where the 8021q module is not loaded.
>
> This patch also handles the case where the 8021q is unloaded
> removing all VLANs from all ports.
>
> This change should not affect bridging, although the rules are
> harmlessly installed anyway. This is in keeping with the behaviour
> for VLANs when the 8021q modules is loaded.
>
> To aid implementation of the above provide a helper
> and use it to replace some existing code.
>
> Signed-off-by: Simon Horman <simon.horman@netronome.com>
> ---
> drivers/net/ethernet/rocker/rocker.c | 51 +++++++++++++++++++++++++++---------
> 1 file changed, 39 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
> index 36f7edfc3c7a..bc00e0abd8b6 100644
> --- a/drivers/net/ethernet/rocker/rocker.c
> +++ b/drivers/net/ethernet/rocker/rocker.c
> @@ -3720,6 +3720,19 @@ static int rocker_port_router_mac(struct rocker_port *rocker_port,
> return err;
> }
>
> +static int rocker_port_vlan_rx_vid(struct rocker_port *rocker_port,
> + enum switchdev_trans trans, int flag,
> + u16 vid)
> +{
> + int err;
> +
> + err = rocker_port_vlan(rocker_port, trans, flag, vid);
> + if (err)
> + return err;
> +
> + return rocker_port_router_mac(rocker_port, trans, flag, htons(vid));
> +}
> +
> static int rocker_port_fwding(struct rocker_port *rocker_port,
> enum switchdev_trans trans)
> {
> @@ -4009,6 +4022,16 @@ static int rocker_port_open(struct net_device *dev)
> goto err_request_rx_irq;
> }
>
> + /* By default accept untagged vlan packets.
> + *
> + * This will occur anyway if the 8021q module is loaded as it will
> + * call rocker_port_vlan_rx_add_vid for vlan 0. This code is here
> + * to handle the case where the 8021q module is not loaded.
> + */
> + err = rocker_port_vlan_rx_vid(rocker_port, SWITCHDEV_TRANS_NONE, 0, 0);
> + if (err)
> + goto err_fwd_enable;
> +
> err = rocker_port_fwd_enable(rocker_port, SWITCHDEV_TRANS_NONE);
> if (err)
> goto err_fwd_enable;
> @@ -4187,29 +4210,33 @@ static int rocker_port_vlan_rx_add_vid(struct net_device *dev,
> __be16 proto, u16 vid)
> {
> struct rocker_port *rocker_port = netdev_priv(dev);
> - int err;
> -
> - err = rocker_port_vlan(rocker_port, SWITCHDEV_TRANS_NONE, 0, vid);
> - if (err)
> - return err;
>
> - return rocker_port_router_mac(rocker_port, SWITCHDEV_TRANS_NONE,
> - 0, htons(vid));
> + return rocker_port_vlan_rx_vid(rocker_port, SWITCHDEV_TRANS_NONE,
> + 0, vid);
> }
>
> static int rocker_port_vlan_rx_kill_vid(struct net_device *dev,
> __be16 proto, u16 vid)
> {
> struct rocker_port *rocker_port = netdev_priv(dev);
> - int err;
> + int err, i;
>
> - err = rocker_port_router_mac(rocker_port, SWITCHDEV_TRANS_NONE,
> - ROCKER_OP_FLAG_REMOVE, htons(vid));
> + err = rocker_port_vlan_rx_vid(rocker_port, SWITCHDEV_TRANS_NONE,
> + ROCKER_OP_FLAG_REMOVE, vid);
> if (err)
> return err;
>
> - return rocker_port_vlan(rocker_port, SWITCHDEV_TRANS_NONE,
> - ROCKER_OP_FLAG_REMOVE, vid);
> + /* If no vlans are set then the last one has been removed;
> + * restore the default behaviour of accepting untagged packets.
> + *
> + * This may occur if the 8021q module is unloaded.
> + */
> + for (i = 0; i < ROCKER_VLAN_BITMAP_LEN; i++)
> + if (rocker_port->vlan_bitmap[i])
> + return 0;
> + return rocker_port_vlan_rx_vid(rocker_port, SWITCHDEV_TRANS_NONE,
> + 0, 0);
> +
> }
>
> static int rocker_port_get_phys_port_name(struct net_device *dev,
> --
> 2.1.4
>
Hi Simon,
Thanks for looking into this one. I looked at your patch and the code
and I think we can streamline it a little bit more. For the
no-8021q-module case we use rocker_port_vlan_add() and
rocker_port_vlan_del() to add/del bridge vlans. We should be able to
move those functions up in the file so they can be called from
rocker_port_vlan_rx_add_vid() and rocker_port_vlan_rx_kill_vid(),
passing trans=SWITCHDEV_TRANS_NONE, but only if vid != 0. Next, like
in your patch, we need to call rocker_port_vlan_add() in
rocker_port_open(), passing in vid=0 for untagged. And in
rocker_port_stop(), call rocker_port_vlan_del(), again passing in
vid=0.
To summarize:
Call rocker_port_vlan_add() from:
1) rocker_port_open with vid=0
2) rocker_port_vlans_add() // bridge vlan
3) rocker_port_vlan_rx_add_vid() if vid != 0 // 8021q vlan
Call rocker_port_vlan_del() from:
1) rocker_port_stop with vid=0
2) rocker_port_vlans_del() // bridge vlan
3) rocker_port_vlan_rx_kill_vid() if vid != 0 // 8021q vlan
Does this look right?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH/RFC net-next] rocker: by default accept untagged packets
2015-05-26 7:28 ` Scott Feldman
@ 2015-05-26 9:04 ` Scott Feldman
2015-05-27 1:07 ` Simon Horman
0 siblings, 1 reply; 6+ messages in thread
From: Scott Feldman @ 2015-05-26 9:04 UTC (permalink / raw)
To: Simon Horman; +Cc: Jiri Pirko, David Miller, Netdev
On Tue, May 26, 2015 at 12:28 AM, Scott Feldman <sfeldma@gmail.com> wrote:
> On Mon, May 25, 2015 at 5:55 PM, Simon Horman
> <simon.horman@netronome.com> wrote:
>> This will occur anyway if the 8021q module is loaded as it will
>> call rocker_port_vlan_rx_add_vid for vlan 0. This code is here
>> to handle the case where the 8021q module is not loaded.
>>
>> This patch also handles the case where the 8021q is unloaded
>> removing all VLANs from all ports.
>>
>> This change should not affect bridging, although the rules are
>> harmlessly installed anyway. This is in keeping with the behaviour
>> for VLANs when the 8021q modules is loaded.
>>
>> To aid implementation of the above provide a helper
>> and use it to replace some existing code.
>>
>> Signed-off-by: Simon Horman <simon.horman@netronome.com>
[cut]
>
> Hi Simon,
>
> Thanks for looking into this one. I looked at your patch and the code
> and I think we can streamline it a little bit more. For the
> no-8021q-module case we use rocker_port_vlan_add() and
> rocker_port_vlan_del() to add/del bridge vlans. We should be able to
> move those functions up in the file so they can be called from
> rocker_port_vlan_rx_add_vid() and rocker_port_vlan_rx_kill_vid(),
> passing trans=SWITCHDEV_TRANS_NONE, but only if vid != 0. Next, like
> in your patch, we need to call rocker_port_vlan_add() in
> rocker_port_open(), passing in vid=0 for untagged. And in
> rocker_port_stop(), call rocker_port_vlan_del(), again passing in
> vid=0.
>
> To summarize:
>
> Call rocker_port_vlan_add() from:
>
> 1) rocker_port_open with vid=0
> 2) rocker_port_vlans_add() // bridge vlan
> 3) rocker_port_vlan_rx_add_vid() if vid != 0 // 8021q vlan
>
> Call rocker_port_vlan_del() from:
>
> 1) rocker_port_stop with vid=0
> 2) rocker_port_vlans_del() // bridge vlan
> 3) rocker_port_vlan_rx_kill_vid() if vid != 0 // 8021q vlan
>
> Does this look right?
Hmmmm...things get simpler if we removed support for 8021q module in
rocker driver by removing NETIF_F_HW_VLAN_CTAG_FILTER. That gets rid
of rocker_port_vlan_rx_add_vid() and rocker_port_vlan_rx_kill_vid().
Leaving us with the bridge VLAN interface to add/del/show vlans on the
port. I'm wondering if we can also avoid setting up untagged traffic
on the port during port open by requiring a explicit command on the
port from the user:
bridge vlan add vid 0 dev DEV master self // enable untagged
traffic on port
Do you have a requirement for 8021q module? I'm leaning towards a
clean break from 8021q and using just the built-in bridge VLAN
support. I'm curious if others have an opinion about 8021q module
used with switchdev devices.
-scott
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH/RFC net-next] rocker: by default accept untagged packets
2015-05-26 9:04 ` Scott Feldman
@ 2015-05-27 1:07 ` Simon Horman
2015-05-27 7:34 ` Scott Feldman
0 siblings, 1 reply; 6+ messages in thread
From: Simon Horman @ 2015-05-27 1:07 UTC (permalink / raw)
To: Scott Feldman; +Cc: Jiri Pirko, David Miller, Netdev
Hi Scott,
On Tue, May 26, 2015 at 02:04:00AM -0700, Scott Feldman wrote:
> On Tue, May 26, 2015 at 12:28 AM, Scott Feldman <sfeldma@gmail.com> wrote:
> > On Mon, May 25, 2015 at 5:55 PM, Simon Horman
> > <simon.horman@netronome.com> wrote:
> >> This will occur anyway if the 8021q module is loaded as it will
> >> call rocker_port_vlan_rx_add_vid for vlan 0. This code is here
> >> to handle the case where the 8021q module is not loaded.
> >>
> >> This patch also handles the case where the 8021q is unloaded
> >> removing all VLANs from all ports.
> >>
> >> This change should not affect bridging, although the rules are
> >> harmlessly installed anyway. This is in keeping with the behaviour
> >> for VLANs when the 8021q modules is loaded.
> >>
> >> To aid implementation of the above provide a helper
> >> and use it to replace some existing code.
> >>
> >> Signed-off-by: Simon Horman <simon.horman@netronome.com>
>
> [cut]
>
> >
> > Hi Simon,
> >
> > Thanks for looking into this one. I looked at your patch and the code
> > and I think we can streamline it a little bit more. For the
> > no-8021q-module case we use rocker_port_vlan_add() and
> > rocker_port_vlan_del() to add/del bridge vlans. We should be able to
> > move those functions up in the file so they can be called from
> > rocker_port_vlan_rx_add_vid() and rocker_port_vlan_rx_kill_vid(),
> > passing trans=SWITCHDEV_TRANS_NONE, but only if vid != 0. Next, like
> > in your patch, we need to call rocker_port_vlan_add() in
> > rocker_port_open(), passing in vid=0 for untagged. And in
> > rocker_port_stop(), call rocker_port_vlan_del(), again passing in
> > vid=0.
> >
> > To summarize:
> >
> > Call rocker_port_vlan_add() from:
> >
> > 1) rocker_port_open with vid=0
> > 2) rocker_port_vlans_add() // bridge vlan
> > 3) rocker_port_vlan_rx_add_vid() if vid != 0 // 8021q vlan
> >
> > Call rocker_port_vlan_del() from:
> >
> > 1) rocker_port_stop with vid=0
> > 2) rocker_port_vlans_del() // bridge vlan
> > 3) rocker_port_vlan_rx_kill_vid() if vid != 0 // 8021q vlan
> >
> > Does this look right?
It seems like it ought to work, I can try implementing the above
idea if you think it is worthwhile.
Can I clarify that its ok to ignore vid != 0 in
rocker_port_vlan_rx_add_vid() and rocker_port_vlan_rx_kill_vid()?
If so I think that leads to an easy simplification of
my proposed change to rocker_port_vlan_rx_kill_vid():
the logic to restore vlan 0 if no vlans are present can be dropped.
Of course your suggestion goes further than that.
> Hmmmm...things get simpler if we removed support for 8021q module in
> rocker driver by removing NETIF_F_HW_VLAN_CTAG_FILTER. That gets rid
> of rocker_port_vlan_rx_add_vid() and rocker_port_vlan_rx_kill_vid().
> Leaving us with the bridge VLAN interface to add/del/show vlans on the
> port. I'm wondering if we can also avoid setting up untagged traffic
> on the port during port open by requiring a explicit command on the
> port from the user:
>
> bridge vlan add vid 0 dev DEV master self // enable untagged
> traffic on port
I have some questions about that approach:
* Does that behaviour differ from other devices
(that don't set NETIF_F_HW_VLAN_CTAG_FILTER)?
It seems like it may be an extra unnecessary step to me.
* Does that behaviour change the current behaviour supported by rocker?
If so it seems unwise to change it.
* Does the scheme described above work when rocker ports are not bridged?
This is the scenario I am interested in at this time.
>
> Do you have a requirement for 8021q module? I'm leaning towards a
> clean break from 8021q and using just the built-in bridge VLAN
> support. I'm curious if others have an opinion about 8021q module
> used with switchdev devices.
I do not have a requirement on the 8021q module at this time.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH/RFC net-next] rocker: by default accept untagged packets
2015-05-27 1:07 ` Simon Horman
@ 2015-05-27 7:34 ` Scott Feldman
2015-05-27 7:42 ` Simon Horman
0 siblings, 1 reply; 6+ messages in thread
From: Scott Feldman @ 2015-05-27 7:34 UTC (permalink / raw)
To: Simon Horman; +Cc: Jiri Pirko, David Miller, Netdev, Arad, Ronen
On Tue, May 26, 2015 at 6:07 PM, Simon Horman
<simon.horman@netronome.com> wrote:
> Hi Scott,
>
> On Tue, May 26, 2015 at 02:04:00AM -0700, Scott Feldman wrote:
>> On Tue, May 26, 2015 at 12:28 AM, Scott Feldman <sfeldma@gmail.com> wrote:
>> > On Mon, May 25, 2015 at 5:55 PM, Simon Horman
>> > <simon.horman@netronome.com> wrote:
>> >> This will occur anyway if the 8021q module is loaded as it will
>> >> call rocker_port_vlan_rx_add_vid for vlan 0. This code is here
>> >> to handle the case where the 8021q module is not loaded.
>> >>
>> >> This patch also handles the case where the 8021q is unloaded
>> >> removing all VLANs from all ports.
>> >>
>> >> This change should not affect bridging, although the rules are
>> >> harmlessly installed anyway. This is in keeping with the behaviour
>> >> for VLANs when the 8021q modules is loaded.
>> >>
>> >> To aid implementation of the above provide a helper
>> >> and use it to replace some existing code.
>> >>
>> >> Signed-off-by: Simon Horman <simon.horman@netronome.com>
>>
>> [cut]
>>
>> >
>> > Hi Simon,
>> >
>> > Thanks for looking into this one. I looked at your patch and the code
>> > and I think we can streamline it a little bit more. For the
>> > no-8021q-module case we use rocker_port_vlan_add() and
>> > rocker_port_vlan_del() to add/del bridge vlans. We should be able to
>> > move those functions up in the file so they can be called from
>> > rocker_port_vlan_rx_add_vid() and rocker_port_vlan_rx_kill_vid(),
>> > passing trans=SWITCHDEV_TRANS_NONE, but only if vid != 0. Next, like
>> > in your patch, we need to call rocker_port_vlan_add() in
>> > rocker_port_open(), passing in vid=0 for untagged. And in
>> > rocker_port_stop(), call rocker_port_vlan_del(), again passing in
>> > vid=0.
>> >
>> > To summarize:
>> >
>> > Call rocker_port_vlan_add() from:
>> >
>> > 1) rocker_port_open with vid=0
>> > 2) rocker_port_vlans_add() // bridge vlan
>> > 3) rocker_port_vlan_rx_add_vid() if vid != 0 // 8021q vlan
>> >
>> > Call rocker_port_vlan_del() from:
>> >
>> > 1) rocker_port_stop with vid=0
>> > 2) rocker_port_vlans_del() // bridge vlan
>> > 3) rocker_port_vlan_rx_kill_vid() if vid != 0 // 8021q vlan
>> >
>> > Does this look right?
>
> It seems like it ought to work, I can try implementing the above
> idea if you think it is worthwhile.
>
> Can I clarify that its ok to ignore vid != 0 in
> rocker_port_vlan_rx_add_vid() and rocker_port_vlan_rx_kill_vid()?
>
> If so I think that leads to an easy simplification of
> my proposed change to rocker_port_vlan_rx_kill_vid():
> the logic to restore vlan 0 if no vlans are present can be dropped.
>
> Of course your suggestion goes further than that.
>
>> Hmmmm...things get simpler if we removed support for 8021q module in
>> rocker driver by removing NETIF_F_HW_VLAN_CTAG_FILTER. That gets rid
>> of rocker_port_vlan_rx_add_vid() and rocker_port_vlan_rx_kill_vid().
>> Leaving us with the bridge VLAN interface to add/del/show vlans on the
>> port. I'm wondering if we can also avoid setting up untagged traffic
>> on the port during port open by requiring a explicit command on the
>> port from the user:
>>
>> bridge vlan add vid 0 dev DEV master self // enable untagged
>> traffic on port
>
> I have some questions about that approach:
>
> * Does that behaviour differ from other devices
> (that don't set NETIF_F_HW_VLAN_CTAG_FILTER)?
> It seems like it may be an extra unnecessary step to me.
The answer is which drivers use bridge_setlink/bridge_dellink for
VLANs, which is none outside of switchdev drivers.
There are some drivers that use bridge_setlink (benet, i40e, ixgbe)
for setting hw_mode, but ignore the IFLA_BRIDGE_VLAN_INFO attr.
> * Does that behaviour change the current behaviour supported by rocker?
> If so it seems unwise to change it.
I did some experiments, and with rocker there currently are two ways
to enable a port for rx untagged traffic:
1) Load the 8021q driver, which in turn will add vid=0 on interface
supporting NETIF_F_HW_VLAN_CTAG_FILTER, when interface opens.
2) Without 8021q driver loaded, by manually adding vid=0 to interface using:
bridge vlan add vid=0 dev DEV self
> * Does the scheme described above work when rocker ports are not bridged?
> This is the scenario I am interested in at this time.
Yes, confusingly, since the user command is "bridge vlan". But the
command works for bridged or un-bridged ports. In either case, by
targeting "self", the port driver's bridge_setlink/bridge_getlink are
called. For switchdev port drivers, that means a call into
switchdev_port_bridge_setlink/switchdev_port_bridge_dellink.
In rocker, as an experiment, I removed NETIF_F_HW_VLAN_CTAG_FILTER
(and associated ndo ops), and was able to run/pass all my tests. For
the tests with unbridged ports, I had to run "bridge vlan add vid=0
dev DEV self" before passing traffic. The advantages of using
bridge_setlink/bridge_dellink for VLANs over 8021q module are:
1) single API to implement in port driver
2) can handle VLAN ranges efficiently (thanks Roopa!)
3) switchdev already handles stacked driver case (and transaction model)
4) includes support for PVID on bridge
5) includes support for egress untagged property on port
6) user space command included with iproute2 pkg
I see no disadvantages over using 8021q module.
One thing that's missing or incomplete is support for bridge_getlink
for VLANs. Currently switchdev defaults to ndo_dflt_bridge_getlink()
which doesn't return any IFLA_BRIDGE_VLAN_INFO attrs. And the "bridge
vlan show" command might need some help in displaying that info, it it
was available. Cc:ing Ronen as he's been looking into bridge_getlink
for switchdev.
So I think since yesterday I'm becoming even more biased against 8021q
for switchdev. For rocker, I think we need to figure out if the
driver does an automatic vid=0 install, say on ndo_open, and then a
removal on ndo_stop. That would simplify the logic in
rocker_port_bridge_join()/leave(). Doing the automatic add makes the
device more like a NIC where passing untagged traffic is the baseline
default.
-scott
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH/RFC net-next] rocker: by default accept untagged packets
2015-05-27 7:34 ` Scott Feldman
@ 2015-05-27 7:42 ` Simon Horman
0 siblings, 0 replies; 6+ messages in thread
From: Simon Horman @ 2015-05-27 7:42 UTC (permalink / raw)
To: Scott Feldman; +Cc: Jiri Pirko, David Miller, Netdev, Arad, Ronen
Hi Scott,
On Wed, May 27, 2015 at 12:34:57AM -0700, Scott Feldman wrote:
> On Tue, May 26, 2015 at 6:07 PM, Simon Horman
> <simon.horman@netronome.com> wrote:
[snip]
> > I have some questions about that approach:
> >
> > * Does that behaviour differ from other devices
> > (that don't set NETIF_F_HW_VLAN_CTAG_FILTER)?
> > It seems like it may be an extra unnecessary step to me.
>
> The answer is which drivers use bridge_setlink/bridge_dellink for
> VLANs, which is none outside of switchdev drivers.
>
> There are some drivers that use bridge_setlink (benet, i40e, ixgbe)
> for setting hw_mode, but ignore the IFLA_BRIDGE_VLAN_INFO attr.
>
> > * Does that behaviour change the current behaviour supported by rocker?
> > If so it seems unwise to change it.
>
> I did some experiments, and with rocker there currently are two ways
> to enable a port for rx untagged traffic:
>
> 1) Load the 8021q driver, which in turn will add vid=0 on interface
> supporting NETIF_F_HW_VLAN_CTAG_FILTER, when interface opens.
>
> 2) Without 8021q driver loaded, by manually adding vid=0 to interface using:
>
> bridge vlan add vid=0 dev DEV self
>
> > * Does the scheme described above work when rocker ports are not bridged?
> > This is the scenario I am interested in at this time.
>
> Yes, confusingly, since the user command is "bridge vlan". But the
> command works for bridged or un-bridged ports. In either case, by
> targeting "self", the port driver's bridge_setlink/bridge_getlink are
> called. For switchdev port drivers, that means a call into
> switchdev_port_bridge_setlink/switchdev_port_bridge_dellink.
That is indeed confusing.
> In rocker, as an experiment, I removed NETIF_F_HW_VLAN_CTAG_FILTER
> (and associated ndo ops), and was able to run/pass all my tests. For
> the tests with unbridged ports, I had to run "bridge vlan add vid=0
> dev DEV self" before passing traffic. The advantages of using
> bridge_setlink/bridge_dellink for VLANs over 8021q module are:
>
> 1) single API to implement in port driver
> 2) can handle VLAN ranges efficiently (thanks Roopa!)
> 3) switchdev already handles stacked driver case (and transaction model)
> 4) includes support for PVID on bridge
> 5) includes support for egress untagged property on port
> 6) user space command included with iproute2 pkg
>
> I see no disadvantages over using 8021q module.
>
> One thing that's missing or incomplete is support for bridge_getlink
> for VLANs. Currently switchdev defaults to ndo_dflt_bridge_getlink()
> which doesn't return any IFLA_BRIDGE_VLAN_INFO attrs. And the "bridge
> vlan show" command might need some help in displaying that info, it it
> was available. Cc:ing Ronen as he's been looking into bridge_getlink
> for switchdev.
>
> So I think since yesterday I'm becoming even more biased against 8021q
> for switchdev. For rocker, I think we need to figure out if the
> driver does an automatic vid=0 install, say on ndo_open, and then a
> removal on ndo_stop. That would simplify the logic in
> rocker_port_bridge_join()/leave(). Doing the automatic add makes the
> device more like a NIC where passing untagged traffic is the baseline
> default.
My feeling at this time is that it would be nice to to the automatic add
to make rocker feel more like a NIC. That is the behaviour that I for
one would expect without other knowledge of the situation.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-05-27 7:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-05-26 0:55 [PATCH/RFC net-next] rocker: by default accept untagged packets Simon Horman
2015-05-26 7:28 ` Scott Feldman
2015-05-26 9:04 ` Scott Feldman
2015-05-27 1:07 ` Simon Horman
2015-05-27 7:34 ` Scott Feldman
2015-05-27 7:42 ` Simon Horman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).