[RFC net-next 0/2] DSA slave with customise netdev features

Some taggers, such as tag_dsa.c, combine VLAN tags with DSA tags, which
currently has a few problems:

1. Unnecessary reallocation on TX:

A central TX reallocation has been used since commit a3b0b6479700
("net: dsa: implement a central TX reallocation procedure"), but for
VLAN-tagged frames, the actual headroom required for DSA taggers which
combine with VLAN tags is smaller.

2. Repeated memmoves:

If a both Marvell EDSA and VLAN tagged frame is received, the current
code will move the (DA,SA) twice: the first in dsa_rcv_ll to convert the
frame to a normal 802.1Q frame, and the second to strip off the 802.1Q
tag. The similar thing happens on TX.

For these tags, it is better to handle DSA and VLAN tags at the same time
in DSA taggers.

This patch set allows taggers to add custom netdev features to DSA
slaves so they can advertise VLAN offload, and converts tag_mtk to use
the TX VLAN offload.

DENG Qingfang (2):
  net: dsa: allow taggers to customise netdev features
  net: dsa: tag_mtk: handle VLAN tag insertion on TX

 include/net/dsa.h |  2 ++
 net/dsa/slave.c   |  3 ++-
 net/dsa/tag_mtk.c | 46 ++++++++++++++++++++++------------------------
 3 files changed, 26 insertions(+), 25 deletions(-)

-- 
2.25.1

Allow taggers to add netdev features, such as VLAN offload, to slave
devices, which will make it possible for taggers to handle VLAN tags
themselves.

Signed-off-by: DENG Qingfang <dqfext@gmail.com>
---
 include/net/dsa.h | 2 ++
 net/dsa/slave.c   | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index f9a17145255a..f65442858f71 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h

@@ -96,6 +96,8 @@ struct dsa_device_ops {
 	 * its RX filter.
 	 */
 	bool promisc_on_master;
+	/* Additional features to be applied to the slave. */
+	netdev_features_t features;
 };
 
 /* This structure defines the control interfaces that are overlayed by the

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 662ff531d4e2..6d6f1aebf1ca 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c

@@ -1885,7 +1885,8 @@ void dsa_slave_setup_tagger(struct net_device *slave)
 
 	p->xmit = cpu_dp->tag_ops->xmit;
 
-	slave->features = master->vlan_features | NETIF_F_HW_TC;
+	slave->features = master->vlan_features | cpu_dp->tag_ops->features |
+			  NETIF_F_HW_TC;
 	slave->hw_features |= NETIF_F_HW_TC;
 	slave->features |= NETIF_F_LLTX;
 	if (slave->needed_tailroom)

-- 
2.25.1

Advertise TX VLAN offload features, and handle VLAN tag insertion in
the tag_xmit function.

Signed-off-by: DENG Qingfang <dqfext@gmail.com>
---
 net/dsa/tag_mtk.c | 46 ++++++++++++++++++++++------------------------
 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
index 415d8ece242a..e407abefa06c 100644
--- a/net/dsa/tag_mtk.c
+++ b/net/dsa/tag_mtk.c

@@ -22,7 +22,6 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
 				    struct net_device *dev)
 {
 	struct dsa_port *dp = dsa_slave_to_port(dev);
-	u8 xmit_tpid;
 	u8 *mtk_tag;
 
 	/* Build the special tag after the MAC Source Address. If VLAN header

@@ -31,33 +30,31 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
 	 * the both special and VLAN tag at the same time and then look up VLAN
 	 * table with VID.
 	 */
-	switch (skb->protocol) {
-	case htons(ETH_P_8021Q):
-		xmit_tpid = MTK_HDR_XMIT_TAGGED_TPID_8100;
-		break;
-	case htons(ETH_P_8021AD):
-		xmit_tpid = MTK_HDR_XMIT_TAGGED_TPID_88A8;
-		break;
-	default:
-		xmit_tpid = MTK_HDR_XMIT_UNTAGGED;
-		skb_push(skb, MTK_HDR_LEN);
-		dsa_alloc_etype_header(skb, MTK_HDR_LEN);
-	}
-
+	skb_push(skb, MTK_HDR_LEN);
+	dsa_alloc_etype_header(skb, MTK_HDR_LEN);
 	mtk_tag = dsa_etype_header_pos_tx(skb);
 
