RE: [PATCH v1 5/6] net: fec: Add Scatter/gather support

From: Eric Dumazet <redacted> Data: Friday, May 30, 2014 12:38 PM

To: Duan Fugang-B38611
Cc: Li Frank-B20596; davem@davemloft.net; ezequiel.garcia@free-
electrons.com; netdev@vger.kernel.org; shawn.guo@linaro.org;
bhutchings@solarflare.com; stephen@networkplumber.org
Subject: Re: [PATCH v1 5/6] net: fec: Add Scatter/gather support

On Fri, 2014-05-30 at 10:05 +0800, Fugang Duan wrote:

quoted

Add Scatter/gather support for FEC.
This feature allows to improve outbound throughput performance.
Running iperf tests shows a 55.4% improvement, tested on imx6dl
sabresd board.

quoted

+			bufaddr = fep->tx_bounce[index];
+		}
+		if (id_entry->driver_data & FEC_QUIRK_SWAP_FRAME)
+			swap_buffer(bufaddr, frag_len);

bufaddr is a page fragment and can be shared (using sendfile() for
example), we cannot modify the content using swap_buffer().

In order to do the swap, you need to force the copy to tx_bounce buffer.

Yes, you are right. I will change it in next version.
I tested on imx6dl platform, FEC support hw swap function, so I don't find issue.
Thanks.

quoted

+		bdp->cbd_bufaddr = dma_map_single(&fep->pdev->dev, bufaddr,
+						frag_len, DMA_TO_DEVICE);
+		if (dma_mapping_error(&fep->pdev->dev, bdp->cbd_bufaddr)) {
+			dev_kfree_skb_any(skb);
+			if (net_ratelimit())
+				netdev_err(ndev, "Tx DMA memory map failed\n");
+			goto dma_mapping_error;
+		}
+
+		bdp->cbd_datlen = frag_len;
+		bdp->cbd_sc = status;
+	}
+
+	fep->cur_tx = bdp;
+
+	return 0;
+
+dma_mapping_error:
 	bdp = fep->cur_tx;
+	for (i = 0; i < frag; i++) {
+		bdp = fec_enet_get_nextdesc(bdp, fep);
+		dma_unmap_single(&fep->pdev->dev, bdp->cbd_bufaddr,
+				bdp->cbd_datlen, DMA_TO_DEVICE);
+	}
+	return NETDEV_TX_OK;
+}

-	status = bdp->cbd_sc;
+static int fec_enet_txq_submit_skb(struct sk_buff *skb, struct
+net_device *ndev) {
+	struct fec_enet_private *fep = netdev_priv(ndev);
+	const struct platform_device_id *id_entry =
+				platform_get_device_id(fep->pdev);
+	int nr_frags = skb_shinfo(skb)->nr_frags;
+	struct bufdesc *bdp, *last_bdp;
+	void *bufaddr;
+	unsigned short status;
+	unsigned short buflen;
+	unsigned int estatus = 0;
+	unsigned int index;
+	int ret;

 	/* Protocol checksum off-load for TCP and UDP. */
 	if (fec_enet_clear_csum(skb, ndev)) { @@ -349,17 +462,18 @@ static
int txq_submit_skb(struct sk_buff *skb, struct net_device *ndev)
 		return NETDEV_TX_OK;
 	}

-	/* Clear all of the status flags */
+	/* Fill in a Tx ring entry */
+	bdp = fep->cur_tx;
+	status = bdp->cbd_sc;
 	status &= ~BD_ENET_TX_STATS;

 	/* Set buffer length and buffer pointer */
 	bufaddr = skb->data;
-	bdp->cbd_datlen = skb->len;
+	buflen = skb_headlen(skb);

 	index = fec_enet_get_bd_index(bdp, fep);
-
 	if (((unsigned long) bufaddr) & FEC_ALIGNMENT) {
-		memcpy(fep->tx_bounce[index], skb->data, skb->len);
+		memcpy(fep->tx_bounce[index], skb->data, buflen);
 		bufaddr = fep->tx_bounce[index];
 	}

@@ -369,62 +483,66 @@ static int txq_submit_skb(struct sk_buff *skb,

struct net_device *ndev)

quoted

 	 * swap every frame going to and coming from the controller.
 	 */
 	if (id_entry->driver_data & FEC_QUIRK_SWAP_FRAME)
-		swap_buffer(bufaddr, skb->len);
-
-	/* Save skb pointer */
-	fep->tx_skbuff[index] = skb;
+		swap_buffer(bufaddr, buflen);

Same problem here, a driver is certainly not allowed to mess with
skb->data.

Thanks, as above.

Andy