* [PATCH net 1/3] net: dsa: tag_8021q: replace dsa_8021q_remove_header with __skb_vlan_pop
2020-03-20 0:00 [PATCH net 0/3] Bugfix, simplification and cleanup in DSA tag_8021q Vladimir Oltean
@ 2020-03-20 0:00 ` Vladimir Oltean
2020-03-20 0:00 ` [PATCH net 2/3] net: dsa: tag_8021q: remove obsolete comment Vladimir Oltean
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Vladimir Oltean @ 2020-03-20 0:00 UTC (permalink / raw)
To: davem; +Cc: netdev, andrew, f.fainelli, vivien.didelot
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Not only did this wheel did not need reinventing, but there is also
an issue with it: It doesn't remove the VLAN header in a way that
preserves the L2 payload checksum when that is being provided by the DSA
master hw. It should recalculate checksum both for the push, before
removing the header, and for the pull afterwards. But the current
implementation is quite dizzying, with pulls followed immediately
afterwards by pushes, the memmove is done before the push, etc. This
makes a DSA master with RX checksumming offload to print stack traces
with the infamous 'hw csum failure' message.
So remove the dsa_8021q_remove_header function and replace it with
something that actually works with inet checksumming.
Fixes: d461933638ae ("net: dsa: tag_8021q: Create helper function for removing VLAN header")
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
include/linux/dsa/8021q.h | 7 -------
net/dsa/tag_8021q.c | 43 ---------------------------------------
net/dsa/tag_sja1105.c | 19 ++++++++---------
3 files changed, 9 insertions(+), 60 deletions(-)
diff --git a/include/linux/dsa/8021q.h b/include/linux/dsa/8021q.h
index 0aa803c451a3..c620d9139c28 100644
--- a/include/linux/dsa/8021q.h
+++ b/include/linux/dsa/8021q.h
@@ -28,8 +28,6 @@ int dsa_8021q_rx_switch_id(u16 vid);
int dsa_8021q_rx_source_port(u16 vid);
-struct sk_buff *dsa_8021q_remove_header(struct sk_buff *skb);
-
#else
int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int index,
@@ -64,11 +62,6 @@ int dsa_8021q_rx_source_port(u16 vid)
return 0;
}
-struct sk_buff *dsa_8021q_remove_header(struct sk_buff *skb)
-{
- return NULL;
-}
-
#endif /* IS_ENABLED(CONFIG_NET_DSA_TAG_8021Q) */
#endif /* _NET_DSA_8021Q_H */
diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
index 2fb6c26294b5..b97ad93d1c1a 100644
--- a/net/dsa/tag_8021q.c
+++ b/net/dsa/tag_8021q.c
@@ -298,47 +298,4 @@ struct sk_buff *dsa_8021q_xmit(struct sk_buff *skb, struct net_device *netdev,
}
EXPORT_SYMBOL_GPL(dsa_8021q_xmit);
-/* In the DSA packet_type handler, skb->data points in the middle of the VLAN
- * tag, after tpid and before tci. This is because so far, ETH_HLEN
- * (DMAC, SMAC, EtherType) bytes were pulled.
- * There are 2 bytes of VLAN tag left in skb->data, and upper
- * layers expect the 'real' EtherType to be consumed as well.
- * Coincidentally, a VLAN header is also of the same size as
- * the number of bytes that need to be pulled.
- *
- * skb_mac_header skb->data
- * | |
- * v v
- * | | | | | | | | | | | | | | | | | | |
- * +-----------------------+-----------------------+-------+-------+-------+
- * | Destination MAC | Source MAC | TPID | TCI | EType |
- * +-----------------------+-----------------------+-------+-------+-------+
- * ^ | |
- * |<--VLAN_HLEN-->to <---VLAN_HLEN--->
- * from |
- * >>>>>>> v
- * >>>>>>> | | | | | | | | | | | | | | |
- * >>>>>>> +-----------------------+-----------------------+-------+
- * >>>>>>> | Destination MAC | Source MAC | EType |
- * +-----------------------+-----------------------+-------+
- * ^ ^
- * (now part of | |
- * skb->head) skb_mac_header skb->data
- */
-struct sk_buff *dsa_8021q_remove_header(struct sk_buff *skb)
-{
- u8 *from = skb_mac_header(skb);
- u8 *dest = from + VLAN_HLEN;
-
- memmove(dest, from, ETH_HLEN - VLAN_HLEN);
- skb_pull(skb, VLAN_HLEN);
- skb_push(skb, ETH_HLEN);
- skb_reset_mac_header(skb);
- skb_reset_mac_len(skb);
- skb_pull_rcsum(skb, ETH_HLEN);
-
- return skb;
-}
-EXPORT_SYMBOL_GPL(dsa_8021q_remove_header);
-
MODULE_LICENSE("GPL v2");
diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index 5366ea430349..d553bf36bd41 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -250,14 +250,14 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
{
struct sja1105_meta meta = {0};
int source_port, switch_id;
- struct vlan_ethhdr *hdr;
+ struct ethhdr *hdr;
u16 tpid, vid, tci;
bool is_link_local;
bool is_tagged;
bool is_meta;
- hdr = vlan_eth_hdr(skb);
- tpid = ntohs(hdr->h_vlan_proto);
+ hdr = eth_hdr(skb);
+ tpid = ntohs(hdr->h_proto);
is_tagged = (tpid == ETH_P_SJA1105);
is_link_local = sja1105_is_link_local(skb);
is_meta = sja1105_is_meta_frame(skb);
@@ -266,7 +266,12 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
if (is_tagged) {
/* Normal traffic path. */
- tci = ntohs(hdr->h_vlan_TCI);
+ skb_push_rcsum(skb, ETH_HLEN);
+ __skb_vlan_pop(skb, &tci);
+ skb_pull_rcsum(skb, ETH_HLEN);
+ skb_reset_network_header(skb);
+ skb_reset_transport_header(skb);
+
vid = tci & VLAN_VID_MASK;
source_port = dsa_8021q_rx_source_port(vid);
switch_id = dsa_8021q_rx_switch_id(vid);
@@ -295,12 +300,6 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
return NULL;
}
- /* Delete/overwrite fake VLAN header, DSA expects to not find
- * it there, see dsa_switch_rcv: skb_push(skb, ETH_HLEN).
- */
- if (is_tagged)
- skb = dsa_8021q_remove_header(skb);
-
return sja1105_rcv_meta_state_machine(skb, &meta, is_link_local,
is_meta);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH net 2/3] net: dsa: tag_8021q: remove obsolete comment
2020-03-20 0:00 [PATCH net 0/3] Bugfix, simplification and cleanup in DSA tag_8021q Vladimir Oltean
2020-03-20 0:00 ` [PATCH net 1/3] net: dsa: tag_8021q: replace dsa_8021q_remove_header with __skb_vlan_pop Vladimir Oltean
@ 2020-03-20 0:00 ` Vladimir Oltean
2020-03-20 0:00 ` [PATCH net 3/3] net: dsa: tag_8021q: get rid of dsa_8021q_xmit Vladimir Oltean
2020-03-24 3:56 ` [PATCH net 0/3] Bugfix, simplification and cleanup in DSA tag_8021q David Miller
3 siblings, 0 replies; 5+ messages in thread
From: Vladimir Oltean @ 2020-03-20 0:00 UTC (permalink / raw)
To: davem; +Cc: netdev, andrew, f.fainelli, vivien.didelot
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Since the commit mentioned below, the dsa_8021q_netdev_ops structure has
been removed, so the comment is no longer true. Remove it.
Fixes: 129bd7ca8ac0 ("net: dsa: Prevent usage of NET_DSA_TAG_8021Q as tagging protocol")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/tag_8021q.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
index b97ad93d1c1a..0d51d4974826 100644
--- a/net/dsa/tag_8021q.c
+++ b/net/dsa/tag_8021q.c
@@ -2,9 +2,7 @@
/* Copyright (c) 2019, Vladimir Oltean <olteanv@gmail.com>
*
* This module is not a complete tagger implementation. It only provides
- * primitives for taggers that rely on 802.1Q VLAN tags to use. The
- * dsa_8021q_netdev_ops is registered for API compliance and not used
- * directly by callers.
+ * primitives for taggers that rely on 802.1Q VLAN tags to use.
*/
#include <linux/if_bridge.h>
#include <linux/if_vlan.h>
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH net 3/3] net: dsa: tag_8021q: get rid of dsa_8021q_xmit
2020-03-20 0:00 [PATCH net 0/3] Bugfix, simplification and cleanup in DSA tag_8021q Vladimir Oltean
2020-03-20 0:00 ` [PATCH net 1/3] net: dsa: tag_8021q: replace dsa_8021q_remove_header with __skb_vlan_pop Vladimir Oltean
2020-03-20 0:00 ` [PATCH net 2/3] net: dsa: tag_8021q: remove obsolete comment Vladimir Oltean
@ 2020-03-20 0:00 ` Vladimir Oltean
2020-03-24 3:56 ` [PATCH net 0/3] Bugfix, simplification and cleanup in DSA tag_8021q David Miller
3 siblings, 0 replies; 5+ messages in thread
From: Vladimir Oltean @ 2020-03-20 0:00 UTC (permalink / raw)
To: davem; +Cc: netdev, andrew, f.fainelli, vivien.didelot
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Calling vlan_insert_tag is easy enough that having a dedicated function
in tag_8021q does not bring any benefit at all.
Fixes: f9bbe4477c30 ("net: dsa: Optional VLAN-based port separation for switches without tagging")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
include/linux/dsa/8021q.h | 9 ---------
net/dsa/tag_8021q.c | 10 ----------
net/dsa/tag_sja1105.c | 8 ++++++--
3 files changed, 6 insertions(+), 21 deletions(-)
diff --git a/include/linux/dsa/8021q.h b/include/linux/dsa/8021q.h
index c620d9139c28..ac2e5cc2c238 100644
--- a/include/linux/dsa/8021q.h
+++ b/include/linux/dsa/8021q.h
@@ -17,9 +17,6 @@ struct packet_type;
int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int index,
bool enabled);
-struct sk_buff *dsa_8021q_xmit(struct sk_buff *skb, struct net_device *netdev,
- u16 tpid, u16 tci);
-
u16 dsa_8021q_tx_vid(struct dsa_switch *ds, int port);
u16 dsa_8021q_rx_vid(struct dsa_switch *ds, int port);
@@ -36,12 +33,6 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int index,
return 0;
}
-struct sk_buff *dsa_8021q_xmit(struct sk_buff *skb, struct net_device *netdev,
- u16 tpid, u16 tci)
-{
- return NULL;
-}
-
u16 dsa_8021q_tx_vid(struct dsa_switch *ds, int port)
{
return 0;
diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
index 0d51d4974826..1ccd3069f977 100644
--- a/net/dsa/tag_8021q.c
+++ b/net/dsa/tag_8021q.c
@@ -286,14 +286,4 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
}
EXPORT_SYMBOL_GPL(dsa_port_setup_8021q_tagging);
-struct sk_buff *dsa_8021q_xmit(struct sk_buff *skb, struct net_device *netdev,
- u16 tpid, u16 tci)
-{
- /* skb->data points at skb_mac_header, which
- * is fine for vlan_insert_tag.
- */
- return vlan_insert_tag(skb, htons(tpid), tci);
-}
-EXPORT_SYMBOL_GPL(dsa_8021q_xmit);
-
MODULE_LICENSE("GPL v2");
diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index d553bf36bd41..ad1cbeb1146f 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -103,6 +103,8 @@ static struct sk_buff *sja1105_xmit(struct sk_buff *skb,
u16 tx_vid = dsa_8021q_tx_vid(dp->ds, dp->index);
u16 queue_mapping = skb_get_queue_mapping(skb);
u8 pcp = netdev_txq_to_tc(netdev, queue_mapping);
+ u16 tci = (pcp << VLAN_PRIO_SHIFT) | tx_vid;
+ u16 tpid = ETH_P_SJA1105;
/* Transmitting management traffic does not rely upon switch tagging,
* but instead SPI-installed management routes. Part 2 of this
@@ -119,8 +121,10 @@ static struct sk_buff *sja1105_xmit(struct sk_buff *skb,
if (dsa_port_is_vlan_filtering(dp))
return skb;
- return dsa_8021q_xmit(skb, netdev, ETH_P_SJA1105,
- ((pcp << VLAN_PRIO_SHIFT) | tx_vid));
+ /* skb->data points at skb_mac_header, which
+ * is fine for vlan_insert_tag.
+ */
+ return vlan_insert_tag(skb, htons(tpid), tci);
}
static void sja1105_transfer_meta(struct sk_buff *skb,
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net 0/3] Bugfix, simplification and cleanup in DSA tag_8021q
2020-03-20 0:00 [PATCH net 0/3] Bugfix, simplification and cleanup in DSA tag_8021q Vladimir Oltean
` (2 preceding siblings ...)
2020-03-20 0:00 ` [PATCH net 3/3] net: dsa: tag_8021q: get rid of dsa_8021q_xmit Vladimir Oltean
@ 2020-03-24 3:56 ` David Miller
3 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2020-03-24 3:56 UTC (permalink / raw)
To: olteanv; +Cc: netdev, andrew, f.fainelli, vivien.didelot
From: Vladimir Oltean <olteanv@gmail.com>
Date: Fri, 20 Mar 2020 02:00:48 +0200
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> This series removes the dsa_8021q_remove_header and dsa_8021q_xmit
> functions and replaces them with saner implementations, thereby
> redefining what the tag_8021q module has to offer (at the moment, just
> the VLAN definitions and install/uninstall rules remain).
Two of the three things you list in your Subject line are not appropriate
for "net", that being simplifications and cleanups.
Bug fixes for 'net' are fine, the rest go to 'net-next'.
^ permalink raw reply [flat|nested] 5+ messages in thread