[PATCH net-next 0/8] net: bcmgenet: Turn on offloads by default

[PATCH net-next 0/8] net: bcmgenet: Turn on offloads by default

[Doug Berger]

This commit stack is based on Florian's commit 4e8aedfe78c7 ("net: 
systemport: Turn on offloads by default") and enables the offloads for
the bcmgenet driver by default.

The first commit adds support for the HIGHDMA feature to the driver.

The second converts the Tx checksum implementation to use the generic
hardware logic rather than the deprecated IP centric methods.

The third modifies the Rx checksum implementation to use the hardware
offload to compute the complete checksum rather than filtering out bad
packets detected by the hardware's IP centric implementation. This may
increase processing load by passing bad packets to the network stack,
but it provides for more flexible handling of packets by the network
stack without requiring software computation of the checksum.

The remaining commits mirror the extensions Florian made to the sysport
driver to retain symmetry with that driver and to make the benefits of
the hardware offloads more ubiquitous.

Doug Berger (8):
  net: bcmgenet: enable NETIF_F_HIGHDMA flag
  net: bcmgenet: enable NETIF_F_HW_CSUM feature
  net: bcmgenet: use CHECKSUM_COMPLETE for NETIF_F_RXCSUM
  net: bcmgenet: Refactor bcmgenet_set_features()
  net: bcmgenet: Utilize bcmgenet_set_features() during resume/open
  net: bcmgenet: Turn on offloads by default
  net: bcmgenet: Be drop monitor friendly while re-allocating headroom
  net: bcmgenet: Add software counters to track reallocations

 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 107 ++++++++++++++-----------
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |   3 +
 2 files changed, 65 insertions(+), 45 deletions(-)

-- 
2.7.4

[PATCH net-next 1/8] net: bcmgenet: enable NETIF_F_HIGHDMA flag

[Doug Berger]

This commit configures the DMA masks for the GENET driver and
sets the NETIF_F_HIGHDMA flag to report support of the feature.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 32f1245a69e2..d9defb8b1e5f 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2,7 +2,7 @@
 /*
  * Broadcom GENET (Gigabit Ethernet) controller driver
  *
- * Copyright (c) 2014-2017 Broadcom
+ * Copyright (c) 2014-2019 Broadcom
  */
 
 #define pr_fmt(fmt)				"bcmgenet: " fmt
@@ -3537,7 +3537,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
 
 	/* Set hardware features */
 	dev->hw_features |= NETIF_F_SG | NETIF_F_IP_CSUM |
-		NETIF_F_IPV6_CSUM | NETIF_F_RXCSUM;
+		NETIF_F_IPV6_CSUM | NETIF_F_HIGHDMA | NETIF_F_RXCSUM;
 
 	/* Request the WOL interrupt and advertise suspend if available */
 	priv->wol_irq_disabled = true;
@@ -3574,6 +3574,14 @@ static int bcmgenet_probe(struct platform_device *pdev)
 
 	bcmgenet_set_hw_params(priv);
 
+	err = -EIO;
+	if (priv->hw_params->flags & GENET_HAS_40BITS)
+		err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(40));
+	if (err)
+		err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+	if (err)
+		goto err;
+
 	/* Mii wait queue */
 	init_waitqueue_head(&priv->wq);
 	/* Always use RX_BUF_LENGTH (2KB) buffer for all chips */
-- 
2.7.4

[PATCH net-next 2/8] net: bcmgenet: enable NETIF_F_HW_CSUM feature

[Doug Berger]

