[PATCH net 0/2] net: systemport: Fix padding vs. TSB insertion

[PATCH net 0/2] net: systemport: Fix padding vs. TSB insertion

[Florian Fainelli]

Hi David,

This patch series fixes how we pad the packets submitted to the SYSTEMPORT
adapter, and how the transmit status block (prepended 8 bytes) fits in the
picture. The first patch is not technically a bug fix, but is required for the
second path to be applied and to greatly simplify the skb length calculation.

Thanks and happy new year!

Florian Fainelli (2):
  net: systemport: Utilize skb_put_padto()
  net: systemport: Pad packet before inserting TSB

 drivers/net/ethernet/broadcom/bcmsysport.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

-- 
2.9.3

[PATCH net 1/2] net: systemport: Utilize skb_put_padto()

[Florian Fainelli]

Since we need to pad our packets, utilize skb_put_padto() which
increases skb->len by how much we need to pad, allowing us to eliminate
the test on skb->len right below.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 25d1eb4933d0..e67908b5edfe 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -1028,13 +1028,12 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
 	 * (including FCS and tag) because the length verification is done after
 	 * the Broadcom tag is stripped off the ingress packet.
 	 */
-	if (skb_padto(skb, ETH_ZLEN + ENET_BRCM_TAG_LEN)) {
+	if (skb_put_padto(skb, ETH_ZLEN + ENET_BRCM_TAG_LEN)) {
 		ret = NETDEV_TX_OK;
 		goto out;
 	}
 
-	skb_len = skb->len < ETH_ZLEN + ENET_BRCM_TAG_LEN ?
-			ETH_ZLEN + ENET_BRCM_TAG_LEN : skb->len;
+	skb_len = skb->len;
 
 	mapping = dma_map_single(kdev, skb->data, skb_len, DMA_TO_DEVICE);
 	if (dma_mapping_error(kdev, mapping)) {
-- 
2.9.3

[PATCH net 2/2] net: systemport: Pad packet before inserting TSB

[Florian Fainelli]

Inserting the TSB means adding an extra 8 bytes in fron the of packet
that is going to be used as metadata information by the TDMA engine, but
stripped off, so it does not really help with the packet padding.

For some odd packet sizes that fall below the 60 bytes payload (e.g: ARP)
we can end-up padding them after the TSB insertion, thus making them 64
bytes, but with the TDMA stripping off the first 8 bytes, they could
still be smaller than 64 bytes which is required to ingress the switch.

Fix this by swapping the padding and TSB insertion, guaranteeing that
the packets have the right sizes.

Fixes: 80105befdb4b ("net: systemport: add Broadcom SYSTEMPORT Ethernet MAC driver")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index e67908b5edfe..7e8cf213fd81 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -1012,15 +1012,6 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
 		goto out;
 	}
 
-	/* Insert TSB and checksum infos */
-	if (priv->tsb_en) {
-		skb = bcm_sysport_insert_tsb(skb, dev);
-		if (!skb) {
-			ret = NETDEV_TX_OK;
-			goto out;
-		}
-	}
-
 	/* The Ethernet switch we are interfaced with needs packets to be at
 	 * least 64 bytes (including FCS) otherwise they will be discarded when
 	 * they enter the switch port logic. When Broadcom tags are enabled, we
@@ -1033,6 +1024,15 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
 		goto out;
 	}
 
+	/* Insert TSB and checksum infos */
+	if (priv->tsb_en) {
+		skb = bcm_sysport_insert_tsb(skb, dev);
+		if (!skb) {
+			ret = NETDEV_TX_OK;
+			goto out;
+		}
+	}
+
 	skb_len = skb->len;
 
 	mapping = dma_map_single(kdev, skb->data, skb_len, DMA_TO_DEVICE);
-- 
2.9.3

Re: [PATCH net 2/2] net: systemport: Pad packet before inserting TSB

[Sergei Shtylyov]

Hello!

On 1/4/2017 3:34 AM, Florian Fainelli wrote:

> Inserting the TSB means adding an extra 8 bytes in fron the of packet

    In front?

> that is going to be used as metadata information by the TDMA engine, but
> stripped off, so it does not really help with the packet padding.
>
> For some odd packet sizes that fall below the 60 bytes payload (e.g: ARP)
> we can end-up padding them after the TSB insertion, thus making them 64
> bytes, but with the TDMA stripping off the first 8 bytes, they could
> still be smaller than 64 bytes which is required to ingress the switch.
>
> Fix this by swapping the padding and TSB insertion, guaranteeing that
> the packets have the right sizes.
>
> Fixes: 80105befdb4b ("net: systemport: add Broadcom SYSTEMPORT Ethernet MAC driver")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
[...]

MBR, Sergei

Re: [PATCH net 0/2] net: systemport: Fix padding vs. TSB insertion

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Tue,  3 Jan 2017 16:34:47 -0800

> This patch series fixes how we pad the packets submitted to the SYSTEMPORT
> adapter, and how the transmit status block (prepended 8 bytes) fits in the
> picture. The first patch is not technically a bug fix, but is required for the
> second path to be applied and to greatly simplify the skb length calculation.

Series applied with the typo in the commit message of patch #2 fixed.

Happy new year.