[PATCH net 0/3] net: bcmgenet: use hardware padding of runt frames

[PATCH net 0/3] net: bcmgenet: use hardware padding of runt frames

[Doug Berger]

Now that scatter-gather and tx-checksumming are enabled by default
it revealed a packet corruption issue that can occur for very short
fragmented packets.

When padding these frames to the minimum length it is possible for
the non-linear (fragment) data to be added to the end of the linear
header in an SKB. Since the number of fragments is read before the
padding and used afterward without reloading, the fragment that
should have been consumed can be tacked on in place of part of the
padding.

The third commit in this set corrects this by removing the software
padding and allowing the hardware to add the pad bytes if necessary.

The first two commits resolve warnings observed by the kbuild test
robot and are included here for simplicity of application.

Doug Berger (3):
  net: bcmgenet: re-remove bcmgenet_hfb_add_filter
  net: bcmgenet: use __be16 for htons(ETH_P_IP)
  net: bcmgenet: use hardware padding of runt frames

 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 88 ++------------------------
 1 file changed, 5 insertions(+), 83 deletions(-)

-- 
2.7.4

[PATCH net 1/3] net: bcmgenet: re-remove bcmgenet_hfb_add_filter

[Doug Berger]

This function was originally removed by Baoyou Xie in
commit e2072600a241 ("net: bcmgenet: remove unused function in
bcmgenet.c") to prevent a build warning.

Some of the functions removed by Baoyou Xie are now used for
WAKE_FILTER support so his commit was reverted, but this function
is still unused and the kbuild test robot dutifully reported the
warning.

This commit once again removes the remaining unused hfb functions.

Fixes: 14da1510fedc ("Revert "net: bcmgenet: remove unused function in bcmgenet.c"")
Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 77 --------------------------
 1 file changed, 77 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index ff31da0ed846..f1fa11665319 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -459,17 +459,6 @@ static inline void bcmgenet_rdma_ring_writel(struct bcmgenet_priv *priv,
 			genet_dma_ring_regs[r]);
 }
 