The GENET hardware should be capable of generating IP checksums
using the NETIF_F_HW_CSUM feature, so switch to using that feature
instead of the depricated NETIF_F_IP_CSUM and NETIF_F_IPV6_CSUM.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 29 +++++++++++---------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index d9defb8b1e5f..cd07b3ad1d53 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -549,7 +549,7 @@ static int bcmgenet_set_tx_csum(struct net_device *dev,
 	tbuf_ctrl = bcmgenet_tbuf_ctrl_get(priv);
 	rbuf_ctrl = bcmgenet_rbuf_readl(priv, RBUF_CTRL);
 
-	desc_64b_en = !!(wanted & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM));
+	desc_64b_en = !!(wanted & NETIF_F_HW_CSUM);
 
 	/* enable 64 bytes descriptor in both directions (RBUF and TBUF) */
 	if (desc_64b_en) {
@@ -574,7 +574,7 @@ static int bcmgenet_set_features(struct net_device *dev,
 	netdev_features_t wanted = dev->wanted_features;
 	int ret = 0;
 
-	if (changed & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM))
+	if (changed & NETIF_F_HW_CSUM)
 		ret = bcmgenet_set_tx_csum(dev, wanted);
 	if (changed & (NETIF_F_RXCSUM))
 		ret = bcmgenet_set_rx_csum(dev, wanted);
@@ -1516,24 +1516,19 @@ static struct sk_buff *bcmgenet_put_tx_csum(struct net_device *dev,
 			ip_proto = ipv6_hdr(skb)->nexthdr;
 			break;
 		default:
-			return skb;
+			/* don't use UDP flag */
+			ip_proto = 0;
+			break;
 		}
 
 		offset = skb_checksum_start_offset(skb) - sizeof(*status);
 		tx_csum_info = (offset << STATUS_TX_CSUM_START_SHIFT) |
-				(offset + skb->csum_offset);
+				(offset + skb->csum_offset) |
+				STATUS_TX_CSUM_LV;
 
-		/* Set the length valid bit for TCP and UDP and just set
-		 * the special UDP flag for IPv4, else just set to 0.
-		 */
-		if (ip_proto == IPPROTO_TCP || ip_proto == IPPROTO_UDP) {
-			tx_csum_info |= STATUS_TX_CSUM_LV;
-			if (ip_proto == IPPROTO_UDP &&
-			    ip_ver == htons(ETH_P_IP))
-				tx_csum_info |= STATUS_TX_CSUM_PROTO_UDP;
-		} else {
-			tx_csum_info = 0;
-		}
+		/* Set the special UDP flag for UDP */
+		if (ip_proto == IPPROTO_UDP)
+			tx_csum_info |= STATUS_TX_CSUM_PROTO_UDP;
 
 		status->tx_csum_info = tx_csum_info;
 	}
@@ -3536,8 +3531,8 @@ static int bcmgenet_probe(struct platform_device *pdev)
 	priv->msg_enable = netif_msg_init(-1, GENET_MSG_DEFAULT);
 
 	/* Set hardware features */
-	dev->hw_features |= NETIF_F_SG | NETIF_F_IP_CSUM |
-		NETIF_F_IPV6_CSUM | NETIF_F_HIGHDMA | NETIF_F_RXCSUM;
+	dev->hw_features |= NETIF_F_SG | NETIF_F_HIGHDMA | NETIF_F_HW_CSUM |
+		NETIF_F_RXCSUM;
 
 	/* Request the WOL interrupt and advertise suspend if available */
 	priv->wol_irq_disabled = true;
-- 
2.7.4

[PATCH net-next 3/8] net: bcmgenet: use CHECKSUM_COMPLETE for NETIF_F_RXCSUM

[Doug Berger]

This commit updates the Rx checksum offload behavior of the driver
to use the more generic CHECKSUM_COMPLETE method that supports all
protocols over the CHECKSUM_UNNECESSARY method that only applies
to some protocols known by the hardware.

