The TSO engine works well when the frames are not VLAN Tagged.
But it will produce broken segments when frames are VLAN Tagged.
The first segment is all good, while the second segment to the
last segment are broken, they lack of required VLAN tag.
An example here:
========
// 1st segment of a VLAN Tagged TSO frame, nothing wrong.
MacSrc > MacDst, ethertype 802.1Q (0x8100), length 1518: vlan 100, p 1, ethertype IPv4 (0x0800), HostA:42643 > HostB:5201: Flags [.], seq 1:1449
// 2nd to last segments of a VLAN Tagged TSO frame, VLAN tag is missing.
MacSrc > MacDst, ethertype IPv4 (0x0800), length 1514: HostA:42643 > HostB:5201: Flags [.], seq 1449:2897
MacSrc > MacDst, ethertype IPv4 (0x0800), length 1514: HostA:42643 > HostB:5201: Flags [.], seq 2897:4345
MacSrc > MacDst, ethertype IPv4 (0x0800), length 1514: HostA:42643 > HostB:5201: Flags [.], seq 4345:5793
MacSrc > MacDst, ethertype IPv4 (0x0800), length 1514: HostA:42643 > HostB:5201: Flags [P.], seq 5793:7241
// normal VLAN Tagged non-TSO frame, nothing wrong.
MacSrc > MacDst, ethertype 802.1Q (0x8100), length 1022: vlan 100, p 1, ethertype IPv4 (0x0800), HostA:42643 > HostB:5201: Flags [P.], seq 7241:8193
MacSrc > MacDst, ethertype 802.1Q (0x8100), length 70: vlan 100, p 1, ethertype IPv4 (0x0800), HostA:42643 > HostB:5201: Flags [F.], seq 8193
========
When transmitting VLAN Tagged TSO frames, never insert VLAN tag by HW,
always insert VLAN tag to SKB payload, then TSO works well on VLANs for
all MAC cores.
Tested on DWMAC CORE 5.10a, DWMAC CORE 5.20a and DWXGMAC CORE 3.20a
Signed-off-by: Furong Xu <[email protected]>
---
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 26 ++++++++++++-------
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index bbedf2a8c60f..d2d09edf5476 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4239,16 +4239,32 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
struct stmmac_txq_stats *txq_stats;
int tmp_pay_len = 0, first_tx;
struct stmmac_tx_queue *tx_q;
- bool has_vlan, set_ic;
+ bool set_ic;
u8 proto_hdr_len, hdr;
u32 pay_len, mss;
dma_addr_t des;
int i;
+ struct vlan_ethhdr *veth;
tx_q = &priv->dma_conf.tx_queue[queue];
txq_stats = &priv->xstats.txq_stats[queue];
first_tx = tx_q->cur_tx;
+ if (skb_vlan_tag_present(skb)) {
+ /* Always insert VLAN tag to SKB payload for TSO frames.
+ *
+ * Never insert VLAN tag by HW, since segments splited by
+ * TSO engine will be un-tagged by mistake.
+ */
+ skb_push(skb, VLAN_HLEN);
+ memmove(skb->data, skb->data + VLAN_HLEN, ETH_ALEN * 2);
+
+ veth = skb_vlan_eth_hdr(skb);
+ veth->h_vlan_proto = skb->vlan_proto;
+ veth->h_vlan_TCI = htons(skb_vlan_tag_get(skb));
+ __vlan_hwaccel_clear_tag(skb);
+ }
+
/* Compute header lengths */
if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) {
proto_hdr_len = skb_transport_offset(skb) + sizeof(struct udphdr);
@@ -4297,9 +4313,6 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
skb->data_len);
}
- /* Check if VLAN can be inserted by HW */
- has_vlan = stmmac_vlan_insert(priv, skb, tx_q);
-
first_entry = tx_q->cur_tx;
WARN_ON(tx_q->tx_skbuff[first_entry]);
@@ -4309,9 +4322,6 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
desc = &tx_q->dma_tx[first_entry];
first = desc;
- if (has_vlan)
- stmmac_set_desc_vlan(priv, first, STMMAC_VLAN_INSERT);
-
/* first descriptor: fill Headers on Buf1 */
des = dma_map_single(priv->device, skb->data, skb_headlen(skb),
DMA_TO_DEVICE);
@@ -7678,8 +7688,6 @@ int stmmac_dvr_probe(struct device *device,
ndev->features |= NETIF_F_RXHASH;
ndev->vlan_features |= ndev->features;
- /* TSO doesn't work on VLANs yet */
- ndev->vlan_features &= ~NETIF_F_TSO;
/* MTU range: 46 - hw-specific max */
ndev->min_mtu = ETH_ZLEN - ETH_HLEN;
--
2.34.1
On Thu, Jun 13, 2024 at 10:38:08AM +0800, Furong Xu wrote:
> @@ -4239,16 +4239,32 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
> struct stmmac_txq_stats *txq_stats;
> int tmp_pay_len = 0, first_tx;
> struct stmmac_tx_queue *tx_q;
> - bool has_vlan, set_ic;
> + bool set_ic;
> u8 proto_hdr_len, hdr;
> u32 pay_len, mss;
> dma_addr_t des;
> int i;
> + struct vlan_ethhdr *veth;
>
> tx_q = &priv->dma_conf.tx_queue[queue];
> txq_stats = &priv->xstats.txq_stats[queue];
> first_tx = tx_q->cur_tx;
>
> + if (skb_vlan_tag_present(skb)) {
> + /* Always insert VLAN tag to SKB payload for TSO frames.
> + *
> + * Never insert VLAN tag by HW, since segments splited by
> + * TSO engine will be un-tagged by mistake.
> + */
> + skb_push(skb, VLAN_HLEN);
> + memmove(skb->data, skb->data + VLAN_HLEN, ETH_ALEN * 2);
> +
> + veth = skb_vlan_eth_hdr(skb);
> + veth->h_vlan_proto = skb->vlan_proto;
> + veth->h_vlan_TCI = htons(skb_vlan_tag_get(skb));
> + __vlan_hwaccel_clear_tag(skb);
> + }
I think drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c::
otx2_sq_append_skb() does something similar, but uses a helper
instead:
if (skb_shinfo(skb)->gso_size && !is_hw_tso_supported(pfvf, skb)) {
/* Insert vlan tag before giving pkt to tso */
if (skb_vlan_tag_present(skb))
skb = __vlan_hwaccel_push_inside(skb);
otx2_sq_append_tso(pfvf, sq, skb, qidx);
return true;
}
Maybe __vlan_hwaccel_push_inside() should be used here?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Thu, 13 Jun 2024 11:45:45 +0100
"Russell King (Oracle)" <[email protected]> wrote:
> On Thu, Jun 13, 2024 at 10:38:08AM +0800, Furong Xu wrote:
> > @@ -4239,16 +4239,32 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
> > struct stmmac_txq_stats *txq_stats;
> > int tmp_pay_len = 0, first_tx;
> > struct stmmac_tx_queue *tx_q;
> > - bool has_vlan, set_ic;
> > + bool set_ic;
> > u8 proto_hdr_len, hdr;
> > u32 pay_len, mss;
> > dma_addr_t des;
> > int i;
> > + struct vlan_ethhdr *veth;
> >
> > tx_q = &priv->dma_conf.tx_queue[queue];
> > txq_stats = &priv->xstats.txq_stats[queue];
> > first_tx = tx_q->cur_tx;
> >
> > + if (skb_vlan_tag_present(skb)) {
> > + /* Always insert VLAN tag to SKB payload for TSO frames.
> > + *
> > + * Never insert VLAN tag by HW, since segments splited by
> > + * TSO engine will be un-tagged by mistake.
> > + */
> > + skb_push(skb, VLAN_HLEN);
> > + memmove(skb->data, skb->data + VLAN_HLEN, ETH_ALEN * 2);
> > +
> > + veth = skb_vlan_eth_hdr(skb);
> > + veth->h_vlan_proto = skb->vlan_proto;
> > + veth->h_vlan_TCI = htons(skb_vlan_tag_get(skb));
> > + __vlan_hwaccel_clear_tag(skb);
> > + }
>
> I think drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c::
> otx2_sq_append_skb() does something similar, but uses a helper
> instead:
>
> if (skb_shinfo(skb)->gso_size && !is_hw_tso_supported(pfvf, skb)) {
> /* Insert vlan tag before giving pkt to tso */
> if (skb_vlan_tag_present(skb))
> skb = __vlan_hwaccel_push_inside(skb);
> otx2_sq_append_tso(pfvf, sq, skb, qidx);
> return true;
> }
>
> Maybe __vlan_hwaccel_push_inside() should be used here?
>
Yes, it should. Thanks for your comments.
I will send a new patch.