-	/* Mark tag attribute on special tag insertion to notify hardware
-	 * whether that's a combined special tag with 802.1Q header.
-	 */
-	mtk_tag[0] = xmit_tpid;
-	mtk_tag[1] = (1 << dp->index) & MTK_HDR_XMIT_DP_BIT_MASK;
-
-	/* Tag control information is kept for 802.1Q */
-	if (xmit_tpid == MTK_HDR_XMIT_UNTAGGED) {
-		mtk_tag[2] = 0;
-		mtk_tag[3] = 0;
+	if (skb_vlan_tag_present(skb)) {
+		switch (skb->vlan_proto) {
+		case htons(ETH_P_8021Q):
+			mtk_tag[0] = MTK_HDR_XMIT_TAGGED_TPID_8100;
+			break;
+		case htons(ETH_P_8021AD):
+			mtk_tag[0] = MTK_HDR_XMIT_TAGGED_TPID_88A8;
+			break;
+		default:
+			return NULL;
+		}
+
+		((__be16 *)mtk_tag)[1] = htons(skb_vlan_tag_get(skb));
+		__vlan_hwaccel_clear_tag(skb);
+	} else {
+		mtk_tag[0] = MTK_HDR_XMIT_UNTAGGED;
+		((__be16 *)mtk_tag)[1] = 0;
 	}
 
+	mtk_tag[1] = (1 << dp->index) & MTK_HDR_XMIT_DP_BIT_MASK;
+
 	return skb;
 }
 

@@ -96,6 +93,7 @@ static const struct dsa_device_ops mtk_netdev_ops = {
 	.xmit		= mtk_tag_xmit,
 	.rcv		= mtk_tag_rcv,
 	.needed_headroom = MTK_HDR_LEN,
+	.features	= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX,
 };
 
 MODULE_LICENSE("GPL");

-- 
2.25.1

On Wed, Aug 25, 2021 at 04:38:29PM +0800, DENG Qingfang wrote:

Some taggers, such as tag_dsa.c, combine VLAN tags with DSA tags, which
currently has a few problems:

1. Unnecessary reallocation on TX:

A central TX reallocation has been used since commit a3b0b6479700
("net: dsa: implement a central TX reallocation procedure"), but for
VLAN-tagged frames, the actual headroom required for DSA taggers which
combine with VLAN tags is smaller.

If true, this would be a major failure of the central TX reallocation idea.

However, for this to fail, it would mean that there is a code path in
the network stack that routinely allocates skbs with skb_needed_headroom(skb)
smaller than dev->needed_headroom. That's the only thing that would trigger
reallocs (beside cloned skbs).

The fact that we ask for a dev->needed_headroom that is a bit larger
than what is needed in some cases is not an issue. We should declare the
largest dev->needed_headroom that should cover all cases.

Would you please let me know what is the stack trace when you apply this
patch?

-----------------------------[ cut here ]-----------------------------

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 6d6f1aebf1ca..1924025ac136 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c

@@ -600,6 +600,10 @@ static int dsa_realloc_skb(struct sk_buff *skb, struct net_device *dev)
 		/* No reallocation needed, yay! */
 		return 0;
 
+	netdev_err(dev, "%s: skb realloc: headroom %u, tailroom %u, cloned %d, needed_headroom %d, needed_tailroom %d\n",
+		  __func__, skb_headroom(skb), skb_tailroom(skb), skb_cloned(skb), needed_headroom, needed_tailroom);
+	WARN_ON(!skb_cloned(skb));
+
 	return pskb_expand_head(skb, needed_headroom, needed_tailroom,
 				GFP_ATOMIC);
 }
-----------------------------[ cut here ]-----------------------------

2. Repeated memmoves:

If a both Marvell EDSA and VLAN tagged frame is received, the current
code will move the (DA,SA) twice: the first in dsa_rcv_ll to convert the
frame to a normal 802.1Q frame, and the second to strip off the 802.1Q
tag. The similar thing happens on TX.

I don't think a lot of people use 8021q uppers with mv88e6xxx. At least
the error messages I've seen during the few times when I've booted the
Turris Mox would seem to suggest that.

The code that converts DSA 'tagged' frames to VLAN tags originates from
Lennert Buytenhek himself.

For these tags, it is better to handle DSA and VLAN tags at the same time
in DSA taggers.

No objection there.

This patch set allows taggers to add custom netdev features to DSA
slaves so they can advertise VLAN offload, and converts tag_mtk to use
the TX VLAN offload.

DENG Qingfang (2):
  net: dsa: allow taggers to customise netdev features
  net: dsa: tag_mtk: handle VLAN tag insertion on TX

 include/net/dsa.h |  2 ++
 net/dsa/slave.c   |  3 ++-
 net/dsa/tag_mtk.c | 46 ++++++++++++++++++++++------------------------
 3 files changed, 26 insertions(+), 25 deletions(-)

--
2.25.1

On Wed, Aug 25, 2021 at 04:38:30PM +0800, DENG Qingfang wrote:

Allow taggers to add netdev features, such as VLAN offload, to slave
devices, which will make it possible for taggers to handle VLAN tags
themselves.

Signed-off-by: DENG Qingfang <dqfext@gmail.com>
---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

On Wed, Aug 25, 2021 at 04:38:31PM +0800, DENG Qingfang wrote:

You cannot just remove the old code. Only things like 8021q uppers will
send packets with the VLAN in the hwaccel area.

If you have an application that puts the VLAN in the actual AF_PACKET
payload, like:

https://github.com/vladimiroltean/tsn-scripts/blob/master/isochron/send.c

then you need to handle the VLAN being in the skb payload.

