RE: [PATCH net-next V2 07/16] net: fec: set cbd_sc without relying on previous value

 From: Troy Kisky [off-list ref] Sent: Saturday, March 05, 2016 12:08 AM

To: Fugang Duan <redacted>; netdev@vger.kernel.org;
davem@davemloft.net; b38611@freescale.com
Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de; andrew@lunn.ch;
tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
kernel@lists.infradead.org; laci@boundarydevices.com; shawnguo@kernel.org;
johannes@sipsolutions.net; stillcompiling@gmail.com;
sergei.shtylyov@cogentembedded.com; arnd@arndb.de
Subject: Re: [PATCH net-next V2 07/16] net: fec: set cbd_sc without relying on
previous value

On 3/4/2016 2:29 AM, Fugang Duan wrote:

quoted

From: Troy Kisky <redacted> Sent: Thursday,
February 25, 2016 8:37 AM

quoted

To: netdev@vger.kernel.org; davem@davemloft.net;

b38611@freescale.com

quoted

quoted

Cc: fabio.estevam@freescale.com; l.stach@pengutronix.de;
andrew@lunn.ch; tremyfr@gmail.com; linux@arm.linux.org.uk; linux-arm-
kernel@lists.infradead.org; laci@boundarydevices.com;
shawnguo@kernel.org; johannes@sipsolutions.net;
stillcompiling@gmail.com; sergei.shtylyov@cogentembedded.com;
arnd@arndb.de; Troy Kisky [off-list ref]
Subject: [PATCH net-next V2 07/16] net: fec: set cbd_sc without
relying on previous value

Relying on the wrap bit to stay valid once initialized when the
controller also writes to this byte seems undesirable since we can
easily know what the value should be.

Signed-off-by: Troy Kisky <redacted>
---
 drivers/net/ethernet/freescale/fec_main.c | 38
+++++++++----------------------
 1 file changed, 11 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c

b/drivers/net/ethernet/freescale/fec_main.c
index 791f385..6ceb5f9 100644

--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c

@@ -340,9 +340,8 @@ fec_enet_txq_submit_frag_skb(struct

fec_enet_priv_tx_q *txq,
 		bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
 		ebdp = (struct bufdesc_ex *)bdp;

-		status = fec16_to_cpu(bdp->cbd_sc);
-		status &= ~BD_ENET_TX_STATS;
-		status |= (BD_ENET_TX_TC | BD_ENET_TX_READY);
+		status = BD_ENET_TX_TC | BD_ENET_TX_READY |
+				((bdp == txq->bd.last) ? BD_SC_WRAP : 0);
 		frag_len = skb_shinfo(skb)->frags[frag].size;

 		/* Handle the last BD specially */ @@ -436,8 +435,6 @@ static

int

quoted

quoted

fec_enet_txq_submit_skb(struct fec_enet_priv_tx_q *txq,
 	/* Fill in a Tx ring entry */
 	bdp = txq->bd.cur;
 	last_bdp = bdp;
-	status = fec16_to_cpu(bdp->cbd_sc);
-	status &= ~BD_ENET_TX_STATS;

 	/* Set buffer length and buffer pointer */
 	bufaddr = skb->data;

@@ -462,6 +459,8 @@ static int fec_enet_txq_submit_skb(struct

fec_enet_priv_tx_q *txq,
 		return NETDEV_TX_OK;
 	}

+	status = BD_ENET_TX_TC | BD_ENET_TX_READY |
+			((bdp == txq->bd.last) ? BD_SC_WRAP : 0);
 	if (nr_frags) {
 		last_bdp = fec_enet_txq_submit_frag_skb(txq, skb, ndev);
 		if (IS_ERR(last_bdp)) {

@@ -512,7 +511,6 @@ static int fec_enet_txq_submit_skb(struct

fec_enet_priv_tx_q *txq,
 	/* Send it on its way.  Tell FEC it's ready, interrupt when done,
 	 * it's the last BD of the frame, and to put the CRC on the end.
 	 */
-	status |= (BD_ENET_TX_READY | BD_ENET_TX_TC);

This is completely error.  We have to prepare all BDs for frag skb, and then

enable "READY" and "TC" bit for the first BD, otherwise uDMA copy un-correct
data to fifo.

quoted

I don't follow. Please read patch again.

Understand, sorry, I take one mistake.  The patch is fine for me.