| Message ID | 20201130140610.4018-5-tobias@waldekranz.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: Link aggregation support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 57 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Mon, Nov 30, 2020 at 03:06:10PM +0100, Tobias Waldekranz wrote: > Packets ingressing on a LAG that egress on the CPU port, which are not > classified as management, will have a FORWARD tag that does not > contain the normal source device/port tuple. Instead the trunk bit > will be set, and the port field holds the LAG id. > > Since the exact source port information is not available in the tag, > frames are injected directly on the LAG interface and thus do never > pass through any DSA port interface on ingress. > > Management frames (TO_CPU) are not affected and will pass through the > DSA port interface as usual. > > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > --- > net/dsa/dsa.c | 12 +++++++++++- > net/dsa/tag_dsa.c | 17 ++++++++++++++++- > 2 files changed, 27 insertions(+), 2 deletions(-) > > diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c > index a1b1dc8a4d87..7325bf4608e9 100644 > --- a/net/dsa/dsa.c > +++ b/net/dsa/dsa.c > @@ -219,11 +219,21 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev, > } > > skb = nskb; > - p = netdev_priv(skb->dev); > skb_push(skb, ETH_HLEN); > skb->pkt_type = PACKET_HOST; > skb->protocol = eth_type_trans(skb, skb->dev); > > + if (unlikely(!dsa_slave_dev_check(skb->dev))) { > + /* Packet is to be injected directly on an upper > + * device, e.g. a team/bond, so skip all DSA-port > + * specific actions. > + */ > + netif_rx(skb); > + return 0; netif_rx returns an int code, it seems odd to ignore it. > + } > + > + p = netdev_priv(skb->dev); > + > if (unlikely(cpu_dp->ds->untag_bridge_pvid)) { > nskb = dsa_untag_bridge_pvid(skb); > if (!nskb) {
On Tue, Dec 01, 2020 at 23:24, Vladimir Oltean <olteanv@gmail.com> wrote: > On Mon, Nov 30, 2020 at 03:06:10PM +0100, Tobias Waldekranz wrote: >> Packets ingressing on a LAG that egress on the CPU port, which are not >> classified as management, will have a FORWARD tag that does not >> contain the normal source device/port tuple. Instead the trunk bit >> will be set, and the port field holds the LAG id. >> >> Since the exact source port information is not available in the tag, >> frames are injected directly on the LAG interface and thus do never >> pass through any DSA port interface on ingress. >> >> Management frames (TO_CPU) are not affected and will pass through the >> DSA port interface as usual. >> >> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> >> --- >> net/dsa/dsa.c | 12 +++++++++++- >> net/dsa/tag_dsa.c | 17 ++++++++++++++++- >> 2 files changed, 27 insertions(+), 2 deletions(-) >> >> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c >> index a1b1dc8a4d87..7325bf4608e9 100644 >> --- a/net/dsa/dsa.c >> +++ b/net/dsa/dsa.c >> @@ -219,11 +219,21 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev, >> } >> >> skb = nskb; >> - p = netdev_priv(skb->dev); >> skb_push(skb, ETH_HLEN); >> skb->pkt_type = PACKET_HOST; >> skb->protocol = eth_type_trans(skb, skb->dev); >> >> + if (unlikely(!dsa_slave_dev_check(skb->dev))) { >> + /* Packet is to be injected directly on an upper >> + * device, e.g. a team/bond, so skip all DSA-port >> + * specific actions. >> + */ >> + netif_rx(skb); >> + return 0; > > netif_rx returns an int code, it seems odd to ignore it. This is exactly the same treatment that the return code from gro_cells_receive gets just a few lines down. They return the same set of codes (NET_RX_{SUCCESS,DROP}). Looking through the source base, there are a few callers that look at the return value (the overwhelming majority ignore it). Actions vary from printing warnings (without rate-limit, yikes), setting variables that are otherwise unused, or bumping a counter (the only reasonable thing I have seen). But looking through enqueue_to_backlog, it seems like there already is a counter for this that is accessible from /proc/net/softnet_data. >> + } >> + >> + p = netdev_priv(skb->dev); >> + >> if (unlikely(cpu_dp->ds->untag_bridge_pvid)) { >> nskb = dsa_untag_bridge_pvid(skb); >> if (!nskb) {
On Tue, Dec 01, 2020 at 11:31:02PM +0100, Tobias Waldekranz wrote: > >> + if (unlikely(!dsa_slave_dev_check(skb->dev))) { > >> + /* Packet is to be injected directly on an upper > >> + * device, e.g. a team/bond, so skip all DSA-port > >> + * specific actions. > >> + */ > >> + netif_rx(skb); > >> + return 0; > > > > netif_rx returns an int code, it seems odd to ignore it. > > This is exactly the same treatment that the return code from > gro_cells_receive gets just a few lines down. They return the same set > of codes (NET_RX_{SUCCESS,DROP}). > > Looking through the source base, there are a few callers that look at > the return value (the overwhelming majority ignore it). Actions vary > from printing warnings (without rate-limit, yikes), setting variables > that are otherwise unused, or bumping a counter (the only reasonable > thing I have seen). > > But looking through enqueue_to_backlog, it seems like there already is a > counter for this that is accessible from /proc/net/softnet_data. And also, more obviously, in ndo_get_stats64 (ip -s -s link). Ok, I think you can ignore the return value.
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c index a1b1dc8a4d87..7325bf4608e9 100644 --- a/net/dsa/dsa.c +++ b/net/dsa/dsa.c @@ -219,11 +219,21 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev, } skb = nskb; - p = netdev_priv(skb->dev); skb_push(skb, ETH_HLEN); skb->pkt_type = PACKET_HOST; skb->protocol = eth_type_trans(skb, skb->dev); + if (unlikely(!dsa_slave_dev_check(skb->dev))) { + /* Packet is to be injected directly on an upper + * device, e.g. a team/bond, so skip all DSA-port + * specific actions. + */ + netif_rx(skb); + return 0; + } + + p = netdev_priv(skb->dev); + if (unlikely(cpu_dp->ds->untag_bridge_pvid)) { nskb = dsa_untag_bridge_pvid(skb); if (!nskb) { diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c index 112c7c6dd568..be7271de8d0b 100644 --- a/net/dsa/tag_dsa.c +++ b/net/dsa/tag_dsa.c @@ -163,6 +163,7 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev, u8 extra) { int source_device, source_port; + bool trunk = false; enum dsa_code code; enum dsa_cmd cmd; u8 *dsa_header; @@ -174,6 +175,8 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev, switch (cmd) { case DSA_CMD_FORWARD: skb->offload_fwd_mark = 1; + + trunk = !!(dsa_header[1] & 7); break; case DSA_CMD_TO_CPU: @@ -216,7 +219,19 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev, source_device = dsa_header[0] & 0x1f; source_port = (dsa_header[1] >> 3) & 0x1f; - skb->dev = dsa_master_find_slave(dev, source_device, source_port); + if (trunk) { + struct dsa_port *cpu_dp = dev->dsa_ptr; + + /* The exact source port is not available in the tag, + * so we inject the frame directly on the upper + * team/bond. + */ + skb->dev = dsa_lag_dev_by_id(cpu_dp->dst, source_port); + } else { + skb->dev = dsa_master_find_slave(dev, source_device, + source_port); + } + if (!skb->dev) return NULL;
Packets ingressing on a LAG that egress on the CPU port, which are not classified as management, will have a FORWARD tag that does not contain the normal source device/port tuple. Instead the trunk bit will be set, and the port field holds the LAG id. Since the exact source port information is not available in the tag, frames are injected directly on the LAG interface and thus do never pass through any DSA port interface on ingress. Management frames (TO_CPU) are not affected and will pass through the DSA port interface as usual. Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> --- net/dsa/dsa.c | 12 +++++++++++- net/dsa/tag_dsa.c | 17 ++++++++++++++++- 2 files changed, 27 insertions(+), 2 deletions(-)