[PATCH net-next 0/3] net: Broadcom drivers coalescing fixes

[PATCH net-next 0/3] net: Broadcom drivers coalescing fixes

[Florian Fainelli]

Hi all,

Following Tal's review of the adaptive RX/TX coalescing feature added to the
SYSTEMPORT and GENET driver a number of things showed up:

- adaptive TX coalescing is not actually a good idea with the current way
  the estimator will program the ring, this results in a higher CPU load, NAPI
  on TX already does a reasonably good job at maintaining the interrupt count low

- both SYSTEMPORT and GENET would suffer from the same issues while configuring
  coalescing parameters where the values would just not be applied correctly
  based on user settings, so we fix that too

Tal, thanks again for your feedback, I would appreciate if you could review that
the new behavior appears to be implemented correctly.

Thanks!

Florian Fainelli (3):
  net: systemport: Remove adaptive TX coalescing
  net: systemport: Fix coalescing settings handling
  net: bcmgenet: Fix coalescing settings handling

 drivers/net/ethernet/broadcom/bcmsysport.c     | 118 ++++++++++---------------
 drivers/net/ethernet/broadcom/bcmsysport.h     |   5 +-
 drivers/net/ethernet/broadcom/genet/bcmgenet.c |  78 +++++++++++-----
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |   4 +-
 4 files changed, 106 insertions(+), 99 deletions(-)

-- 
2.14.1

[PATCH net-next 1/3] net: systemport: Remove adaptive TX coalescing

[Florian Fainelli]

Adaptive TX coalescing is not currently giving us any advantages and
ends up making the CPU spin more frequently until TX completion. Deny
and disable adaptive TX coalescing for now and rely on static
configuration, we can always add it back later.

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

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 4e26f606a7f2..1e52bb7d822e 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -15,7 +15,6 @@
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/netdevice.h>
-#include <linux/net_dim.h>
 #include <linux/etherdevice.h>
 #include <linux/platform_device.h>
 #include <linux/of.h>
@@ -588,7 +587,8 @@ static void bcm_sysport_set_rx_coalesce(struct bcm_sysport_priv *priv)
 	rdma_writel(priv, reg, RDMA_MBDONE_INTR);
 }
 
-static void bcm_sysport_set_tx_coalesce(struct bcm_sysport_tx_ring *ring)
+static void bcm_sysport_set_tx_coalesce(struct bcm_sysport_tx_ring *ring,
+					struct ethtool_coalesce *ec)
 {
 	struct bcm_sysport_priv *priv = ring->priv;
 	u32 reg;
@@ -596,8 +596,8 @@ static void bcm_sysport_set_tx_coalesce(struct bcm_sysport_tx_ring *ring)
 	reg = tdma_readl(priv, TDMA_DESC_RING_INTR_CONTROL(ring->index));
 	reg &= ~(RING_INTR_THRESH_MASK |
 		 RING_TIMEOUT_MASK << RING_TIMEOUT_SHIFT);
-	reg |= ring->dim.coal_pkts;
-	reg |= DIV_ROUND_UP(ring->dim.coal_usecs * 1000, 8192) <<
+	reg |= ec->tx_max_coalesced_frames;
+	reg |= DIV_ROUND_UP(ec->tx_coalesce_usecs * 1000, 8192) <<
 			    RING_TIMEOUT_SHIFT;
 	tdma_writel(priv, reg, TDMA_DESC_RING_INTR_CONTROL(ring->index));
 }