On Thu, Aug 26, 2021 at 03:03:49AM +0300, Vladimir Oltean wrote:

You cannot just remove the old code. Only things like 8021q uppers will
send packets with the VLAN in the hwaccel area.

If you have an application that puts the VLAN in the actual AF_PACKET
payload, like:

https://github.com/vladimiroltean/tsn-scripts/blob/master/isochron/send.c

then you need to handle the VLAN being in the skb payload.

I've actually tested this (only apply patch 2 without .features) and it
still worked.

The comment says the VLAN tag need to be combined with the special tag in
order to perform VLAN table lookup, so we can set its destination port
vector to all zeroes and the switch will forward it like a data frame
(TX forward offload), but as we allow multiple bridges which are either
VLAN-unaware or VLAN-aware with the same VID, there is no way to determine
the destination bridge unless we maintain some VLAN translation mapping.

On 8/25/2021 10:38 AM, DENG Qingfang wrote:

Allow taggers to add netdev features, such as VLAN offload, to slave
devices, which will make it possible for taggers to handle VLAN tags
themselves.

Signed-off-by: DENG Qingfang <dqfext@gmail.com>

Besides transmit VLAN tag offload, do you think there are other netdev 
feature bits that would warrant something like this as opposed to a more 
structured approach with adding a specific boolean/flag that is specific 
to the netdev feature you want to propagate towards the DSA slave_dev?

Still:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

On 8/26/2021 7:29 AM, DENG Qingfang wrote:

On Thu, Aug 26, 2021 at 03:03:49AM +0300, Vladimir Oltean wrote:

quoted

You cannot just remove the old code. Only things like 8021q uppers will
send packets with the VLAN in the hwaccel area.

If you have an application that puts the VLAN in the actual AF_PACKET
payload, like:

https://github.com/vladimiroltean/tsn-scripts/blob/master/isochron/send.c

then you need to handle the VLAN being in the skb payload.

I've actually tested this (only apply patch 2 without .features) and it
still worked.

OK and this works because now you always push a MTK_HDR_LEN tag which 
you were not doing before for VLAN-tagged frames, right?

You don't seem to be clearing mtk_tag[2] and mtk_tag[3] anymore, is that 
intended?

The comment says the VLAN tag need to be combined with the special tag in
order to perform VLAN table lookup, so we can set its destination port
vector to all zeroes and the switch will forward it like a data frame
(TX forward offload), but as we allow multiple bridges which are either
VLAN-unaware or VLAN-aware with the same VID, there is no way to determine
the destination bridge unless we maintain some VLAN translation mapping.

There is no "bridge" on the other side from the CPU port's perspective 
on egress, only physical ports, so how does that comment apply?
-- 
Florian

On Thu, Aug 26, 2021 at 01:29:56PM +0800, DENG Qingfang wrote:

On Thu, Aug 26, 2021 at 03:03:49AM +0300, Vladimir Oltean wrote:

quoted

You cannot just remove the old code. Only things like 8021q uppers will
send packets with the VLAN in the hwaccel area.

If you have an application that puts the VLAN in the actual AF_PACKET
payload, like:

https://github.com/vladimiroltean/tsn-scripts/blob/master/isochron/send.c

then you need to handle the VLAN being in the skb payload.

I've actually tested this (only apply patch 2 without .features) and it
still worked.

The comment says the VLAN tag need to be combined with the special tag in
order to perform VLAN table lookup,

It does say this.

so we can set its destination port vector to all zeroes and the switch
will forward it like a data frame (TX forward offload),

And it does not say this. So this is supported after all with mt7530?
Are you looking to add support for that?

but as we allow multiple bridges which are either VLAN-unaware or
VLAN-aware with the same VID, there is no way to determine the
destination bridge unless we maintain some VLAN translation mapping.

What does "VLAN translation mapping" mean, practically?
Other drivers which cannot remap VIDs to internal VLANs just restrict a
single VLAN-aware bridge, and potentially multiple VLAN-unaware ones.

On Thu, Aug 26, 2021 at 05:13:26PM +0300, Vladimir Oltean wrote:

On Thu, Aug 26, 2021 at 01:29:56PM +0800, DENG Qingfang wrote:

quoted

The comment says the VLAN tag need to be combined with the special tag in
order to perform VLAN table lookup,

It does say this.

quoted

so we can set its destination port vector to all zeroes and the switch
will forward it like a data frame (TX forward offload),

And it does not say this. So this is supported after all with mt7530?
Are you looking to add support for that?

I already run-tested that, it works, as long as there is only one bridge.

quoted

but as we allow multiple bridges which are either VLAN-unaware or
VLAN-aware with the same VID, there is no way to determine the
destination bridge unless we maintain some VLAN translation mapping.

What does "VLAN translation mapping" mean, practically?

It's just VLAN remapping, as you stated below.

Other drivers which cannot remap VIDs to internal VLANs just restrict a
single VLAN-aware bridge, and potentially multiple VLAN-unaware ones.