-static bool bcmgenet_hfb_is_filter_enabled(struct bcmgenet_priv *priv,
-					   u32 f_index)
-{
-	u32 offset;
-	u32 reg;
-
-	offset = HFB_FLT_ENABLE_V3PLUS + (f_index < 32) * sizeof(u32);
-	reg = bcmgenet_hfb_reg_readl(priv, offset);
-	return !!(reg & (1 << (f_index % 32)));
-}
-
 static void bcmgenet_hfb_enable_filter(struct bcmgenet_priv *priv, u32 f_index)
 {
 	u32 offset;
@@ -533,19 +522,6 @@ static void bcmgenet_hfb_set_filter_length(struct bcmgenet_priv *priv,
 	bcmgenet_hfb_reg_writel(priv, reg, offset);
 }
 
-static int bcmgenet_hfb_find_unused_filter(struct bcmgenet_priv *priv)
-{
-	u32 f_index;
-
-	/* First MAX_NUM_OF_FS_RULES are reserved for Rx NFC filters */
-	for (f_index = MAX_NUM_OF_FS_RULES;
-	     f_index < priv->hw_params->hfb_filter_cnt; f_index++)
-		if (!bcmgenet_hfb_is_filter_enabled(priv, f_index))
-			return f_index;
-
-	return -ENOMEM;
-}
-
 static int bcmgenet_hfb_validate_mask(void *mask, size_t size)
 {
 	while (size) {
@@ -744,59 +720,6 @@ static int bcmgenet_hfb_create_rxnfc_filter(struct bcmgenet_priv *priv,
 	return err;
 }
 
-/* bcmgenet_hfb_add_filter
- *
- * Add new filter to Hardware Filter Block to match and direct Rx traffic to
- * desired Rx queue.
- *
- * f_data is an array of unsigned 32-bit integers where each 32-bit integer
- * provides filter data for 2 bytes (4 nibbles) of Rx frame:
- *
- * bits 31:20 - unused
- * bit  19    - nibble 0 match enable
- * bit  18    - nibble 1 match enable
- * bit  17    - nibble 2 match enable
- * bit  16    - nibble 3 match enable
- * bits 15:12 - nibble 0 data
- * bits 11:8  - nibble 1 data
- * bits 7:4   - nibble 2 data
- * bits 3:0   - nibble 3 data
- *
- * Example:
- * In order to match:
- * - Ethernet frame type = 0x0800 (IP)
- * - IP version field = 4
- * - IP protocol field = 0x11 (UDP)
- *
- * The following filter is needed:
- * u32 hfb_filter_ipv4_udp[] = {
- *   Rx frame offset 0x00: 0x00000000, 0x00000000, 0x00000000, 0x00000000,
- *   Rx frame offset 0x08: 0x00000000, 0x00000000, 0x000F0800, 0x00084000,
- *   Rx frame offset 0x10: 0x00000000, 0x00000000, 0x00000000, 0x00030011,
- * };
- *
- * To add the filter to HFB and direct the traffic to Rx queue 0, call:
- * bcmgenet_hfb_add_filter(priv, hfb_filter_ipv4_udp,
- *                         ARRAY_SIZE(hfb_filter_ipv4_udp), 0);
- */
-int bcmgenet_hfb_add_filter(struct bcmgenet_priv *priv, u32 *f_data,
-			    u32 f_length, u32 rx_queue)
-{
-	int f_index;
-
-	f_index = bcmgenet_hfb_find_unused_filter(priv);
-	if (f_index < 0)
-		return -ENOMEM;
-
-	if (f_length > priv->hw_params->hfb_filter_size)
-		return -EINVAL;
-
-	bcmgenet_hfb_set_filter(priv, f_data, f_length, rx_queue, f_index);
-	bcmgenet_hfb_enable_filter(priv, f_index);
-
-	return 0;
-}
-
 /* bcmgenet_hfb_clear
  *
  * Clear Hardware Filter Block and disable all filtering.
-- 
2.7.4

Re: [PATCH net 1/3] net: bcmgenet: re-remove bcmgenet_hfb_add_filter

[Florian Fainelli]

On 6/24/2020 6:14 PM, Doug Berger wrote:
> This function was originally removed by Baoyou Xie in
> commit e2072600a241 ("net: bcmgenet: remove unused function in
> bcmgenet.c") to prevent a build warning.
> 
> Some of the functions removed by Baoyou Xie are now used for
> WAKE_FILTER support so his commit was reverted, but this function
> is still unused and the kbuild test robot dutifully reported the
> warning.
> 
> This commit once again removes the remaining unused hfb functions.
> 
> Fixes: 14da1510fedc ("Revert "net: bcmgenet: remove unused function in bcmgenet.c"")
> Reported-by: kbuild test robot <lkp@intel.com>
> Signed-off-by: Doug Berger <opendmb@gmail.com>

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

[PATCH net 2/3] net: bcmgenet: use __be16 for htons(ETH_P_IP)

[Doug Berger]

The 16-bit value that holds a short in network byte order should
be declared as a restricted big endian type to allow type checks
to succeed during assignment.

Fixes: 3e370952287c ("net: bcmgenet: add support for ethtool rxnfc flows")
Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index f1fa11665319..c63f01e2bb03 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -610,8 +610,9 @@ static int bcmgenet_hfb_create_rxnfc_filter(struct bcmgenet_priv *priv,
 {
 	struct ethtool_rx_flow_spec *fs = &rule->fs;
 	int err = 0, offset = 0, f_length = 0;
-	u16 val_16, mask_16;
 	u8 val_8, mask_8;
+	__be16 val_16;
+	u16 mask_16;
 	size_t size;
 	u32 *f_data;
 
-- 
2.7.4

Re: [PATCH net 2/3] net: bcmgenet: use __be16 for htons(ETH_P_IP)

[Florian Fainelli]

On 6/24/2020 6:14 PM, Doug Berger wrote:
> The 16-bit value that holds a short in network byte order should
> be declared as a restricted big endian type to allow type checks
> to succeed during assignment.
> 
> Fixes: 3e370952287c ("net: bcmgenet: add support for ethtool rxnfc flows")
> Reported-by: kbuild test robot <lkp@intel.com>
> Signed-off-by: Doug Berger <opendmb@gmail.com>

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

[PATCH net 3/3] net: bcmgenet: use hardware padding of runt frames

[Doug Berger]

When commit 474ea9cafc45 ("net: bcmgenet: correctly pad short
packets") added the call to skb_padto() it should have been
located before the nr_frags parameter was read since that value
could be changed when padding packets with lengths between 55
and 59 bytes (inclusive).

The use of a stale nr_frags value can cause corruption of the
pad data when tx-scatter-gather is enabled. This corruption of
the pad can cause invalid checksum computation when hardware
offload of tx-checksum is also enabled.

Since the original reason for the padding was corrected by
commit 7dd399130efb ("net: bcmgenet: fix skb_len in
bcmgenet_xmit_single()") we can remove the software padding all
together and make use of hardware padding of short frames as
long as the hardware also always appends the FCS value to the
frame.

Fixes: 474ea9cafc45 ("net: bcmgenet: correctly pad short packets")
Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index c63f01e2bb03..af924a8b885f 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2042,11 +2042,6 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 		goto out;
 	}
 
-	if (skb_padto(skb, ETH_ZLEN)) {
-		ret = NETDEV_TX_OK;
-		goto out;
-	}
-
 	/* Retain how many bytes will be sent on the wire, without TSB inserted
 	 * by transmit checksum offload
 	 */
@@ -2093,6 +2088,9 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 		len_stat = (size << DMA_BUFLENGTH_SHIFT) |
 			   (priv->hw_params->qtag_mask << DMA_TX_QTAG_SHIFT);
 
+		/* Note: if we ever change from DMA_TX_APPEND_CRC below we
+		 * will need to restore software padding of "runt" packets
+		 */
 		if (!i) {
 			len_stat |= DMA_TX_APPEND_CRC | DMA_SOP;
 			if (skb->ip_summed == CHECKSUM_PARTIAL)
-- 
2.7.4

Re: [PATCH net 3/3] net: bcmgenet: use hardware padding of runt frames

[Florian Fainelli]

On 6/24/2020 6:14 PM, Doug Berger wrote:
> When commit 474ea9cafc45 ("net: bcmgenet: correctly pad short
> packets") added the call to skb_padto() it should have been
> located before the nr_frags parameter was read since that value
> could be changed when padding packets with lengths between 55
> and 59 bytes (inclusive).
> 
> The use of a stale nr_frags value can cause corruption of the
> pad data when tx-scatter-gather is enabled. This corruption of
> the pad can cause invalid checksum computation when hardware
> offload of tx-checksum is also enabled.
> 
> Since the original reason for the padding was corrected by
> commit 7dd399130efb ("net: bcmgenet: fix skb_len in
> bcmgenet_xmit_single()") we can remove the software padding all
> together and make use of hardware padding of short frames as
> long as the hardware also always appends the FCS value to the
> frame.
> 
> Fixes: 474ea9cafc45 ("net: bcmgenet: correctly pad short packets")
> Signed-off-by: Doug Berger <opendmb@gmail.com>

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

Re: [PATCH net 0/3] net: bcmgenet: use hardware padding of runt frames

[David Miller]

From: Doug Berger <opendmb@gmail.com>
Date: Wed, 24 Jun 2020 18:14:52 -0700

> Now that scatter-gather and tx-checksumming are enabled by default
> it revealed a packet corruption issue that can occur for very short
> fragmented packets.
> 
> When padding these frames to the minimum length it is possible for
> the non-linear (fragment) data to be added to the end of the linear
> header in an SKB. Since the number of fragments is read before the
> padding and used afterward without reloading, the fragment that
> should have been consumed can be tacked on in place of part of the
> padding.
> 
> The third commit in this set corrects this by removing the software
> padding and allowing the hardware to add the pad bytes if necessary.
> 
> The first two commits resolve warnings observed by the kbuild test
> robot and are included here for simplicity of application.

Series applied, thank you.