This behavior is perceived to be superior.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 13 +++++--------
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |  1 +
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index cd07b3ad1d53..5674dc304032 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -521,7 +521,7 @@ static int bcmgenet_set_rx_csum(struct net_device *dev,
 
 	/* enable rx checksumming */
 	if (rx_csum_en)
-		rbuf_chk_ctrl |= RBUF_RXCHK_EN;
+		rbuf_chk_ctrl |= RBUF_RXCHK_EN | RBUF_L3_PARSE_DIS;
 	else
 		rbuf_chk_ctrl &= ~RBUF_RXCHK_EN;
 	priv->desc_rxchk_en = rx_csum_en;
@@ -1739,7 +1739,6 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
 	unsigned int bytes_processed = 0;
 	unsigned int p_index, mask;
 	unsigned int discards;
-	unsigned int chksum_ok = 0;
 
 	/* Clear status before servicing to reduce spurious interrupts */
 	if (ring->index == DESC_INDEX) {
@@ -1793,6 +1792,10 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
 
 			status = (struct status_64 *)skb->data;
 			dma_length_status = status->length_status;
+			if (priv->desc_rxchk_en) {
+				skb->csum = ntohs(status->rx_csum & 0xffff);
+				skb->ip_summed = CHECKSUM_COMPLETE;
+			}
 		}
 
 		/* DMA flags and length are still valid no matter how
@@ -1835,18 +1838,12 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
 			goto next;
 		} /* error packet */
 
-		chksum_ok = (dma_flag & priv->dma_rx_chk_bit) &&
-			     priv->desc_rxchk_en;
-
 		skb_put(skb, len);
 		if (priv->desc_64b_en) {
 			skb_pull(skb, 64);
 			len -= 64;
 		}
 
-		if (likely(chksum_ok))
-			skb->ip_summed = CHECKSUM_UNNECESSARY;
-
 		/* remove hardware 2bytes added for IP alignment */
 		skb_pull(skb, 2);
 		len -= 2;
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index a5659197598f..11645cccc207 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -251,6 +251,7 @@ struct bcmgenet_mib_counters {
 #define RBUF_CHK_CTRL			0x14
 #define  RBUF_RXCHK_EN			(1 << 0)
 #define  RBUF_SKIP_FCS			(1 << 4)
+#define  RBUF_L3_PARSE_DIS		(1 << 5)
 
 #define RBUF_ENERGY_CTRL		0x9c
 #define  RBUF_EEE_EN			(1 << 0)
-- 
2.7.4

[PATCH net-next 4/8] net: bcmgenet: Refactor bcmgenet_set_features()

[Doug Berger]

In preparation for unconditionally enabling TX and RX checksum
offloads, refactor bcmgenet_set_features() a bit such that
__netdev_update_features() during register_netdev() can make sure
that features are correctly programmed during network device
registration.

Since we can now be called during register_netdev() with clocks
gated, we need to temporarily turn them on/off in order to have a
successful register programming.

We also move the CRC forward setting read into
bcmgenet_set_features() since priv->crc_fwd_en matters while
turning on RX checksum offload, that way we are guaranteed they
are in sync in case we ever add support for NETIF_F_RXFCS at some
point in the future.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 38 +++++++++++++-------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 5674dc304032..8afa675b45da 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -508,8 +508,8 @@ static int bcmgenet_set_link_ksettings(struct net_device *dev,
 	return phy_ethtool_ksettings_set(dev->phydev, cmd);
 }
 
-static int bcmgenet_set_rx_csum(struct net_device *dev,
-				netdev_features_t wanted)
+static void bcmgenet_set_rx_csum(struct net_device *dev,
+				 netdev_features_t wanted)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
 	u32 rbuf_chk_ctrl;
@@ -535,12 +535,10 @@ static int bcmgenet_set_rx_csum(struct net_device *dev,
 		rbuf_chk_ctrl &= ~RBUF_SKIP_FCS;
 
 	bcmgenet_rbuf_writel(priv, rbuf_chk_ctrl, RBUF_CHK_CTRL);
-
-	return 0;
 }
 
-static int bcmgenet_set_tx_csum(struct net_device *dev,
-				netdev_features_t wanted)
+static void bcmgenet_set_tx_csum(struct net_device *dev,
+				 netdev_features_t wanted)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
 	bool desc_64b_en;
@@ -563,21 +561,27 @@ static int bcmgenet_set_tx_csum(struct net_device *dev,
 
 	bcmgenet_tbuf_ctrl_set(priv, tbuf_ctrl);
 	bcmgenet_rbuf_writel(priv, rbuf_ctrl, RBUF_CTRL);
-
-	return 0;
 }
 
 static int bcmgenet_set_features(struct net_device *dev,
 				 netdev_features_t features)
 {
-	netdev_features_t changed = features ^ dev->features;
-	netdev_features_t wanted = dev->wanted_features;
-	int ret = 0;
+	struct bcmgenet_priv *priv = netdev_priv(dev);
+	u32 reg;
+	int ret;
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		return ret;
+
+	/* Make sure we reflect the value of CRC_CMD_FWD */
+	reg = bcmgenet_umac_readl(priv, UMAC_CMD);
+	priv->crc_fwd_en = !!(reg & CMD_CRC_FWD);
 
-	if (changed & NETIF_F_HW_CSUM)
-		ret = bcmgenet_set_tx_csum(dev, wanted);
-	if (changed & (NETIF_F_RXCSUM))
-		ret = bcmgenet_set_rx_csum(dev, wanted);
+	bcmgenet_set_tx_csum(dev, features);
+	bcmgenet_set_rx_csum(dev, features);
+
+	clk_disable_unprepare(priv->clk);
 
 	return ret;
 }
@@ -2878,10 +2882,6 @@ static int bcmgenet_open(struct net_device *dev)
 
 	init_umac(priv);
 
-	/* Make sure we reflect the value of CRC_CMD_FWD */
-	reg = bcmgenet_umac_readl(priv, UMAC_CMD);
-	priv->crc_fwd_en = !!(reg & CMD_CRC_FWD);
-
 	bcmgenet_set_hw_addr(priv, dev->dev_addr);
 
 	if (priv->internal_phy) {
-- 
2.7.4

[PATCH net-next 5/8] net: bcmgenet: Utilize bcmgenet_set_features() during resume/open

[Doug Berger]

During driver resume and open, the HW may have lost its context/state,
utilize bcmgenet_set_features() to make sure we do restore the correct
set of features that were previously configured.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 8afa675b45da..0df44c7076f1 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2882,6 +2882,11 @@ static int bcmgenet_open(struct net_device *dev)
 
 	init_umac(priv);
 
+	/* Apply features again in case we changed them while interface was
+	 * down
+	 */
+	bcmgenet_set_features(dev, dev->features);
+
 	bcmgenet_set_hw_addr(priv, dev->dev_addr);
 
 	if (priv->internal_phy) {
@@ -3689,6 +3694,9 @@ static int bcmgenet_resume(struct device *d)
 	genphy_config_aneg(dev->phydev);
 	bcmgenet_mii_config(priv->dev, false);
 
+	/* Restore enabled features */
+	bcmgenet_set_features(dev, dev->features);
+
 	bcmgenet_set_hw_addr(priv, dev->dev_addr);
 
 	if (priv->internal_phy) {
-- 
2.7.4

[PATCH net-next 6/8] net: bcmgenet: Turn on offloads by default

[Doug Berger]

We can turn on the RX/TX checksum offloads and the scatter/gather
features by default and make sure that those are properly reflected
back to e.g: stacked devices such as VLAN.

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

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 0df44c7076f1..b751fa76d0b0 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -3532,9 +3532,11 @@ static int bcmgenet_probe(struct platform_device *pdev)
 
 	priv->msg_enable = netif_msg_init(-1, GENET_MSG_DEFAULT);
 
-	/* Set hardware features */
-	dev->hw_features |= NETIF_F_SG | NETIF_F_HIGHDMA | NETIF_F_HW_CSUM |
-		NETIF_F_RXCSUM;
+	/* Set default features */
+	dev->features |= NETIF_F_SG | NETIF_F_HIGHDMA | NETIF_F_HW_CSUM |
+			 NETIF_F_RXCSUM;
+	dev->hw_features |= dev->features;
+	dev->vlan_features |= dev->features;
 
 	/* Request the WOL interrupt and advertise suspend if available */
 	priv->wol_irq_disabled = true;
-- 
2.7.4

[PATCH net-next 7/8] net: bcmgenet: Be drop monitor friendly while re-allocating headroom

[Doug Berger]

During bcmgenet_put_tx_csum() make sure we differentiate a SKB
headroom re-allocation failure from the normal swap and replace
path.

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 b751fa76d0b0..0280e76bb60f 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1499,11 +1499,12 @@ static struct sk_buff *bcmgenet_put_tx_csum(struct net_device *dev,
 		 * enough headroom for us to insert 64B status block.
 		 */
 		new_skb = skb_realloc_headroom(skb, sizeof(*status));
-		dev_kfree_skb(skb);
 		if (!new_skb) {
+			dev_kfree_skb_any(skb);
 			dev->stats.tx_dropped++;
 			return NULL;
 		}
+		dev_consume_skb_any(skb);
 		skb = new_skb;
 	}
 
-- 
2.7.4

[PATCH net-next 8/8] net: bcmgenet: Add software counters to track reallocations

[Doug Berger]

When inserting the TSB, keep track of how many times we had to do
it and if there was a failure in doing so, this helps profile the
driver for possibly incorrect headroom settings.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 6 ++++++
 drivers/net/ethernet/broadcom/genet/bcmgenet.h | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 0280e76bb60f..e0109b86c054 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -861,6 +861,9 @@ static const struct bcmgenet_stats bcmgenet_gstrings_stats[] = {
 	STAT_GENET_SOFT_MIB("alloc_rx_buff_failed", mib.alloc_rx_buff_failed),
 	STAT_GENET_SOFT_MIB("rx_dma_failed", mib.rx_dma_failed),
 	STAT_GENET_SOFT_MIB("tx_dma_failed", mib.tx_dma_failed),
+	STAT_GENET_SOFT_MIB("tx_realloc_tsb", mib.tx_realloc_tsb),
+	STAT_GENET_SOFT_MIB("tx_realloc_tsb_failed",
+			    mib.tx_realloc_tsb_failed),
 	/* Per TX queues */
 	STAT_GENET_Q(0),
 	STAT_GENET_Q(1),
@@ -1487,6 +1490,7 @@ static void bcmgenet_tx_reclaim_all(struct net_device *dev)
 static struct sk_buff *bcmgenet_put_tx_csum(struct net_device *dev,
 					    struct sk_buff *skb)
 {
+	struct bcmgenet_priv *priv = netdev_priv(dev);
 	struct status_64 *status = NULL;
 	struct sk_buff *new_skb;
 	u16 offset;
@@ -1501,11 +1505,13 @@ static struct sk_buff *bcmgenet_put_tx_csum(struct net_device *dev,
 		new_skb = skb_realloc_headroom(skb, sizeof(*status));
 		if (!new_skb) {
 			dev_kfree_skb_any(skb);
+			priv->mib.tx_realloc_tsb_failed++;
 			dev->stats.tx_dropped++;
 			return NULL;
 		}
 		dev_consume_skb_any(skb);
 		skb = new_skb;
+		priv->mib.tx_realloc_tsb++;
 	}
 
 	skb_push(skb, sizeof(*status));
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index 11645cccc207..178d000e462f 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -144,6 +144,8 @@ struct bcmgenet_mib_counters {
 	u32	alloc_rx_buff_failed;
 	u32	rx_dma_failed;
 	u32	tx_dma_failed;
+	u32	tx_realloc_tsb;
+	u32	tx_realloc_tsb_failed;
 };
 
 #define UMAC_HD_BKP_CTRL		0x004
-- 
2.7.4

Re: [PATCH net-next 3/8] net: bcmgenet: use CHECKSUM_COMPLETE for NETIF_F_RXCSUM

[David Miller]

From: Doug Berger <opendmb@gmail.com>
Date: Tue, 17 Dec 2019 13:02:24 -0800

> This commit updates the Rx checksum offload behavior of the driver
> to use the more generic CHECKSUM_COMPLETE method that supports all
> protocols over the CHECKSUM_UNNECESSARY method that only applies
> to some protocols known by the hardware.
> 
> This behavior is perceived to be superior.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

This has to be done in the same patch that you change to use
the NETIF_F_HW_CSUM feature flag.

Re: [PATCH net-next 1/8] net: bcmgenet: enable NETIF_F_HIGHDMA flag

[Florian Fainelli]

On 12/17/19 1:02 PM, Doug Berger wrote:
> This commit configures the DMA masks for the GENET driver and
> sets the NETIF_F_HIGHDMA flag to report support of the feature.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

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

Re: [PATCH net-next 3/8] net: bcmgenet: use CHECKSUM_COMPLETE for NETIF_F_RXCSUM

[Florian Fainelli]

On 12/17/19 1:16 PM, David Miller wrote:
> From: Doug Berger <opendmb@gmail.com>
> Date: Tue, 17 Dec 2019 13:02:24 -0800
> 
>> This commit updates the Rx checksum offload behavior of the driver
>> to use the more generic CHECKSUM_COMPLETE method that supports all
>> protocols over the CHECKSUM_UNNECESSARY method that only applies
>> to some protocols known by the hardware.
>>
>> This behavior is perceived to be superior.
>>
>> Signed-off-by: Doug Berger <opendmb@gmail.com>
> 
> This has to be done in the same patch that you change to use
> the NETIF_F_HW_CSUM feature flag.

Even if we were already advertising support for NETIF_F_RXCSUM before
patch #2? Not questioning your comment, just trying to understand why
this is deemed necessary here since it does not affect the same "direction".
-- 
Florian

Re: [PATCH net-next 3/8] net: bcmgenet: use CHECKSUM_COMPLETE for NETIF_F_RXCSUM

[Florian Fainelli]

On 12/17/19 1:02 PM, Doug Berger wrote:
> This commit updates the Rx checksum offload behavior of the driver
> to use the more generic CHECKSUM_COMPLETE method that supports all
> protocols over the CHECKSUM_UNNECESSARY method that only applies
> to some protocols known by the hardware.
> 
> This behavior is perceived to be superior.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

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

You could also remove priv->dma_rx_chk_bit which is now write only after
this patch and not used anymore.
-- 
Florian

Re: [PATCH net-next 8/8] net: bcmgenet: Add software counters to track reallocations

[Florian Fainelli]

On 12/17/19 1:02 PM, Doug Berger wrote:
> When inserting the TSB, keep track of how many times we had to do
> it and if there was a failure in doing so, this helps profile the
> driver for possibly incorrect headroom settings.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

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

Re: [PATCH net-next 7/8] net: bcmgenet: Be drop monitor friendly while re-allocating headroom

[Florian Fainelli]

On 12/17/19 1:02 PM, Doug Berger wrote:
> During bcmgenet_put_tx_csum() make sure we differentiate a SKB
> headroom re-allocation failure from the normal swap and replace
> path.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

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

Re: [PATCH net-next 5/8] net: bcmgenet: Utilize bcmgenet_set_features() during resume/open

[Florian Fainelli]

On 12/17/19 1:02 PM, Doug Berger wrote:
> During driver resume and open, the HW may have lost its context/state,
> utilize bcmgenet_set_features() to make sure we do restore the correct
> set of features that were previously configured.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

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

Re: [PATCH net-next 4/8] net: bcmgenet: Refactor bcmgenet_set_features()

[Florian Fainelli]

On 12/17/19 1:02 PM, Doug Berger wrote:
> In preparation for unconditionally enabling TX and RX checksum
> offloads, refactor bcmgenet_set_features() a bit such that
> __netdev_update_features() during register_netdev() can make sure
> that features are correctly programmed during network device
> registration.
> 
> Since we can now be called during register_netdev() with clocks
> gated, we need to temporarily turn them on/off in order to have a
> successful register programming.
> 
> We also move the CRC forward setting read into
> bcmgenet_set_features() since priv->crc_fwd_en matters while
> turning on RX checksum offload, that way we are guaranteed they
> are in sync in case we ever add support for NETIF_F_RXFCS at some
> point in the future.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

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

Re: [PATCH net-next 6/8] net: bcmgenet: Turn on offloads by default

[Florian Fainelli]

On 12/17/19 1:02 PM, Doug Berger wrote:
> We can turn on the RX/TX checksum offloads and the scatter/gather
> features by default and make sure that those are properly reflected
> back to e.g: stacked devices such as VLAN.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

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

Re: [PATCH net-next 3/8] net: bcmgenet: use CHECKSUM_COMPLETE for NETIF_F_RXCSUM

[Jakub Kicinski]

On Tue, 17 Dec 2019 13:02:24 -0800, Doug Berger wrote:
> @@ -1793,6 +1792,10 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
>  
>  			status = (struct status_64 *)skb->data;
>  			dma_length_status = status->length_status;
> +			if (priv->desc_rxchk_en) {
> +				skb->csum = ntohs(status->rx_csum & 0xffff);

This adds a new warning for a W=1 C=1 build:

../drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:45: warning: cast to res\
tricted __be16                                                                  
../drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:45: warning: cast to res\
tricted __be16                                                                  
../drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:45: warning: cast to res\
tricted __be16                                                                  
../drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:45: warning: cast to res\
tricted __be16                                                                  
../drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:43: warning: incorrect t\
ype in assignment (different base types)                                        
../drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:43:    expected restrict\
ed __wsum [usertype] csum                                                       
../drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:43:    got int           
   
Could you perhaps consider a __force cast or such?

> +				skb->ip_summed = CHECKSUM_COMPLETE;
> +			}
>  		}
>  
>  		/* DMA flags and length are still valid no matter how

Re: [PATCH net-next 3/8] net: bcmgenet: use CHECKSUM_COMPLETE for NETIF_F_RXCSUM

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Tue, 17 Dec 2019 14:52:07 -0800

> On 12/17/19 1:16 PM, David Miller wrote:
>> From: Doug Berger <opendmb@gmail.com>
>> Date: Tue, 17 Dec 2019 13:02:24 -0800
>> 
>>> This commit updates the Rx checksum offload behavior of the driver
>>> to use the more generic CHECKSUM_COMPLETE method that supports all
>>> protocols over the CHECKSUM_UNNECESSARY method that only applies
>>> to some protocols known by the hardware.
>>>
>>> This behavior is perceived to be superior.
>>>
>>> Signed-off-by: Doug Berger <opendmb@gmail.com>
>> 
>> This has to be done in the same patch that you change to use
>> the NETIF_F_HW_CSUM feature flag.
> 
> Even if we were already advertising support for NETIF_F_RXCSUM before
> patch #2? Not questioning your comment, just trying to understand why
> this is deemed necessary here since it does not affect the same "direction".

My bad... I misunderstood the situation.

Ignore me :-)

Re: [PATCH net-next 3/8] net: bcmgenet: use CHECKSUM_COMPLETE for NETIF_F_RXCSUM

[Doug Berger]

On 12/17/19 2:52 PM, Florian Fainelli wrote:
> On 12/17/19 1:02 PM, Doug Berger wrote:
>> This commit updates the Rx checksum offload behavior of the driver
>> to use the more generic CHECKSUM_COMPLETE method that supports all
>> protocols over the CHECKSUM_UNNECESSARY method that only applies
>> to some protocols known by the hardware.
>>
>> This behavior is perceived to be superior.
>>
>> Signed-off-by: Doug Berger <opendmb@gmail.com>
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> You could also remove priv->dma_rx_chk_bit which is now write only after
> this patch and not used anymore.
> 
Good idea. I'll resubmit shortly.

-Doug

Re: [PATCH net-next 3/8] net: bcmgenet: use CHECKSUM_COMPLETE for NETIF_F_RXCSUM

[kbuild test robot]

Hi Doug,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]
[also build test WARNING on net/master v5.5-rc3 next-20191220]
[cannot apply to sparc-next/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Doug-Berger/net-bcmgenet-Turn-on-offloads-by-default/20191221-050220
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git d8e419da048e45a8258b9721e85232dbb25ac618
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-129-g341daf20-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:45: sparse: sparse: cast to restricted __be16
>> drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:45: sparse: sparse: cast to restricted __be16
>> drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:45: sparse: sparse: cast to restricted __be16
>> drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:45: sparse: sparse: cast to restricted __be16
   drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:43: sparse: sparse: incorrect type in assignment (different base types)
>> drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:43: sparse:    expected restricted __wsum [usertype] csum
>> drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:43: sparse:    got int
   include/linux/netdevice.h:3931:20: sparse: sparse: shift count is negative (-1)

vim +1796 drivers/net/ethernet/broadcom/genet/bcmgenet.c

  1724	
  1725	/* bcmgenet_desc_rx - descriptor based rx process.
  1726	 * this could be called from bottom half, or from NAPI polling method.
  1727	 */
  1728	static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
  1729					     unsigned int budget)
  1730	{
  1731		struct bcmgenet_priv *priv = ring->priv;
  1732		struct net_device *dev = priv->dev;
  1733		struct enet_cb *cb;
  1734		struct sk_buff *skb;
  1735		u32 dma_length_status;
  1736		unsigned long dma_flag;
  1737		int len;
  1738		unsigned int rxpktprocessed = 0, rxpkttoprocess;
  1739		unsigned int bytes_processed = 0;
  1740		unsigned int p_index, mask;
  1741		unsigned int discards;
  1742	
  1743		/* Clear status before servicing to reduce spurious interrupts */
  1744		if (ring->index == DESC_INDEX) {
  1745			bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_RXDMA_DONE,
  1746						 INTRL2_CPU_CLEAR);
  1747		} else {
  1748			mask = 1 << (UMAC_IRQ1_RX_INTR_SHIFT + ring->index);
  1749			bcmgenet_intrl2_1_writel(priv,
  1750						 mask,
  1751						 INTRL2_CPU_CLEAR);
  1752		}
  1753	
  1754		p_index = bcmgenet_rdma_ring_readl(priv, ring->index, RDMA_PROD_INDEX);
  1755	
  1756		discards = (p_index >> DMA_P_INDEX_DISCARD_CNT_SHIFT) &
  1757			   DMA_P_INDEX_DISCARD_CNT_MASK;
  1758		if (discards > ring->old_discards) {
  1759			discards = discards - ring->old_discards;
  1760			ring->errors += discards;
  1761			ring->old_discards += discards;
  1762	
  1763			/* Clear HW register when we reach 75% of maximum 0xFFFF */
  1764			if (ring->old_discards >= 0xC000) {
  1765				ring->old_discards = 0;
  1766				bcmgenet_rdma_ring_writel(priv, ring->index, 0,
  1767							  RDMA_PROD_INDEX);
  1768			}
  1769		}
  1770	
  1771		p_index &= DMA_P_INDEX_MASK;
  1772		rxpkttoprocess = (p_index - ring->c_index) & DMA_C_INDEX_MASK;
  1773	
  1774		netif_dbg(priv, rx_status, dev,
  1775			  "RDMA: rxpkttoprocess=%d\n", rxpkttoprocess);
  1776	
  1777		while ((rxpktprocessed < rxpkttoprocess) &&
  1778		       (rxpktprocessed < budget)) {
  1779			cb = &priv->rx_cbs[ring->read_ptr];
  1780			skb = bcmgenet_rx_refill(priv, cb);
  1781	
  1782			if (unlikely(!skb)) {
  1783				ring->dropped++;
  1784				goto next;
  1785			}
  1786	
  1787			if (!priv->desc_64b_en) {
  1788				dma_length_status =
  1789					dmadesc_get_length_status(priv, cb->bd_addr);
  1790			} else {
  1791				struct status_64 *status;
  1792	
  1793				status = (struct status_64 *)skb->data;
  1794				dma_length_status = status->length_status;
  1795				if (priv->desc_rxchk_en) {
> 1796					skb->csum = ntohs(status->rx_csum & 0xffff);
  1797					skb->ip_summed = CHECKSUM_COMPLETE;
  1798				}
  1799			}
  1800	
  1801			/* DMA flags and length are still valid no matter how
  1802			 * we got the Receive Status Vector (64B RSB or register)
  1803			 */
  1804			dma_flag = dma_length_status & 0xffff;
  1805			len = dma_length_status >> DMA_BUFLENGTH_SHIFT;
  1806	
  1807			netif_dbg(priv, rx_status, dev,
  1808				  "%s:p_ind=%d c_ind=%d read_ptr=%d len_stat=0x%08x\n",
  1809				  __func__, p_index, ring->c_index,
  1810				  ring->read_ptr, dma_length_status);
  1811	
  1812			if (unlikely(!(dma_flag & DMA_EOP) || !(dma_flag & DMA_SOP))) {
  1813				netif_err(priv, rx_status, dev,
  1814					  "dropping fragmented packet!\n");
  1815				ring->errors++;
  1816				dev_kfree_skb_any(skb);
  1817				goto next;
  1818			}
  1819	
  1820			/* report errors */
  1821			if (unlikely(dma_flag & (DMA_RX_CRC_ERROR |
  1822							DMA_RX_OV |
  1823							DMA_RX_NO |
  1824							DMA_RX_LG |
  1825							DMA_RX_RXER))) {
  1826				netif_err(priv, rx_status, dev, "dma_flag=0x%x\n",
  1827					  (unsigned int)dma_flag);
  1828				if (dma_flag & DMA_RX_CRC_ERROR)
  1829					dev->stats.rx_crc_errors++;
  1830				if (dma_flag & DMA_RX_OV)
  1831					dev->stats.rx_over_errors++;
  1832				if (dma_flag & DMA_RX_NO)
  1833					dev->stats.rx_frame_errors++;
  1834				if (dma_flag & DMA_RX_LG)
  1835					dev->stats.rx_length_errors++;
  1836				dev->stats.rx_errors++;
  1837				dev_kfree_skb_any(skb);
  1838				goto next;
  1839			} /* error packet */
  1840	
  1841			skb_put(skb, len);
  1842			if (priv->desc_64b_en) {
  1843				skb_pull(skb, 64);
  1844				len -= 64;
  1845			}
  1846	
  1847			/* remove hardware 2bytes added for IP alignment */
  1848			skb_pull(skb, 2);
  1849			len -= 2;
  1850	
  1851			if (priv->crc_fwd_en) {
  1852				skb_trim(skb, len - ETH_FCS_LEN);
  1853				len -= ETH_FCS_LEN;
  1854			}
  1855	
  1856			bytes_processed += len;
  1857	
  1858			/*Finish setting up the received SKB and send it to the kernel*/
  1859			skb->protocol = eth_type_trans(skb, priv->dev);
  1860			ring->packets++;
  1861			ring->bytes += len;
  1862			if (dma_flag & DMA_RX_MULT)
  1863				dev->stats.multicast++;
  1864	
  1865			/* Notify kernel */
  1866			napi_gro_receive(&ring->napi, skb);
  1867			netif_dbg(priv, rx_status, dev, "pushed up to kernel\n");
  1868	
  1869	next:
  1870			rxpktprocessed++;
  1871			if (likely(ring->read_ptr < ring->end_ptr))
  1872				ring->read_ptr++;
  1873			else
  1874				ring->read_ptr = ring->cb_ptr;
  1875	
  1876			ring->c_index = (ring->c_index + 1) & DMA_C_INDEX_MASK;
  1877			bcmgenet_rdma_ring_writel(priv, ring->index, ring->c_index, RDMA_CONS_INDEX);
  1878		}
  1879	
  1880		ring->dim.bytes = bytes_processed;
  1881		ring->dim.packets = rxpktprocessed;
  1882	
  1883		return rxpktprocessed;
  1884	}
  1885	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation