[PATCH net-next v1 0/7] Fix, cleanup, optimize gfar_private and the Rx path

[PATCH net-next v1 0/7] Fix, cleanup, optimize gfar_private and the Rx path

[Claudiu Manoil]

Cc: Paul Gortmaker <paul.gortmaker@windriver.com>,
  "David S. Miller" <davem@davemloft.net>


Hi Dave,
These patches went already through a review round with Paul.
Changes since the initial version of the patches:
* split the 1st patch into two patches (01 & 02, as suggested by Paul)
* added the suggested trivial cleanup patch:
  gianfar: gfar_process_frame returns void
* provided "long logs" with detailed comments and explanations

Thank you.

Regards,
Claudiu


Claudiu Manoil (7):
  gianfar: Remove unused device_node ref in gfar_private
  gianfar: Add device ref (dev) in gfar_private
  gianfar: Cleanup and optimize struct gfar_private
  gianfar: GRO_DROP is unlikely
  gianfar: gfar_process_frame returns void
  gianfar: Remove wrong buffer size conditioning to VLAN h/w offload
  gianfar: Fix and cleanup Rx FCB indication

 drivers/net/ethernet/freescale/gianfar.c |   85 ++++++++++++---------------
 drivers/net/ethernet/freescale/gianfar.h |   96 +++++++++++++++--------------
 2 files changed, 88 insertions(+), 93 deletions(-)

-- 
1.7.7.4

[PATCH net-next 1/7] gianfar: Remove unused device_node ref in gfar_private

[Claudiu Manoil]

Remove unused device node pointer.
Remove duplicated SET_NETDEV_DEV().

Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>
---
 drivers/net/ethernet/freescale/gianfar.c |    4 ----
 drivers/net/ethernet/freescale/gianfar.h |    1 -
 2 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index c82f677..2c6b569 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -663,7 +663,6 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
 		return -ENOMEM;
 
 	priv = netdev_priv(dev);
-	priv->node = ofdev->dev.of_node;
 	priv->ndev = dev;
 
 	priv->num_tx_queues = num_tx_qs;
@@ -1001,7 +1000,6 @@ static int gfar_probe(struct platform_device *ofdev)
 	priv = netdev_priv(dev);
 	priv->ndev = dev;
 	priv->ofdev = ofdev;
-	priv->node = ofdev->dev.of_node;
 	SET_NETDEV_DEV(dev, &ofdev->dev);
 
 	spin_lock_init(&priv->bflock);
@@ -1038,8 +1036,6 @@ static int gfar_probe(struct platform_device *ofdev)
 	/* Set the dev->base_addr to the gfar reg region */
 	dev->base_addr = (unsigned long) regs;
 
-	SET_NETDEV_DEV(dev, &ofdev->dev);
-
 	/* Fill in the dev structure */
 	dev->watchdog_timeo = TX_TIMEOUT;
 	dev->mtu = 1500;
diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h
index 78125f1..8b4de57 100644
--- a/drivers/net/ethernet/freescale/gianfar.h
+++ b/drivers/net/ethernet/freescale/gianfar.h
@@ -1060,7 +1060,6 @@ struct gfar_private {
 	unsigned int total_tx_ring_size;
 	unsigned int total_rx_ring_size;
 
-	struct device_node *node;
 	struct net_device *ndev;
 	struct platform_device *ofdev;
 	enum gfar_errata errata;
-- 
1.7.7.4

[PATCH net-next 2/7] gianfar: Add device ref (dev) in gfar_private

[Claudiu Manoil]

Use device pointer (dev) to simplify the code and to
avoid double indirections, especially on the hot path.

Basically, instead of accessing priv to get the ofdev
reference and then accessing the ofdev structure to
dereference the needed dev pointer, we will get the
dev pointer directly from priv.

The dev pointer is required on the hot path, see gfar_new_rxbdp
or gfar_clean_rx_ring (or xmit), and this patch makes
it available directly from priv's 1st cacheline.

This change is reflected at asm level too, taking (the hot)
gfar_new_rxbdp():
initial version -
    18c0:	7c 7e 1b 78 	mr      r30,r3

    18d0:	81 69 04 3c 	lwz     r11,1084(r9)

    18d8:	34 6b 00 10 	addic.  r3,r11,16
    18dc:	41 82 00 08 	beq-    18e4

patched version -
    18d0:	80 69 04 38 	lwz     r3,1080(r9)

    18d8:	2f 83 00 00 	cmpwi   cr7,r3,0
    18dc:	41 9e 00 08 	beq-    cr7,18e4

Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>
---
 drivers/net/ethernet/freescale/gianfar.c |   25 +++++++++++++------------
 drivers/net/ethernet/freescale/gianfar.h |    1 +
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 2c6b569..592d297 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -231,7 +231,7 @@ static int gfar_alloc_skb_resources(struct net_device *ndev)
 	dma_addr_t addr;
 	int i, j, k;
 	struct gfar_private *priv = netdev_priv(ndev);
-	struct device *dev = &priv->ofdev->dev;
+	struct device *dev = priv->dev;
 	struct gfar_priv_tx_q *tx_queue = NULL;
 	struct gfar_priv_rx_q *rx_queue = NULL;
 
@@ -1000,6 +1000,7 @@ static int gfar_probe(struct platform_device *ofdev)
 	priv = netdev_priv(dev);
 	priv->ndev = dev;
 	priv->ofdev = ofdev;
+	priv->dev = &ofdev->dev;
 	SET_NETDEV_DEV(dev, &ofdev->dev);
 
 	spin_lock_init(&priv->bflock);
@@ -1713,13 +1714,13 @@ static void free_skb_tx_queue(struct gfar_priv_tx_q *tx_queue)
 		if (!tx_queue->tx_skbuff[i])
 			continue;
 
-		dma_unmap_single(&priv->ofdev->dev, txbdp->bufPtr,
+		dma_unmap_single(priv->dev, txbdp->bufPtr,
 				 txbdp->length, DMA_TO_DEVICE);
 		txbdp->lstatus = 0;
 		for (j = 0; j < skb_shinfo(tx_queue->tx_skbuff[i])->nr_frags;
 		     j++) {
 			txbdp++;
-			dma_unmap_page(&priv->ofdev->dev, txbdp->bufPtr,
+			dma_unmap_page(priv->dev, txbdp->bufPtr,
 				       txbdp->length, DMA_TO_DEVICE);
 		}
 		txbdp++;
@@ -1740,8 +1741,8 @@ static void free_skb_rx_queue(struct gfar_priv_rx_q *rx_queue)
 
 	for (i = 0; i < rx_queue->rx_ring_size; i++) {
 		if (rx_queue->rx_skbuff[i]) {
-			dma_unmap_single(&priv->ofdev->dev,
-					 rxbdp->bufPtr, priv->rx_buffer_size,
+			dma_unmap_single(priv->dev, rxbdp->bufPtr,
+					 priv->rx_buffer_size,
 					 DMA_FROM_DEVICE);
 			dev_kfree_skb_any(rx_queue->rx_skbuff[i]);
 			rx_queue->rx_skbuff[i] = NULL;
@@ -1780,7 +1781,7 @@ static void free_skb_resources(struct gfar_private *priv)
 			free_skb_rx_queue(rx_queue);
 	}
 
-	dma_free_coherent(&priv->ofdev->dev,
+	dma_free_coherent(priv->dev,
 			  sizeof(struct txbd8) * priv->total_tx_ring_size +
 			  sizeof(struct rxbd8) * priv->total_rx_ring_size,
 			  priv->tx_queue[0]->tx_bd_base,
@@ -2160,7 +2161,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
 			if (i == nr_frags - 1)
 				lstatus |= BD_LFLAG(TXBD_LAST | TXBD_INTERRUPT);
 
-			bufaddr = skb_frag_dma_map(&priv->ofdev->dev,
+			bufaddr = skb_frag_dma_map(priv->dev,
 						   &skb_shinfo(skb)->frags[i],
 						   0,
 						   length,
@@ -2212,7 +2213,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		lstatus |= BD_LFLAG(TXBD_TOE);
 	}
 
-	txbdp_start->bufPtr = dma_map_single(&priv->ofdev->dev, skb->data,
+	txbdp_start->bufPtr = dma_map_single(priv->dev, skb->data,
 					     skb_headlen(skb), DMA_TO_DEVICE);
 
 	/* If time stamping is requested one additional TxBD must be set up. The
@@ -2525,7 +2526,7 @@ static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue)
 		} else
 			buflen = bdp->length;
 
-		dma_unmap_single(&priv->ofdev->dev, bdp->bufPtr,
+		dma_unmap_single(priv->dev, bdp->bufPtr,
 				 buflen, DMA_TO_DEVICE);
 
 		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS)) {
@@ -2544,7 +2545,7 @@ static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue)
 		bdp = next_txbd(bdp, base, tx_ring_size);
 
 		for (i = 0; i < frags; i++) {
-			dma_unmap_page(&priv->ofdev->dev, bdp->bufPtr,
+			dma_unmap_page(priv->dev, bdp->bufPtr,
 				       bdp->length, DMA_TO_DEVICE);
 			bdp->lstatus &= BD_LFLAG(TXBD_WRAP);
 			bdp = next_txbd(bdp, base, tx_ring_size);
@@ -2610,7 +2611,7 @@ static void gfar_new_rxbdp(struct gfar_priv_rx_q *rx_queue, struct rxbd8 *bdp,
 	struct gfar_private *priv = netdev_priv(dev);
 	dma_addr_t buf;
 
-	buf = dma_map_single(&priv->ofdev->dev, skb->data,
+	buf = dma_map_single(priv->dev, skb->data,
 			     priv->rx_buffer_size, DMA_FROM_DEVICE);
 	gfar_init_rxbdp(rx_queue, bdp, buf);
 }
@@ -2775,7 +2776,7 @@ int gfar_clean_rx_ring(struct gfar_priv_rx_q *rx_queue, int rx_work_limit)
 
 		skb = rx_queue->rx_skbuff[rx_queue->skb_currx];
 
-		dma_unmap_single(&priv->ofdev->dev, bdp->bufPtr,
+		dma_unmap_single(priv->dev, bdp->bufPtr,
 				 priv->rx_buffer_size, DMA_FROM_DEVICE);
 
 		if (unlikely(!(bdp->status & RXBD_ERR) &&
diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h
index 8b4de57..8b27e5f 100644
--- a/drivers/net/ethernet/freescale/gianfar.h
+++ b/drivers/net/ethernet/freescale/gianfar.h
@@ -1060,6 +1060,7 @@ struct gfar_private {
 	unsigned int total_tx_ring_size;
 	unsigned int total_rx_ring_size;
 
+	struct device *dev;
 	struct net_device *ndev;
 	struct platform_device *ofdev;
 	enum gfar_errata errata;
-- 
1.7.7.4

[PATCH net-next 3/7] gianfar: Cleanup and optimize struct gfar_private

[Claudiu Manoil]

Group run-time critical fields within the 1st cacheline (32B)
followed by the tx|rx_queue reference arrays and the interrupt
group instances (gfargrp), all cacheline aligned.

This has several benefits. Firstly comes the performance benefit
by having the members required by the driver's hot path re-grouped
in the structure's first cache lines, whereas the unimportant
members were pushed towards the end of the struct.
Another benefit comes from eliminating a 24 byte memory hole that
was rendering gfar_priv's 2nd cacheline useless. The default gcc
layout of gfar_private leaves an implicit 24 byte hole after the
errata (enum) member. This patch fixes it.

The uchar bitfields were pushed towards the end of the struct
as these are not run-time performance critical (used for init
time operations). Because there is no other 2 byte member
around to couple the uchar bitfields memeber with, we will
have an addititnal 2 byte hole after the bitfields. This is
unsignificant however, and it doesn't influence gfar_priv's
size, because the whole structure is padded to be a 32B multiple.

Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>
---
 drivers/net/ethernet/freescale/gianfar.h |   93 +++++++++++++++--------------
 1 files changed, 48 insertions(+), 45 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h
index 8b27e5f..1e2ce8b 100644
--- a/drivers/net/ethernet/freescale/gianfar.h
+++ b/drivers/net/ethernet/freescale/gianfar.h
@@ -1049,28 +1049,64 @@ enum gfar_errata {
  * the buffer descriptor determines the actual condition.
  */
 struct gfar_private {
-
-	/* Indicates how many tx, rx queues are enabled */
-	unsigned int num_tx_queues;
 	unsigned int num_rx_queues;
-	unsigned int num_grps;
-	unsigned int mode;
-
-	/* The total tx and rx ring size for the enabled queues */
-	unsigned int total_tx_ring_size;
-	unsigned int total_rx_ring_size;
 
 	struct device *dev;
 	struct net_device *ndev;
-	struct platform_device *ofdev;
 	enum gfar_errata errata;
+	unsigned int rx_buffer_size;
+
+	u16 padding;
+
+	/* HW time stamping enabled flag */
+	int hwts_rx_en;
+	int hwts_tx_en;
 
-	struct gfar_priv_grp gfargrp[MAXGROUPS];
 	struct gfar_priv_tx_q *tx_queue[MAX_TX_QS];
 	struct gfar_priv_rx_q *rx_queue[MAX_RX_QS];
+	struct gfar_priv_grp gfargrp[MAXGROUPS];
+
+	u32 device_flags;
+
+	unsigned int mode;
+	unsigned int num_tx_queues;
+	unsigned int num_grps;
+
+	/* Network Statistics */
+	struct gfar_extra_stats extra_stats;
+
+	/* PHY stuff */
+	phy_interface_t interface;
+	struct device_node *phy_node;
+	struct device_node *tbi_node;
+	struct phy_device *phydev;
+	struct mii_bus *mii_bus;
+	int oldspeed;
+	int oldduplex;
+	int oldlink;
+
+	/* Bitfield update lock */
+	spinlock_t bflock;
+
+	uint32_t msg_enable;
+
+	struct work_struct reset_task;
+
+	struct platform_device *ofdev;
+	unsigned char
+		extended_hash:1,
+		bd_stash_en:1,
+		rx_filer_enable:1,
+		/* Wake-on-LAN enabled */
+		wol_en:1,
+		/* Enable priorty based Tx scheduling in Hw */
+		prio_sched_en:1;
+
+	/* The total tx and rx ring size for the enabled queues */
+	unsigned int total_tx_ring_size;
+	unsigned int total_rx_ring_size;
 
 	/* RX per device parameters */
-	unsigned int rx_buffer_size;
 	unsigned int rx_stash_size;
 	unsigned int rx_stash_index;
 
@@ -1089,39 +1125,6 @@ struct gfar_private {
 	unsigned int fifo_starve;
 	unsigned int fifo_starve_off;
 
-	/* Bitfield update lock */
-	spinlock_t bflock;
-
-	phy_interface_t interface;
-	struct device_node *phy_node;
-	struct device_node *tbi_node;
-	u32 device_flags;
-	unsigned char
-		extended_hash:1,
-		bd_stash_en:1,
-		rx_filer_enable:1,
-		wol_en:1, /* Wake-on-LAN enabled */
-		prio_sched_en:1; /* Enable priorty based Tx scheduling in Hw */
-	unsigned short padding;
-
-	/* PHY stuff */
-	struct phy_device *phydev;
-	struct mii_bus *mii_bus;
-	int oldspeed;
-	int oldduplex;
-	int oldlink;
-
-	uint32_t msg_enable;
-
-	struct work_struct reset_task;
-
-	/* Network Statistics */
-	struct gfar_extra_stats extra_stats;
-
-	/* HW time stamping enabled flag */
-	int hwts_rx_en;
-	int hwts_tx_en;
-
 	/*Filer table*/
 	unsigned int ftp_rqfpr[MAX_FILER_IDX + 1];
 	unsigned int ftp_rqfcr[MAX_FILER_IDX + 1];
-- 
1.7.7.4

[PATCH net-next 4/7] gianfar: GRO_DROP is unlikely

[Claudiu Manoil]

The change is significant since it affects the rx hot path.
Paul observed and documented the effects at asm level, see
below:

"It turns out that it does make a difference, since gfar_process_frame
gets inlined, and so the increment code gets moved out of line (I have
marked the if statment with * and the increment code within "-----"):

  ------------------------- as is currently ------------------
     4d14:       80 61 00 18     lwz     r3,24(r1)
     4d18:       7f c4 f3 78     mr      r4,r30
     4d1c:       48 00 00 01     bl      4d1c <gfar_clean_rx_ring+0x10c>
  *  4d20:       2f 83 00 04     cmpwi   cr7,r3,4
     4d24:       40 9e 00 1c     bne-    cr7,4d40
<gfar_clean_rx_ring+0x130>
        ----------------------------
     4d28:       81 3c 01 f8     lwz     r9,504(r28)
     4d2c:       81 5c 01 fc     lwz     r10,508(r28)
     4d30:       31 4a 00 01     addic   r10,r10,1
     4d34:       7d 29 01 94     addze   r9,r9
     4d38:       91 3c 01 f8     stw     r9,504(r28)
     4d3c:       91 5c 01 fc     stw     r10,508(r28)
        ----------------------------
     4d40:       a0 1f 00 24     lhz     r0,36(r31)
     4d44:       81 3f 00 00     lwz     r9,0(r31)
     4d48:       7f a4 eb 78     mr      r4,r29
     4d4c:       7f e3 fb 78     mr      r3,r31

  -------------------------- unlikely ------------------------
     4d14:       80 61 00 18     lwz     r3,24(r1)
     4d18:       7f c4 f3 78     mr      r4,r30
     4d1c:       48 00 00 01     bl      4d1c <gfar_clean_rx_ring+0x10c>
  *  4d20:       2f 83 00 04     cmpwi   cr7,r3,4
     4d24:       41 9e 03 94     beq-    cr7,50b8
<gfar_clean_rx_ring+0x4a8>
     4d28:       a0 1f 00 24     lhz     r0,36(r31)
     4d2c:       81 3f 00 00     lwz     r9,0(r31)
     4d30:       7f a4 eb 78     mr      r4,r29
     4d34:       7f e3 fb 78     mr      r3,r31
[...]
     50b8:       81 3c 01 f8     lwz     r9,504(r28)
     50bc:       81 5c 01 fc     lwz     r10,508(r28)
     50c0:       31 4a 00 01     addic   r10,r10,1
     50c4:       7d 29 01 94     addze   r9,r9
     50c8:       91 3c 01 f8     stw     r9,504(r28)
     50cc:       91 5c 01 fc     stw     r10,508(r28)
     50d0:       4b ff fc 58     b       4d28 <gfar_clean_rx_ring+0x118>

So, the increment does actually get moved ~1k away."

Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>
---
 drivers/net/ethernet/freescale/gianfar.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 592d297..af0f4d6 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -2740,7 +2740,7 @@ static int gfar_process_frame(struct net_device *dev, struct sk_buff *skb,
 	/* Send the packet up the stack */
 	ret = napi_gro_receive(napi, skb);
 
-	if (GRO_DROP == ret)
+	if (unlikely(GRO_DROP == ret))
 		atomic64_inc(&priv->extra_stats.kernel_dropped);
 
 	return 0;
-- 
1.7.7.4

[PATCH net-next 5/7] gianfar: gfar_process_frame returns void

[Claudiu Manoil]

No return code is expected from gfar_process_frame(), hence
change it to return void.

Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>
---
 drivers/net/ethernet/freescale/gianfar.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index af0f4d6..d70f74c 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -133,8 +133,8 @@ static void gfar_netpoll(struct net_device *dev);
 #endif
 int gfar_clean_rx_ring(struct gfar_priv_rx_q *rx_queue, int rx_work_limit);
 static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue);
-static int gfar_process_frame(struct net_device *dev, struct sk_buff *skb,
-			      int amount_pull, struct napi_struct *napi);
+static void gfar_process_frame(struct net_device *dev, struct sk_buff *skb,
+			       int amount_pull, struct napi_struct *napi);
 void gfar_halt(struct net_device *dev);
 static void gfar_halt_nodisable(struct net_device *dev);
 void gfar_start(struct net_device *dev);
@@ -2692,8 +2692,8 @@ static inline void gfar_rx_checksum(struct sk_buff *skb, struct rxfcb *fcb)
 
 
 /* gfar_process_frame() -- handle one incoming packet if skb isn't NULL. */
-static int gfar_process_frame(struct net_device *dev, struct sk_buff *skb,
-			      int amount_pull, struct napi_struct *napi)
+static void gfar_process_frame(struct net_device *dev, struct sk_buff *skb,
+			       int amount_pull, struct napi_struct *napi)
 {
 	struct gfar_private *priv = netdev_priv(dev);
 	struct rxfcb *fcb = NULL;
@@ -2742,8 +2742,6 @@ static int gfar_process_frame(struct net_device *dev, struct sk_buff *skb,
 
 	if (unlikely(GRO_DROP == ret))
 		atomic64_inc(&priv->extra_stats.kernel_dropped);
-
-	return 0;
 }
 
 /* gfar_clean_rx_ring() -- Processes each frame in the rx ring
-- 
1.7.7.4

[PATCH net-next 6/7] gianfar: Remove wrong buffer size conditioning to VLAN h/w offload

[Claudiu Manoil]

The controller's ref manual states clearly that when the hw Rx vlan
offload feature is enabled, meaning that the VLEX bit from RCTRL is
correctly enabled, then the hw performs automatic VLAN tag extraction
and deletion from the ethernet frames. So there's no point in trying to
increase the rx buff size when rxvlan is on, as the frame is actually
smaller.
And the Tx vlan hw accel feature (VLINS) has nothing to do with rx buff
size computation.

Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>
---
 drivers/net/ethernet/freescale/gianfar.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index d70f74c..b2c6077 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -2385,9 +2385,6 @@ static int gfar_change_mtu(struct net_device *dev, int new_mtu)
 	int oldsize = priv->rx_buffer_size;
 	int frame_size = new_mtu + ETH_HLEN;
 
-	if (gfar_is_vlan_on(priv))
-		frame_size += VLAN_HLEN;
-
 	if ((frame_size < 64) || (frame_size > JUMBO_FRAME_SIZE)) {
 		netif_err(priv, drv, dev, "Invalid MTU setting\n");
 		return -EINVAL;
-- 
1.7.7.4

[PATCH net-next 7/7] gianfar: Fix and cleanup Rx FCB indication

[Claudiu Manoil]

This fixes a less obvious error on one hand, and prevents futher
similar errors by disambiguating and optimizing RxFCB indication,
on the other hand.

The error consists in NETIF_F_HW_VLAN_TX flag being used as an
indication of Rx FCB insertion. This happened as soon gfar_uses_fcb(),
which despite its name indicates Rx FCB insertion, started
incorporating is_vlan_on().
is_vlan_on(), on the other hand, is also a misleading construct because
we need to differentiate b/w hw VLAN extraction/VLEX (marked by VLAN_RX
flag) and hw VLAN insertion/VLINS (VLAN_TX flag), which are different
mechanisms using different types of FCBs.

The hw spec for the RxFCB feature is as follows:
In the case of RxBD rings, FCBs (Frame Control Block) are inserted by
the eTSEC whenever RCTRL[PRSDEP] is set to a non-zero value. Only one
FCB is inserted per frame (in the buffer pointed to by the RxBD with
bit F set). TOE acceleration for receive is enabled for all rx frames
in this case.

This patch introduces priv->uses_rxfcb field to quickly signal RxFCB
insertion in accordance with the specification above.

The dependency on FSL_GIANFAR_DEV_HAS_TIMER was also eliminated as
another source of confusion. The actual dependency is to priv->hwts_rx_en.
Upon changing priv->hwts_rx_en via IOCTL, the gfar device is being
restarted and on init_mac() the priv->hwts_rx_en flag determines RxFCB
insertion, and rctrl is programmed accordingly. The patch takes care
of this case too.

Though maybe not as self documenting as the inlining version uses_fcb(),
priv->uses_rxfcb has the main purpose to quickly signal, on the hot path,
that the incoming frame has a *Rx* FCB block inserted which needs to be
pulled out before passing the skb to the stack. This is a performance
critical operation, it needs to happen fast, that's why uses_rxfcb is
placed in the first cacheline of gfar_private.
This is also why a cached rctrl cannot be used instead: 1) because
we don't have 32 bits available in the first cacheline of gfar_priv
(but only 16); 2) bit operations are expensive on the hot path.

Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>
---
 drivers/net/ethernet/freescale/gianfar.c |   41 ++++++++++++++---------------
 drivers/net/ethernet/freescale/gianfar.h |    1 +
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index b2c6077..4b5e8a6 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -344,6 +344,9 @@ static void gfar_init_mac(struct net_device *ndev)
 	/* Configure the coalescing support */
 	gfar_configure_coalescing(priv, 0xFF, 0xFF);
 
+	/* set this when rx hw offload (TOE) functions are being used */
+	priv->uses_rxfcb = 0;
+
 	if (priv->rx_filer_enable) {
 		rctrl |= RCTRL_FILREN;
 		/* Program the RIR0 reg with the required distribution */
@@ -354,8 +357,10 @@ static void gfar_init_mac(struct net_device *ndev)
 	if (ndev->flags & IFF_PROMISC)
 		rctrl |= RCTRL_PROM;
 
-	if (ndev->features & NETIF_F_RXCSUM)
+	if (ndev->features & NETIF_F_RXCSUM) {
 		rctrl |= RCTRL_CHECKSUMMING;
+		priv->uses_rxfcb = 1;
+	}
 
 	if (priv->extended_hash) {
 		rctrl |= RCTRL_EXTHASH;
@@ -377,11 +382,15 @@ static void gfar_init_mac(struct net_device *ndev)
 	}
 
 	/* Enable HW time stamping if requested from user space */
-	if (priv->hwts_rx_en)
+	if (priv->hwts_rx_en) {
 		rctrl |= RCTRL_PRSDEP_INIT | RCTRL_TS_ENABLE;
+		priv->uses_rxfcb = 1;
+	}
 
-	if (ndev->features & NETIF_F_HW_VLAN_RX)
+	if (ndev->features & NETIF_F_HW_VLAN_RX) {
 		rctrl |= RCTRL_VLEX | RCTRL_PRSDEP_INIT;
+		priv->uses_rxfcb = 1;
+	}
 
 	/* Init rctrl based on our settings */
 	gfar_write(&regs->rctrl, rctrl);
@@ -500,20 +509,6 @@ void unlock_tx_qs(struct gfar_private *priv)
 		spin_unlock(&priv->tx_queue[i]->txlock);
 }
 
-static bool gfar_is_vlan_on(struct gfar_private *priv)
-{
-	return (priv->ndev->features & NETIF_F_HW_VLAN_RX) ||
-	       (priv->ndev->features & NETIF_F_HW_VLAN_TX);
-}
-
-/* Returns 1 if incoming frames use an FCB */
-static inline int gfar_uses_fcb(struct gfar_private *priv)
-{
-	return gfar_is_vlan_on(priv) ||
-	       (priv->ndev->features & NETIF_F_RXCSUM) ||
-	       (priv->device_flags & FSL_GIANFAR_DEV_HAS_TIMER);
-}
-
 static void free_tx_pointers(struct gfar_private *priv)
 {
 	int i;
@@ -2326,10 +2321,13 @@ void gfar_check_rx_parser_mode(struct gfar_private *priv)
 
 	tempval = gfar_read(&regs->rctrl);
 	/* If parse is no longer required, then disable parser */
-	if (tempval & RCTRL_REQ_PARSER)
+	if (tempval & RCTRL_REQ_PARSER) {
 		tempval |= RCTRL_PRSDEP_INIT;
-	else
+		priv->uses_rxfcb = 1;
+	} else {
 		tempval &= ~RCTRL_PRSDEP_INIT;
+		priv->uses_rxfcb = 0;
+	}
 	gfar_write(&regs->rctrl, tempval);
 }
 
@@ -2362,6 +2360,7 @@ void gfar_vlan_mode(struct net_device *dev, netdev_features_t features)
 		tempval = gfar_read(&regs->rctrl);
 		tempval |= (RCTRL_VLEX | RCTRL_PRSDEP_INIT);
 		gfar_write(&regs->rctrl, tempval);
+		priv->uses_rxfcb = 1;
 	} else {
 		/* Disable VLAN tag extraction */
 		tempval = gfar_read(&regs->rctrl);
@@ -2390,7 +2389,7 @@ static int gfar_change_mtu(struct net_device *dev, int new_mtu)
 		return -EINVAL;
 	}
 
-	if (gfar_uses_fcb(priv))
+	if (priv->uses_rxfcb)
 		frame_size += GMAC_FCB_LEN;
 
 	frame_size += priv->padding;
@@ -2759,7 +2758,7 @@ int gfar_clean_rx_ring(struct gfar_priv_rx_q *rx_queue, int rx_work_limit)
 	bdp = rx_queue->cur_rx;
 	base = rx_queue->rx_bd_base;
 
-	amount_pull = (gfar_uses_fcb(priv) ? GMAC_FCB_LEN : 0);
+	amount_pull = priv->uses_rxfcb ? GMAC_FCB_LEN : 0;
 
 	while (!((bdp->status & RXBD_EMPTY) || (--rx_work_limit < 0))) {
 		struct sk_buff *newskb;
diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h
index 1e2ce8b..63a28d2 100644
--- a/drivers/net/ethernet/freescale/gianfar.h
+++ b/drivers/net/ethernet/freescale/gianfar.h
@@ -1056,6 +1056,7 @@ struct gfar_private {
 	enum gfar_errata errata;
 	unsigned int rx_buffer_size;
 
+	u16 uses_rxfcb;
 	u16 padding;
 
 	/* HW time stamping enabled flag */
-- 
1.7.7.4

Re: [PATCH net-next v1 0/7] Fix, cleanup, optimize gfar_private and the Rx path

[Paul Gortmaker]

On 13-02-14 10:00 AM, Claudiu Manoil wrote:
> Cc: Paul Gortmaker <paul.gortmaker@windriver.com>,
>   "David S. Miller" <davem@davemloft.net>
> 
> 
> Hi Dave,
> These patches went already through a review round with Paul.
> Changes since the initial version of the patches:
> * split the 1st patch into two patches (01 & 02, as suggested by Paul)
> * added the suggested trivial cleanup patch:
>   gianfar: gfar_process_frame returns void
> * provided "long logs" with detailed comments and explanations

The commit headers do look a lot better, thanks.  Passes basic boot
test on today's net-next with sbc8548 and NFS root.

Paul.
--

> 
> Thank you.
> 
> Regards,
> Claudiu
> 
> 
> Claudiu Manoil (7):
>   gianfar: Remove unused device_node ref in gfar_private
>   gianfar: Add device ref (dev) in gfar_private
>   gianfar: Cleanup and optimize struct gfar_private
>   gianfar: GRO_DROP is unlikely
>   gianfar: gfar_process_frame returns void
>   gianfar: Remove wrong buffer size conditioning to VLAN h/w offload
>   gianfar: Fix and cleanup Rx FCB indication
> 
>  drivers/net/ethernet/freescale/gianfar.c |   85 ++++++++++++---------------
>  drivers/net/ethernet/freescale/gianfar.h |   96 +++++++++++++++--------------
>  2 files changed, 88 insertions(+), 93 deletions(-)
> 

Re: [PATCH net-next v1 0/7] Fix, cleanup, optimize gfar_private and the Rx path

[David Miller]

From: Claudiu Manoil <claudiu.manoil@freescale.com>
Date: Thu, 14 Feb 2013 17:00:00 +0200

> These patches went already through a review round with Paul.
> Changes since the initial version of the patches:
> * split the 1st patch into two patches (01 & 02, as suggested by Paul)
> * added the suggested trivial cleanup patch:
>   gianfar: gfar_process_frame returns void
> * provided "long logs" with detailed comments and explanations

Series applied, thanks.