@@ -606,18 +606,12 @@ static int bcm_sysport_get_coalesce(struct net_device *dev,
 				    struct ethtool_coalesce *ec)
 {
 	struct bcm_sysport_priv *priv = netdev_priv(dev);
-	struct bcm_sysport_tx_ring *ring;
-	unsigned int i;
 	u32 reg;
 
 	reg = tdma_readl(priv, TDMA_DESC_RING_INTR_CONTROL(0));
 
 	ec->tx_coalesce_usecs = (reg >> RING_TIMEOUT_SHIFT) * 8192 / 1000;
 	ec->tx_max_coalesced_frames = reg & RING_INTR_THRESH_MASK;
-	for (i = 0; i < dev->num_tx_queues; i++) {
-		ring = &priv->tx_rings[i];
-		ec->use_adaptive_tx_coalesce |= ring->dim.use_dim;
-	}
 
 	reg = rdma_readl(priv, RDMA_MBDONE_INTR);
 
@@ -632,7 +626,6 @@ static int bcm_sysport_set_coalesce(struct net_device *dev,
 				    struct ethtool_coalesce *ec)
 {
 	struct bcm_sysport_priv *priv = netdev_priv(dev);
-	struct bcm_sysport_tx_ring *ring;
 	unsigned int i;
 
 	/* Base system clock is 125Mhz, DMA timeout is this reference clock
@@ -646,20 +639,12 @@ static int bcm_sysport_set_coalesce(struct net_device *dev,
 		return -EINVAL;
 
 	if ((ec->tx_coalesce_usecs == 0 && ec->tx_max_coalesced_frames == 0) ||
-	    (ec->rx_coalesce_usecs == 0 && ec->rx_max_coalesced_frames == 0))
+	    (ec->rx_coalesce_usecs == 0 && ec->rx_max_coalesced_frames == 0) ||
+	    ec->use_adaptive_tx_coalesce)
 		return -EINVAL;
 
-	for (i = 0; i < dev->num_tx_queues; i++) {
-		ring = &priv->tx_rings[i];
-		ring->dim.coal_pkts = ec->tx_max_coalesced_frames;
-		ring->dim.coal_usecs = ec->tx_coalesce_usecs;
-		if (!ec->use_adaptive_tx_coalesce && ring->dim.use_dim) {
-			ring->dim.coal_pkts = 1;
-			ring->dim.coal_usecs = 0;
-		}
-		ring->dim.use_dim = ec->use_adaptive_tx_coalesce;
-		bcm_sysport_set_tx_coalesce(ring);
-	}
+	for (i = 0; i < dev->num_tx_queues; i++)
+		bcm_sysport_set_tx_coalesce(&priv->tx_rings[i], ec);
 
 	priv->dim.coal_usecs = ec->rx_coalesce_usecs;
 	priv->dim.coal_pkts = ec->rx_max_coalesced_frames;
@@ -940,8 +925,6 @@ static unsigned int __bcm_sysport_tx_reclaim(struct bcm_sysport_priv *priv,
 	ring->packets += pkts_compl;
 	ring->bytes += bytes_compl;
 	u64_stats_update_end(&priv->syncp);
-	ring->dim.packets = pkts_compl;
-	ring->dim.bytes = bytes_compl;
 
 	ring->c_index = c_index;
 
@@ -987,7 +970,6 @@ static int bcm_sysport_tx_poll(struct napi_struct *napi, int budget)
 {
 	struct bcm_sysport_tx_ring *ring =
 		container_of(napi, struct bcm_sysport_tx_ring, napi);
-	struct net_dim_sample dim_sample;
 	unsigned int work_done = 0;
 
 	work_done = bcm_sysport_tx_reclaim(ring->priv, ring);
@@ -1004,12 +986,6 @@ static int bcm_sysport_tx_poll(struct napi_struct *napi, int budget)
 		return 0;
 	}
 
-	if (ring->dim.use_dim) {
-		net_dim_sample(ring->dim.event_ctr, ring->dim.packets,
-			       ring->dim.bytes, &dim_sample);
-		net_dim(&ring->dim.dim, dim_sample);
-	}
-
 	return budget;
 }
 
@@ -1089,23 +1065,6 @@ static void bcm_sysport_dim_work(struct work_struct *work)
 	dim->state = NET_DIM_START_MEASURE;
 }
 
-static void bcm_sysport_dim_tx_work(struct work_struct *work)
-{
-	struct net_dim *dim = container_of(work, struct net_dim, work);
-	struct bcm_sysport_net_dim *ndim =
-			container_of(dim, struct bcm_sysport_net_dim, dim);
-	struct bcm_sysport_tx_ring *ring =
-			container_of(ndim, struct bcm_sysport_tx_ring, dim);
-	struct net_dim_cq_moder cur_profile =
-				net_dim_get_profile(dim->mode, dim->profile_ix);
-
-	ring->dim.coal_usecs = cur_profile.usec;
-	ring->dim.coal_pkts = cur_profile.pkts;
-
-	bcm_sysport_set_tx_coalesce(ring);
-	dim->state = NET_DIM_START_MEASURE;
-}
-
 /* RX and misc interrupt routine */
 static irqreturn_t bcm_sysport_rx_isr(int irq, void *dev_id)
 {
@@ -1152,7 +1111,6 @@ static irqreturn_t bcm_sysport_rx_isr(int irq, void *dev_id)
 			continue;
 
 		txr = &priv->tx_rings[ring];
-		txr->dim.event_ctr++;
 
 		if (likely(napi_schedule_prep(&txr->napi))) {
 			intrl2_0_mask_set(priv, ring_bit);
@@ -1185,7 +1143,6 @@ static irqreturn_t bcm_sysport_tx_isr(int irq, void *dev_id)
 			continue;
 
 		txr = &priv->tx_rings[ring];
-		txr->dim.event_ctr++;
 
 		if (likely(napi_schedule_prep(&txr->napi))) {
 			intrl2_1_mask_set(priv, BIT(ring));
@@ -1551,7 +1508,6 @@ static int bcm_sysport_init_tx_ring(struct bcm_sysport_priv *priv,
 	reg |= (1 << index);
 	tdma_writel(priv, reg, TDMA_TIER1_ARB_0_QUEUE_EN);
 
-	bcm_sysport_init_dim(&ring->dim, bcm_sysport_dim_tx_work);
 	napi_enable(&ring->napi);
 
 	netif_dbg(priv, hw, priv->netdev,
@@ -1582,7 +1538,6 @@ static void bcm_sysport_fini_tx_ring(struct bcm_sysport_priv *priv,
 		return;
 
 	napi_disable(&ring->napi);
-	cancel_work_sync(&ring->dim.dim.work);
 	netif_napi_del(&ring->napi);
 
 	bcm_sysport_tx_clean(priv, ring);
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.h b/drivers/net/ethernet/broadcom/bcmsysport.h
index e1c97d4a82b4..57e18ef8f206 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.h
+++ b/drivers/net/ethernet/broadcom/bcmsysport.h
@@ -723,7 +723,6 @@ struct bcm_sysport_tx_ring {
 	struct bcm_sysport_priv *priv;	/* private context backpointer */
 	unsigned long	packets;	/* packets statistics */
 	unsigned long	bytes;		/* bytes statistics */
-	struct bcm_sysport_net_dim dim;	/* Net DIM context */
 	unsigned int	switch_queue;	/* switch port queue number */
 	unsigned int	switch_port;	/* switch port queue number */
 	bool		inspect;	/* inspect switch port and queue */
-- 
2.14.1

[PATCH net-next 2/3] net: systemport: Fix coalescing settings handling

[Florian Fainelli]

There were a number of issues with setting the RX coalescing parameters:

- we would not be preserving values that would have been configured
  across close/open calls, instead we would always reset to no timeout
  and 1 interrupt per packet, this would also prevent DIM from setting its
  default usec/pkts values

- when adaptive RX would be turned on, we woud not be fetching the
  default parameters, we would stay with no timeout/1 packet per
  interrupt until the estimator kicks in and changes that

- finally disabling adaptive RX coalescing while providing parameters
  would not be honored, and we would stay with whatever DIM had
  previously determined instead of the user requested parameters

Fixes: b6e0e875421e ("net: systemport: Implement adaptive interrupt coalescing")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 57 ++++++++++++++++++++----------
 drivers/net/ethernet/broadcom/bcmsysport.h |  4 +--
 2 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 1e52bb7d822e..43ad6300c351 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -574,16 +574,16 @@ static int bcm_sysport_set_wol(struct net_device *dev,
 	return 0;
 }
 
-static void bcm_sysport_set_rx_coalesce(struct bcm_sysport_priv *priv)
+static void bcm_sysport_set_rx_coalesce(struct bcm_sysport_priv *priv,
+					u32 usecs, u32 pkts)
 {
 	u32 reg;
 
 	reg = rdma_readl(priv, RDMA_MBDONE_INTR);
 	reg &= ~(RDMA_INTR_THRESH_MASK |
 		 RDMA_TIMEOUT_MASK << RDMA_TIMEOUT_SHIFT);
-	reg |= priv->dim.coal_pkts;
-	reg |= DIV_ROUND_UP(priv->dim.coal_usecs * 1000, 8192) <<
-			    RDMA_TIMEOUT_SHIFT;
+	reg |= pkts;
+	reg |= DIV_ROUND_UP(usecs * 1000, 8192) << RDMA_TIMEOUT_SHIFT;
 	rdma_writel(priv, reg, RDMA_MBDONE_INTR);
 }
 
@@ -626,6 +626,8 @@ static int bcm_sysport_set_coalesce(struct net_device *dev,
 				    struct ethtool_coalesce *ec)
 {
 	struct bcm_sysport_priv *priv = netdev_priv(dev);
+	struct net_dim_cq_moder moder;
+	u32 usecs, pkts;
 	unsigned int i;
 
 	/* Base system clock is 125Mhz, DMA timeout is this reference clock
@@ -646,15 +648,22 @@ static int bcm_sysport_set_coalesce(struct net_device *dev,
 	for (i = 0; i < dev->num_tx_queues; i++)
 		bcm_sysport_set_tx_coalesce(&priv->tx_rings[i], ec);
 
-	priv->dim.coal_usecs = ec->rx_coalesce_usecs;
-	priv->dim.coal_pkts = ec->rx_max_coalesced_frames;
+	priv->rx_coalesce_usecs = ec->rx_coalesce_usecs;
+	priv->rx_max_coalesced_frames = ec->rx_max_coalesced_frames;
+	usecs = priv->rx_coalesce_usecs;
+	pkts = priv->rx_max_coalesced_frames;
 
-	if (!ec->use_adaptive_rx_coalesce && priv->dim.use_dim) {
-		priv->dim.coal_pkts = 1;
-		priv->dim.coal_usecs = 0;
+	/* If DIM is enabled, immediately obtain default parameters */
+	if (ec->use_adaptive_rx_coalesce) {
+		moder = net_dim_get_def_profile(priv->dim.dim.mode);
+		usecs = moder.usec;
+		pkts = moder.pkts;
 	}
+
 	priv->dim.use_dim = ec->use_adaptive_rx_coalesce;
-	bcm_sysport_set_rx_coalesce(priv);
+
+	/* Apply desired coalescing parameters */
+	bcm_sysport_set_rx_coalesce(priv, usecs, pkts);
 
 	return 0;
 }
@@ -1058,10 +1067,7 @@ static void bcm_sysport_dim_work(struct work_struct *work)
 	struct net_dim_cq_moder cur_profile =
 				net_dim_get_profile(dim->mode, dim->profile_ix);
 
-	priv->dim.coal_usecs = cur_profile.usec;
-	priv->dim.coal_pkts = cur_profile.pkts;
-
-	bcm_sysport_set_rx_coalesce(priv);
+	bcm_sysport_set_rx_coalesce(priv, cur_profile.usec, cur_profile.pkts);
 	dim->state = NET_DIM_START_MEASURE;
 }
 
@@ -1408,14 +1414,30 @@ static void bcm_sysport_adj_link(struct net_device *dev)
 		phy_print_status(phydev);
 }
 
-static void bcm_sysport_init_dim(struct bcm_sysport_net_dim *dim,
+static void bcm_sysport_init_dim(struct bcm_sysport_priv *priv,
 				 void (*cb)(struct work_struct *work))
 {
+	struct bcm_sysport_net_dim *dim = &priv->dim;
+	struct net_dim_cq_moder moder;
+	u32 usecs, pkts;
+
 	INIT_WORK(&dim->dim.work, cb);
 	dim->dim.mode = NET_DIM_CQ_PERIOD_MODE_START_FROM_EQE;
 	dim->event_ctr = 0;
 	dim->packets = 0;
 	dim->bytes = 0;
+
+	usecs = priv->rx_coalesce_usecs;
+	pkts = priv->rx_max_coalesced_frames;
+
+	/* If DIM was enabled, re-apply default parameters */
+	if (dim->use_dim) {
+		moder = net_dim_get_def_profile(dim->dim.mode);
+		usecs = moder.usec;
+		pkts = moder.pkts;
+	}
+
+	bcm_sysport_set_rx_coalesce(priv, usecs, pkts);
 }
 
 static int bcm_sysport_init_tx_ring(struct bcm_sysport_priv *priv,
@@ -1658,8 +1680,6 @@ static int bcm_sysport_init_rx_ring(struct bcm_sysport_priv *priv)
 	rdma_writel(priv, 0, RDMA_END_ADDR_HI);
 	rdma_writel(priv, priv->num_rx_desc_words - 1, RDMA_END_ADDR_LO);
 
-	rdma_writel(priv, 1, RDMA_MBDONE_INTR);
-
 	netif_dbg(priv, hw, priv->netdev,
 		  "RDMA cfg, num_rx_bds=%d, rx_bds=%p\n",
 		  priv->num_rx_bds, priv->rx_bds);
@@ -1827,7 +1847,7 @@ static void bcm_sysport_netif_start(struct net_device *dev)
 	struct bcm_sysport_priv *priv = netdev_priv(dev);
 
 	/* Enable NAPI */
-	bcm_sysport_init_dim(&priv->dim, bcm_sysport_dim_work);
+	bcm_sysport_init_dim(priv, bcm_sysport_dim_work);
 	napi_enable(&priv->napi);
 
 	/* Enable RX interrupt and TX ring full interrupt */
@@ -2333,6 +2353,7 @@ static int bcm_sysport_probe(struct platform_device *pdev)
 	/* libphy will adjust the link state accordingly */
 	netif_carrier_off(dev);
 
+	priv->rx_max_coalesced_frames = 1;
 	u64_stats_init(&priv->syncp);
 
 	priv->dsa_notifier.notifier_call = bcm_sysport_dsa_notifier;
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.h b/drivers/net/ethernet/broadcom/bcmsysport.h
index 57e18ef8f206..d6e5d0cbf3a3 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.h
+++ b/drivers/net/ethernet/broadcom/bcmsysport.h
@@ -701,8 +701,6 @@ struct bcm_sysport_net_dim {
 	u16			event_ctr;
 	unsigned long		packets;
 	unsigned long		bytes;
-	u32			coal_usecs;
-	u32			coal_pkts;
 	struct net_dim		dim;
 };
 
@@ -755,6 +753,8 @@ struct bcm_sysport_priv {
 	unsigned int		rx_c_index;
 
 	struct bcm_sysport_net_dim	dim;
+	u32			rx_max_coalesced_frames;
+	u32			rx_coalesce_usecs;
 
 	/* PHY device */
 	struct device_node	*phy_dn;
-- 
2.14.1

[PATCH net-next 3/3] net: bcmgenet: Fix coalescing settings handling

[Florian Fainelli]

There were a number of issues with setting the RX coalescing parameters:

- we would not be preserving values that would have been configured
  across close/open calls, instead we would always reset to no timeout
  and 1 interrupt per packet, this would also prevent DIM from setting its
  default usec/pkts values

- when adaptive RX would be turned on, we woud not be fetching the
  default parameters, we would stay with no timeout/1 packet per interrupt
  until the estimator kicks in and changes that

- finally disabling adaptive RX coalescing while providing parameters
  would not be honored, and we would stay with whatever DIM had previously
  determined instead of the user requested parameters

Fixes: 9f4ca05827a2 ("net: bcmgenet: Add support for adaptive RX coalescing")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 78 ++++++++++++++++++--------
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |  4 +-
 2 files changed, 57 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 7db8edc643ec..76409debb796 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -625,18 +625,18 @@ static int bcmgenet_get_coalesce(struct net_device *dev,
 	return 0;
 }
 
-static void bcmgenet_set_rx_coalesce(struct bcmgenet_rx_ring *ring)
+static void bcmgenet_set_rx_coalesce(struct bcmgenet_rx_ring *ring,
+				     u32 usecs, u32 pkts)
 {
 	struct bcmgenet_priv *priv = ring->priv;
 	unsigned int i = ring->index;
 	u32 reg;
 
-	bcmgenet_rdma_ring_writel(priv, i, ring->dim.coal_pkts,
-				  DMA_MBUF_DONE_THRESH);
+	bcmgenet_rdma_ring_writel(priv, i, pkts, DMA_MBUF_DONE_THRESH);
 
 	reg = bcmgenet_rdma_readl(priv, DMA_RING0_TIMEOUT + i);
 	reg &= ~DMA_TIMEOUT_MASK;
-	reg |= DIV_ROUND_UP(ring->dim.coal_usecs * 1000, 8192);
+	reg |= DIV_ROUND_UP(usecs * 1000, 8192);
 	bcmgenet_rdma_writel(priv, reg, DMA_RING0_TIMEOUT + i);
 }
 
@@ -645,6 +645,8 @@ static int bcmgenet_set_coalesce(struct net_device *dev,
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
 	struct bcmgenet_rx_ring *ring;
+	struct net_dim_cq_moder moder;
+	u32 usecs, pkts;
 	unsigned int i;
 
 	/* Base system clock is 125Mhz, DMA timeout is this reference clock
@@ -682,25 +684,37 @@ static int bcmgenet_set_coalesce(struct net_device *dev,
 
 	for (i = 0; i < priv->hw_params->rx_queues; i++) {
 		ring = &priv->rx_rings[i];
-		ring->dim.coal_usecs = ec->rx_coalesce_usecs;
-		ring->dim.coal_pkts = ec->rx_max_coalesced_frames;
-		if (!ec->use_adaptive_rx_coalesce && ring->dim.use_dim) {
-			ring->dim.coal_pkts = 1;
-			ring->dim.coal_usecs = 0;
+
+		ring->rx_coalesce_usecs = ec->rx_coalesce_usecs;
+		ring->rx_max_coalesced_frames = ec->rx_max_coalesced_frames;
+		usecs = ring->rx_coalesce_usecs;
+		pkts = ring->rx_max_coalesced_frames;
+
+		if (ec->use_adaptive_rx_coalesce) {
+			moder = net_dim_get_def_profile(ring->dim.dim.mode);
+			usecs = moder.usec;
+			pkts = moder.pkts;
 		}
+
 		ring->dim.use_dim = ec->use_adaptive_rx_coalesce;
-		bcmgenet_set_rx_coalesce(ring);
+		bcmgenet_set_rx_coalesce(ring, usecs, pkts);
 	}
 
 	ring = &priv->rx_rings[DESC_INDEX];
-	ring->dim.coal_usecs = ec->rx_coalesce_usecs;
-	ring->dim.coal_pkts = ec->rx_max_coalesced_frames;
-	if (!ec->use_adaptive_rx_coalesce && ring->dim.use_dim) {
-		ring->dim.coal_pkts = 1;
-		ring->dim.coal_usecs = 0;
+
+	ring->rx_coalesce_usecs = ec->rx_coalesce_usecs;
+	ring->rx_max_coalesced_frames = ec->rx_max_coalesced_frames;
+	usecs = ring->rx_coalesce_usecs;
+	pkts = ring->rx_max_coalesced_frames;
+
+	if (ec->use_adaptive_rx_coalesce) {
+		moder = net_dim_get_def_profile(ring->dim.dim.mode);
+		usecs = moder.usec;
+		pkts = moder.pkts;
 	}
+
 	ring->dim.use_dim = ec->use_adaptive_rx_coalesce;
-	bcmgenet_set_rx_coalesce(ring);
+	bcmgenet_set_rx_coalesce(ring, usecs, pkts);
 
 	return 0;
 }
@@ -1924,10 +1938,7 @@ static void bcmgenet_dim_work(struct work_struct *work)
 	struct net_dim_cq_moder cur_profile =
 			net_dim_get_profile(dim->mode, dim->profile_ix);
 
-	ring->dim.coal_usecs = cur_profile.usec;
-	ring->dim.coal_pkts = cur_profile.pkts;
-
-	bcmgenet_set_rx_coalesce(ring);
+	bcmgenet_set_rx_coalesce(ring, cur_profile.usec, cur_profile.pkts);
 	dim->state = NET_DIM_START_MEASURE;
 }
 
@@ -2079,14 +2090,30 @@ static void init_umac(struct bcmgenet_priv *priv)
 	dev_dbg(kdev, "done init umac\n");
 }
 
-static void bcmgenet_init_dim(struct bcmgenet_net_dim *dim,
+static void bcmgenet_init_dim(struct bcmgenet_rx_ring *ring,
 			      void (*cb)(struct work_struct *work))
 {
+	struct bcmgenet_net_dim *dim = &ring->dim;
+	struct net_dim_cq_moder moder;
+	u32 usecs, pkts;
+
 	INIT_WORK(&dim->dim.work, cb);
 	dim->dim.mode = NET_DIM_CQ_PERIOD_MODE_START_FROM_EQE;
 	dim->event_ctr = 0;
 	dim->packets = 0;
 	dim->bytes = 0;
+
+	usecs = ring->rx_coalesce_usecs;
+	pkts = ring->rx_max_coalesced_frames;
+
+	/* If DIM was enabled, re-apply default parameters */
+	if (dim->use_dim) {
+		moder = net_dim_get_def_profile(dim->dim.mode);
+		usecs = moder.usec;
+		pkts = moder.pkts;
+	}
+
+	bcmgenet_set_rx_coalesce(ring, usecs, pkts);
 }
 
 /* Initialize a Tx ring along with corresponding hardware registers */
@@ -2178,7 +2205,7 @@ static int bcmgenet_init_rx_ring(struct bcmgenet_priv *priv,
 	if (ret)
 		return ret;
 
-	bcmgenet_init_dim(&ring->dim, bcmgenet_dim_work);
+	bcmgenet_init_dim(ring, bcmgenet_dim_work);
 
 	/* Initialize Rx NAPI */
 	netif_napi_add(priv->dev, &ring->napi, bcmgenet_rx_poll,
@@ -2186,7 +2213,6 @@ static int bcmgenet_init_rx_ring(struct bcmgenet_priv *priv,
 
 	bcmgenet_rdma_ring_writel(priv, index, 0, RDMA_PROD_INDEX);
 	bcmgenet_rdma_ring_writel(priv, index, 0, RDMA_CONS_INDEX);
-	bcmgenet_rdma_ring_writel(priv, index, 1, DMA_MBUF_DONE_THRESH);
 	bcmgenet_rdma_ring_writel(priv, index,
 				  ((size << DMA_RING_SIZE_SHIFT) |
 				   RX_BUF_LENGTH), DMA_RING_BUF_SIZE);
@@ -3424,6 +3450,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
 	struct net_device *dev;
 	const void *macaddr;
 	struct resource *r;
+	unsigned int i;
 	int err = -EIO;
 	const char *phy_mode_str;
 
@@ -3552,6 +3579,11 @@ static int bcmgenet_probe(struct platform_device *pdev)
 	netif_set_real_num_tx_queues(priv->dev, priv->hw_params->tx_queues + 1);
 	netif_set_real_num_rx_queues(priv->dev, priv->hw_params->rx_queues + 1);
 
+	/* Set default coalescing parameters */
+	for (i = 0; i < priv->hw_params->rx_queues; i++)
+		priv->rx_rings[i].rx_max_coalesced_frames = 1;
+	priv->rx_rings[DESC_INDEX].rx_max_coalesced_frames = 1;
+
 	/* libphy will determine the link state */
 	netif_carrier_off(dev);
 
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index 22c41e0430fb..b773bc07edf7 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -578,8 +578,6 @@ struct bcmgenet_net_dim {
 	u16		event_ctr;
 	unsigned long	packets;
 	unsigned long	bytes;
-	u32		coal_usecs;
-	u32		coal_pkts;
 	struct net_dim	dim;
 };
 
@@ -598,6 +596,8 @@ struct bcmgenet_rx_ring {
 	unsigned int	end_ptr;	/* Rx ring end CB ptr */
 	unsigned int	old_discards;
 	struct bcmgenet_net_dim dim;
+	u32		rx_max_coalesced_frames;
+	u32		rx_coalesce_usecs;
 	void (*int_enable)(struct bcmgenet_rx_ring *);
 	void (*int_disable)(struct bcmgenet_rx_ring *);
 	struct bcmgenet_priv *priv;
-- 
2.14.1

Re: [PATCH net-next 1/3] net: systemport: Remove adaptive TX coalescing

[Tal Gilboa]

On 3/27/2018 10:47 PM, Florian Fainelli wrote:
> Adaptive TX coalescing is not currently giving us any advantages and
> ends up making the CPU spin more frequently until TX completion. Deny
> and disable adaptive TX coalescing for now and rely on static
> configuration, we can always add it back later.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>   drivers/net/ethernet/broadcom/bcmsysport.c | 61 ++++--------------------------
>   drivers/net/ethernet/broadcom/bcmsysport.h |  1 -
>   2 files changed, 8 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index 4e26f606a7f2..1e52bb7d822e 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -15,7 +15,6 @@
>   #include <linux/module.h>
>   #include <linux/kernel.h>
>   #include <linux/netdevice.h>
> -#include <linux/net_dim.h>
>   #include <linux/etherdevice.h>
>   #include <linux/platform_device.h>
>   #include <linux/of.h>
> @@ -588,7 +587,8 @@ static void bcm_sysport_set_rx_coalesce(struct bcm_sysport_priv *priv)
>   	rdma_writel(priv, reg, RDMA_MBDONE_INTR);
>   }
>   
> -static void bcm_sysport_set_tx_coalesce(struct bcm_sysport_tx_ring *ring)
> +static void bcm_sysport_set_tx_coalesce(struct bcm_sysport_tx_ring *ring,
> +					struct ethtool_coalesce *ec)
>   {
>   	struct bcm_sysport_priv *priv = ring->priv;
>   	u32 reg;
> @@ -596,8 +596,8 @@ static void bcm_sysport_set_tx_coalesce(struct bcm_sysport_tx_ring *ring)
>   	reg = tdma_readl(priv, TDMA_DESC_RING_INTR_CONTROL(ring->index));
>   	reg &= ~(RING_INTR_THRESH_MASK |
>   		 RING_TIMEOUT_MASK << RING_TIMEOUT_SHIFT);
> -	reg |= ring->dim.coal_pkts;
> -	reg |= DIV_ROUND_UP(ring->dim.coal_usecs * 1000, 8192) <<
> +	reg |= ec->tx_max_coalesced_frames;
> +	reg |= DIV_ROUND_UP(ec->tx_coalesce_usecs * 1000, 8192) <<
>   			    RING_TIMEOUT_SHIFT;
>   	tdma_writel(priv, reg, TDMA_DESC_RING_INTR_CONTROL(ring->index));
>   }
> @@ -606,18 +606,12 @@ static int bcm_sysport_get_coalesce(struct net_device *dev,
>   				    struct ethtool_coalesce *ec)
>   {
>   	struct bcm_sysport_priv *priv = netdev_priv(dev);
> -	struct bcm_sysport_tx_ring *ring;
> -	unsigned int i;
>   	u32 reg;
>   
>   	reg = tdma_readl(priv, TDMA_DESC_RING_INTR_CONTROL(0));
>   
>   	ec->tx_coalesce_usecs = (reg >> RING_TIMEOUT_SHIFT) * 8192 / 1000;
>   	ec->tx_max_coalesced_frames = reg & RING_INTR_THRESH_MASK;
> -	for (i = 0; i < dev->num_tx_queues; i++) {
> -		ring = &priv->tx_rings[i];
> -		ec->use_adaptive_tx_coalesce |= ring->dim.use_dim;
> -	}
>   
>   	reg = rdma_readl(priv, RDMA_MBDONE_INTR);
>   
> @@ -632,7 +626,6 @@ static int bcm_sysport_set_coalesce(struct net_device *dev,
>   				    struct ethtool_coalesce *ec)
>   {
>   	struct bcm_sysport_priv *priv = netdev_priv(dev);
> -	struct bcm_sysport_tx_ring *ring;
>   	unsigned int i;
>   
>   	/* Base system clock is 125Mhz, DMA timeout is this reference clock
> @@ -646,20 +639,12 @@ static int bcm_sysport_set_coalesce(struct net_device *dev,
>   		return -EINVAL;
>   
>   	if ((ec->tx_coalesce_usecs == 0 && ec->tx_max_coalesced_frames == 0) ||
> -	    (ec->rx_coalesce_usecs == 0 && ec->rx_max_coalesced_frames == 0))
> +	    (ec->rx_coalesce_usecs == 0 && ec->rx_max_coalesced_frames == 0) ||
> +	    ec->use_adaptive_tx_coalesce)
>   		return -EINVAL;
>   
> -	for (i = 0; i < dev->num_tx_queues; i++) {
> -		ring = &priv->tx_rings[i];
> -		ring->dim.coal_pkts = ec->tx_max_coalesced_frames;
> -		ring->dim.coal_usecs = ec->tx_coalesce_usecs;
> -		if (!ec->use_adaptive_tx_coalesce && ring->dim.use_dim) {
> -			ring->dim.coal_pkts = 1;
> -			ring->dim.coal_usecs = 0;
> -		}
> -		ring->dim.use_dim = ec->use_adaptive_tx_coalesce;
> -		bcm_sysport_set_tx_coalesce(ring);
> -	}
> +	for (i = 0; i < dev->num_tx_queues; i++)
> +		bcm_sysport_set_tx_coalesce(&priv->tx_rings[i], ec);
>   
>   	priv->dim.coal_usecs = ec->rx_coalesce_usecs;
>   	priv->dim.coal_pkts = ec->rx_max_coalesced_frames;
> @@ -940,8 +925,6 @@ static unsigned int __bcm_sysport_tx_reclaim(struct bcm_sysport_priv *priv,
>   	ring->packets += pkts_compl;
>   	ring->bytes += bytes_compl;
>   	u64_stats_update_end(&priv->syncp);
> -	ring->dim.packets = pkts_compl;
> -	ring->dim.bytes = bytes_compl;
>   
>   	ring->c_index = c_index;
>   
> @@ -987,7 +970,6 @@ static int bcm_sysport_tx_poll(struct napi_struct *napi, int budget)
>   {
>   	struct bcm_sysport_tx_ring *ring =
>   		container_of(napi, struct bcm_sysport_tx_ring, napi);
> -	struct net_dim_sample dim_sample;
>   	unsigned int work_done = 0;
>   
>   	work_done = bcm_sysport_tx_reclaim(ring->priv, ring);
> @@ -1004,12 +986,6 @@ static int bcm_sysport_tx_poll(struct napi_struct *napi, int budget)
>   		return 0;
>   	}
>   
> -	if (ring->dim.use_dim) {
> -		net_dim_sample(ring->dim.event_ctr, ring->dim.packets,
> -			       ring->dim.bytes, &dim_sample);
> -		net_dim(&ring->dim.dim, dim_sample);
> -	}
> -
>   	return budget;
>   }
>   
> @@ -1089,23 +1065,6 @@ static void bcm_sysport_dim_work(struct work_struct *work)
>   	dim->state = NET_DIM_START_MEASURE;
>   }
>   
> -static void bcm_sysport_dim_tx_work(struct work_struct *work)
> -{
> -	struct net_dim *dim = container_of(work, struct net_dim, work);
> -	struct bcm_sysport_net_dim *ndim =
> -			container_of(dim, struct bcm_sysport_net_dim, dim);
> -	struct bcm_sysport_tx_ring *ring =
> -			container_of(ndim, struct bcm_sysport_tx_ring, dim);
> -	struct net_dim_cq_moder cur_profile =
> -				net_dim_get_profile(dim->mode, dim->profile_ix);
> -
> -	ring->dim.coal_usecs = cur_profile.usec;
> -	ring->dim.coal_pkts = cur_profile.pkts;
> -
> -	bcm_sysport_set_tx_coalesce(ring);
> -	dim->state = NET_DIM_START_MEASURE;
> -}
> -
>   /* RX and misc interrupt routine */
>   static irqreturn_t bcm_sysport_rx_isr(int irq, void *dev_id)
>   {
> @@ -1152,7 +1111,6 @@ static irqreturn_t bcm_sysport_rx_isr(int irq, void *dev_id)
>   			continue;
>   
>   		txr = &priv->tx_rings[ring];
> -		txr->dim.event_ctr++;
>   
>   		if (likely(napi_schedule_prep(&txr->napi))) {
>   			intrl2_0_mask_set(priv, ring_bit);
> @@ -1185,7 +1143,6 @@ static irqreturn_t bcm_sysport_tx_isr(int irq, void *dev_id)
>   			continue;
>   
>   		txr = &priv->tx_rings[ring];
> -		txr->dim.event_ctr++;
>   
>   		if (likely(napi_schedule_prep(&txr->napi))) {
>   			intrl2_1_mask_set(priv, BIT(ring));
> @@ -1551,7 +1508,6 @@ static int bcm_sysport_init_tx_ring(struct bcm_sysport_priv *priv,
>   	reg |= (1 << index);
>   	tdma_writel(priv, reg, TDMA_TIER1_ARB_0_QUEUE_EN);
>   
> -	bcm_sysport_init_dim(&ring->dim, bcm_sysport_dim_tx_work);
>   	napi_enable(&ring->napi);
>   
>   	netif_dbg(priv, hw, priv->netdev,
> @@ -1582,7 +1538,6 @@ static void bcm_sysport_fini_tx_ring(struct bcm_sysport_priv *priv,
>   		return;
>   
>   	napi_disable(&ring->napi);
> -	cancel_work_sync(&ring->dim.dim.work);
>   	netif_napi_del(&ring->napi);
>   
>   	bcm_sysport_tx_clean(priv, ring);
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.h b/drivers/net/ethernet/broadcom/bcmsysport.h
> index e1c97d4a82b4..57e18ef8f206 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.h
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.h
> @@ -723,7 +723,6 @@ struct bcm_sysport_tx_ring {
>   	struct bcm_sysport_priv *priv;	/* private context backpointer */
>   	unsigned long	packets;	/* packets statistics */
>   	unsigned long	bytes;		/* bytes statistics */
> -	struct bcm_sysport_net_dim dim;	/* Net DIM context */
>   	unsigned int	switch_queue;	/* switch port queue number */
>   	unsigned int	switch_port;	/* switch port queue number */
>   	bool		inspect;	/* inspect switch port and queue */
> 

Reviewed-by: Tal Gilboa <talgi@mellanox.com>

Re: [PATCH net-next 3/3] net: bcmgenet: Fix coalescing settings handling

[Tal Gilboa]

On 3/27/2018 10:47 PM, Florian Fainelli wrote:
> There were a number of issues with setting the RX coalescing parameters:
> 
> - we would not be preserving values that would have been configured
>    across close/open calls, instead we would always reset to no timeout
>    and 1 interrupt per packet, this would also prevent DIM from setting its
>    default usec/pkts values
> 
> - when adaptive RX would be turned on, we woud not be fetching the
>    default parameters, we would stay with no timeout/1 packet per interrupt
>    until the estimator kicks in and changes that
> 
> - finally disabling adaptive RX coalescing while providing parameters
>    would not be honored, and we would stay with whatever DIM had previously
>    determined instead of the user requested parameters
> 
> Fixes: 9f4ca05827a2 ("net: bcmgenet: Add support for adaptive RX coalescing")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>   drivers/net/ethernet/broadcom/genet/bcmgenet.c | 78 ++++++++++++++++++--------
>   drivers/net/ethernet/broadcom/genet/bcmgenet.h |  4 +-
>   2 files changed, 57 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 7db8edc643ec..76409debb796 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -625,18 +625,18 @@ static int bcmgenet_get_coalesce(struct net_device *dev,
>   	return 0;
>   }
>   
> -static void bcmgenet_set_rx_coalesce(struct bcmgenet_rx_ring *ring)
> +static void bcmgenet_set_rx_coalesce(struct bcmgenet_rx_ring *ring,
> +				     u32 usecs, u32 pkts)
>   {
>   	struct bcmgenet_priv *priv = ring->priv;
>   	unsigned int i = ring->index;
>   	u32 reg;
>   
> -	bcmgenet_rdma_ring_writel(priv, i, ring->dim.coal_pkts,
> -				  DMA_MBUF_DONE_THRESH);
> +	bcmgenet_rdma_ring_writel(priv, i, pkts, DMA_MBUF_DONE_THRESH);
>   
>   	reg = bcmgenet_rdma_readl(priv, DMA_RING0_TIMEOUT + i);
>   	reg &= ~DMA_TIMEOUT_MASK;
> -	reg |= DIV_ROUND_UP(ring->dim.coal_usecs * 1000, 8192);
> +	reg |= DIV_ROUND_UP(usecs * 1000, 8192);
>   	bcmgenet_rdma_writel(priv, reg, DMA_RING0_TIMEOUT + i);
>   }
>   
> @@ -645,6 +645,8 @@ static int bcmgenet_set_coalesce(struct net_device *dev,
>   {
>   	struct bcmgenet_priv *priv = netdev_priv(dev);
>   	struct bcmgenet_rx_ring *ring;
> +	struct net_dim_cq_moder moder;
> +	u32 usecs, pkts;
>   	unsigned int i;
>   
>   	/* Base system clock is 125Mhz, DMA timeout is this reference clock
> @@ -682,25 +684,37 @@ static int bcmgenet_set_coalesce(struct net_device *dev,
>   
>   	for (i = 0; i < priv->hw_params->rx_queues; i++) {
>   		ring = &priv->rx_rings[i];
> -		ring->dim.coal_usecs = ec->rx_coalesce_usecs;
> -		ring->dim.coal_pkts = ec->rx_max_coalesced_frames;
> -		if (!ec->use_adaptive_rx_coalesce && ring->dim.use_dim) {
> -			ring->dim.coal_pkts = 1;
> -			ring->dim.coal_usecs = 0;
> +
> +		ring->rx_coalesce_usecs = ec->rx_coalesce_usecs;
> +		ring->rx_max_coalesced_frames = ec->rx_max_coalesced_frames;
> +		usecs = ring->rx_coalesce_usecs;
> +		pkts = ring->rx_max_coalesced_frames;
> +
> +		if (ec->use_adaptive_rx_coalesce) {
> +			moder = net_dim_get_def_profile(ring->dim.dim.mode);
> +			usecs = moder.usec;
> +			pkts = moder.pkts;
>   		}

What if DIM is already in use? We don't want to set default values if 
DIM is already working.

> +
>   		ring->dim.use_dim = ec->use_adaptive_rx_coalesce;
> -		bcmgenet_set_rx_coalesce(ring);
> +		bcmgenet_set_rx_coalesce(ring, usecs, pkts);
>   	}
>   
>   	ring = &priv->rx_rings[DESC_INDEX];
> -	ring->dim.coal_usecs = ec->rx_coalesce_usecs;
> -	ring->dim.coal_pkts = ec->rx_max_coalesced_frames;
> -	if (!ec->use_adaptive_rx_coalesce && ring->dim.use_dim) {
> -		ring->dim.coal_pkts = 1;
> -		ring->dim.coal_usecs = 0;
> +
> +	ring->rx_coalesce_usecs = ec->rx_coalesce_usecs;
> +	ring->rx_max_coalesced_frames = ec->rx_max_coalesced_frames;
> +	usecs = ring->rx_coalesce_usecs;
> +	pkts = ring->rx_max_coalesced_frames;
> +
> +	if (ec->use_adaptive_rx_coalesce) {
> +		moder = net_dim_get_def_profile(ring->dim.dim.mode);
> +		usecs = moder.usec;
> +		pkts = moder.pkts;
>   	}

Same as above.

> +
>   	ring->dim.use_dim = ec->use_adaptive_rx_coalesce;
> -	bcmgenet_set_rx_coalesce(ring);
> +	bcmgenet_set_rx_coalesce(ring, usecs, pkts);
>   
>   	return 0;
>   }
> @@ -1924,10 +1938,7 @@ static void bcmgenet_dim_work(struct work_struct *work)
>   	struct net_dim_cq_moder cur_profile =
>   			net_dim_get_profile(dim->mode, dim->profile_ix);
>   
> -	ring->dim.coal_usecs = cur_profile.usec;
> -	ring->dim.coal_pkts = cur_profile.pkts;
> -
> -	bcmgenet_set_rx_coalesce(ring);
> +	bcmgenet_set_rx_coalesce(ring, cur_profile.usec, cur_profile.pkts);
>   	dim->state = NET_DIM_START_MEASURE;
>   }
>   
> @@ -2079,14 +2090,30 @@ static void init_umac(struct bcmgenet_priv *priv)
>   	dev_dbg(kdev, "done init umac\n");
>   }
>   
> -static void bcmgenet_init_dim(struct bcmgenet_net_dim *dim,
> +static void bcmgenet_init_dim(struct bcmgenet_rx_ring *ring,
>   			      void (*cb)(struct work_struct *work))
>   {
> +	struct bcmgenet_net_dim *dim = &ring->dim;
> +	struct net_dim_cq_moder moder;
> +	u32 usecs, pkts;
> +
>   	INIT_WORK(&dim->dim.work, cb);
>   	dim->dim.mode = NET_DIM_CQ_PERIOD_MODE_START_FROM_EQE;
>   	dim->event_ctr = 0;
>   	dim->packets = 0;
>   	dim->bytes = 0;

I would expect only dim related code in a function called init_dim(). I 
think the code below should be elsewhere. Or maybe change the function 
name to init_rx_coalesce().

> +
> +	usecs = ring->rx_coalesce_usecs;
> +	pkts = ring->rx_max_coalesced_frames;
> +
> +	/* If DIM was enabled, re-apply default parameters */
> +	if (dim->use_dim) {
> +		moder = net_dim_get_def_profile(dim->dim.mode);
> +		usecs = moder.usec;
> +		pkts = moder.pkts;
> +	}
> +
> +	bcmgenet_set_rx_coalesce(ring, usecs, pkts);
>   }
>   
>   /* Initialize a Tx ring along with corresponding hardware registers */
> @@ -2178,7 +2205,7 @@ static int bcmgenet_init_rx_ring(struct bcmgenet_priv *priv,
>   	if (ret)
>   		return ret;
>   
> -	bcmgenet_init_dim(&ring->dim, bcmgenet_dim_work);
> +	bcmgenet_init_dim(ring, bcmgenet_dim_work);
>   
>   	/* Initialize Rx NAPI */
>   	netif_napi_add(priv->dev, &ring->napi, bcmgenet_rx_poll,
> @@ -2186,7 +2213,6 @@ static int bcmgenet_init_rx_ring(struct bcmgenet_priv *priv,
>   
>   	bcmgenet_rdma_ring_writel(priv, index, 0, RDMA_PROD_INDEX);
>   	bcmgenet_rdma_ring_writel(priv, index, 0, RDMA_CONS_INDEX);
> -	bcmgenet_rdma_ring_writel(priv, index, 1, DMA_MBUF_DONE_THRESH);
>   	bcmgenet_rdma_ring_writel(priv, index,
>   				  ((size << DMA_RING_SIZE_SHIFT) |
>   				   RX_BUF_LENGTH), DMA_RING_BUF_SIZE);
> @@ -3424,6 +3450,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
>   	struct net_device *dev;
>   	const void *macaddr;
>   	struct resource *r;
> +	unsigned int i;
>   	int err = -EIO;
>   	const char *phy_mode_str;
>   
> @@ -3552,6 +3579,11 @@ static int bcmgenet_probe(struct platform_device *pdev)
>   	netif_set_real_num_tx_queues(priv->dev, priv->hw_params->tx_queues + 1);
>   	netif_set_real_num_rx_queues(priv->dev, priv->hw_params->rx_queues + 1);
>   
> +	/* Set default coalescing parameters */
> +	for (i = 0; i < priv->hw_params->rx_queues; i++)
> +		priv->rx_rings[i].rx_max_coalesced_frames = 1;
> +	priv->rx_rings[DESC_INDEX].rx_max_coalesced_frames = 1;
> +
>   	/* libphy will determine the link state */
>   	netif_carrier_off(dev);
>   
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> index 22c41e0430fb..b773bc07edf7 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> @@ -578,8 +578,6 @@ struct bcmgenet_net_dim {
>   	u16		event_ctr;
>   	unsigned long	packets;
>   	unsigned long	bytes;
> -	u32		coal_usecs;
> -	u32		coal_pkts;
>   	struct net_dim	dim;
>   };
>   
> @@ -598,6 +596,8 @@ struct bcmgenet_rx_ring {
>   	unsigned int	end_ptr;	/* Rx ring end CB ptr */
>   	unsigned int	old_discards;
>   	struct bcmgenet_net_dim dim;
> +	u32		rx_max_coalesced_frames;
> +	u32		rx_coalesce_usecs;
>   	void (*int_enable)(struct bcmgenet_rx_ring *);
>   	void (*int_disable)(struct bcmgenet_rx_ring *);
>   	struct bcmgenet_priv *priv;
> 

Re: [PATCH net-next 2/3] net: systemport: Fix coalescing settings handling

[Tal Gilboa]

On 3/27/2018 10:47 PM, Florian Fainelli wrote:
> There were a number of issues with setting the RX coalescing parameters:
> 
> - we would not be preserving values that would have been configured
>    across close/open calls, instead we would always reset to no timeout
>    and 1 interrupt per packet, this would also prevent DIM from setting its
>    default usec/pkts values
> 
> - when adaptive RX would be turned on, we woud not be fetching the
>    default parameters, we would stay with no timeout/1 packet per
>    interrupt until the estimator kicks in and changes that
> 
> - finally disabling adaptive RX coalescing while providing parameters
>    would not be honored, and we would stay with whatever DIM had
>    previously determined instead of the user requested parameters
> 
> Fixes: b6e0e875421e ("net: systemport: Implement adaptive interrupt coalescing")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>   drivers/net/ethernet/broadcom/bcmsysport.c | 57 ++++++++++++++++++++----------
>   drivers/net/ethernet/broadcom/bcmsysport.h |  4 +--
>   2 files changed, 41 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index 1e52bb7d822e..43ad6300c351 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -574,16 +574,16 @@ static int bcm_sysport_set_wol(struct net_device *dev,
>   	return 0;
>   }
>   
> -static void bcm_sysport_set_rx_coalesce(struct bcm_sysport_priv *priv)
> +static void bcm_sysport_set_rx_coalesce(struct bcm_sysport_priv *priv,
> +					u32 usecs, u32 pkts)
>   {
>   	u32 reg;
>   
>   	reg = rdma_readl(priv, RDMA_MBDONE_INTR);
>   	reg &= ~(RDMA_INTR_THRESH_MASK |
>   		 RDMA_TIMEOUT_MASK << RDMA_TIMEOUT_SHIFT);
> -	reg |= priv->dim.coal_pkts;
> -	reg |= DIV_ROUND_UP(priv->dim.coal_usecs * 1000, 8192) <<
> -			    RDMA_TIMEOUT_SHIFT;
> +	reg |= pkts;
> +	reg |= DIV_ROUND_UP(usecs * 1000, 8192) << RDMA_TIMEOUT_SHIFT;
>   	rdma_writel(priv, reg, RDMA_MBDONE_INTR);
>   }
>   
> @@ -626,6 +626,8 @@ static int bcm_sysport_set_coalesce(struct net_device *dev,
>   				    struct ethtool_coalesce *ec)
>   {
>   	struct bcm_sysport_priv *priv = netdev_priv(dev);
> +	struct net_dim_cq_moder moder;
> +	u32 usecs, pkts;
>   	unsigned int i;
>   
>   	/* Base system clock is 125Mhz, DMA timeout is this reference clock
> @@ -646,15 +648,22 @@ static int bcm_sysport_set_coalesce(struct net_device *dev,
>   	for (i = 0; i < dev->num_tx_queues; i++)
>   		bcm_sysport_set_tx_coalesce(&priv->tx_rings[i], ec);
>   
> -	priv->dim.coal_usecs = ec->rx_coalesce_usecs;
> -	priv->dim.coal_pkts = ec->rx_max_coalesced_frames;
> +	priv->rx_coalesce_usecs = ec->rx_coalesce_usecs;
> +	priv->rx_max_coalesced_frames = ec->rx_max_coalesced_frames;
> +	usecs = priv->rx_coalesce_usecs;
> +	pkts = priv->rx_max_coalesced_frames;
>   
> -	if (!ec->use_adaptive_rx_coalesce && priv->dim.use_dim) {
> -		priv->dim.coal_pkts = 1;
> -		priv->dim.coal_usecs = 0;
> +	/* If DIM is enabled, immediately obtain default parameters */
> +	if (ec->use_adaptive_rx_coalesce) {
> +		moder = net_dim_get_def_profile(priv->dim.dim.mode);
> +		usecs = moder.usec;
> +		pkts = moder.pkts;
>   	}

What if DIM is already in use? We don't want to set default values if 
DIM is already working.

> +
>   	priv->dim.use_dim = ec->use_adaptive_rx_coalesce;
> -	bcm_sysport_set_rx_coalesce(priv);
> +
> +	/* Apply desired coalescing parameters */
> +	bcm_sysport_set_rx_coalesce(priv, usecs, pkts);
>   
>   	return 0;
>   }
> @@ -1058,10 +1067,7 @@ static void bcm_sysport_dim_work(struct work_struct *work)
>   	struct net_dim_cq_moder cur_profile =
>   				net_dim_get_profile(dim->mode, dim->profile_ix);
>   
> -	priv->dim.coal_usecs = cur_profile.usec;
> -	priv->dim.coal_pkts = cur_profile.pkts;
> -
> -	bcm_sysport_set_rx_coalesce(priv);
> +	bcm_sysport_set_rx_coalesce(priv, cur_profile.usec, cur_profile.pkts);
>   	dim->state = NET_DIM_START_MEASURE;
>   }
>   
> @@ -1408,14 +1414,30 @@ static void bcm_sysport_adj_link(struct net_device *dev)
>   		phy_print_status(phydev);
>   }
>   
> -static void bcm_sysport_init_dim(struct bcm_sysport_net_dim *dim,
> +static void bcm_sysport_init_dim(struct bcm_sysport_priv *priv,
>   				 void (*cb)(struct work_struct *work))
>   {
> +	struct bcm_sysport_net_dim *dim = &priv->dim;
> +	struct net_dim_cq_moder moder;
> +	u32 usecs, pkts;
> +
>   	INIT_WORK(&dim->dim.work, cb);
>   	dim->dim.mode = NET_DIM_CQ_PERIOD_MODE_START_FROM_EQE;
>   	dim->event_ctr = 0;
>   	dim->packets = 0;
>   	dim->bytes = 0;

I would expect only dim related code in a function called init_dim(). I 
think the code below should be elsewhere. Or maybe change the function 
name to init_rx_coalesce().

> +
> +	usecs = priv->rx_coalesce_usecs;
> +	pkts = priv->rx_max_coalesced_frames;
> +
> +	/* If DIM was enabled, re-apply default parameters */
> +	if (dim->use_dim) {
> +		moder = net_dim_get_def_profile(dim->dim.mode);
> +		usecs = moder.usec;
> +		pkts = moder.pkts;
> +	}
> +
> +	bcm_sysport_set_rx_coalesce(priv, usecs, pkts);
>   }
>   
>   static int bcm_sysport_init_tx_ring(struct bcm_sysport_priv *priv,
> @@ -1658,8 +1680,6 @@ static int bcm_sysport_init_rx_ring(struct bcm_sysport_priv *priv)
>   	rdma_writel(priv, 0, RDMA_END_ADDR_HI);
>   	rdma_writel(priv, priv->num_rx_desc_words - 1, RDMA_END_ADDR_LO);
>   
> -	rdma_writel(priv, 1, RDMA_MBDONE_INTR);
> -
>   	netif_dbg(priv, hw, priv->netdev,
>   		  "RDMA cfg, num_rx_bds=%d, rx_bds=%p\n",
>   		  priv->num_rx_bds, priv->rx_bds);
> @@ -1827,7 +1847,7 @@ static void bcm_sysport_netif_start(struct net_device *dev)
>   	struct bcm_sysport_priv *priv = netdev_priv(dev);
>   
>   	/* Enable NAPI */
> -	bcm_sysport_init_dim(&priv->dim, bcm_sysport_dim_work);
> +	bcm_sysport_init_dim(priv, bcm_sysport_dim_work);
>   	napi_enable(&priv->napi);
>   
>   	/* Enable RX interrupt and TX ring full interrupt */
> @@ -2333,6 +2353,7 @@ static int bcm_sysport_probe(struct platform_device *pdev)
>   	/* libphy will adjust the link state accordingly */
>   	netif_carrier_off(dev);
>   
> +	priv->rx_max_coalesced_frames = 1;
>   	u64_stats_init(&priv->syncp);
>   
>   	priv->dsa_notifier.notifier_call = bcm_sysport_dsa_notifier;
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.h b/drivers/net/ethernet/broadcom/bcmsysport.h
> index 57e18ef8f206..d6e5d0cbf3a3 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.h
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.h
> @@ -701,8 +701,6 @@ struct bcm_sysport_net_dim {
>   	u16			event_ctr;
>   	unsigned long		packets;
>   	unsigned long		bytes;
> -	u32			coal_usecs;
> -	u32			coal_pkts;
>   	struct net_dim		dim;
>   };
>   
> @@ -755,6 +753,8 @@ struct bcm_sysport_priv {
>   	unsigned int		rx_c_index;
>   
>   	struct bcm_sysport_net_dim	dim;
> +	u32			rx_max_coalesced_frames;
> +	u32			rx_coalesce_usecs;
>   
>   	/* PHY device */
>   	struct device_node	*phy_dn;
> 

Re: [PATCH net-next 2/3] net: systemport: Fix coalescing settings handling

[Florian Fainelli]

On 03/28/2018 02:23 PM, Tal Gilboa wrote:
> On 3/27/2018 10:47 PM, Florian Fainelli wrote:
>> There were a number of issues with setting the RX coalescing parameters:
>>
>> - we would not be preserving values that would have been configured
>>    across close/open calls, instead we would always reset to no timeout
>>    and 1 interrupt per packet, this would also prevent DIM from
>> setting its
>>    default usec/pkts values
>>
>> - when adaptive RX would be turned on, we woud not be fetching the
>>    default parameters, we would stay with no timeout/1 packet per
>>    interrupt until the estimator kicks in and changes that
>>
>> - finally disabling adaptive RX coalescing while providing parameters
>>    would not be honored, and we would stay with whatever DIM had
>>    previously determined instead of the user requested parameters
>>
>> Fixes: b6e0e875421e ("net: systemport: Implement adaptive interrupt
>> coalescing")
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>   drivers/net/ethernet/broadcom/bcmsysport.c | 57
>> ++++++++++++++++++++----------
>>   drivers/net/ethernet/broadcom/bcmsysport.h |  4 +--
>>   2 files changed, 41 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c
>> b/drivers/net/ethernet/broadcom/bcmsysport.c
>> index 1e52bb7d822e..43ad6300c351 100644
>> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
>> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
>> @@ -574,16 +574,16 @@ static int bcm_sysport_set_wol(struct net_device
>> *dev,
>>       return 0;
>>   }
>>   -static void bcm_sysport_set_rx_coalesce(struct bcm_sysport_priv *priv)
>> +static void bcm_sysport_set_rx_coalesce(struct bcm_sysport_priv *priv,
>> +                    u32 usecs, u32 pkts)
>>   {
>>       u32 reg;
>>         reg = rdma_readl(priv, RDMA_MBDONE_INTR);
>>       reg &= ~(RDMA_INTR_THRESH_MASK |
>>            RDMA_TIMEOUT_MASK << RDMA_TIMEOUT_SHIFT);
>> -    reg |= priv->dim.coal_pkts;
>> -    reg |= DIV_ROUND_UP(priv->dim.coal_usecs * 1000, 8192) <<
>> -                RDMA_TIMEOUT_SHIFT;
>> +    reg |= pkts;
>> +    reg |= DIV_ROUND_UP(usecs * 1000, 8192) << RDMA_TIMEOUT_SHIFT;
>>       rdma_writel(priv, reg, RDMA_MBDONE_INTR);
>>   }
>>   @@ -626,6 +626,8 @@ static int bcm_sysport_set_coalesce(struct
>> net_device *dev,
>>                       struct ethtool_coalesce *ec)
>>   {
>>       struct bcm_sysport_priv *priv = netdev_priv(dev);
>> +    struct net_dim_cq_moder moder;
>> +    u32 usecs, pkts;
>>       unsigned int i;
>>         /* Base system clock is 125Mhz, DMA timeout is this reference
>> clock
>> @@ -646,15 +648,22 @@ static int bcm_sysport_set_coalesce(struct
>> net_device *dev,
>>       for (i = 0; i < dev->num_tx_queues; i++)
>>           bcm_sysport_set_tx_coalesce(&priv->tx_rings[i], ec);
>>   -    priv->dim.coal_usecs = ec->rx_coalesce_usecs;
>> -    priv->dim.coal_pkts = ec->rx_max_coalesced_frames;
>> +    priv->rx_coalesce_usecs = ec->rx_coalesce_usecs;
>> +    priv->rx_max_coalesced_frames = ec->rx_max_coalesced_frames;
>> +    usecs = priv->rx_coalesce_usecs;
>> +    pkts = priv->rx_max_coalesced_frames;
>>   -    if (!ec->use_adaptive_rx_coalesce && priv->dim.use_dim) {
>> -        priv->dim.coal_pkts = 1;
>> -        priv->dim.coal_usecs = 0;
>> +    /* If DIM is enabled, immediately obtain default parameters */
>> +    if (ec->use_adaptive_rx_coalesce) {
>> +        moder = net_dim_get_def_profile(priv->dim.dim.mode);
>> +        usecs = moder.usec;
>> +        pkts = moder.pkts;
>>       }
> 
> What if DIM is already in use? We don't want to set default values if
> DIM is already working.

Indeed, good catch, thank you!

> 
>> +
>>       priv->dim.use_dim = ec->use_adaptive_rx_coalesce;
>> -    bcm_sysport_set_rx_coalesce(priv);
>> +
>> +    /* Apply desired coalescing parameters */
>> +    bcm_sysport_set_rx_coalesce(priv, usecs, pkts);
>>         return 0;
>>   }
>> @@ -1058,10 +1067,7 @@ static void bcm_sysport_dim_work(struct
>> work_struct *work)
>>       struct net_dim_cq_moder cur_profile =
>>                   net_dim_get_profile(dim->mode, dim->profile_ix);
>>   -    priv->dim.coal_usecs = cur_profile.usec;
>> -    priv->dim.coal_pkts = cur_profile.pkts;
>> -
>> -    bcm_sysport_set_rx_coalesce(priv);
>> +    bcm_sysport_set_rx_coalesce(priv, cur_profile.usec,
>> cur_profile.pkts);
>>       dim->state = NET_DIM_START_MEASURE;
>>   }
>>   @@ -1408,14 +1414,30 @@ static void bcm_sysport_adj_link(struct
>> net_device *dev)
>>           phy_print_status(phydev);
>>   }
>>   -static void bcm_sysport_init_dim(struct bcm_sysport_net_dim *dim,
>> +static void bcm_sysport_init_dim(struct bcm_sysport_priv *priv,
>>                    void (*cb)(struct work_struct *work))
>>   {
>> +    struct bcm_sysport_net_dim *dim = &priv->dim;
>> +    struct net_dim_cq_moder moder;
>> +    u32 usecs, pkts;
>> +
>>       INIT_WORK(&dim->dim.work, cb);
>>       dim->dim.mode = NET_DIM_CQ_PERIOD_MODE_START_FROM_EQE;
>>       dim->event_ctr = 0;
>>       dim->packets = 0;
>>       dim->bytes = 0;
> 
> I would expect only dim related code in a function called init_dim(). I
> think the code below should be elsewhere. Or maybe change the function
> name to init_rx_coalesce().

Sure, I can definitively do split the initialization, that makes sense.
-- 
Florian

Re: [PATCH net-next 3/3] net: bcmgenet: Fix coalescing settings handling

[Doug Berger]

On 03/27/2018 12:47 PM, Florian Fainelli wrote:
> There were a number of issues with setting the RX coalescing parameters:
> 
> - we would not be preserving values that would have been configured
>   across close/open calls, instead we would always reset to no timeout
>   and 1 interrupt per packet, this would also prevent DIM from setting its
>   default usec/pkts values
> 
> - when adaptive RX would be turned on, we woud not be fetching the
>   default parameters, we would stay with no timeout/1 packet per interrupt
>   until the estimator kicks in and changes that
> 
> - finally disabling adaptive RX coalescing while providing parameters
>   would not be honored, and we would stay with whatever DIM had previously
>   determined instead of the user requested parameters
> 
> Fixes: 9f4ca05827a2 ("net: bcmgenet: Add support for adaptive RX coalescing")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 78 ++++++++++++++++++--------
>  drivers/net/ethernet/broadcom/genet/bcmgenet.h |  4 +-
>  2 files changed, 57 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 7db8edc643ec..76409debb796 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -625,18 +625,18 @@ static int bcmgenet_get_coalesce(struct net_device *dev,
>  	return 0;
>  }
>  
> -static void bcmgenet_set_rx_coalesce(struct bcmgenet_rx_ring *ring)
> +static void bcmgenet_set_rx_coalesce(struct bcmgenet_rx_ring *ring,
> +				     u32 usecs, u32 pkts)
>  {
>  	struct bcmgenet_priv *priv = ring->priv;
>  	unsigned int i = ring->index;
>  	u32 reg;
>  
> -	bcmgenet_rdma_ring_writel(priv, i, ring->dim.coal_pkts,
> -				  DMA_MBUF_DONE_THRESH);
> +	bcmgenet_rdma_ring_writel(priv, i, pkts, DMA_MBUF_DONE_THRESH);
>  
>  	reg = bcmgenet_rdma_readl(priv, DMA_RING0_TIMEOUT + i);
>  	reg &= ~DMA_TIMEOUT_MASK;
> -	reg |= DIV_ROUND_UP(ring->dim.coal_usecs * 1000, 8192);
> +	reg |= DIV_ROUND_UP(usecs * 1000, 8192);
>  	bcmgenet_rdma_writel(priv, reg, DMA_RING0_TIMEOUT + i);
>  }
>  
> @@ -645,6 +645,8 @@ static int bcmgenet_set_coalesce(struct net_device *dev,
>  {
>  	struct bcmgenet_priv *priv = netdev_priv(dev);
>  	struct bcmgenet_rx_ring *ring;
> +	struct net_dim_cq_moder moder;
> +	u32 usecs, pkts;
>  	unsigned int i;
>  
>  	/* Base system clock is 125Mhz, DMA timeout is this reference clock
> @@ -682,25 +684,37 @@ static int bcmgenet_set_coalesce(struct net_device *dev,
>  
>  	for (i = 0; i < priv->hw_params->rx_queues; i++) {
>  		ring = &priv->rx_rings[i];
> -		ring->dim.coal_usecs = ec->rx_coalesce_usecs;
> -		ring->dim.coal_pkts = ec->rx_max_coalesced_frames;
> -		if (!ec->use_adaptive_rx_coalesce && ring->dim.use_dim) {
> -			ring->dim.coal_pkts = 1;
> -			ring->dim.coal_usecs = 0;
> +
> +		ring->rx_coalesce_usecs = ec->rx_coalesce_usecs;
> +		ring->rx_max_coalesced_frames = ec->rx_max_coalesced_frames;
> +		usecs = ring->rx_coalesce_usecs;
> +		pkts = ring->rx_max_coalesced_frames;
> +
> +		if (ec->use_adaptive_rx_coalesce) {
> +			moder = net_dim_get_def_profile(ring->dim.dim.mode);
> +			usecs = moder.usec;
> +			pkts = moder.pkts;
>  		}
> +
>  		ring->dim.use_dim = ec->use_adaptive_rx_coalesce;
> -		bcmgenet_set_rx_coalesce(ring);
> +		bcmgenet_set_rx_coalesce(ring, usecs, pkts);
You might want to put this loop code in a separate function with ring
and ec parameters

>  	}
>  
>  	ring = &priv->rx_rings[DESC_INDEX];
> -	ring->dim.coal_usecs = ec->rx_coalesce_usecs;
> -	ring->dim.coal_pkts = ec->rx_max_coalesced_frames;
> -	if (!ec->use_adaptive_rx_coalesce && ring->dim.use_dim) {
> -		ring->dim.coal_pkts = 1;
> -		ring->dim.coal_usecs = 0;
> +
> +	ring->rx_coalesce_usecs = ec->rx_coalesce_usecs;
> +	ring->rx_max_coalesced_frames = ec->rx_max_coalesced_frames;
> +	usecs = ring->rx_coalesce_usecs;
> +	pkts = ring->rx_max_coalesced_frames;
> +
> +	if (ec->use_adaptive_rx_coalesce) {
> +		moder = net_dim_get_def_profile(ring->dim.dim.mode);
> +		usecs = moder.usec;
> +		pkts = moder.pkts;
>  	}
> +
>  	ring->dim.use_dim = ec->use_adaptive_rx_coalesce;
> -	bcmgenet_set_rx_coalesce(ring);
> +	bcmgenet_set_rx_coalesce(ring, usecs, pkts);
so you don't need to repeat it here. (just for maintenance reasons)

>  
>  	return 0;
>  }
> @@ -1924,10 +1938,7 @@ static void bcmgenet_dim_work(struct work_struct *work)
>  	struct net_dim_cq_moder cur_profile =
>  			net_dim_get_profile(dim->mode, dim->profile_ix);
>  
> -	ring->dim.coal_usecs = cur_profile.usec;
> -	ring->dim.coal_pkts = cur_profile.pkts;
> -
> -	bcmgenet_set_rx_coalesce(ring);
> +	bcmgenet_set_rx_coalesce(ring, cur_profile.usec, cur_profile.pkts);
>  	dim->state = NET_DIM_START_MEASURE;
>  }
>  
> @@ -2079,14 +2090,30 @@ static void init_umac(struct bcmgenet_priv *priv)
>  	dev_dbg(kdev, "done init umac\n");
>  }
>  
> -static void bcmgenet_init_dim(struct bcmgenet_net_dim *dim,
> +static void bcmgenet_init_dim(struct bcmgenet_rx_ring *ring,
>  			      void (*cb)(struct work_struct *work))
>  {
> +	struct bcmgenet_net_dim *dim = &ring->dim;
> +	struct net_dim_cq_moder moder;
> +	u32 usecs, pkts;
> +
>  	INIT_WORK(&dim->dim.work, cb);
>  	dim->dim.mode = NET_DIM_CQ_PERIOD_MODE_START_FROM_EQE;
>  	dim->event_ctr = 0;
>  	dim->packets = 0;
>  	dim->bytes = 0;
> +
> +	usecs = ring->rx_coalesce_usecs;
> +	pkts = ring->rx_max_coalesced_frames;
> +
> +	/* If DIM was enabled, re-apply default parameters */
> +	if (dim->use_dim) {
> +		moder = net_dim_get_def_profile(dim->dim.mode);
> +		usecs = moder.usec;
> +		pkts = moder.pkts;
> +	}
> +
> +	bcmgenet_set_rx_coalesce(ring, usecs, pkts);
>  }
>  
>  /* Initialize a Tx ring along with corresponding hardware registers */
> @@ -2178,7 +2205,7 @@ static int bcmgenet_init_rx_ring(struct bcmgenet_priv *priv,
>  	if (ret)
>  		return ret;
>  
> -	bcmgenet_init_dim(&ring->dim, bcmgenet_dim_work);
> +	bcmgenet_init_dim(ring, bcmgenet_dim_work);
>  
>  	/* Initialize Rx NAPI */
>  	netif_napi_add(priv->dev, &ring->napi, bcmgenet_rx_poll,
> @@ -2186,7 +2213,6 @@ static int bcmgenet_init_rx_ring(struct bcmgenet_priv *priv,
>  
>  	bcmgenet_rdma_ring_writel(priv, index, 0, RDMA_PROD_INDEX);
>  	bcmgenet_rdma_ring_writel(priv, index, 0, RDMA_CONS_INDEX);
> -	bcmgenet_rdma_ring_writel(priv, index, 1, DMA_MBUF_DONE_THRESH);
>  	bcmgenet_rdma_ring_writel(priv, index,
>  				  ((size << DMA_RING_SIZE_SHIFT) |
>  				   RX_BUF_LENGTH), DMA_RING_BUF_SIZE);
> @@ -3424,6 +3450,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
>  	struct net_device *dev;
>  	const void *macaddr;
>  	struct resource *r;
> +	unsigned int i;
>  	int err = -EIO;
>  	const char *phy_mode_str;
>  
> @@ -3552,6 +3579,11 @@ static int bcmgenet_probe(struct platform_device *pdev)
>  	netif_set_real_num_tx_queues(priv->dev, priv->hw_params->tx_queues + 1);
>  	netif_set_real_num_rx_queues(priv->dev, priv->hw_params->rx_queues + 1);
>  
> +	/* Set default coalescing parameters */
> +	for (i = 0; i < priv->hw_params->rx_queues; i++)
> +		priv->rx_rings[i].rx_max_coalesced_frames = 1;
> +	priv->rx_rings[DESC_INDEX].rx_max_coalesced_frames = 1;
> +
>  	/* libphy will determine the link state */
>  	netif_carrier_off(dev);
>  
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> index 22c41e0430fb..b773bc07edf7 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> @@ -578,8 +578,6 @@ struct bcmgenet_net_dim {
>  	u16		event_ctr;
>  	unsigned long	packets;
>  	unsigned long	bytes;
> -	u32		coal_usecs;
> -	u32		coal_pkts;
>  	struct net_dim	dim;
>  };
>  
> @@ -598,6 +596,8 @@ struct bcmgenet_rx_ring {
>  	unsigned int	end_ptr;	/* Rx ring end CB ptr */
>  	unsigned int	old_discards;
>  	struct bcmgenet_net_dim dim;
> +	u32		rx_max_coalesced_frames;
> +	u32		rx_coalesce_usecs;
>  	void (*int_enable)(struct bcmgenet_rx_ring *);
>  	void (*int_disable)(struct bcmgenet_rx_ring *);
>  	struct bcmgenet_priv *priv;
> 

Re: [PATCH net-next 3/3] net: bcmgenet: Fix coalescing settings handling

[Doug Berger]

On 03/29/2018 10:51 AM, Doug Berger wrote:
> On 03/27/2018 12:47 PM, Florian Fainelli wrote:
>> There were a number of issues with setting the RX coalescing parameters:
>>
>> - we would not be preserving values that would have been configured
>>   across close/open calls, instead we would always reset to no timeout
>>   and 1 interrupt per packet, this would also prevent DIM from setting its
>>   default usec/pkts values
>>
>> - when adaptive RX would be turned on, we woud not be fetching the
>>   default parameters, we would stay with no timeout/1 packet per interrupt
>>   until the estimator kicks in and changes that
>>
>> - finally disabling adaptive RX coalescing while providing parameters
>>   would not be honored, and we would stay with whatever DIM had previously
>>   determined instead of the user requested parameters
>>
>> Fixes: 9f4ca05827a2 ("net: bcmgenet: Add support for adaptive RX coalescing")
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 78 ++++++++++++++++++--------
>>  drivers/net/ethernet/broadcom/genet/bcmgenet.h |  4 +-
>>  2 files changed, 57 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> index 7db8edc643ec..76409debb796 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> @@ -625,18 +625,18 @@ static int bcmgenet_get_coalesce(struct net_device *dev,
>>  	return 0;
>>  }
>>  
>> -static void bcmgenet_set_rx_coalesce(struct bcmgenet_rx_ring *ring)
>> +static void bcmgenet_set_rx_coalesce(struct bcmgenet_rx_ring *ring,
>> +				     u32 usecs, u32 pkts)
>>  {
>>  	struct bcmgenet_priv *priv = ring->priv;
>>  	unsigned int i = ring->index;
>>  	u32 reg;
>>  
>> -	bcmgenet_rdma_ring_writel(priv, i, ring->dim.coal_pkts,
>> -				  DMA_MBUF_DONE_THRESH);
>> +	bcmgenet_rdma_ring_writel(priv, i, pkts, DMA_MBUF_DONE_THRESH);
>>  
>>  	reg = bcmgenet_rdma_readl(priv, DMA_RING0_TIMEOUT + i);
>>  	reg &= ~DMA_TIMEOUT_MASK;
>> -	reg |= DIV_ROUND_UP(ring->dim.coal_usecs * 1000, 8192);
>> +	reg |= DIV_ROUND_UP(usecs * 1000, 8192);
>>  	bcmgenet_rdma_writel(priv, reg, DMA_RING0_TIMEOUT + i);
>>  }
>>  
>> @@ -645,6 +645,8 @@ static int bcmgenet_set_coalesce(struct net_device *dev,
>>  {
>>  	struct bcmgenet_priv *priv = netdev_priv(dev);
>>  	struct bcmgenet_rx_ring *ring;
>> +	struct net_dim_cq_moder moder;
>> +	u32 usecs, pkts;
>>  	unsigned int i;
>>  
>>  	/* Base system clock is 125Mhz, DMA timeout is this reference clock
>> @@ -682,25 +684,37 @@ static int bcmgenet_set_coalesce(struct net_device *dev,
>>  
>>  	for (i = 0; i < priv->hw_params->rx_queues; i++) {
>>  		ring = &priv->rx_rings[i];
>> -		ring->dim.coal_usecs = ec->rx_coalesce_usecs;
>> -		ring->dim.coal_pkts = ec->rx_max_coalesced_frames;
>> -		if (!ec->use_adaptive_rx_coalesce && ring->dim.use_dim) {
>> -			ring->dim.coal_pkts = 1;
>> -			ring->dim.coal_usecs = 0;
>> +
>> +		ring->rx_coalesce_usecs = ec->rx_coalesce_usecs;
>> +		ring->rx_max_coalesced_frames = ec->rx_max_coalesced_frames;
>> +		usecs = ring->rx_coalesce_usecs;
>> +		pkts = ring->rx_max_coalesced_frames;
>> +
>> +		if (ec->use_adaptive_rx_coalesce) {
>> +			moder = net_dim_get_def_profile(ring->dim.dim.mode);
>> +			usecs = moder.usec;
>> +			pkts = moder.pkts;
>>  		}
>> +
>>  		ring->dim.use_dim = ec->use_adaptive_rx_coalesce;
>> -		bcmgenet_set_rx_coalesce(ring);
>> +		bcmgenet_set_rx_coalesce(ring, usecs, pkts);
> You might want to put this loop code in a separate function with ring
> and ec parameters
> 
>>  	}
>>  
>>  	ring = &priv->rx_rings[DESC_INDEX];
>> -	ring->dim.coal_usecs = ec->rx_coalesce_usecs;
>> -	ring->dim.coal_pkts = ec->rx_max_coalesced_frames;
>> -	if (!ec->use_adaptive_rx_coalesce && ring->dim.use_dim) {
>> -		ring->dim.coal_pkts = 1;
>> -		ring->dim.coal_usecs = 0;
>> +
>> +	ring->rx_coalesce_usecs = ec->rx_coalesce_usecs;
>> +	ring->rx_max_coalesced_frames = ec->rx_max_coalesced_frames;
>> +	usecs = ring->rx_coalesce_usecs;
>> +	pkts = ring->rx_max_coalesced_frames;
>> +
>> +	if (ec->use_adaptive_rx_coalesce) {
>> +		moder = net_dim_get_def_profile(ring->dim.dim.mode);
>> +		usecs = moder.usec;
>> +		pkts = moder.pkts;
>>  	}
>> +
>>  	ring->dim.use_dim = ec->use_adaptive_rx_coalesce;
>> -	bcmgenet_set_rx_coalesce(ring);
>> +	bcmgenet_set_rx_coalesce(ring, usecs, pkts);
> so you don't need to repeat it here. (just for maintenance reasons)
Looks like you already did in V2 :)!

I should read my mail more frequently ;).

> 
>>  
>>  	return 0;
>>  }
>> @@ -1924,10 +1938,7 @@ static void bcmgenet_dim_work(struct work_struct *work)
>>  	struct net_dim_cq_moder cur_profile =
>>  			net_dim_get_profile(dim->mode, dim->profile_ix);
>>  
>> -	ring->dim.coal_usecs = cur_profile.usec;
>> -	ring->dim.coal_pkts = cur_profile.pkts;
>> -
>> -	bcmgenet_set_rx_coalesce(ring);
>> +	bcmgenet_set_rx_coalesce(ring, cur_profile.usec, cur_profile.pkts);
>>  	dim->state = NET_DIM_START_MEASURE;
>>  }
>>  
>> @@ -2079,14 +2090,30 @@ static void init_umac(struct bcmgenet_priv *priv)
>>  	dev_dbg(kdev, "done init umac\n");
>>  }
>>  
>> -static void bcmgenet_init_dim(struct bcmgenet_net_dim *dim,
>> +static void bcmgenet_init_dim(struct bcmgenet_rx_ring *ring,
>>  			      void (*cb)(struct work_struct *work))
>>  {
>> +	struct bcmgenet_net_dim *dim = &ring->dim;
>> +	struct net_dim_cq_moder moder;
>> +	u32 usecs, pkts;
>> +
>>  	INIT_WORK(&dim->dim.work, cb);
>>  	dim->dim.mode = NET_DIM_CQ_PERIOD_MODE_START_FROM_EQE;
>>  	dim->event_ctr = 0;
>>  	dim->packets = 0;
>>  	dim->bytes = 0;
>> +
>> +	usecs = ring->rx_coalesce_usecs;
>> +	pkts = ring->rx_max_coalesced_frames;
>> +
>> +	/* If DIM was enabled, re-apply default parameters */
>> +	if (dim->use_dim) {
>> +		moder = net_dim_get_def_profile(dim->dim.mode);
>> +		usecs = moder.usec;
>> +		pkts = moder.pkts;
>> +	}
>> +
>> +	bcmgenet_set_rx_coalesce(ring, usecs, pkts);
>>  }
>>  
>>  /* Initialize a Tx ring along with corresponding hardware registers */
>> @@ -2178,7 +2205,7 @@ static int bcmgenet_init_rx_ring(struct bcmgenet_priv *priv,
>>  	if (ret)
>>  		return ret;
>>  
>> -	bcmgenet_init_dim(&ring->dim, bcmgenet_dim_work);
>> +	bcmgenet_init_dim(ring, bcmgenet_dim_work);
>>  
>>  	/* Initialize Rx NAPI */
>>  	netif_napi_add(priv->dev, &ring->napi, bcmgenet_rx_poll,
>> @@ -2186,7 +2213,6 @@ static int bcmgenet_init_rx_ring(struct bcmgenet_priv *priv,
>>  
>>  	bcmgenet_rdma_ring_writel(priv, index, 0, RDMA_PROD_INDEX);
>>  	bcmgenet_rdma_ring_writel(priv, index, 0, RDMA_CONS_INDEX);
>> -	bcmgenet_rdma_ring_writel(priv, index, 1, DMA_MBUF_DONE_THRESH);
>>  	bcmgenet_rdma_ring_writel(priv, index,
>>  				  ((size << DMA_RING_SIZE_SHIFT) |
>>  				   RX_BUF_LENGTH), DMA_RING_BUF_SIZE);
>> @@ -3424,6 +3450,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
>>  	struct net_device *dev;
>>  	const void *macaddr;
>>  	struct resource *r;
>> +	unsigned int i;
>>  	int err = -EIO;
>>  	const char *phy_mode_str;
>>  
>> @@ -3552,6 +3579,11 @@ static int bcmgenet_probe(struct platform_device *pdev)
>>  	netif_set_real_num_tx_queues(priv->dev, priv->hw_params->tx_queues + 1);
>>  	netif_set_real_num_rx_queues(priv->dev, priv->hw_params->rx_queues + 1);
>>  
>> +	/* Set default coalescing parameters */
>> +	for (i = 0; i < priv->hw_params->rx_queues; i++)
>> +		priv->rx_rings[i].rx_max_coalesced_frames = 1;
>> +	priv->rx_rings[DESC_INDEX].rx_max_coalesced_frames = 1;
>> +
>>  	/* libphy will determine the link state */
>>  	netif_carrier_off(dev);
>>  
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
>> index 22c41e0430fb..b773bc07edf7 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
>> @@ -578,8 +578,6 @@ struct bcmgenet_net_dim {
>>  	u16		event_ctr;
>>  	unsigned long	packets;
>>  	unsigned long	bytes;
>> -	u32		coal_usecs;
>> -	u32		coal_pkts;
>>  	struct net_dim	dim;
>>  };
>>  
>> @@ -598,6 +596,8 @@ struct bcmgenet_rx_ring {
>>  	unsigned int	end_ptr;	/* Rx ring end CB ptr */
>>  	unsigned int	old_discards;
>>  	struct bcmgenet_net_dim dim;
>> +	u32		rx_max_coalesced_frames;
>> +	u32		rx_coalesce_usecs;
>>  	void (*int_enable)(struct bcmgenet_rx_ring *);
>>  	void (*int_disable)(struct bcmgenet_rx_ring *);
>>  	struct bcmgenet_priv *priv;
>>
> 

Re: [PATCH net-next 3/3] net: bcmgenet: Fix coalescing settings handling

[Florian Fainelli]

On 03/29/2018 10:51 AM, Doug Berger wrote:
> On 03/27/2018 12:47 PM, Florian Fainelli wrote:
>> There were a number of issues with setting the RX coalescing parameters:
>>
>> - we would not be preserving values that would have been configured
>>   across close/open calls, instead we would always reset to no timeout
>>   and 1 interrupt per packet, this would also prevent DIM from setting its
>>   default usec/pkts values
>>
>> - when adaptive RX would be turned on, we woud not be fetching the
>>   default parameters, we would stay with no timeout/1 packet per interrupt
>>   until the estimator kicks in and changes that
>>
>> - finally disabling adaptive RX coalescing while providing parameters
>>   would not be honored, and we would stay with whatever DIM had previously
>>   determined instead of the user requested parameters
>>
>> Fixes: 9f4ca05827a2 ("net: bcmgenet: Add support for adaptive RX coalescing")
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 78 ++++++++++++++++++--------
>>  drivers/net/ethernet/broadcom/genet/bcmgenet.h |  4 +-
>>  2 files changed, 57 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> index 7db8edc643ec..76409debb796 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> @@ -625,18 +625,18 @@ static int bcmgenet_get_coalesce(struct net_device *dev,
>>  	return 0;
>>  }
>>  
>> -static void bcmgenet_set_rx_coalesce(struct bcmgenet_rx_ring *ring)
>> +static void bcmgenet_set_rx_coalesce(struct bcmgenet_rx_ring *ring,
>> +				     u32 usecs, u32 pkts)
>>  {
>>  	struct bcmgenet_priv *priv = ring->priv;
>>  	unsigned int i = ring->index;
>>  	u32 reg;
>>  
>> -	bcmgenet_rdma_ring_writel(priv, i, ring->dim.coal_pkts,
>> -				  DMA_MBUF_DONE_THRESH);
>> +	bcmgenet_rdma_ring_writel(priv, i, pkts, DMA_MBUF_DONE_THRESH);
>>  
>>  	reg = bcmgenet_rdma_readl(priv, DMA_RING0_TIMEOUT + i);
>>  	reg &= ~DMA_TIMEOUT_MASK;
>> -	reg |= DIV_ROUND_UP(ring->dim.coal_usecs * 1000, 8192);
>> +	reg |= DIV_ROUND_UP(usecs * 1000, 8192);
>>  	bcmgenet_rdma_writel(priv, reg, DMA_RING0_TIMEOUT + i);
>>  }
>>  
>> @@ -645,6 +645,8 @@ static int bcmgenet_set_coalesce(struct net_device *dev,
>>  {
>>  	struct bcmgenet_priv *priv = netdev_priv(dev);
>>  	struct bcmgenet_rx_ring *ring;
>> +	struct net_dim_cq_moder moder;
>> +	u32 usecs, pkts;
>>  	unsigned int i;
>>  
>>  	/* Base system clock is 125Mhz, DMA timeout is this reference clock
>> @@ -682,25 +684,37 @@ static int bcmgenet_set_coalesce(struct net_device *dev,
>>  
>>  	for (i = 0; i < priv->hw_params->rx_queues; i++) {
>>  		ring = &priv->rx_rings[i];
>> -		ring->dim.coal_usecs = ec->rx_coalesce_usecs;
>> -		ring->dim.coal_pkts = ec->rx_max_coalesced_frames;
>> -		if (!ec->use_adaptive_rx_coalesce && ring->dim.use_dim) {
>> -			ring->dim.coal_pkts = 1;
>> -			ring->dim.coal_usecs = 0;
>> +
>> +		ring->rx_coalesce_usecs = ec->rx_coalesce_usecs;
>> +		ring->rx_max_coalesced_frames = ec->rx_max_coalesced_frames;
>> +		usecs = ring->rx_coalesce_usecs;
>> +		pkts = ring->rx_max_coalesced_frames;
>> +
>> +		if (ec->use_adaptive_rx_coalesce) {
>> +			moder = net_dim_get_def_profile(ring->dim.dim.mode);
>> +			usecs = moder.usec;
>> +			pkts = moder.pkts;
>>  		}
>> +
>>  		ring->dim.use_dim = ec->use_adaptive_rx_coalesce;
>> -		bcmgenet_set_rx_coalesce(ring);
>> +		bcmgenet_set_rx_coalesce(ring, usecs, pkts);
> You might want to put this loop code in a separate function with ring
> and ec parameters

Indeed, that also itched me, v2 has this in a function:

http://patchwork.ozlabs.org/patch/892509/

(gmail coalescing threads, oh oh oh, what a good pun).

Thanks!
-- 
Florian