[PATCH net-next v3 1/1] net: fec: Enable imx6 enet checksum acceleration.

[PATCH net-next v3 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Jim Baxter]

Enables hardware generation of IP header and
protocol specific checksums for transmitted
packets.

Enabled hardware discarding of received packets with
invalid IP header or protocol specific checksums.

The feature is enabled by default but can be
enabled/disabled by ethtool.

Signed-off-by: Fugang Duan <B38611@freescale.com>
Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
---
Changed NETIF_F_GRO feature to NETIF_F_RX_CSUM
Changed NETIF_F_HW_CSUM feature to NETIF_F_IP_CSUM
Removed error stats for receive checksum error and made code more readable.
Removed incorrect check for NETIF_F_HW_CSUM feature in checksum code.
Spelling corrections.


 drivers/net/ethernet/freescale/fec.h      |   10 ++-
 drivers/net/ethernet/freescale/fec_main.c |  133 ++++++++++++++++++++++++++++-
 2 files changed, 140 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index eb43729..d44f65b 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -52,6 +52,7 @@
 #define FEC_R_FIFO_RSEM		0x194 /* Receive FIFO section empty threshold */
 #define FEC_R_FIFO_RAEM		0x198 /* Receive FIFO almost empty threshold */
 #define FEC_R_FIFO_RAFL		0x19c /* Receive FIFO almost full threshold */
+#define FEC_RACC		0x1C4 /* Receive Accelerator function */
 #define FEC_MIIGSK_CFGR		0x300 /* MIIGSK Configuration reg */
 #define FEC_MIIGSK_ENR		0x308 /* MIIGSK Enable reg */
 
@@ -164,9 +165,11 @@ struct bufdesc_ex {
 #define BD_ENET_TX_CSL          ((ushort)0x0001)
 #define BD_ENET_TX_STATS        ((ushort)0x03ff)        /* All status bits */
 
-/*enhanced buffer desciptor control/status used by Ethernet transmit*/
+/*enhanced buffer descriptor control/status used by Ethernet transmit*/
 #define BD_ENET_TX_INT          0x40000000
 #define BD_ENET_TX_TS           0x20000000
+#define BD_ENET_TX_PINS         0x10000000
+#define BD_ENET_TX_IINS         0x08000000
 
 
 /* This device has up to three irqs on some platforms */
@@ -190,6 +193,10 @@ struct bufdesc_ex {
 
 #define BD_ENET_RX_INT          0x00800000
 #define BD_ENET_RX_PTP          ((ushort)0x0400)
+#define BD_ENET_RX_ICE		0x00000020
+#define BD_ENET_RX_PCR		0x00000010
+#define FLAG_RX_CSUM_ENABLED	(BD_ENET_RX_ICE | BD_ENET_RX_PCR)
+#define FLAG_RX_CSUM_ERROR	(BD_ENET_RX_ICE | BD_ENET_RX_PCR)
 
 /* The FEC buffer descriptors track the ring buffers.  The rx_bd_base and
  * tx_bd_base always point to the base of the buffer descriptors.  The
@@ -247,6 +254,7 @@ struct fec_enet_private {
 	int	pause_flag;
 
 	struct	napi_struct napi;
+	int	csum_flags;
 
 	struct ptp_clock *ptp_clock;
 	struct ptp_clock_info ptp_caps;
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index d7657a4..b626608 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -34,6 +34,12 @@
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/skbuff.h>
+#include <linux/in.h>
+#include <linux/ip.h>
+#include <net/ip.h>
+#include <linux/tcp.h>
+#include <linux/udp.h>
+#include <linux/icmp.h>
 #include <linux/spinlock.h>
 #include <linux/workqueue.h>
 #include <linux/bitops.h>
@@ -181,6 +187,10 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
 #define PKT_MINBUF_SIZE		64
 #define PKT_MAXBLR_SIZE		1520
 
+/* FEC receive acceleration */
+#define FEC_RACC_IPDIS		(1 << 1)
+#define FEC_RACC_PRODIS		(1 << 2)
+
 /*
  * The 5270/5271/5280/5282/532x RX control register also contains maximum frame
  * size bits. Other FEC hardware does not, so we need to take that into
@@ -241,6 +251,58 @@ static void *swap_buffer(void *bufaddr, int len)
 	return bufaddr;
 }
 
+static void
+fec_enet_clear_csum(struct sk_buff *skb, struct net_device *ndev)
+{
+	int hdr_len = 0;
+
+	/* Only run for packets requiring a checksum. */
+	if (skb->ip_summed != CHECKSUM_PARTIAL)
+		return;
+	if (skb->len < (ETH_HLEN + sizeof(struct iphdr)))
+		return;
+
+	if (skb->protocol == htons(ETH_P_IP)) {
+		ip_hdr(skb)->check = 0;
+
+		switch (ip_hdr(skb)->protocol) {
+		case IPPROTO_UDP:
+			hdr_len = (ETH_HLEN +
+					(ip_hdr(skb)->ihl << 2) +
+					sizeof(struct udphdr));
+			if (skb->len < hdr_len)
+				return;
+			skb_cow_head(skb, hdr_len);
+			skb_set_transport_header(skb,
+					ETH_HLEN + ip_hdrlen(skb));
+			udp_hdr(skb)->check = 0;
+			break;
+		case IPPROTO_TCP:
+			hdr_len = (ETH_HLEN +
+					(ip_hdr(skb)->ihl << 2) +
+					sizeof(struct tcphdr));
+			if (skb->len < hdr_len)
+				return;
+			skb_cow_head(skb, hdr_len);
+			if (tcp_hdr(skb))
+				tcp_hdr(skb)->check = 0;
+			break;
+		case IPPROTO_ICMP:
+			hdr_len = (ETH_HLEN +
+					(ip_hdr(skb)->ihl << 2) +
+					sizeof(struct icmphdr));
+			if (skb->len < hdr_len)
+				return;
+			skb_cow_head(skb, hdr_len);
+			if (icmp_hdr(skb))
+				icmp_hdr(skb)->checksum = 0;
+			break;
+		default:
+			break;
+		}
+	}
+}
+
 static netdev_tx_t
 fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 {
@@ -277,6 +339,9 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	bufaddr = skb->data;
 	bdp->cbd_datlen = skb->len;
 
+	/* HW acceleration for ICMP TCP UDP checksum */
+	fec_enet_clear_csum(skb, ndev);
+
 	/*
 	 * On some FEC implementations data must be aligned on
 	 * 4-byte boundaries. Use bounce buffers to copy data
@@ -326,8 +391,10 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 			ebdp->cbd_esc = (BD_ENET_TX_TS | BD_ENET_TX_INT);
 			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 		} else {
-
 			ebdp->cbd_esc = BD_ENET_TX_INT;
+			if (skb->ip_summed == CHECKSUM_PARTIAL)
+				ebdp->cbd_esc |= BD_ENET_TX_PINS
+						| BD_ENET_TX_IINS;
 		}
 	}
 	/* If this was the last BD in the ring, start at the beginning again. */
@@ -407,6 +474,7 @@ fec_restart(struct net_device *ndev, int duplex)
 	const struct platform_device_id *id_entry =
 				platform_get_device_id(fep->pdev);
 	int i;
+	u32 val;
 	u32 temp_mac[2];
 	u32 rcntl = OPT_FRAME_SIZE | 0x04;
 	u32 ecntl = 0x2; /* ETHEREN */
@@ -473,6 +541,14 @@ fec_restart(struct net_device *ndev, int duplex)
 	/* Set MII speed */
 	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
 
+	/* set RX checksum */
+	val = readl(fep->hwp + FEC_RACC);
+	if (fep->csum_flags & FLAG_RX_CSUM_ENABLED)
+		val |= (FEC_RACC_IPDIS | FEC_RACC_PRODIS);
+	else
+		val &= ~(FEC_RACC_IPDIS | FEC_RACC_PRODIS);
+	writel(val, fep->hwp + FEC_RACC);
+
 	/*
 	 * The phy interface and speed need to get configured
 	 * differently on enet-mac.
@@ -530,7 +606,7 @@ fec_restart(struct net_device *ndev, int duplex)
 	     fep->phy_dev && fep->phy_dev->pause)) {
 		rcntl |= FEC_ENET_FCE;
 
-		/* set FIFO thresh hold parameter to reduce overrun */
+		/* set FIFO threshold parameter to reduce overrun */
 		writel(FEC_ENET_RSEM_V, fep->hwp + FEC_R_FIFO_RSEM);
 		writel(FEC_ENET_RSFL_V, fep->hwp + FEC_R_FIFO_RSFL);
 		writel(FEC_ENET_RAEM_V, fep->hwp + FEC_R_FIFO_RAEM);
@@ -818,6 +894,18 @@ fec_enet_rx(struct net_device *ndev, int budget)
 				spin_unlock_irqrestore(&fep->tmreg_lock, flags);
 			}
 
+			if (fep->bufdesc_ex &&
+				(fep->csum_flags & FLAG_RX_CSUM_ENABLED)) {
+				struct bufdesc_ex *ebdp =
+					(struct bufdesc_ex *)bdp;
+				if (!(ebdp->cbd_esc & FLAG_RX_CSUM_ERROR)) {
+					/* don't check it */
+					skb->ip_summed = CHECKSUM_UNNECESSARY;
+				} else {
+					skb_checksum_none_assert(skb);
+				}
+			}
+
 			if (!skb_defer_rx_timestamp(skb))
 				napi_gro_receive(&fep->napi, skb);
 		}
@@ -1439,6 +1527,9 @@ static int fec_enet_alloc_buffers(struct net_device *ndev)
 		if (fep->bufdesc_ex) {
 			struct bufdesc_ex *ebdp = (struct bufdesc_ex *)bdp;
 			ebdp->cbd_esc = BD_ENET_TX_INT;
+			if (ndev->features & NETIF_F_HW_CSUM)
+				ebdp->cbd_esc |= BD_ENET_TX_PINS
+						| BD_ENET_TX_IINS;
 		}
 
 		bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
@@ -1618,6 +1709,38 @@ static void fec_poll_controller(struct net_device *dev)
 }
 #endif
 
+static int fec_set_features(struct net_device *netdev,
+	netdev_features_t features)
+{
+	struct fec_enet_private *fep = netdev_priv(netdev);
+	netdev_features_t changed = features ^ netdev->features;
+	bool restart_required = false;
+
+	netdev->features = features;
+
+	/* Receive checksum has been changed */
+	if (changed & NETIF_F_RXCSUM) {
+		restart_required = true;
+		if (features & NETIF_F_RXCSUM)
+			fep->csum_flags |= FLAG_RX_CSUM_ENABLED;
+		else
+			fep->csum_flags &= ~FLAG_RX_CSUM_ENABLED;
+	}
+
+	/* Restart the network interface */
+	if (true == restart_required) {
+		if (netif_running(netdev)) {
+			fec_stop(netdev);
+			fec_restart(netdev, fep->phy_dev->duplex);
+			netif_wake_queue(netdev);
+		} else {
+			fec_restart(netdev, fep->phy_dev->duplex);
+		}
+	}
+
+	return 0;
+}
+
 static const struct net_device_ops fec_netdev_ops = {
 	.ndo_open		= fec_enet_open,
 	.ndo_stop		= fec_enet_close,
@@ -1631,6 +1754,7 @@ static const struct net_device_ops fec_netdev_ops = {
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= fec_poll_controller,
 #endif
+	.ndo_set_features	= fec_set_features,
 };
 
  /*
@@ -1672,6 +1796,11 @@ static int fec_enet_init(struct net_device *ndev)
 	writel(FEC_RX_DISABLED_IMASK, fep->hwp + FEC_IMASK);
 	netif_napi_add(ndev, &fep->napi, fec_enet_rx_napi, FEC_NAPI_WEIGHT);
 
+	/* enable hw accelerator */
+	ndev->features |= (NETIF_F_IP_CSUM | NETIF_F_RXCSUM);
+	ndev->hw_features |= (NETIF_F_IP_CSUM | NETIF_F_RXCSUM);
+	fep->csum_flags |= FLAG_RX_CSUM_ENABLED;
+
 	fec_restart(ndev, 0);
 
 	return 0;
-- 
1.7.10.4

Re: [PATCH net-next v3 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Eric Dumazet]

On Wed, 2013-04-17 at 21:07 +0100, Jim Baxter wrote:
> Enables hardware generation of IP header and
> protocol specific checksums for transmitted
> packets.
> 
> Enabled hardware discarding of received packets with
> invalid IP header or protocol specific checksums.
> 
> The feature is enabled by default but can be
> enabled/disabled by ethtool.
> 
> Signed-off-by: Fugang Duan <B38611@freescale.com>
> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
> ---
>  
> +static void
> +fec_enet_clear_csum(struct sk_buff *skb, struct net_device *ndev)
> +{
> +	int hdr_len = 0;
> +
> +	/* Only run for packets requiring a checksum. */
> +	if (skb->ip_summed != CHECKSUM_PARTIAL)
> +		return;
> +	if (skb->len < (ETH_HLEN + sizeof(struct iphdr)))
> +		return;
> +

You could do the skb_cow_head() here.

> +	if (skb->protocol == htons(ETH_P_IP)) {
> +		ip_hdr(skb)->check = 0;
> +
> +		switch (ip_hdr(skb)->protocol) {
> +		case IPPROTO_UDP:
> +			hdr_len = (ETH_HLEN +
> +					(ip_hdr(skb)->ihl << 2) +
> +					sizeof(struct udphdr));
> +			if (skb->len < hdr_len)
> +				return;
> +			skb_cow_head(skb, hdr_len);

You should not ignore the return value...

> +			skb_set_transport_header(skb,
> +					ETH_HLEN + ip_hdrlen(skb));
> +			udp_hdr(skb)->check = 0;
> +			break;
> +		case IPPROTO_TCP:
> +			hdr_len = (ETH_HLEN +
> +					(ip_hdr(skb)->ihl << 2) +
> +					sizeof(struct tcphdr));
> +			if (skb->len < hdr_len)
> +				return;
> +			skb_cow_head(skb, hdr_len);

same here

> +			if (tcp_hdr(skb))
> +				tcp_hdr(skb)->check = 0;
> +			break;
> +		case IPPROTO_ICMP:
> +			hdr_len = (ETH_HLEN +
> +					(ip_hdr(skb)->ihl << 2) +
> +					sizeof(struct icmphdr));
> +			if (skb->len < hdr_len)
> +				return;
> +			skb_cow_head(skb, hdr_len);

same here

> +			if (icmp_hdr(skb))
> +				icmp_hdr(skb)->checksum = 0;
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +}
> +

Re: [PATCH net-next v3 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Jim Baxter]

On 17/04/13 22:37, Eric Dumazet wrote:
> On Wed, 2013-04-17 at 21:07 +0100, Jim Baxter wrote:
>> Enables hardware generation of IP header and
>> protocol specific checksums for transmitted
>> packets.
>>
>> Enabled hardware discarding of received packets with
>> invalid IP header or protocol specific checksums.
>>
>> The feature is enabled by default but can be
>> enabled/disabled by ethtool.
>>
>> Signed-off-by: Fugang Duan <B38611@freescale.com>
>> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
>> ---
>>  
>> +static void
>> +fec_enet_clear_csum(struct sk_buff *skb, struct net_device *ndev)
>> +{
>> +	int hdr_len = 0;
>> +
>> +	/* Only run for packets requiring a checksum. */
>> +	if (skb->ip_summed != CHECKSUM_PARTIAL)
>> +		return;
>> +	if (skb->len < (ETH_HLEN + sizeof(struct iphdr)))
>> +		return;
>> +
> 
> You could do the skb_cow_head() here.

I do not know the full length of the header at this point, so I think
that would be tricky.

> 
>> +	if (skb->protocol == htons(ETH_P_IP)) {
>> +		ip_hdr(skb)->check = 0;
>> +
>> +		switch (ip_hdr(skb)->protocol) {
>> +		case IPPROTO_UDP:
>> +			hdr_len = (ETH_HLEN +
>> +					(ip_hdr(skb)->ihl << 2) +
>> +					sizeof(struct udphdr));
>> +			if (skb->len < hdr_len)
>> +				return;
>> +			skb_cow_head(skb, hdr_len);
> 
> You should not ignore the return value...
I agree.
> 
>> +			skb_set_transport_header(skb,
>> +					ETH_HLEN + ip_hdrlen(skb));
>> +			udp_hdr(skb)->check = 0;
>> +			break;
>> +		case IPPROTO_TCP:
>> +			hdr_len = (ETH_HLEN +
>> +					(ip_hdr(skb)->ihl << 2) +
>> +					sizeof(struct tcphdr));
>> +			if (skb->len < hdr_len)
>> +				return;
>> +			skb_cow_head(skb, hdr_len);
> 
> same here
Do I need to call skb_cow_head here, I am not changing the size of the
header?
> 
>> +			if (tcp_hdr(skb))
>> +				tcp_hdr(skb)->check = 0;
>> +			break;
>> +		case IPPROTO_ICMP:
>> +			hdr_len = (ETH_HLEN +
>> +					(ip_hdr(skb)->ihl << 2) +
>> +					sizeof(struct icmphdr));
>> +			if (skb->len < hdr_len)
>> +				return;
>> +			skb_cow_head(skb, hdr_len);
> 
> same here
Again, do I need this here?
> 
>> +			if (icmp_hdr(skb))
>> +				icmp_hdr(skb)->checksum = 0;
>> +			break;
>> +		default:
>> +			break;
>> +		}
>> +	}
>> +}
>> +
> 
> 
> 

Re: [PATCH net-next v3 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Ben Hutchings]

On Thu, 2013-04-18 at 13:49 +0100, Jim Baxter wrote:
> On 17/04/13 22:37, Eric Dumazet wrote:
> > On Wed, 2013-04-17 at 21:07 +0100, Jim Baxter wrote:
> >> Enables hardware generation of IP header and
> >> protocol specific checksums for transmitted
> >> packets.
> >>
> >> Enabled hardware discarding of received packets with
> >> invalid IP header or protocol specific checksums.
> >>
> >> The feature is enabled by default but can be
> >> enabled/disabled by ethtool.
> >>
> >> Signed-off-by: Fugang Duan <B38611@freescale.com>
> >> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
> >> ---
> >>  
> >> +static void
> >> +fec_enet_clear_csum(struct sk_buff *skb, struct net_device *ndev)
> >> +{
> >> +	int hdr_len = 0;
> >> +
> >> +	/* Only run for packets requiring a checksum. */
> >> +	if (skb->ip_summed != CHECKSUM_PARTIAL)
> >> +		return;
> >> +	if (skb->len < (ETH_HLEN + sizeof(struct iphdr)))
> >> +		return;
> >> +
> > 
> > You could do the skb_cow_head() here.
> 
> I do not know the full length of the header at this point, so I think
> that would be tricky.
> 
> > 
> >> +	if (skb->protocol == htons(ETH_P_IP)) {
> >> +		ip_hdr(skb)->check = 0;

But you're already changing it here.

> >> +		switch (ip_hdr(skb)->protocol) {
> >> +		case IPPROTO_UDP:
> >> +			hdr_len = (ETH_HLEN +
> >> +					(ip_hdr(skb)->ihl << 2) +
> >> +					sizeof(struct udphdr));
> >> +			if (skb->len < hdr_len)
> >> +				return;
> >> +			skb_cow_head(skb, hdr_len);
> > 
> > You should not ignore the return value...
> I agree.
> > 
> >> +			skb_set_transport_header(skb,
> >> +					ETH_HLEN + ip_hdrlen(skb));
> >> +			udp_hdr(skb)->check = 0;
> >> +			break;
> >> +		case IPPROTO_TCP:
> >> +			hdr_len = (ETH_HLEN +
> >> +					(ip_hdr(skb)->ihl << 2) +
> >> +					sizeof(struct tcphdr));
> >> +			if (skb->len < hdr_len)
> >> +				return;
> >> +			skb_cow_head(skb, hdr_len);
> > 
> > same here
> Do I need to call skb_cow_head here, I am not changing the size of the
> header?
[...]

The length passed to skb_cow_head() may be significant when the skb has
paged fragments.  Since you aren't (yet) implementing scatter-gather,
that won't happen.  And the headers shouldn't be in paged fragments
anyway.  I think you can safely use skb_cow_head(skb, 0).

But you don't actually need to check protocol numbers at all, as the
kernel already specifies where the checksum should be.

So I think this function should look like:
{
	if (skb->ip_summed != CHECKSUM_PARTIAL)
		return 0;

	if (unlikely(skb_cow_head(skb, 0)))
		return -1;

	*(__sum16 *)(skb->head + skb->csum_start + skb->csum_offset) = 0;
	return 0;
}

The caller needs to check for the failure, and free the skb
(kfree_skb()) rather than transmitting it.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next v3 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Ben Hutchings]

On Thu, 2013-04-18 at 17:16 +0100, Ben Hutchings wrote:
[...]
> But you don't actually need to check protocol numbers at all, as the
> kernel already specifies where the checksum should be.
> 
> So I think this function should look like:
> {
> 	if (skb->ip_summed != CHECKSUM_PARTIAL)
> 		return 0;
> 
> 	if (unlikely(skb_cow_head(skb, 0)))
> 		return -1;
> 
> 	*(__sum16 *)(skb->head + skb->csum_start + skb->csum_offset) = 0;
> 	return 0;
> }
> 
> The caller needs to check for the failure, and free the skb
> (kfree_skb()) rather than transmitting it.

Oh, presumably you still need to clear the IPv4 header checksum as well.
But it might be better to enable only TCP/UDP checksumming, because
Linux always calculates the IPv4 header checksum in software.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next v3 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Jim Baxter]

On 18/04/13 17:16, Ben Hutchings wrote:
> On Thu, 2013-04-18 at 13:49 +0100, Jim Baxter wrote:
>> On 17/04/13 22:37, Eric Dumazet wrote:
>>> On Wed, 2013-04-17 at 21:07 +0100, Jim Baxter wrote:
>>>
>>>> +			skb_set_transport_header(skb,
>>>> +					ETH_HLEN + ip_hdrlen(skb));
>>>> +			udp_hdr(skb)->check = 0;
>>>> +			break;
>>>> +		case IPPROTO_TCP:
>>>> +			hdr_len = (ETH_HLEN +
>>>> +					(ip_hdr(skb)->ihl << 2) +
>>>> +					sizeof(struct tcphdr));
>>>> +			if (skb->len < hdr_len)
>>>> +				return;
>>>> +			skb_cow_head(skb, hdr_len);
>>>
>>> same here
>> Do I need to call skb_cow_head here, I am not changing the size of the
>> header?
> [...]
> 
> The length passed to skb_cow_head() may be significant when the skb has
> paged fragments.  Since you aren't (yet) implementing scatter-gather,
> that won't happen.  And the headers shouldn't be in paged fragments
> anyway.  I think you can safely use skb_cow_head(skb, 0).
> 
> But you don't actually need to check protocol numbers at all, as the
> kernel already specifies where the checksum should be.
> 
> So I think this function should look like:
> {
> 	if (skb->ip_summed != CHECKSUM_PARTIAL)
> 		return 0;
> 
> 	if (unlikely(skb_cow_head(skb, 0)))
> 		return -1;
> 
> 	*(__sum16 *)(skb->head + skb->csum_start + skb->csum_offset) = 0;
> 	return 0;
> }
> 
> The caller needs to check for the failure, and free the skb
> (kfree_skb()) rather than transmitting it.
> 
> Ben.
> 

Which checksum does skb->csum (skb->csum_start + skb->csum_offset)
relate to?
The network card can generate IP header and protocol (UDP/TCP/ICMP)
checksums as long as the checksums are zeroed?

Jim

Re: [PATCH net-next v3 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Ben Hutchings]

On Thu, 2013-04-18 at 18:08 +0100, Jim Baxter wrote:
> On 18/04/13 17:16, Ben Hutchings wrote:
> > On Thu, 2013-04-18 at 13:49 +0100, Jim Baxter wrote:
> >> On 17/04/13 22:37, Eric Dumazet wrote:
> >>> On Wed, 2013-04-17 at 21:07 +0100, Jim Baxter wrote:
> >>>
> >>>> +			skb_set_transport_header(skb,
> >>>> +					ETH_HLEN + ip_hdrlen(skb));
> >>>> +			udp_hdr(skb)->check = 0;
> >>>> +			break;
> >>>> +		case IPPROTO_TCP:
> >>>> +			hdr_len = (ETH_HLEN +
> >>>> +					(ip_hdr(skb)->ihl << 2) +
> >>>> +					sizeof(struct tcphdr));
> >>>> +			if (skb->len < hdr_len)
> >>>> +				return;
> >>>> +			skb_cow_head(skb, hdr_len);
> >>>
> >>> same here
> >> Do I need to call skb_cow_head here, I am not changing the size of the
> >> header?
> > [...]
> > 
> > The length passed to skb_cow_head() may be significant when the skb has
> > paged fragments.  Since you aren't (yet) implementing scatter-gather,
> > that won't happen.  And the headers shouldn't be in paged fragments
> > anyway.  I think you can safely use skb_cow_head(skb, 0).
> > 
> > But you don't actually need to check protocol numbers at all, as the
> > kernel already specifies where the checksum should be.
> > 
> > So I think this function should look like:
> > {
> > 	if (skb->ip_summed != CHECKSUM_PARTIAL)
> > 		return 0;
> > 
> > 	if (unlikely(skb_cow_head(skb, 0)))
> > 		return -1;
> > 
> > 	*(__sum16 *)(skb->head + skb->csum_start + skb->csum_offset) = 0;
> > 	return 0;
> > }
> > 
> > The caller needs to check for the failure, and free the skb
> > (kfree_skb()) rather than transmitting it.
> > 
> > Ben.
> > 
> 
> Which checksum does skb->csum (skb->csum_start + skb->csum_offset)
> relate to?

TCP or UDP.

> The network card can generate IP header and protocol (UDP/TCP/ICMP)
> checksums as long as the checksums are zeroed?

You don't need to offload the IPv4 header checksum.  TX checksum offload
will not be requested for ICMP.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next v3 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Jim Baxter]

On 18/04/13 18:12, Ben Hutchings wrote:
> On Thu, 2013-04-18 at 18:08 +0100, Jim Baxter wrote:
>> On 18/04/13 17:16, Ben Hutchings wrote:
>>> On Thu, 2013-04-18 at 13:49 +0100, Jim Baxter wrote:
>>>> On 17/04/13 22:37, Eric Dumazet wrote:
>>>>> On Wed, 2013-04-17 at 21:07 +0100, Jim Baxter wrote:
>>>>>
>>>>>> +			skb_set_transport_header(skb,
>>>>>> +					ETH_HLEN + ip_hdrlen(skb));
>>>>>> +			udp_hdr(skb)->check = 0;
>>>>>> +			break;
>>>>>> +		case IPPROTO_TCP:
>>>>>> +			hdr_len = (ETH_HLEN +
>>>>>> +					(ip_hdr(skb)->ihl << 2) +
>>>>>> +					sizeof(struct tcphdr));
>>>>>> +			if (skb->len < hdr_len)
>>>>>> +				return;
>>>>>> +			skb_cow_head(skb, hdr_len);
>>>>>
>>>>> same here
>>>> Do I need to call skb_cow_head here, I am not changing the size of the
>>>> header?
>>> [...]
>>>
>>> The length passed to skb_cow_head() may be significant when the skb has
>>> paged fragments.  Since you aren't (yet) implementing scatter-gather,
>>> that won't happen.  And the headers shouldn't be in paged fragments
>>> anyway.  I think you can safely use skb_cow_head(skb, 0).
>>>
>>> But you don't actually need to check protocol numbers at all, as the
>>> kernel already specifies where the checksum should be.
>>>
>>> So I think this function should look like:
>>> {
>>> 	if (skb->ip_summed != CHECKSUM_PARTIAL)
>>> 		return 0;
>>>
>>> 	if (unlikely(skb_cow_head(skb, 0)))
>>> 		return -1;
>>>
>>> 	*(__sum16 *)(skb->head + skb->csum_start + skb->csum_offset) = 0;
>>> 	return 0;
>>> }
>>>
>>> The caller needs to check for the failure, and free the skb
>>> (kfree_skb()) rather than transmitting it.
>>>
>>> Ben.
>>>
>>
>> Which checksum does skb->csum (skb->csum_start + skb->csum_offset)
>> relate to?
> 
> TCP or UDP.
> 
>> The network card can generate IP header and protocol (UDP/TCP/ICMP)
>> checksums as long as the checksums are zeroed?
> 
> You don't need to offload the IPv4 header checksum.  TX checksum offload
> will not be requested for ICMP.

So the kernel only offloads TCP and UDP checksum's. All other transport
protocols (ICMP, ICMPIPV6 etc.) will be done by the kernel.

> 
> Ben.
> 

Thanks, though I saw you replied to my question before I could send it.

It sounds like there is no reason wasting CPU cycles clearing the IP
header checksum, so I can keep the code efficient.

Jim

Re: [PATCH net-next v3 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Jim Baxter]

On 18/04/13 18:21, Jim Baxter wrote:
> On 18/04/13 18:12, Ben Hutchings wrote:
>> On Thu, 2013-04-18 at 18:08 +0100, Jim Baxter wrote:
>>> On 18/04/13 17:16, Ben Hutchings wrote:
>>>> On Thu, 2013-04-18 at 13:49 +0100, Jim Baxter wrote:
>>>>> On 17/04/13 22:37, Eric Dumazet wrote:
>>>>>> On Wed, 2013-04-17 at 21:07 +0100, Jim Baxter wrote:
>>>>>>
>>>>>>> +			skb_set_transport_header(skb,
>>>>>>> +					ETH_HLEN + ip_hdrlen(skb));
>>>>>>> +			udp_hdr(skb)->check = 0;
>>>>>>> +			break;
>>>>>>> +		case IPPROTO_TCP:
>>>>>>> +			hdr_len = (ETH_HLEN +
>>>>>>> +					(ip_hdr(skb)->ihl << 2) +
>>>>>>> +					sizeof(struct tcphdr));
>>>>>>> +			if (skb->len < hdr_len)
>>>>>>> +				return;
>>>>>>> +			skb_cow_head(skb, hdr_len);
>>>>>>
>>>>>> same here
>>>>> Do I need to call skb_cow_head here, I am not changing the size of the
>>>>> header?
>>>> [...]
>>>>
>>>> The length passed to skb_cow_head() may be significant when the skb has
>>>> paged fragments.  Since you aren't (yet) implementing scatter-gather,
>>>> that won't happen.  And the headers shouldn't be in paged fragments
>>>> anyway.  I think you can safely use skb_cow_head(skb, 0).
>>>>
>>>> But you don't actually need to check protocol numbers at all, as the
>>>> kernel already specifies where the checksum should be.
>>>>
>>>> So I think this function should look like:
>>>> {
>>>> 	if (skb->ip_summed != CHECKSUM_PARTIAL)
>>>> 		return 0;
>>>>
>>>> 	if (unlikely(skb_cow_head(skb, 0)))
>>>> 		return -1;
>>>>
>>>> 	*(__sum16 *)(skb->head + skb->csum_start + skb->csum_offset) = 0;
>>>> 	return 0;
>>>> }
>>>>
>>>> The caller needs to check for the failure, and free the skb
>>>> (kfree_skb()) rather than transmitting it.
>>>>
>>>> Ben.
>>>>
>>>
>>> Which checksum does skb->csum (skb->csum_start + skb->csum_offset)
>>> relate to?
>>
>> TCP or UDP.
>>
>>> The network card can generate IP header and protocol (UDP/TCP/ICMP)
>>> checksums as long as the checksums are zeroed?
>>
>> You don't need to offload the IPv4 header checksum.  TX checksum offload
>> will not be requested for ICMP.
> 
> So the kernel only offloads TCP and UDP checksum's. All other transport
> protocols (ICMP, ICMPIPV6 etc.) will be done by the kernel.
> 
>>
>> Ben.
>>
> 
> Thanks, though I saw you replied to my question before I could send it.
> 
> It sounds like there is no reason wasting CPU cycles clearing the IP
> header checksum, so I can keep the code efficient.
> 
> Jim
> 

Does anybody know if the kernel uses checksum offloading for IPV6, if I
enable the feature NETIF_F_HW_CSUM, I only receive CHECKSUM_NONE when
the protocol is 0x86DD?

I realise there is no IP header checksum, but I thought the protocols
still use a checksum.

I am using iperf with the -V option in UDP mode, which offloads for IPV4.


Thank you,
Jim

Re: [PATCH net-next v3 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Ben Hutchings]

On Thu, 2013-04-18 at 22:27 +0100, Jim Baxter wrote:
> On 18/04/13 18:21, Jim Baxter wrote:
> > On 18/04/13 18:12, Ben Hutchings wrote:
> >> On Thu, 2013-04-18 at 18:08 +0100, Jim Baxter wrote:
> >>> On 18/04/13 17:16, Ben Hutchings wrote:
> >>>> On Thu, 2013-04-18 at 13:49 +0100, Jim Baxter wrote:
> >>>>> On 17/04/13 22:37, Eric Dumazet wrote:
> >>>>>> On Wed, 2013-04-17 at 21:07 +0100, Jim Baxter wrote:
> >>>>>>
> >>>>>>> +			skb_set_transport_header(skb,
> >>>>>>> +					ETH_HLEN + ip_hdrlen(skb));
> >>>>>>> +			udp_hdr(skb)->check = 0;
> >>>>>>> +			break;
> >>>>>>> +		case IPPROTO_TCP:
> >>>>>>> +			hdr_len = (ETH_HLEN +
> >>>>>>> +					(ip_hdr(skb)->ihl << 2) +
> >>>>>>> +					sizeof(struct tcphdr));
> >>>>>>> +			if (skb->len < hdr_len)
> >>>>>>> +				return;
> >>>>>>> +			skb_cow_head(skb, hdr_len);
> >>>>>>
> >>>>>> same here
> >>>>> Do I need to call skb_cow_head here, I am not changing the size of the
> >>>>> header?
> >>>> [...]
> >>>>
> >>>> The length passed to skb_cow_head() may be significant when the skb has
> >>>> paged fragments.  Since you aren't (yet) implementing scatter-gather,
> >>>> that won't happen.  And the headers shouldn't be in paged fragments
> >>>> anyway.  I think you can safely use skb_cow_head(skb, 0).
> >>>>
> >>>> But you don't actually need to check protocol numbers at all, as the
> >>>> kernel already specifies where the checksum should be.
> >>>>
> >>>> So I think this function should look like:
> >>>> {
> >>>> 	if (skb->ip_summed != CHECKSUM_PARTIAL)
> >>>> 		return 0;
> >>>>
> >>>> 	if (unlikely(skb_cow_head(skb, 0)))
> >>>> 		return -1;
> >>>>
> >>>> 	*(__sum16 *)(skb->head + skb->csum_start + skb->csum_offset) = 0;
> >>>> 	return 0;
> >>>> }
> >>>>
> >>>> The caller needs to check for the failure, and free the skb
> >>>> (kfree_skb()) rather than transmitting it.
> >>>>
> >>>> Ben.
> >>>>
> >>>
> >>> Which checksum does skb->csum (skb->csum_start + skb->csum_offset)
> >>> relate to?
> >>
> >> TCP or UDP.
> >>
> >>> The network card can generate IP header and protocol (UDP/TCP/ICMP)
> >>> checksums as long as the checksums are zeroed?
> >>
> >> You don't need to offload the IPv4 header checksum.  TX checksum offload
> >> will not be requested for ICMP.
> > 
> > So the kernel only offloads TCP and UDP checksum's. All other transport
> > protocols (ICMP, ICMPIPV6 etc.) will be done by the kernel.
> > 
> >>
> >> Ben.
> >>
> > 
> > Thanks, though I saw you replied to my question before I could send it.
> > 
> > It sounds like there is no reason wasting CPU cycles clearing the IP
> > header checksum, so I can keep the code efficient.
> > 
> > Jim
> > 
> 
> Does anybody know if the kernel uses checksum offloading for IPV6, if I
> enable the feature NETIF_F_HW_CSUM, I only receive CHECKSUM_NONE when
> the protocol is 0x86DD?

NETIF_F_HW_CSUM means you can generate TCP-style checksums from an
arbitrary starting offset.  You would then use skb->csum_start and
skb->csum_offset to construct the TX descriptor(s).

If the hardware generates checksums for specific protocols based on
parsing the packet contents, you should set NETIF_F_IP_CSUM (TCP and UDP
on IPv4) and/or NETIF_F_IPV6_CSUM (TCP and UDP on IPv6).  It appears
from your original patch that the hardware behaves like this.

Ben.

> I realise there is no IP header checksum, but I thought the protocols
> still use a checksum.
> 
> I am using iperf with the -V option in UDP mode, which offloads for IPV4.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next v3 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Francois Romieu]

Jim Baxter <jim_baxter@mentor.com> :
[...]
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index d7657a4..b626608 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
[...]
> @@ -241,6 +251,58 @@ static void *swap_buffer(void *bufaddr, int len)
>  	return bufaddr;
>  }
>  
> +static void
> +fec_enet_clear_csum(struct sk_buff *skb, struct net_device *ndev)
> +{
> +	int hdr_len = 0;

Useless init (and scope may be reduced).

> +
> +	/* Only run for packets requiring a checksum. */
> +	if (skb->ip_summed != CHECKSUM_PARTIAL)
> +		return;
> +	if (skb->len < (ETH_HLEN + sizeof(struct iphdr)))
> +		return;
> +
> +	if (skb->protocol == htons(ETH_P_IP)) {
> +		ip_hdr(skb)->check = 0;
> +

You may break early on !ETH_P_IP and/or init hdr_len at
ETH_HLEN + ip_hdrlen(skb) here before incrementing it later with the
size of the adequate header. It will save some forced line breaks as
well as some code duplication (you are open coding ip_hdrlen a few times
btw).

[...]
> @@ -473,6 +541,14 @@ fec_restart(struct net_device *ndev, int duplex)
>  	/* Set MII speed */
>  	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
>  
> +	/* set RX checksum */
> +	val = readl(fep->hwp + FEC_RACC);
> +	if (fep->csum_flags & FLAG_RX_CSUM_ENABLED)
> +		val |= (FEC_RACC_IPDIS | FEC_RACC_PRODIS);
> +	else
> +		val &= ~(FEC_RACC_IPDIS | FEC_RACC_PRODIS);

Shorten (FEC_RACC_IPDIS | FEC_RACC_PRODIS) behind a single #define ?

[...]
> @@ -1618,6 +1709,38 @@ static void fec_poll_controller(struct net_device *dev)
>  }
>  #endif
>  
> +static int fec_set_features(struct net_device *netdev,
> +	netdev_features_t features)
> +{
> +	struct fec_enet_private *fep = netdev_priv(netdev);
> +	netdev_features_t changed = features ^ netdev->features;
> +	bool restart_required = false;
> +
> +	netdev->features = features;
> +
> +	/* Receive checksum has been changed */
> +	if (changed & NETIF_F_RXCSUM) {
> +		restart_required = true;
> +		if (features & NETIF_F_RXCSUM)
> +			fep->csum_flags |= FLAG_RX_CSUM_ENABLED;
> +		else
> +			fep->csum_flags &= ~FLAG_RX_CSUM_ENABLED;

---(snip)----8<------------------------------------------------

> +	}
> +
> +	/* Restart the network interface */
> +	if (true == restart_required) {

---(snip)--------------------------------------->8-------------

Then remove the "restart_required" variable ?

-- 
Ueimor

Re: [PATCH net-next v3 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Jim Baxter]

Thank you for you comments, I will fix the issues.
One question below:

On 17/04/13 23:45, Francois Romieu wrote:
>> +static int fec_set_features(struct net_device *netdev,
>> +	netdev_features_t features)
>> +{
>> +	struct fec_enet_private *fep = netdev_priv(netdev);
>> +	netdev_features_t changed = features ^ netdev->features;
>> +	bool restart_required = false;
>> +
>> +	netdev->features = features;
>> +
>> +	/* Receive checksum has been changed */
>> +	if (changed & NETIF_F_RXCSUM) {
>> +		restart_required = true;
>> +		if (features & NETIF_F_RXCSUM)
>> +			fep->csum_flags |= FLAG_RX_CSUM_ENABLED;
>> +		else
>> +			fep->csum_flags &= ~FLAG_RX_CSUM_ENABLED;
> 
> ---(snip)----8<------------------------------------------------
> 
>> +	}
>> +
>> +	/* Restart the network interface */
>> +	if (true == restart_required) {
> 
> ---(snip)--------------------------------------->8-------------
> 
> Then remove the "restart_required" variable ?
> 
What you mean by remove the "restart_required" variable, I only want a
restart in certain situations? Am I misunderstanding you comment?

Jim

Re: [PATCH net-next v3 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Fabio Estevam]

On Thu, Apr 18, 2013 at 7:18 AM, Jim Baxter <jim_baxter@mentor.com> wrote:
> Thank you for you comments, I will fix the issues.
> One question below:
>
> On 17/04/13 23:45, Francois Romieu wrote:
>>> +static int fec_set_features(struct net_device *netdev,
>>> +    netdev_features_t features)
>>> +{
>>> +    struct fec_enet_private *fep = netdev_priv(netdev);
>>> +    netdev_features_t changed = features ^ netdev->features;
>>> +    bool restart_required = false;
>>> +
>>> +    netdev->features = features;
>>> +
>>> +    /* Receive checksum has been changed */
>>> +    if (changed & NETIF_F_RXCSUM) {
>>> +            restart_required = true;
>>> +            if (features & NETIF_F_RXCSUM)
>>> +                    fep->csum_flags |= FLAG_RX_CSUM_ENABLED;
>>> +            else
>>> +                    fep->csum_flags &= ~FLAG_RX_CSUM_ENABLED;
>>
>> ---(snip)----8<------------------------------------------------
>>
>>> +    }
>>> +
>>> +    /* Restart the network interface */
>>> +    if (true == restart_required) {
>>
>> ---(snip)--------------------------------------->8-------------
>>
>> Then remove the "restart_required" variable ?
>>
> What you mean by remove the "restart_required" variable, I only want a
> restart in certain situations? Am I misunderstanding you comment?

I think he meant to use:

  /* Restart the network interface */
  if (restart_required) {

Re: [PATCH net-next v3 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Francois Romieu]

Jim Baxter <jim_baxter@mentor.com> :
[...]
> What you mean by remove the "restart_required" variable, I only want a
> restart in certain situations? Am I misunderstanding you comment?

	bool b = false;

	if (pouet) {
		b = true;
		foo();
	}

	if (b)
		bar();

can be rewritten as

	if (pouet) {
		foo();
		bar();
	}
	
i.e. your code can be reformulated as:

static int fec_set_features(struct net_device *netdev,
	netdev_features_t features)
{
	struct fec_enet_private *fep = netdev_priv(netdev);
	netdev_features_t changed = features ^ netdev->features;

	netdev->features = features;

	/* Receive checksum has been changed */
	if (changed & NETIF_F_RXCSUM) {
		if (features & NETIF_F_RXCSUM)
			fep->csum_flags |= FLAG_RX_CSUM_ENABLED;
		else
			fep->csum_flags &= ~FLAG_RX_CSUM_ENABLED;

		if (netif_running(netdev)) {
			fec_stop(netdev);
			fec_restart(netdev, fep->phy_dev->duplex);
			netif_wake_queue(netdev);
		} else {
			fec_restart(netdev, fep->phy_dev->duplex);
		}
	}

	return 0;
}

-- 
Ueimor

Re: [PATCH net-next v3 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Jim Baxter]

On 18/04/13 22:54, Francois Romieu wrote:
> Jim Baxter <jim_baxter@mentor.com> :
> [...]
>> What you mean by remove the "restart_required" variable, I only want a
>> restart in certain situations? Am I misunderstanding you comment?
> 
> 	bool b = false;
> 
> 	if (pouet) {
> 		b = true;
> 		foo();
> 	}
> 
> 	if (b)
> 		bar();
> 
> can be rewritten as
> 
> 	if (pouet) {
> 		foo();
> 		bar();
> 	}
> 	
> i.e. your code can be reformulated as:
> 
> static int fec_set_features(struct net_device *netdev,
> 	netdev_features_t features)
> {
> 	struct fec_enet_private *fep = netdev_priv(netdev);
> 	netdev_features_t changed = features ^ netdev->features;
> 
> 	netdev->features = features;
> 
> 	/* Receive checksum has been changed */
> 	if (changed & NETIF_F_RXCSUM) {
> 		if (features & NETIF_F_RXCSUM)
> 			fep->csum_flags |= FLAG_RX_CSUM_ENABLED;
> 		else
> 			fep->csum_flags &= ~FLAG_RX_CSUM_ENABLED;
> 
> 		if (netif_running(netdev)) {
> 			fec_stop(netdev);
> 			fec_restart(netdev, fep->phy_dev->duplex);
> 			netif_wake_queue(netdev);
> 		} else {
> 			fec_restart(netdev, fep->phy_dev->duplex);
> 		}
> 	}
> 
> 	return 0;
> }
> 

Thank you, I understand your point now. I was thinking ahead too far, in
case something else needed to restart in set_features. Best to make that
change later if required.

Jim

[PATCH net-next v4 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Jim Baxter]

Enables hardware generation of IP header and
protocol specific checksums for transmitted
packets.

Enabled hardware discarding of received packets with
invalid IP header or protocol specific checksums.

The feature is enabled by default but can be
enabled/disabled by ethtool.

Signed-off-by: Fugang Duan <B38611@freescale.com>
Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
---

- Added IPV6 support.
- Simplified checksum function to remove unnecessary IP header checksum
generation and only update the protocol checksum in
skb->csum_start + skb->csum_offset.
- fec_enet_clear_csum() now returns error value which is checked by
fec_enet_start_xmit() which returns NETDEV_TX_BUSY.
- Simplified fec_set_features() by removing unrequired variables.


 drivers/net/ethernet/freescale/fec.h      |   10 ++-
 drivers/net/ethernet/freescale/fec_main.c |  106 ++++++++++++++++++++++++++++-
 2 files changed, 112 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index eb43729..d44f65b 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -52,6 +52,7 @@
 #define FEC_R_FIFO_RSEM		0x194 /* Receive FIFO section empty threshold */
 #define FEC_R_FIFO_RAEM		0x198 /* Receive FIFO almost empty threshold */
 #define FEC_R_FIFO_RAFL		0x19c /* Receive FIFO almost full threshold */
+#define FEC_RACC		0x1C4 /* Receive Accelerator function */
 #define FEC_MIIGSK_CFGR		0x300 /* MIIGSK Configuration reg */
 #define FEC_MIIGSK_ENR		0x308 /* MIIGSK Enable reg */
 
@@ -164,9 +165,11 @@ struct bufdesc_ex {
 #define BD_ENET_TX_CSL          ((ushort)0x0001)
 #define BD_ENET_TX_STATS        ((ushort)0x03ff)        /* All status bits */
 
-/*enhanced buffer desciptor control/status used by Ethernet transmit*/
+/*enhanced buffer descriptor control/status used by Ethernet transmit*/
 #define BD_ENET_TX_INT          0x40000000
 #define BD_ENET_TX_TS           0x20000000
+#define BD_ENET_TX_PINS         0x10000000
+#define BD_ENET_TX_IINS         0x08000000
 
 
 /* This device has up to three irqs on some platforms */
@@ -190,6 +193,10 @@ struct bufdesc_ex {
 
 #define BD_ENET_RX_INT          0x00800000
 #define BD_ENET_RX_PTP          ((ushort)0x0400)
+#define BD_ENET_RX_ICE		0x00000020
+#define BD_ENET_RX_PCR		0x00000010
+#define FLAG_RX_CSUM_ENABLED	(BD_ENET_RX_ICE | BD_ENET_RX_PCR)
+#define FLAG_RX_CSUM_ERROR	(BD_ENET_RX_ICE | BD_ENET_RX_PCR)
 
 /* The FEC buffer descriptors track the ring buffers.  The rx_bd_base and
  * tx_bd_base always point to the base of the buffer descriptors.  The
@@ -247,6 +254,7 @@ struct fec_enet_private {
 	int	pause_flag;
 
 	struct	napi_struct napi;
+	int	csum_flags;
 
 	struct ptp_clock *ptp_clock;
 	struct ptp_clock_info ptp_caps;
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index d7657a4..d6694fc 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -34,6 +34,12 @@
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/skbuff.h>
+#include <linux/in.h>
+#include <linux/ip.h>
+#include <net/ip.h>
+#include <linux/tcp.h>
+#include <linux/udp.h>
+#include <linux/icmp.h>
 #include <linux/spinlock.h>
 #include <linux/workqueue.h>
 #include <linux/bitops.h>
@@ -181,6 +187,11 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
 #define PKT_MINBUF_SIZE		64
 #define PKT_MAXBLR_SIZE		1520
 
+/* FEC receive acceleration */
+#define FEC_RACC_IPDIS		(1 << 1)
+#define FEC_RACC_PRODIS		(1 << 2)
+#define FEC_RACC_OPTIONS	(FEC_RACC_IPDIS | FEC_RACC_PRODIS)
+
 /*
  * The 5270/5271/5280/5282/532x RX control register also contains maximum frame
  * size bits. Other FEC hardware does not, so we need to take that into
@@ -241,6 +252,21 @@ static void *swap_buffer(void *bufaddr, int len)
 	return bufaddr;
 }
 
+static int
+fec_enet_clear_csum(struct sk_buff *skb, struct net_device *ndev)
+{
+	/* Only run for packets requiring a checksum. */
+	if (skb->ip_summed != CHECKSUM_PARTIAL)
+		return 0;
+
+	if (unlikely(skb_cow_head(skb, 0)))
+		return -1;
+
+	*(__sum16 *)(skb->head + skb->csum_start + skb->csum_offset) = 0;
+
+	return 0;
+}
+
 static netdev_tx_t
 fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 {
@@ -253,7 +279,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	unsigned int index;
 
 	if (!fep->link) {
-		/* Link is down or autonegotiation is in progress. */
+		/* Link is down or auto-negotiation is in progress. */
 		return NETDEV_TX_BUSY;
 	}
 
@@ -270,6 +296,10 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 		return NETDEV_TX_BUSY;
 	}
 
+	/* HW acceleration for ICMP TCP UDP checksum */
+	if (fec_enet_clear_csum(skb, ndev))
+		return NETDEV_TX_BUSY;
+
 	/* Clear all of the status flags */
 	status &= ~BD_ENET_TX_STATS;
 
@@ -326,8 +356,14 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 			ebdp->cbd_esc = (BD_ENET_TX_TS | BD_ENET_TX_INT);
 			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 		} else {
-
 			ebdp->cbd_esc = BD_ENET_TX_INT;
+
+			/* Enable protocol checksum flags
+			 * We do not bother with the IP Checksum bits as they
+			 * are done by the kernel
+			 */
+			if (skb->ip_summed == CHECKSUM_PARTIAL)
+				ebdp->cbd_esc |= BD_ENET_TX_PINS;
 		}
 	}
 	/* If this was the last BD in the ring, start at the beginning again. */
@@ -407,6 +443,7 @@ fec_restart(struct net_device *ndev, int duplex)
 	const struct platform_device_id *id_entry =
 				platform_get_device_id(fep->pdev);
 	int i;
+	u32 val;
 	u32 temp_mac[2];
 	u32 rcntl = OPT_FRAME_SIZE | 0x04;
 	u32 ecntl = 0x2; /* ETHEREN */
@@ -473,6 +510,14 @@ fec_restart(struct net_device *ndev, int duplex)
 	/* Set MII speed */
 	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
 
+	/* set RX checksum */
+	val = readl(fep->hwp + FEC_RACC);
+	if (fep->csum_flags & FLAG_RX_CSUM_ENABLED)
+		val |= FEC_RACC_OPTIONS;
+	else
+		val &= ~FEC_RACC_OPTIONS;
+	writel(val, fep->hwp + FEC_RACC);
+
 	/*
 	 * The phy interface and speed need to get configured
 	 * differently on enet-mac.
@@ -530,7 +575,7 @@ fec_restart(struct net_device *ndev, int duplex)
 	     fep->phy_dev && fep->phy_dev->pause)) {
 		rcntl |= FEC_ENET_FCE;
 
-		/* set FIFO thresh hold parameter to reduce overrun */
+		/* set FIFO threshold parameter to reduce overrun */
 		writel(FEC_ENET_RSEM_V, fep->hwp + FEC_R_FIFO_RSEM);
 		writel(FEC_ENET_RSFL_V, fep->hwp + FEC_R_FIFO_RSFL);
 		writel(FEC_ENET_RAEM_V, fep->hwp + FEC_R_FIFO_RAEM);
@@ -818,6 +863,18 @@ fec_enet_rx(struct net_device *ndev, int budget)
 				spin_unlock_irqrestore(&fep->tmreg_lock, flags);
 			}
 
+			if (fep->bufdesc_ex &&
+				(fep->csum_flags & FLAG_RX_CSUM_ENABLED)) {
+				struct bufdesc_ex *ebdp =
+					(struct bufdesc_ex *)bdp;
+				if (!(ebdp->cbd_esc & FLAG_RX_CSUM_ERROR)) {
+					/* don't check it */
+					skb->ip_summed = CHECKSUM_UNNECESSARY;
+				} else {
+					skb_checksum_none_assert(skb);
+				}
+			}
+
 			if (!skb_defer_rx_timestamp(skb))
 				napi_gro_receive(&fep->napi, skb);
 		}
@@ -1439,6 +1496,14 @@ static int fec_enet_alloc_buffers(struct net_device *ndev)
 		if (fep->bufdesc_ex) {
 			struct bufdesc_ex *ebdp = (struct bufdesc_ex *)bdp;
 			ebdp->cbd_esc = BD_ENET_TX_INT;
+
+			/* Enable protocol checksum flags
+			 * We do not bother with the IP Checksum bits as they
+			 * are done by the kernel
+			 */
+			if (ndev->features &
+					(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM))
+				ebdp->cbd_esc |= BD_ENET_TX_PINS;
 		}
 
 		bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
@@ -1618,6 +1683,33 @@ static void fec_poll_controller(struct net_device *dev)
 }
 #endif
 
+static int fec_set_features(struct net_device *netdev,
+	netdev_features_t features)
+{
+	struct fec_enet_private *fep = netdev_priv(netdev);
+	netdev_features_t changed = features ^ netdev->features;
+
+	netdev->features = features;
+
+	/* Receive checksum has been changed */
+	if (changed & NETIF_F_RXCSUM) {
+		if (features & NETIF_F_RXCSUM)
+			fep->csum_flags |= FLAG_RX_CSUM_ENABLED;
+		else
+			fep->csum_flags &= ~FLAG_RX_CSUM_ENABLED;
+
+		if (netif_running(netdev)) {
+			fec_stop(netdev);
+			fec_restart(netdev, fep->phy_dev->duplex);
+			netif_wake_queue(netdev);
+		} else {
+			fec_restart(netdev, fep->phy_dev->duplex);
+		}
+	}
+
+	return 0;
+}
+
 static const struct net_device_ops fec_netdev_ops = {
 	.ndo_open		= fec_enet_open,
 	.ndo_stop		= fec_enet_close,
@@ -1631,6 +1723,7 @@ static const struct net_device_ops fec_netdev_ops = {
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= fec_poll_controller,
 #endif
+	.ndo_set_features	= fec_set_features,
 };
 
  /*
@@ -1672,6 +1765,13 @@ static int fec_enet_init(struct net_device *ndev)
 	writel(FEC_RX_DISABLED_IMASK, fep->hwp + FEC_IMASK);
 	netif_napi_add(ndev, &fep->napi, fec_enet_rx_napi, FEC_NAPI_WEIGHT);
 
+	/* enable hw accelerator */
+	ndev->features |= (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM
+			| NETIF_F_RXCSUM);
+	ndev->hw_features |= (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM
+			| NETIF_F_RXCSUM);
+	fep->csum_flags |= FLAG_RX_CSUM_ENABLED;
+
 	fec_restart(ndev, 0);
 
 	return 0;
-- 
1.7.10.4

Re: [PATCH net-next v4 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Eric Dumazet]

On Fri, 2013-04-19 at 16:10 +0100, Jim Baxter wrote:
>  
> +static int
> +fec_enet_clear_csum(struct sk_buff *skb, struct net_device *ndev)
> +{
> +	/* Only run for packets requiring a checksum. */
> +	if (skb->ip_summed != CHECKSUM_PARTIAL)
> +		return 0;
> +
> +	if (unlikely(skb_cow_head(skb, 0)))
> +		return -1;
> +
> +	*(__sum16 *)(skb->head + skb->csum_start + skb->csum_offset) = 0;
> +
> +	return 0;
> +}


 
> +	/* HW acceleration for ICMP TCP UDP checksum */
> +	if (fec_enet_clear_csum(skb, ndev))
> +		return NETDEV_TX_BUSY;

No : You must drop the packet and return NETDEV_TX_OK

Re: [PATCH net-next v4 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Jim Baxter]

On 19/04/13 16:29, Eric Dumazet wrote:
> On Fri, 2013-04-19 at 16:10 +0100, Jim Baxter wrote:
>>  
>> +static int
>> +fec_enet_clear_csum(struct sk_buff *skb, struct net_device *ndev)
>> +{
>> +	/* Only run for packets requiring a checksum. */
>> +	if (skb->ip_summed != CHECKSUM_PARTIAL)
>> +		return 0;
>> +
>> +	if (unlikely(skb_cow_head(skb, 0)))
>> +		return -1;
>> +
>> +	*(__sum16 *)(skb->head + skb->csum_start + skb->csum_offset) = 0;
>> +
>> +	return 0;
>> +}
> 
> 
>  
>> +	/* HW acceleration for ICMP TCP UDP checksum */
>> +	if (fec_enet_clear_csum(skb, ndev))
>> +		return NETDEV_TX_BUSY;
> 
> No : You must drop the packet and return NETDEV_TX_OK
> 
> 
> 

I have no issue with changing it, but I am curious, by returning OK will
the kernel not regard it a sent packet and it will be lost?

Thank you,
Jim

Re: [PATCH net-next v4 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Ben Hutchings]

On Fri, 2013-04-19 at 16:55 +0100, Jim Baxter wrote:
> On 19/04/13 16:29, Eric Dumazet wrote:
> > On Fri, 2013-04-19 at 16:10 +0100, Jim Baxter wrote:
> >>  
> >> +static int
> >> +fec_enet_clear_csum(struct sk_buff *skb, struct net_device *ndev)
> >> +{
> >> +	/* Only run for packets requiring a checksum. */
> >> +	if (skb->ip_summed != CHECKSUM_PARTIAL)
> >> +		return 0;
> >> +
> >> +	if (unlikely(skb_cow_head(skb, 0)))
> >> +		return -1;
> >> +
> >> +	*(__sum16 *)(skb->head + skb->csum_start + skb->csum_offset) = 0;
> >> +
> >> +	return 0;
> >> +}
> > 
> > 
> >  
> >> +	/* HW acceleration for ICMP TCP UDP checksum */
> >> +	if (fec_enet_clear_csum(skb, ndev))
> >> +		return NETDEV_TX_BUSY;
> > 
> > No : You must drop the packet and return NETDEV_TX_OK
> > 
> > 
> > 
> 
> I have no issue with changing it, but I am curious, by returning OK will
> the kernel not regard it a sent packet and it will be lost?

Correct.  This is normal behaviour when short of memory; look at any
other driver that allocates memory on the TX path.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next v4 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Eric Dumazet]

On Fri, 2013-04-19 at 16:55 +0100, Jim Baxter wrote:

> 
> I have no issue with changing it, but I am curious, by returning OK will
> the kernel not regard it a sent packet and it will be lost?

Answer is : The sender will loop forever trying to send this packet.
Just drop it and avoid a possible deadlock.

NETDEV_TX_BUSY is deprecated and its use is not for solving this kind of
problem.

Re: [PATCH net-next v4 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Ben Hutchings]

On Fri, 2013-04-19 at 16:10 +0100, Jim Baxter wrote:
> Enables hardware generation of IP header and
> protocol specific checksums for transmitted
> packets.
> 
> Enabled hardware discarding of received packets with
> invalid IP header or protocol specific checksums.
> 
> The feature is enabled by default but can be
> enabled/disabled by ethtool.
> 
> Signed-off-by: Fugang Duan <B38611@freescale.com>
> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
> ---
> 
> - Added IPV6 support.
[...]
> @@ -1439,6 +1496,14 @@ static int fec_enet_alloc_buffers(struct net_device *ndev)
>  		if (fep->bufdesc_ex) {
>  			struct bufdesc_ex *ebdp = (struct bufdesc_ex *)bdp;
>  			ebdp->cbd_esc = BD_ENET_TX_INT;
> +
> +			/* Enable protocol checksum flags
> +			 * We do not bother with the IP Checksum bits as they
> +			 * are done by the kernel
> +			 */
> +			if (ndev->features &
> +					(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM))
> +				ebdp->cbd_esc |= BD_ENET_TX_PINS;
>  		}
>  
>  		bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
[...]

This condition needs to be based on skb->ip_summed.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next v4 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Jim Baxter]

On 19/04/13 16:34, Ben Hutchings wrote:
> On Fri, 2013-04-19 at 16:10 +0100, Jim Baxter wrote:
>> Enables hardware generation of IP header and
>> protocol specific checksums for transmitted
>> packets.
>>
>> Enabled hardware discarding of received packets with
>> invalid IP header or protocol specific checksums.
>>
>> The feature is enabled by default but can be
>> enabled/disabled by ethtool.
>>
>> Signed-off-by: Fugang Duan <B38611@freescale.com>
>> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
>> ---
>>
>> - Added IPV6 support.
> [...]
>> @@ -1439,6 +1496,14 @@ static int fec_enet_alloc_buffers(struct net_device *ndev)
>>  		if (fep->bufdesc_ex) {
>>  			struct bufdesc_ex *ebdp = (struct bufdesc_ex *)bdp;
>>  			ebdp->cbd_esc = BD_ENET_TX_INT;
>> +
>> +			/* Enable protocol checksum flags
>> +			 * We do not bother with the IP Checksum bits as they
>> +			 * are done by the kernel
>> +			 */
>> +			if (ndev->features &
>> +					(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM))
>> +				ebdp->cbd_esc |= BD_ENET_TX_PINS;
>>  		}
>>  
>>  		bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> [...]
> 
> This condition needs to be based on skb->ip_summed.
> 
> Ben.
> 


These buffers are setup when the network driver is opened
(fec_enet_open), with the same settings as the transmit buffers.

Thinking about it, they should probably not be set here and only set
during the fec_enet_start_xmit() function.

Jim

Re: [PATCH net-next v4 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Ben Hutchings]

On Fri, 2013-04-19 at 17:16 +0100, Jim Baxter wrote:
> On 19/04/13 16:34, Ben Hutchings wrote:
> > On Fri, 2013-04-19 at 16:10 +0100, Jim Baxter wrote:
> >> Enables hardware generation of IP header and
> >> protocol specific checksums for transmitted
> >> packets.
> >>
> >> Enabled hardware discarding of received packets with
> >> invalid IP header or protocol specific checksums.
> >>
> >> The feature is enabled by default but can be
> >> enabled/disabled by ethtool.
> >>
> >> Signed-off-by: Fugang Duan <B38611@freescale.com>
> >> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
> >> ---
> >>
> >> - Added IPV6 support.
> > [...]
> >> @@ -1439,6 +1496,14 @@ static int fec_enet_alloc_buffers(struct net_device *ndev)
> >>  		if (fep->bufdesc_ex) {
> >>  			struct bufdesc_ex *ebdp = (struct bufdesc_ex *)bdp;
> >>  			ebdp->cbd_esc = BD_ENET_TX_INT;
> >> +
> >> +			/* Enable protocol checksum flags
> >> +			 * We do not bother with the IP Checksum bits as they
> >> +			 * are done by the kernel
> >> +			 */
> >> +			if (ndev->features &
> >> +					(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM))
> >> +				ebdp->cbd_esc |= BD_ENET_TX_PINS;
> >>  		}
> >>  
> >>  		bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> > [...]
> > 
> > This condition needs to be based on skb->ip_summed.
> > 
> > Ben.
> > 
> 
> 
> These buffers are setup when the network driver is opened
> (fec_enet_open), with the same settings as the transmit buffers.
> 
> Thinking about it, they should probably not be set here and only set
> during the fec_enet_start_xmit() function.

Right.  Even when TX checksum offload is enabled, the kernel might not
want you to offload TX checksums for every packet.  For example, if your
interface is part of a bridge, packets should be forwarded even if they
have a bad layer-4 checksum on RX, and the checksum must not be
'corrected' on TX.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH net-next v5 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Jim Baxter]

Enables hardware generation of IP header and
protocol specific checksums for transmitted
packets.

Enabled hardware discarding of received packets with
invalid IP header or protocol specific checksums.

The feature is enabled by default but can be
enabled/disabled by ethtool.

Signed-off-by: Fugang Duan <B38611@freescale.com>
Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
---
Removed unnecessary initialisation of TX ring buffer.
The skb is now freed if the checksum clearing fails.
  
 drivers/net/ethernet/freescale/fec.h      |   10 ++-
 drivers/net/ethernet/freescale/fec_main.c |  100 ++++++++++++++++++++++++++++-
 2 files changed, 106 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index eb43729..d44f65b 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -52,6 +52,7 @@
 #define FEC_R_FIFO_RSEM		0x194 /* Receive FIFO section empty threshold */
 #define FEC_R_FIFO_RAEM		0x198 /* Receive FIFO almost empty threshold */
 #define FEC_R_FIFO_RAFL		0x19c /* Receive FIFO almost full threshold */
+#define FEC_RACC		0x1C4 /* Receive Accelerator function */
 #define FEC_MIIGSK_CFGR		0x300 /* MIIGSK Configuration reg */
 #define FEC_MIIGSK_ENR		0x308 /* MIIGSK Enable reg */
 
@@ -164,9 +165,11 @@ struct bufdesc_ex {
 #define BD_ENET_TX_CSL          ((ushort)0x0001)
 #define BD_ENET_TX_STATS        ((ushort)0x03ff)        /* All status bits */
 
-/*enhanced buffer desciptor control/status used by Ethernet transmit*/
+/*enhanced buffer descriptor control/status used by Ethernet transmit*/
 #define BD_ENET_TX_INT          0x40000000
 #define BD_ENET_TX_TS           0x20000000
+#define BD_ENET_TX_PINS         0x10000000
+#define BD_ENET_TX_IINS         0x08000000
 
 
 /* This device has up to three irqs on some platforms */
@@ -190,6 +193,10 @@ struct bufdesc_ex {
 
 #define BD_ENET_RX_INT          0x00800000
 #define BD_ENET_RX_PTP          ((ushort)0x0400)
+#define BD_ENET_RX_ICE		0x00000020
+#define BD_ENET_RX_PCR		0x00000010
+#define FLAG_RX_CSUM_ENABLED	(BD_ENET_RX_ICE | BD_ENET_RX_PCR)
+#define FLAG_RX_CSUM_ERROR	(BD_ENET_RX_ICE | BD_ENET_RX_PCR)
 
 /* The FEC buffer descriptors track the ring buffers.  The rx_bd_base and
  * tx_bd_base always point to the base of the buffer descriptors.  The
@@ -247,6 +254,7 @@ struct fec_enet_private {
 	int	pause_flag;
 
 	struct	napi_struct napi;
+	int	csum_flags;
 
 	struct ptp_clock *ptp_clock;
 	struct ptp_clock_info ptp_caps;
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index d7657a4..1b4a5a5 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -34,6 +34,12 @@
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/skbuff.h>
+#include <linux/in.h>
+#include <linux/ip.h>
+#include <net/ip.h>
+#include <linux/tcp.h>
+#include <linux/udp.h>
+#include <linux/icmp.h>
 #include <linux/spinlock.h>
 #include <linux/workqueue.h>
 #include <linux/bitops.h>
@@ -181,6 +187,11 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
 #define PKT_MINBUF_SIZE		64
 #define PKT_MAXBLR_SIZE		1520
 
+/* FEC receive acceleration */
+#define FEC_RACC_IPDIS		(1 << 1)
+#define FEC_RACC_PRODIS		(1 << 2)
+#define FEC_RACC_OPTIONS	(FEC_RACC_IPDIS | FEC_RACC_PRODIS)
+
 /*
  * The 5270/5271/5280/5282/532x RX control register also contains maximum frame
  * size bits. Other FEC hardware does not, so we need to take that into
@@ -241,6 +252,21 @@ static void *swap_buffer(void *bufaddr, int len)
 	return bufaddr;
 }
 
+static int
+fec_enet_clear_csum(struct sk_buff *skb, struct net_device *ndev)
+{
+	/* Only run for packets requiring a checksum. */
+	if (skb->ip_summed != CHECKSUM_PARTIAL)
+		return 0;
+
+	if (unlikely(skb_cow_head(skb, 0)))
+		return -1;
+
+	*(__sum16 *)(skb->head + skb->csum_start + skb->csum_offset) = 0;
+
+	return 0;
+}
+
 static netdev_tx_t
 fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 {
@@ -253,7 +279,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	unsigned int index;
 
 	if (!fep->link) {
-		/* Link is down or autonegotiation is in progress. */
+		/* Link is down or auto-negotiation is in progress. */
 		return NETDEV_TX_BUSY;
 	}
 
@@ -270,6 +296,12 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 		return NETDEV_TX_BUSY;
 	}
 
+	/* Protocol checksum off-load for TCP and UDP. */
+	if (fec_enet_clear_csum(skb, ndev)) {
+		kfree_skb(skb);
+		return NETDEV_TX_OK;
+	}
+
 	/* Clear all of the status flags */
 	status &= ~BD_ENET_TX_STATS;
 
@@ -326,8 +358,14 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 			ebdp->cbd_esc = (BD_ENET_TX_TS | BD_ENET_TX_INT);
 			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 		} else {
-
 			ebdp->cbd_esc = BD_ENET_TX_INT;
+
+			/* Enable protocol checksum flags
+			 * We do not bother with the IP Checksum bits as they
+			 * are done by the kernel
+			 */
+			if (skb->ip_summed == CHECKSUM_PARTIAL)
+				ebdp->cbd_esc |= BD_ENET_TX_PINS;
 		}
 	}
 	/* If this was the last BD in the ring, start at the beginning again. */
@@ -407,6 +445,7 @@ fec_restart(struct net_device *ndev, int duplex)
 	const struct platform_device_id *id_entry =
 				platform_get_device_id(fep->pdev);
 	int i;
+	u32 val;
 	u32 temp_mac[2];
 	u32 rcntl = OPT_FRAME_SIZE | 0x04;
 	u32 ecntl = 0x2; /* ETHEREN */
@@ -473,6 +512,14 @@ fec_restart(struct net_device *ndev, int duplex)
 	/* Set MII speed */
 	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
 
+	/* set RX checksum */
+	val = readl(fep->hwp + FEC_RACC);
+	if (fep->csum_flags & FLAG_RX_CSUM_ENABLED)
+		val |= FEC_RACC_OPTIONS;
+	else
+		val &= ~FEC_RACC_OPTIONS;
+	writel(val, fep->hwp + FEC_RACC);
+
 	/*
 	 * The phy interface and speed need to get configured
 	 * differently on enet-mac.
@@ -530,7 +577,7 @@ fec_restart(struct net_device *ndev, int duplex)
 	     fep->phy_dev && fep->phy_dev->pause)) {
 		rcntl |= FEC_ENET_FCE;
 
-		/* set FIFO thresh hold parameter to reduce overrun */
+		/* set FIFO threshold parameter to reduce overrun */
 		writel(FEC_ENET_RSEM_V, fep->hwp + FEC_R_FIFO_RSEM);
 		writel(FEC_ENET_RSFL_V, fep->hwp + FEC_R_FIFO_RSFL);
 		writel(FEC_ENET_RAEM_V, fep->hwp + FEC_R_FIFO_RAEM);
@@ -818,6 +865,18 @@ fec_enet_rx(struct net_device *ndev, int budget)
 				spin_unlock_irqrestore(&fep->tmreg_lock, flags);
 			}
 
+			if (fep->bufdesc_ex &&
+				(fep->csum_flags & FLAG_RX_CSUM_ENABLED)) {
+				struct bufdesc_ex *ebdp =
+					(struct bufdesc_ex *)bdp;
+				if (!(ebdp->cbd_esc & FLAG_RX_CSUM_ERROR)) {
+					/* don't check it */
+					skb->ip_summed = CHECKSUM_UNNECESSARY;
+				} else {
+					skb_checksum_none_assert(skb);
+				}
+			}
+
 			if (!skb_defer_rx_timestamp(skb))
 				napi_gro_receive(&fep->napi, skb);
 		}
@@ -1618,6 +1677,33 @@ static void fec_poll_controller(struct net_device *dev)
 }
 #endif
 
+static int fec_set_features(struct net_device *netdev,
+	netdev_features_t features)
+{
+	struct fec_enet_private *fep = netdev_priv(netdev);
+	netdev_features_t changed = features ^ netdev->features;
+
+	netdev->features = features;
+
+	/* Receive checksum has been changed */
+	if (changed & NETIF_F_RXCSUM) {
+		if (features & NETIF_F_RXCSUM)
+			fep->csum_flags |= FLAG_RX_CSUM_ENABLED;
+		else
+			fep->csum_flags &= ~FLAG_RX_CSUM_ENABLED;
+
+		if (netif_running(netdev)) {
+			fec_stop(netdev);
+			fec_restart(netdev, fep->phy_dev->duplex);
+			netif_wake_queue(netdev);
+		} else {
+			fec_restart(netdev, fep->phy_dev->duplex);
+		}
+	}
+
+	return 0;
+}
+
 static const struct net_device_ops fec_netdev_ops = {
 	.ndo_open		= fec_enet_open,
 	.ndo_stop		= fec_enet_close,
@@ -1631,6 +1717,7 @@ static const struct net_device_ops fec_netdev_ops = {
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= fec_poll_controller,
 #endif
+	.ndo_set_features	= fec_set_features,
 };
 
  /*
@@ -1672,6 +1759,13 @@ static int fec_enet_init(struct net_device *ndev)
 	writel(FEC_RX_DISABLED_IMASK, fep->hwp + FEC_IMASK);
 	netif_napi_add(ndev, &fep->napi, fec_enet_rx_napi, FEC_NAPI_WEIGHT);
 
+	/* enable hw accelerator */
+	ndev->features |= (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM
+			| NETIF_F_RXCSUM);
+	ndev->hw_features |= (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM
+			| NETIF_F_RXCSUM);
+	fep->csum_flags |= FLAG_RX_CSUM_ENABLED;
+
 	fec_restart(ndev, 0);
 
 	return 0;
-- 
1.7.10.4

Re: [PATCH net-next v5 1/1] net: fec: Enable imx6 enet checksum acceleration.

[Ben Hutchings]

On Fri, 2013-04-19 at 19:10 +0100, Jim Baxter wrote:
> Enables hardware generation of IP header and
> protocol specific checksums for transmitted
> packets.
> 
> Enabled hardware discarding of received packets with
> invalid IP header or protocol specific checksums.
> 
> The feature is enabled by default but can be
> enabled/disabled by ethtool.
> 
> Signed-off-by: Fugang Duan <B38611@freescale.com>
> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>

Reviewed-by: Ben Hutchings <bhutchings@solarflare.com>

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next v5 1/1] net: fec: Enable imx6 enet checksum acceleration.

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Fri, 19 Apr 2013 19:50:44 +0100

> On Fri, 2013-04-19 at 19:10 +0100, Jim Baxter wrote:
>> Enables hardware generation of IP header and
>> protocol specific checksums for transmitted
>> packets.
>> 
>> Enabled hardware discarding of received packets with
>> invalid IP header or protocol specific checksums.
>> 
>> The feature is enabled by default but can be
>> enabled/disabled by ethtool.
>> 
>> Signed-off-by: Fugang Duan <B38611@freescale.com>
>> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
> 
> Reviewed-by: Ben Hutchings <bhutchings@solarflare.com>

Applied.

[PATCH net-next v1 1/1] net: fec: Add VLAN receive HW support.

[Jim Baxter]

This enables the driver to take adavantage of the FEC VLAN
indicator to improve performance.

Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
---
 drivers/net/ethernet/freescale/fec.h      |    3 ++
 drivers/net/ethernet/freescale/fec_main.c |   64 +++++++++++++++++++++++------
 2 files changed, 55 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index e3ed6c5..85e75d9 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -148,6 +148,9 @@ struct bufdesc_ex {
 #define BD_ENET_RX_CL           ((ushort)0x0001)
 #define BD_ENET_RX_STATS        ((ushort)0x013f)        /* All status bits */
 
+/* Enhanced buffer descriptor control/status used by Ethernet receive */
+#define BD_ENET_RX_VLAN         0x00000004
+
 /* Buffer descriptor control/status used by Ethernet transmit.
 */
 #define BD_ENET_TX_READY        ((ushort)0x8000)
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 46f2544..851d836 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -54,6 +54,7 @@
 #include <linux/of_gpio.h>
 #include <linux/of_net.h>
 #include <linux/regulator/consumer.h>
+#include <linux/if_vlan.h>
 
 #include <asm/cacheflush.h>
 
@@ -88,6 +89,8 @@
 #define FEC_QUIRK_HAS_BUFDESC_EX	(1 << 4)
 /* Controller has hardware checksum support */
 #define FEC_QUIRK_HAS_CSUM		(1 << 5)
+/* Controller has hardware vlan support */
+#define FEC_QUIRK_HAS_VLAN		(1 << 6)
 
 static struct platform_device_id fec_devtype[] = {
 	{
@@ -106,7 +109,8 @@ static struct platform_device_id fec_devtype[] = {
 	}, {
 		.name = "imx6q-fec",
 		.driver_data = FEC_QUIRK_ENET_MAC | FEC_QUIRK_HAS_GBIT |
-				FEC_QUIRK_HAS_BUFDESC_EX | FEC_QUIRK_HAS_CSUM,
+				FEC_QUIRK_HAS_BUFDESC_EX | FEC_QUIRK_HAS_CSUM |
+				FEC_QUIRK_HAS_VLAN,
 	}, {
 		.name = "mvf600-fec",
 		.driver_data = FEC_QUIRK_ENET_MAC,
@@ -177,11 +181,11 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
 #define FEC_DEFAULT_IMASK (FEC_ENET_TXF | FEC_ENET_RXF | FEC_ENET_MII)
 #define FEC_RX_DISABLED_IMASK (FEC_DEFAULT_IMASK & (~FEC_ENET_RXF))
 
-/* The FEC stores dest/src/type, data, and checksum for receive packets.
+/* The FEC stores dest/src/type/vlan, data, and checksum for receive packets.
  */
-#define PKT_MAXBUF_SIZE		1518
+#define PKT_MAXBUF_SIZE		1522
 #define PKT_MINBUF_SIZE		64
-#define PKT_MAXBLR_SIZE		1520
+#define PKT_MAXBLR_SIZE		1540
 
 /* FEC receive acceleration */
 #define FEC_RACC_IPDIS		(1 << 1)
@@ -795,6 +799,9 @@ fec_enet_rx(struct net_device *ndev, int budget)
 	ushort	pkt_len;
 	__u8 *data;
 	int	pkt_received = 0;
+	struct	bufdesc_ex *ebdp = NULL;
+	bool	vlan_packet_rcvd = false;
+	u16	vlan_tag;
 
 #ifdef CONFIG_M532x
 	flush_cache_all();
@@ -858,6 +865,24 @@ fec_enet_rx(struct net_device *ndev, int budget)
 		if (id_entry->driver_data & FEC_QUIRK_SWAP_FRAME)
 			swap_buffer(data, pkt_len);
 
+		/* Extract the enhanced buffer descriptor */
+		ebdp = NULL;
+		if (fep->bufdesc_ex)
+			ebdp = (struct bufdesc_ex *)bdp;
+
+		/* If this is a VLAN packet remove the VLAN Tag */
+		vlan_packet_rcvd = false;
+		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
+				ebdp && (ebdp->cbd_esc & BD_ENET_RX_VLAN)) {
+			/* Push and remove the vlan tag */
+			struct vlan_hdr *vlan_header =
+					(struct vlan_hdr *) (data + ETH_HLEN);
+			vlan_tag = ntohs(vlan_header->h_vlan_TCI);
+			pkt_len -= VLAN_HLEN;
+
+			vlan_packet_rcvd = true;
+		}
+
 		/* This does 16 byte alignment, exactly what we need.
 		 * The packet length includes FCS, but we don't want to
 		 * include that when passing upstream as it messes up
@@ -868,18 +893,25 @@ fec_enet_rx(struct net_device *ndev, int budget)
 		if (unlikely(!skb)) {
 			ndev->stats.rx_dropped++;
 		} else {
+			int payload_offset = (2 * ETH_ALEN);
 			skb_reserve(skb, NET_IP_ALIGN);
 			skb_put(skb, pkt_len - 4);	/* Make room */
-			skb_copy_to_linear_data(skb, data, pkt_len - 4);
+
+			/* Extract the frame data without the VLAN header. */
+			skb_copy_to_linear_data(skb, data, (2 * ETH_ALEN));
+			if (vlan_packet_rcvd)
+				payload_offset = (2 * ETH_ALEN) + VLAN_HLEN;
+			skb_copy_to_linear_data_offset(skb, (2 * ETH_ALEN),
+					data + payload_offset,
+					pkt_len - 4 - (2 * ETH_ALEN));
+
 			skb->protocol = eth_type_trans(skb, ndev);
 
 			/* Get receive timestamp from the skb */
-			if (fep->hwts_rx_en && fep->bufdesc_ex) {
+			if (fep->hwts_rx_en && ebdp) {
 				struct skb_shared_hwtstamps *shhwtstamps =
 							    skb_hwtstamps(skb);
 				unsigned long flags;
-				struct bufdesc_ex *ebdp =
-					(struct bufdesc_ex *)bdp;
 
 				memset(shhwtstamps, 0, sizeof(*shhwtstamps));
 
@@ -889,10 +921,7 @@ fec_enet_rx(struct net_device *ndev, int budget)
 				spin_unlock_irqrestore(&fep->tmreg_lock, flags);
 			}
 
-			if (fep->bufdesc_ex &&
-				(fep->csum_flags & FLAG_RX_CSUM_ENABLED)) {
-				struct bufdesc_ex *ebdp =
-					(struct bufdesc_ex *)bdp;
+			if (ebdp && (fep->csum_flags & FLAG_RX_CSUM_ENABLED)) {
 				if (!(ebdp->cbd_esc & FLAG_RX_CSUM_ERROR)) {
 					/* don't check it */
 					skb->ip_summed = CHECKSUM_UNNECESSARY;
@@ -901,6 +930,11 @@ fec_enet_rx(struct net_device *ndev, int budget)
 				}
 			}
 
+			/* Handle received VLAN packets */
+			if (vlan_packet_rcvd)
+				__vlan_hwaccel_put_tag(skb,
+						htons(ETH_P_8021Q), vlan_tag);
+
 			if (!skb_defer_rx_timestamp(skb))
 				napi_gro_receive(&fep->napi, skb);
 		}
@@ -1802,6 +1836,12 @@ static int fec_enet_init(struct net_device *ndev)
 	writel(FEC_RX_DISABLED_IMASK, fep->hwp + FEC_IMASK);
 	netif_napi_add(ndev, &fep->napi, fec_enet_rx_napi, FEC_NAPI_WEIGHT);
 
+	if (id_entry->driver_data & FEC_QUIRK_HAS_VLAN) {
+		/* enable hw VLAN support */
+		ndev->features |= NETIF_F_HW_VLAN_CTAG_RX;
+		ndev->hw_features |= NETIF_F_HW_VLAN_CTAG_RX;
+	}
+
 	if (id_entry->driver_data & FEC_QUIRK_HAS_CSUM) {
 		/* enable hw accelerator */
 		ndev->features |= (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM
-- 
1.7.10.4

RE: [PATCH net-next v1 1/1] net: fec: Add VLAN receive HW support.

[Duan Fugang-B38611]

Comments.[Andy] 

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Jim Baxter
Sent: Wednesday, June 26, 2013 7:55 AM
To: David S. Miller
Cc: Li Frank-B20596; Duan Fugang-B38611; Estevam Fabio-R49496; Lucas Stach; Shawn Guo; netdev@vger.kernel.org; Ben Hutchings
Subject: [PATCH net-next v1 1/1] net: fec: Add VLAN receive HW support.

This enables the driver to take adavantage of the FEC VLAN indicator to improve performance.

Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
---
 drivers/net/ethernet/freescale/fec.h      |    3 ++
 drivers/net/ethernet/freescale/fec_main.c |   64 +++++++++++++++++++++++------
 2 files changed, 55 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index e3ed6c5..85e75d9 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -148,6 +148,9 @@ struct bufdesc_ex {
 #define BD_ENET_RX_CL           ((ushort)0x0001)
 #define BD_ENET_RX_STATS        ((ushort)0x013f)        /* All status bits */
 
+/* Enhanced buffer descriptor control/status used by Ethernet receive */
+#define BD_ENET_RX_VLAN         0x00000004
+
 /* Buffer descriptor control/status used by Ethernet transmit.
 */
 #define BD_ENET_TX_READY        ((ushort)0x8000)
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 46f2544..851d836 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -54,6 +54,7 @@
 #include <linux/of_gpio.h>
 #include <linux/of_net.h>
 #include <linux/regulator/consumer.h>
+#include <linux/if_vlan.h>
 
 #include <asm/cacheflush.h>
 
@@ -88,6 +89,8 @@
 #define FEC_QUIRK_HAS_BUFDESC_EX	(1 << 4)
 /* Controller has hardware checksum support */
 #define FEC_QUIRK_HAS_CSUM		(1 << 5)
+/* Controller has hardware vlan support */
+#define FEC_QUIRK_HAS_VLAN		(1 << 6)
 
 static struct platform_device_id fec_devtype[] = {
 	{
@@ -106,7 +109,8 @@ static struct platform_device_id fec_devtype[] = {
 	}, {
 		.name = "imx6q-fec",
 		.driver_data = FEC_QUIRK_ENET_MAC | FEC_QUIRK_HAS_GBIT |
-				FEC_QUIRK_HAS_BUFDESC_EX | FEC_QUIRK_HAS_CSUM,
+				FEC_QUIRK_HAS_BUFDESC_EX | FEC_QUIRK_HAS_CSUM |
+				FEC_QUIRK_HAS_VLAN,
 	}, {
 		.name = "mvf600-fec",
 		.driver_data = FEC_QUIRK_ENET_MAC,
@@ -177,11 +181,11 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");  #define FEC_DEFAULT_IMASK (FEC_ENET_TXF | FEC_ENET_RXF | FEC_ENET_MII)  #define FEC_RX_DISABLED_IMASK (FEC_DEFAULT_IMASK & (~FEC_ENET_RXF))
 
-/* The FEC stores dest/src/type, data, and checksum for receive packets.
+/* The FEC stores dest/src/type/vlan, data, and checksum for receive packets.
  */
-#define PKT_MAXBUF_SIZE		1518
+#define PKT_MAXBUF_SIZE		1522
 #define PKT_MINBUF_SIZE		64
-#define PKT_MAXBLR_SIZE		1520
+#define PKT_MAXBLR_SIZE		1540
[Andy]Pls define the MAXBLR size to 1536, which is multiplied by 64
	 #define PKT_MAXBLR_SIZE         1536 
 
 /* FEC receive acceleration */
 #define FEC_RACC_IPDIS		(1 << 1)
@@ -795,6 +799,9 @@ fec_enet_rx(struct net_device *ndev, int budget)
 	ushort	pkt_len;
 	__u8 *data;
 	int	pkt_received = 0;
+	struct	bufdesc_ex *ebdp = NULL;
+	bool	vlan_packet_rcvd = false;
+	u16	vlan_tag;
 
 #ifdef CONFIG_M532x
 	flush_cache_all();
@@ -858,6 +865,24 @@ fec_enet_rx(struct net_device *ndev, int budget)
 		if (id_entry->driver_data & FEC_QUIRK_SWAP_FRAME)
 			swap_buffer(data, pkt_len);
 
+		/* Extract the enhanced buffer descriptor */
+		ebdp = NULL;
+		if (fep->bufdesc_ex)
+			ebdp = (struct bufdesc_ex *)bdp;
+
+		/* If this is a VLAN packet remove the VLAN Tag */
+		vlan_packet_rcvd = false;
+		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
+				ebdp && (ebdp->cbd_esc & BD_ENET_RX_VLAN)) {
+			/* Push and remove the vlan tag */
+			struct vlan_hdr *vlan_header =
+					(struct vlan_hdr *) (data + ETH_HLEN);
+			vlan_tag = ntohs(vlan_header->h_vlan_TCI);
+			pkt_len -= VLAN_HLEN;
+
+			vlan_packet_rcvd = true;
+		}
+
 		/* This does 16 byte alignment, exactly what we need.
 		 * The packet length includes FCS, but we don't want to
 		 * include that when passing upstream as it messes up @@ -868,18 +893,25 @@ fec_enet_rx(struct net_device *ndev, int budget)
 		if (unlikely(!skb)) {
 			ndev->stats.rx_dropped++;
 		} else {
+			int payload_offset = (2 * ETH_ALEN);
 			skb_reserve(skb, NET_IP_ALIGN);
 			skb_put(skb, pkt_len - 4);	/* Make room */
-			skb_copy_to_linear_data(skb, data, pkt_len - 4);
+
+			/* Extract the frame data without the VLAN header. */
+			skb_copy_to_linear_data(skb, data, (2 * ETH_ALEN));
+			if (vlan_packet_rcvd)
+				payload_offset = (2 * ETH_ALEN) + VLAN_HLEN;
+			skb_copy_to_linear_data_offset(skb, (2 * ETH_ALEN),
+					data + payload_offset,
+					pkt_len - 4 - (2 * ETH_ALEN));
+
[Andy] because the IP don't support HW VLAN tag extraction, it need sw to do it. It is ok for me. 

 			skb->protocol = eth_type_trans(skb, ndev);
 
 			/* Get receive timestamp from the skb */
-			if (fep->hwts_rx_en && fep->bufdesc_ex) {
+			if (fep->hwts_rx_en && ebdp) {
 				struct skb_shared_hwtstamps *shhwtstamps =
 							    skb_hwtstamps(skb);
 				unsigned long flags;
-				struct bufdesc_ex *ebdp =
-					(struct bufdesc_ex *)bdp;
 
 				memset(shhwtstamps, 0, sizeof(*shhwtstamps));
 
@@ -889,10 +921,7 @@ fec_enet_rx(struct net_device *ndev, int budget)
 				spin_unlock_irqrestore(&fep->tmreg_lock, flags);
 			}
 
-			if (fep->bufdesc_ex &&
-				(fep->csum_flags & FLAG_RX_CSUM_ENABLED)) {
-				struct bufdesc_ex *ebdp =
-					(struct bufdesc_ex *)bdp;
+			if (ebdp && (fep->csum_flags & FLAG_RX_CSUM_ENABLED)) {
 				if (!(ebdp->cbd_esc & FLAG_RX_CSUM_ERROR)) {
 					/* don't check it */
 					skb->ip_summed = CHECKSUM_UNNECESSARY; @@ -901,6 +930,11 @@ fec_enet_rx(struct net_device *ndev, int budget)
 				}
 			}
 
+			/* Handle received VLAN packets */
+			if (vlan_packet_rcvd)
+				__vlan_hwaccel_put_tag(skb,
+						htons(ETH_P_8021Q), vlan_tag);
+
 			if (!skb_defer_rx_timestamp(skb))
 				napi_gro_receive(&fep->napi, skb);
 		}
@@ -1802,6 +1836,12 @@ static int fec_enet_init(struct net_device *ndev)
 	writel(FEC_RX_DISABLED_IMASK, fep->hwp + FEC_IMASK);
 	netif_napi_add(ndev, &fep->napi, fec_enet_rx_napi, FEC_NAPI_WEIGHT);
 
+	if (id_entry->driver_data & FEC_QUIRK_HAS_VLAN) {
+		/* enable hw VLAN support */
+		ndev->features |= NETIF_F_HW_VLAN_CTAG_RX;
+		ndev->hw_features |= NETIF_F_HW_VLAN_CTAG_RX;
+	}
+
 	if (id_entry->driver_data & FEC_QUIRK_HAS_CSUM) {
 		/* enable hw accelerator */
 		ndev->features |= (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM
--
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH net-next v1 1/1] net: fec: Add VLAN receive HW support.

[Duan Fugang-B38611]

In addition,
	1.	enet IP support hw VLAN detect such as imx28-fec, imx6q-fec and mvf600-fec, so you can add the "BD_ENET_RX_VLAN " to those platform driver_data.
	2.	Enable the VLAN tag extraction with "NETIF_F_HW_VLAN_CTAG_RX" feature in .ndo_fix_features, which is configured by user.

Thanks,
Andy

-----Original Message-----
From: Duan Fugang-B38611 
Sent: Wednesday, June 26, 2013 10:22 AM
To: 'Jim Baxter'; David S. Miller
Cc: Li Frank-B20596; Estevam Fabio-R49496; Lucas Stach; Shawn Guo; netdev@vger.kernel.org; Ben Hutchings
Subject: RE: [PATCH net-next v1 1/1] net: fec: Add VLAN receive HW support.

Comments.[Andy] 

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Jim Baxter
Sent: Wednesday, June 26, 2013 7:55 AM
To: David S. Miller
Cc: Li Frank-B20596; Duan Fugang-B38611; Estevam Fabio-R49496; Lucas Stach; Shawn Guo; netdev@vger.kernel.org; Ben Hutchings
Subject: [PATCH net-next v1 1/1] net: fec: Add VLAN receive HW support.

This enables the driver to take adavantage of the FEC VLAN indicator to improve performance.

Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
---
 drivers/net/ethernet/freescale/fec.h      |    3 ++
 drivers/net/ethernet/freescale/fec_main.c |   64 +++++++++++++++++++++++------
 2 files changed, 55 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index e3ed6c5..85e75d9 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -148,6 +148,9 @@ struct bufdesc_ex {
 #define BD_ENET_RX_CL           ((ushort)0x0001)
 #define BD_ENET_RX_STATS        ((ushort)0x013f)        /* All status bits */
 
+/* Enhanced buffer descriptor control/status used by Ethernet receive */
+#define BD_ENET_RX_VLAN         0x00000004
+
 /* Buffer descriptor control/status used by Ethernet transmit.
 */
 #define BD_ENET_TX_READY        ((ushort)0x8000)
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 46f2544..851d836 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -54,6 +54,7 @@
 #include <linux/of_gpio.h>
 #include <linux/of_net.h>
 #include <linux/regulator/consumer.h>
+#include <linux/if_vlan.h>
 
 #include <asm/cacheflush.h>
 
@@ -88,6 +89,8 @@
 #define FEC_QUIRK_HAS_BUFDESC_EX	(1 << 4)
 /* Controller has hardware checksum support */
 #define FEC_QUIRK_HAS_CSUM		(1 << 5)
+/* Controller has hardware vlan support */
+#define FEC_QUIRK_HAS_VLAN		(1 << 6)
 
 static struct platform_device_id fec_devtype[] = {
 	{
@@ -106,7 +109,8 @@ static struct platform_device_id fec_devtype[] = {
 	}, {
 		.name = "imx6q-fec",
 		.driver_data = FEC_QUIRK_ENET_MAC | FEC_QUIRK_HAS_GBIT |
-				FEC_QUIRK_HAS_BUFDESC_EX | FEC_QUIRK_HAS_CSUM,
+				FEC_QUIRK_HAS_BUFDESC_EX | FEC_QUIRK_HAS_CSUM |
+				FEC_QUIRK_HAS_VLAN,
 	}, {
 		.name = "mvf600-fec",
 		.driver_data = FEC_QUIRK_ENET_MAC,
@@ -177,11 +181,11 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");  #define FEC_DEFAULT_IMASK (FEC_ENET_TXF | FEC_ENET_RXF | FEC_ENET_MII)  #define FEC_RX_DISABLED_IMASK (FEC_DEFAULT_IMASK & (~FEC_ENET_RXF))
 
-/* The FEC stores dest/src/type, data, and checksum for receive packets.
+/* The FEC stores dest/src/type/vlan, data, and checksum for receive packets.
  */
-#define PKT_MAXBUF_SIZE		1518
+#define PKT_MAXBUF_SIZE		1522
 #define PKT_MINBUF_SIZE		64
-#define PKT_MAXBLR_SIZE		1520
+#define PKT_MAXBLR_SIZE		1540
[Andy]Pls define the MAXBLR size to 1536, which is multiplied by 64
	 #define PKT_MAXBLR_SIZE         1536 
 
 /* FEC receive acceleration */
 #define FEC_RACC_IPDIS		(1 << 1)
@@ -795,6 +799,9 @@ fec_enet_rx(struct net_device *ndev, int budget)
 	ushort	pkt_len;
 	__u8 *data;
 	int	pkt_received = 0;
+	struct	bufdesc_ex *ebdp = NULL;
+	bool	vlan_packet_rcvd = false;
+	u16	vlan_tag;
 
 #ifdef CONFIG_M532x
 	flush_cache_all();
@@ -858,6 +865,24 @@ fec_enet_rx(struct net_device *ndev, int budget)
 		if (id_entry->driver_data & FEC_QUIRK_SWAP_FRAME)
 			swap_buffer(data, pkt_len);
 
+		/* Extract the enhanced buffer descriptor */
+		ebdp = NULL;
+		if (fep->bufdesc_ex)
+			ebdp = (struct bufdesc_ex *)bdp;
+
+		/* If this is a VLAN packet remove the VLAN Tag */
+		vlan_packet_rcvd = false;
+		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
+				ebdp && (ebdp->cbd_esc & BD_ENET_RX_VLAN)) {
+			/* Push and remove the vlan tag */
+			struct vlan_hdr *vlan_header =
+					(struct vlan_hdr *) (data + ETH_HLEN);
+			vlan_tag = ntohs(vlan_header->h_vlan_TCI);
+			pkt_len -= VLAN_HLEN;
+
+			vlan_packet_rcvd = true;
+		}
+
 		/* This does 16 byte alignment, exactly what we need.
 		 * The packet length includes FCS, but we don't want to
 		 * include that when passing upstream as it messes up @@ -868,18 +893,25 @@ fec_enet_rx(struct net_device *ndev, int budget)
 		if (unlikely(!skb)) {
 			ndev->stats.rx_dropped++;
 		} else {
+			int payload_offset = (2 * ETH_ALEN);
 			skb_reserve(skb, NET_IP_ALIGN);
 			skb_put(skb, pkt_len - 4);	/* Make room */
-			skb_copy_to_linear_data(skb, data, pkt_len - 4);
+
+			/* Extract the frame data without the VLAN header. */
+			skb_copy_to_linear_data(skb, data, (2 * ETH_ALEN));
+			if (vlan_packet_rcvd)
+				payload_offset = (2 * ETH_ALEN) + VLAN_HLEN;
+			skb_copy_to_linear_data_offset(skb, (2 * ETH_ALEN),
+					data + payload_offset,
+					pkt_len - 4 - (2 * ETH_ALEN));
+
[Andy] because the IP don't support HW VLAN tag extraction, it need sw to do it. It is ok for me. 

 			skb->protocol = eth_type_trans(skb, ndev);
 
 			/* Get receive timestamp from the skb */
-			if (fep->hwts_rx_en && fep->bufdesc_ex) {
+			if (fep->hwts_rx_en && ebdp) {
 				struct skb_shared_hwtstamps *shhwtstamps =
 							    skb_hwtstamps(skb);
 				unsigned long flags;
-				struct bufdesc_ex *ebdp =
-					(struct bufdesc_ex *)bdp;
 
 				memset(shhwtstamps, 0, sizeof(*shhwtstamps));
 
@@ -889,10 +921,7 @@ fec_enet_rx(struct net_device *ndev, int budget)
 				spin_unlock_irqrestore(&fep->tmreg_lock, flags);
 			}
 
-			if (fep->bufdesc_ex &&
-				(fep->csum_flags & FLAG_RX_CSUM_ENABLED)) {
-				struct bufdesc_ex *ebdp =
-					(struct bufdesc_ex *)bdp;
+			if (ebdp && (fep->csum_flags & FLAG_RX_CSUM_ENABLED)) {
 				if (!(ebdp->cbd_esc & FLAG_RX_CSUM_ERROR)) {
 					/* don't check it */
 					skb->ip_summed = CHECKSUM_UNNECESSARY; @@ -901,6 +930,11 @@ fec_enet_rx(struct net_device *ndev, int budget)
 				}
 			}
 
+			/* Handle received VLAN packets */
+			if (vlan_packet_rcvd)
+				__vlan_hwaccel_put_tag(skb,
+						htons(ETH_P_8021Q), vlan_tag);
+
 			if (!skb_defer_rx_timestamp(skb))
 				napi_gro_receive(&fep->napi, skb);
 		}
@@ -1802,6 +1836,12 @@ static int fec_enet_init(struct net_device *ndev)
 	writel(FEC_RX_DISABLED_IMASK, fep->hwp + FEC_IMASK);
 	netif_napi_add(ndev, &fep->napi, fec_enet_rx_napi, FEC_NAPI_WEIGHT);
 
+	if (id_entry->driver_data & FEC_QUIRK_HAS_VLAN) {
+		/* enable hw VLAN support */
+		ndev->features |= NETIF_F_HW_VLAN_CTAG_RX;
+		ndev->hw_features |= NETIF_F_HW_VLAN_CTAG_RX;
+	}
+
 	if (id_entry->driver_data & FEC_QUIRK_HAS_CSUM) {
 		/* enable hw accelerator */
 		ndev->features |= (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM
--
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next v1 1/1] net: fec: Add VLAN receive HW support.

[Shawn Guo]

On Wed, Jun 26, 2013 at 02:31:40AM +0000, Duan Fugang-B38611 wrote:
> In addition,
> 	1.	enet IP support hw VLAN detect such as imx28-fec, imx6q-fec and mvf600-fec, so you can add the "BD_ENET_RX_VLAN " to those platform driver_data.

I guess Jim only tested the code on imx6q-fec.  We shouldn't enable it
for the other two platforms until it's been tested on them.  As the
counter example, although we expect FEC_QUIRK_HAS_CSUM should also work
for imx28-fec, if we enable FEC_QUIRK_HAS_CSUM for it, fec driver will
just break right way on imx28.

Shawn

RE: [PATCH net-next v1 1/1] net: fec: Add VLAN receive HW support.

[Duan Fugang-B38611]

Yes, you are right.

Before we sent our our patch, we must test it on all covered platforms.


Thanks,
Andy 

-----Original Message-----
From: Shawn Guo [mailto:shawn.guo@linaro.org] 
Sent: Wednesday, June 26, 2013 10:57 AM
To: Duan Fugang-B38611
Cc: Jim Baxter; David S. Miller; Li Frank-B20596; Estevam Fabio-R49496; Lucas Stach; netdev@vger.kernel.org; Ben Hutchings
Subject: Re: [PATCH net-next v1 1/1] net: fec: Add VLAN receive HW support.

On Wed, Jun 26, 2013 at 02:31:40AM +0000, Duan Fugang-B38611 wrote:
> In addition,
> 	1.	enet IP support hw VLAN detect such as imx28-fec, imx6q-fec and mvf600-fec, so you can add the "BD_ENET_RX_VLAN " to those platform driver_data.

I guess Jim only tested the code on imx6q-fec.  We shouldn't enable it for the other two platforms until it's been tested on them.  As the counter example, although we expect FEC_QUIRK_HAS_CSUM should also work for imx28-fec, if we enable FEC_QUIRK_HAS_CSUM for it, fec driver will just break right way on imx28.

Shawn

Re: [PATCH net-next v1 1/1] net: fec: Add VLAN receive HW support.

[David Miller]

Do not top post, thank you.

Re: [PATCH net-next v1 1/1] net: fec: Add VLAN receive HW support.

[Jim Baxter]

On 26/06/13 03:31, Duan Fugang-B38611 wrote:
> In addition,
> 	1.	enet IP support hw VLAN detect such as imx28-fec, imx6q-fec and mvf600-fec, so you can add the "BD_ENET_RX_VLAN " to those platform driver_data.
I have only tested it on the i.MX6, if someone wants to check it on the
imx28 later they can add it to that option.

> 	2.	Enable the VLAN tag extraction with "NETIF_F_HW_VLAN_CTAG_RX" feature in .ndo_fix_features, which is configured by user.
What do you mean by this, I am only enabling the VLAN code if
(ndev->features & NETIF_F_HW_VLAN_CTAG_RX) is set. I am pretty sure this
is set by

>   */
> -#define PKT_MAXBUF_SIZE		1518
> +#define PKT_MAXBUF_SIZE		1522
>  #define PKT_MINBUF_SIZE		64
> -#define PKT_MAXBLR_SIZE		1520
> +#define PKT_MAXBLR_SIZE		1540
> [Andy]Pls define the MAXBLR size to 1536, which is multiplied by 64
> 	 #define PKT_MAXBLR_SIZE         1536
I will change this.


> +			int payload_offset = (2 * ETH_ALEN);
>  			skb_reserve(skb, NET_IP_ALIGN);
>  			skb_put(skb, pkt_len - 4);	/* Make room */
> -			skb_copy_to_linear_data(skb, data, pkt_len - 4);
> +
> +			/* Extract the frame data without the VLAN header. */
> +			skb_copy_to_linear_data(skb, data, (2 * ETH_ALEN));
> +			if (vlan_packet_rcvd)
> +				payload_offset = (2 * ETH_ALEN) + VLAN_HLEN;
> +			skb_copy_to_linear_data_offset(skb, (2 * ETH_ALEN),
> +					data + payload_offset,
> +					pkt_len - 4 - (2 * ETH_ALEN));
> +
> [Andy] because the IP don't support HW VLAN tag extraction, it need sw to do it. It is ok for me. 
Yes it needs to be done in software as the Ethernet card only detects
the VLAN packet, doing it here is more efficient then the kernel doing it.


Thank you,
Jim

RE: [PATCH net-next v1 1/1] net: fec: Add VLAN receive HW support.

[Duan Fugang-B38611]

On 06/26/13 18:09, Jim Baxter wrote:

> > In addition,
> > 	1.	enet IP support hw VLAN detect such as imx28-fec, imx6q-fec and mvf600-fec, so you can add the "BD_ENET_RX_VLAN " to those platform driver_data.
> I have only tested it on the i.MX6, if someone wants to check it on the
> imx28 later they can add it to that option.

> > 	2.	Enable the VLAN tag extraction with "NETIF_F_HW_VLAN_CTAG_RX" feature in .ndo_fix_features, which is configured by user.
> What do you mean by this, I am only enabling the VLAN code if (ndev->features & NETIF_F_HW_VLAN_CTAG_RX) is set. I am pretty sure this is set by

You can enable the feature like below, which is reasonable.
.ndo_fix_features   = fec_enet_fix_features

static netdev_features_t fec_enet_fix_features (struct net_device *dev, 
        netdev_features_t features)
{
        struct fec_enet_private *fep = netdev_priv(dev);
	  const struct platform_device_id *id_entry =
					platform_get_device_id(fep->pdev);

	  if (id_entry->driver_data & FEC_QUIRK_HAS_VLAN)
        	features |= NETIF_F_HW_VLAN_CTAG_RX;

	  return features;
}


Thank you,
Andy

Re: [PATCH net-next v1 1/1] net: fec: Add VLAN receive HW support.

[Jim Baxter]

On 26/06/13 12:18, Duan Fugang-B38611 wrote:
> On 06/26/13 18:09, Jim Baxter wrote:
> 
>>> In addition,
>>> 	1.	enet IP support hw VLAN detect such as imx28-fec, imx6q-fec and mvf600-fec, so you can add the "BD_ENET_RX_VLAN " to those platform driver_data.
>> I have only tested it on the i.MX6, if someone wants to check it on the
>> imx28 later they can add it to that option.
> 
>>> 	2.	Enable the VLAN tag extraction with "NETIF_F_HW_VLAN_CTAG_RX" feature in .ndo_fix_features, which is configured by user.
>> What do you mean by this, I am only enabling the VLAN code if (ndev->features & NETIF_F_HW_VLAN_CTAG_RX) is set. I am pretty sure this is set by
> 
> You can enable the feature like below, which is reasonable.
> .ndo_fix_features   = fec_enet_fix_features
> 
> static netdev_features_t fec_enet_fix_features (struct net_device *dev, 
>         netdev_features_t features)
> {
>         struct fec_enet_private *fep = netdev_priv(dev);
> 	  const struct platform_device_id *id_entry =
> 					platform_get_device_id(fep->pdev);
> 
> 	  if (id_entry->driver_data & FEC_QUIRK_HAS_VLAN)
>         	features |= NETIF_F_HW_VLAN_CTAG_RX;
> 
> 	  return features;
> }
> 
> 
> Thank you,
> Andy
> 
> 

I currently set the features in fec_enet_init() along with setting
hw_features, is fec_enet_fix_features() a more correct design for driver
implementation point?

RE: [PATCH net-next v1 1/1] net: fec: Add VLAN receive HW support.

[Duan Fugang-B38611]

On 06/26/13 19:45, Jim Baxter wrote:

> On 26/06/13 12:18, Duan Fugang-B38611 wrote:
> > On 06/26/13 18:09, Jim Baxter wrote:
> >> 
> >>> In addition,
> >>> 	1.	enet IP support hw VLAN detect such as imx28-fec, imx6q-fec and mvf600-fec, so you can add the "BD_ENET_RX_VLAN " to those platform driver_data.
> >> I have only tested it on the i.MX6, if someone wants to check it on 
> >> the
> >> imx28 later they can add it to that option.
> > 
> >>> 	2.	Enable the VLAN tag extraction with "NETIF_F_HW_VLAN_CTAG_RX" feature in .ndo_fix_features, which is configured by user.
> >> What do you mean by this, I am only enabling the VLAN code if 
> >> (ndev->features & NETIF_F_HW_VLAN_CTAG_RX) is set. I am pretty sure 
> >> this is set by
> > 
> > You can enable the feature like below, which is reasonable.
> > .ndo_fix_features   = fec_enet_fix_features
> > 
> > static netdev_features_t fec_enet_fix_features (struct net_device *dev, 
> >         netdev_features_t features)
> > {
> >         struct fec_enet_private *fep = netdev_priv(dev);
> > 	  const struct platform_device_id *id_entry =
> > 					platform_get_device_id(fep->pdev);
> > 
> > 	  if (id_entry->driver_data & FEC_QUIRK_HAS_VLAN)
> >         	features |= NETIF_F_HW_VLAN_CTAG_RX;
> > 
> > 	  return features;
> > }
> > 
> > 
> > Thank you,
> > Andy
> > 
> > 
>
> I currently set the features in fec_enet_init() along with setting hw_features, is fec_enet_fix_features() a more correct design for driver implementation point?

Yes, I think it is more reasonable.
Can you hold on the patch to hear others' idea.

Thanks,
Andy

Re: [PATCH net-next v1 1/1] net: fec: Add VLAN receive HW support.

[Jim Baxter]

On 27/06/13 02:41, Duan Fugang-B38611 wrote:
> On 06/26/13 19:45, Jim Baxter wrote:
> 
>> On 26/06/13 12:18, Duan Fugang-B38611 wrote:
>>> On 06/26/13 18:09, Jim Baxter wrote:
>>>>
>>>>> In addition,
>>>>> 	1.	enet IP support hw VLAN detect such as imx28-fec, imx6q-fec and mvf600-fec, so you can add the "BD_ENET_RX_VLAN " to those platform driver_data.
>>>> I have only tested it on the i.MX6, if someone wants to check it on 
>>>> the
>>>> imx28 later they can add it to that option.
>>>
>>>>> 	2.	Enable the VLAN tag extraction with "NETIF_F_HW_VLAN_CTAG_RX" feature in .ndo_fix_features, which is configured by user.
>>>> What do you mean by this, I am only enabling the VLAN code if 
>>>> (ndev->features & NETIF_F_HW_VLAN_CTAG_RX) is set. I am pretty sure 
>>>> this is set by
>>>
>>> You can enable the feature like below, which is reasonable.
>>> .ndo_fix_features   = fec_enet_fix_features
>>>
>>> static netdev_features_t fec_enet_fix_features (struct net_device *dev, 
>>>         netdev_features_t features)
>>> {
>>>         struct fec_enet_private *fep = netdev_priv(dev);
>>> 	  const struct platform_device_id *id_entry =
>>> 					platform_get_device_id(fep->pdev);
>>>
>>> 	  if (id_entry->driver_data & FEC_QUIRK_HAS_VLAN)
>>>         	features |= NETIF_F_HW_VLAN_CTAG_RX;
>>>
>>> 	  return features;
>>> }
>>>
>>>
>>> Thank you,
>>> Andy
>>>
>>>
>>
>> I currently set the features in fec_enet_init() along with setting hw_features, is fec_enet_fix_features() a more correct design for driver implementation point?
> 
> Yes, I think it is more reasonable.
> Can you hold on the patch to hear others' idea.
> 
> Thanks,
> Andy
> 
> 

Yes, I am will wait for other comments.

Thank you,
Jim

Re: [PATCH net-next v1 1/1] net: fec: Add VLAN receive HW support.

[Lucas Stach]

Am Mittwoch, den 26.06.2013, 12:45 +0100 schrieb Jim Baxter:
> On 26/06/13 12:18, Duan Fugang-B38611 wrote:
> > On 06/26/13 18:09, Jim Baxter wrote:
> > 
> >>> In addition,
> >>> 	1.	enet IP support hw VLAN detect such as imx28-fec, imx6q-fec and mvf600-fec, so you can add the "BD_ENET_RX_VLAN " to those platform driver_data.
> >> I have only tested it on the i.MX6, if someone wants to check it on the
> >> imx28 later they can add it to that option.
> > 
> >>> 	2.	Enable the VLAN tag extraction with "NETIF_F_HW_VLAN_CTAG_RX" feature in .ndo_fix_features, which is configured by user.
> >> What do you mean by this, I am only enabling the VLAN code if (ndev->features & NETIF_F_HW_VLAN_CTAG_RX) is set. I am pretty sure this is set by
> > 
> > You can enable the feature like below, which is reasonable.
> > .ndo_fix_features   = fec_enet_fix_features
> > 
> > static netdev_features_t fec_enet_fix_features (struct net_device *dev, 
> >         netdev_features_t features)
> > {
> >         struct fec_enet_private *fep = netdev_priv(dev);
> > 	  const struct platform_device_id *id_entry =
> > 					platform_get_device_id(fep->pdev);
> > 
> > 	  if (id_entry->driver_data & FEC_QUIRK_HAS_VLAN)
> >         	features |= NETIF_F_HW_VLAN_CTAG_RX;
> > 
> > 	  return features;
> > }
> > 
> > 
> > Thank you,
> > Andy
> > 
> > 
> 
> I currently set the features in fec_enet_init() along with setting
> hw_features, is fec_enet_fix_features() a more correct design for driver
> implementation point?
> 
I agree that VLAN tag extraction should not be enabled by default, but
at the users request. But according to the documentation there is no
need to implement the fix_features callback for this to work.

Just only set the hw_features on init, the networking core should take
care of never trying to enable a feature not found in there.

Regards,
Lucas
-- 
Pengutronix e.K.                           | Lucas Stach                 |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v1 1/1] net: fec: Add VLAN receive HW support.

[Jim Baxter]

On 27/06/13 10:44, Lucas Stach wrote:
> Am Mittwoch, den 26.06.2013, 12:45 +0100 schrieb Jim Baxter:
>> On 26/06/13 12:18, Duan Fugang-B38611 wrote:
>>> On 06/26/13 18:09, Jim Baxter wrote:
>>>
>>>>> In addition,
>>>>> 	1.	enet IP support hw VLAN detect such as imx28-fec, imx6q-fec and mvf600-fec, so you can add the "BD_ENET_RX_VLAN " to those platform driver_data.
>>>> I have only tested it on the i.MX6, if someone wants to check it on the
>>>> imx28 later they can add it to that option.
>>>
>>>>> 	2.	Enable the VLAN tag extraction with "NETIF_F_HW_VLAN_CTAG_RX" feature in .ndo_fix_features, which is configured by user.
>>>> What do you mean by this, I am only enabling the VLAN code if (ndev->features & NETIF_F_HW_VLAN_CTAG_RX) is set. I am pretty sure this is set by
>>>
>>> You can enable the feature like below, which is reasonable.
>>> .ndo_fix_features   = fec_enet_fix_features
>>>
>>> static netdev_features_t fec_enet_fix_features (struct net_device *dev, 
>>>         netdev_features_t features)
>>> {
>>>         struct fec_enet_private *fep = netdev_priv(dev);
>>> 	  const struct platform_device_id *id_entry =
>>> 					platform_get_device_id(fep->pdev);
>>>
>>> 	  if (id_entry->driver_data & FEC_QUIRK_HAS_VLAN)
>>>         	features |= NETIF_F_HW_VLAN_CTAG_RX;
>>>
>>> 	  return features;
>>> }
>>>
>>>
>>> Thank you,
>>> Andy
>>>
>>>
>>
>> I currently set the features in fec_enet_init() along with setting
>> hw_features, is fec_enet_fix_features() a more correct design for driver
>> implementation point?
>>
> I agree that VLAN tag extraction should not be enabled by default, but
> at the users request. But according to the documentation there is no
> need to implement the fix_features callback for this to work.
> 
> Just only set the hw_features on init, the networking core should take
> care of never trying to enable a feature not found in there.
> 
> Regards,
> Lucas
> 
Hi Lucas,

Yes that is how the patch currently operates, the user can still choose
whether the feature is enabled.

Thank you,
Jim

[PATCH net v1 1/1] net: fec: Fix Transmitted bytes counter

[Jim Baxter]

The tx_bytes field was not being updated so the
network card statistics showed 0.0B transmitted.

Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
---
 drivers/net/ethernet/freescale/fec_main.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index ed6180e..05a5f76 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -738,6 +738,7 @@ fec_enet_tx(struct net_device *ndev)
 				ndev->stats.tx_carrier_errors++;
 		} else {
 			ndev->stats.tx_packets++;
+			ndev->stats.tx_bytes += bdp->cbd_datlen;
 		}
 
 		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) &&
-- 
1.7.10.4

RE: [PATCH net v1 1/1] net: fec: Fix Transmitted bytes counter

[Duan Fugang-B38611]

On 06/28/13 02:25, Jim Baxter wrote:

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Jim Baxter
> Sent: Friday, June 28, 2013 2:25 AM
> To: David S. Miller
> Cc: Estevam Fabio-R49496; Li Frank-B20596; Shawn Guo; netdev@vger.kernel.org
> Subject: [PATCH net v1 1/1] net: fec: Fix Transmitted bytes counter
>
> The tx_bytes field was not being updated so the network card statistics showed 0.0B transmitted.
>
> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index ed6180e..05a5f76 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -738,6 +738,7 @@ fec_enet_tx(struct net_device *ndev)
>  				ndev->stats.tx_carrier_errors++;
>  		} else {
>  			ndev->stats.tx_packets++;
> +			ndev->stats.tx_bytes += bdp->cbd_datlen;
>  		}
>  
>  		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) &&
> --
You cannot stat. tx_bytes in here, since stat.tx_bytes indicate that all bytes sent by MAC regardless whether there have error packets or not.
You must add the stat. at xmit function as below:
...
fep->tx_skbuff[index] = skb;
+ ndev->stats.tx_bytes += skb->len;
...

Thanks,
Andy

Re: [PATCH net v1 1/1] net: fec: Fix Transmitted bytes counter

[David Miller]

From: Duan Fugang-B38611 <B38611@freescale.com>
Date: Fri, 28 Jun 2013 02:11:30 +0000

> On 06/28/13 02:25, Jim Baxter wrote:
> 
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
>> index ed6180e..05a5f76 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -738,6 +738,7 @@ fec_enet_tx(struct net_device *ndev)
>>  				ndev->stats.tx_carrier_errors++;
>>  		} else {
>>  			ndev->stats.tx_packets++;
>> +			ndev->stats.tx_bytes += bdp->cbd_datlen;
>>  		}
>>  
>>  		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) &&
>> --
> You cannot stat. tx_bytes in here, since stat.tx_bytes indicate that all bytes sent by MAC regardless whether there have error packets or not.
> You must add the stat. at xmit function as below:

I completely disagree.

tx_bytes indicates what actually made it to the wires, so Jim's original
patch is the correct one.

Every single driver I have ever written, always increments tx_bytes in
the transmit completion handler, not when the device layer gives the
packet to the driver.

Re: [PATCH net v1 1/1] net: fec: Fix Transmitted bytes counter

[Jim Baxter]

On 01/07/13 21:40, David Miller wrote:
> From: Duan Fugang-B38611 <B38611@freescale.com>
> Date: Fri, 28 Jun 2013 02:11:30 +0000
> 
>> On 06/28/13 02:25, Jim Baxter wrote:
>>
>>> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
>>> index ed6180e..05a5f76 100644
>>> --- a/drivers/net/ethernet/freescale/fec_main.c
>>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>>> @@ -738,6 +738,7 @@ fec_enet_tx(struct net_device *ndev)
>>>  				ndev->stats.tx_carrier_errors++;
>>>  		} else {
>>>  			ndev->stats.tx_packets++;
>>> +			ndev->stats.tx_bytes += bdp->cbd_datlen;
>>>  		}
>>>  
>>>  		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) &&
>>> --
>> You cannot stat. tx_bytes in here, since stat.tx_bytes indicate that all bytes sent by MAC regardless whether there have error packets or not.
>> You must add the stat. at xmit function as below:
> 
> I completely disagree.
> 
> tx_bytes indicates what actually made it to the wires, so Jim's original
> patch is the correct one.
> 
> Every single driver I have ever written, always increments tx_bytes in
> the transmit completion handler, not when the device layer gives the
> packet to the driver.
> 

If this should be the first version of the patch, can I cancel the
second version and set the first version to new in patchwork, or is it
best to submit a third version?

Thank you,
Jim

Re: [PATCH net v1 1/1] net: fec: Fix Transmitted bytes counter

[David Miller]

From: Jim Baxter <jim_baxter@mentor.com>
Date: Tue, 2 Jul 2013 09:32:31 +0100

> If this should be the first version of the patch, can I cancel the
> second version and set the first version to new in patchwork, or is it
> best to submit a third version?

I applied your v1 patch, thanks.

[PATCH net v2 1/1] net: fec: Fix Transmitted bytes counter

[Jim Baxter]

The tx_bytes field was not being updated so the
network card statistics showed 0.0B transmitted.

Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
---
Change from v1 to v2
Moved the tx_bytes incrementing to include all packets passed to the
network card.

 drivers/net/ethernet/freescale/fec_main.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index d48099f..51493ce 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -332,6 +332,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 
 	/* Save skb pointer */
 	fep->tx_skbuff[index] = skb;
+	ndev->stats.tx_bytes += skb->len;
 
 	/* Push the data cache so the CPM does not get stale memory
 	 * data.
-- 
1.7.10.4

RE: [PATCH net v2 1/1] net: fec: Fix Transmitted bytes counter

[Duan Fugang-B38611]

On 06/28/13 17:51, Jim Baxter wrote:
> The tx_bytes field was not being updated so the network card statistics showed 0.0B transmitted.
>
> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
> ---
> Change from v1 to v2
> Moved the tx_bytes incrementing to include all packets passed to the network card.
>
>  drivers/net/ethernet/freescale/fec_main.c |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index d48099f..51493ce 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -332,6 +332,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  
>  	/* Save skb pointer */
>  	fep->tx_skbuff[index] = skb;
> +	ndev->stats.tx_bytes += skb->len;
>  
>  	/* Push the data cache so the CPM does not get stale memory
>  	 * data.
> --
> 1.7.10.4

Acked-By: Fugang Duan  <B38611@freescale.com>

[PATCH net-next v2 1/1] net: fec: Add VLAN receive HW support.

[Jim Baxter]

This enables the driver to take advantage of the FEC VLAN
indicator to improve performance.

Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
---
Change from v1 to v2
Changed Max recieve Buffer Size Register from 1540 to 1536 so it is
divisible by 64.

 drivers/net/ethernet/freescale/fec.h      |    3 ++
 drivers/net/ethernet/freescale/fec_main.c |   64 +++++++++++++++++++++++------
 2 files changed, 55 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 8362a03..2b0a0ea 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -203,6 +203,9 @@ struct bufdesc_ex {
 #define BD_ENET_RX_CL           ((ushort)0x0001)
 #define BD_ENET_RX_STATS        ((ushort)0x013f)        /* All status bits */
 
+/* Enhanced buffer descriptor control/status used by Ethernet receive */
+#define BD_ENET_RX_VLAN         0x00000004
+
 /* Buffer descriptor control/status used by Ethernet transmit.
 */
 #define BD_ENET_TX_READY        ((ushort)0x8000)
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index ed6180e..b94dde2 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -54,6 +54,7 @@
 #include <linux/of_gpio.h>
 #include <linux/of_net.h>
 #include <linux/regulator/consumer.h>
+#include <linux/if_vlan.h>
 
 #include <asm/cacheflush.h>
 
@@ -88,6 +89,8 @@
 #define FEC_QUIRK_HAS_BUFDESC_EX	(1 << 4)
 /* Controller has hardware checksum support */
 #define FEC_QUIRK_HAS_CSUM		(1 << 5)
+/* Controller has hardware vlan support */
+#define FEC_QUIRK_HAS_VLAN		(1 << 6)
 
 static struct platform_device_id fec_devtype[] = {
 	{
@@ -106,7 +109,8 @@ static struct platform_device_id fec_devtype[] = {
 	}, {
 		.name = "imx6q-fec",
 		.driver_data = FEC_QUIRK_ENET_MAC | FEC_QUIRK_HAS_GBIT |
-				FEC_QUIRK_HAS_BUFDESC_EX | FEC_QUIRK_HAS_CSUM,
+				FEC_QUIRK_HAS_BUFDESC_EX | FEC_QUIRK_HAS_CSUM |
+				FEC_QUIRK_HAS_VLAN,
 	}, {
 		.name = "mvf600-fec",
 		.driver_data = FEC_QUIRK_ENET_MAC,
@@ -177,11 +181,11 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
 #define FEC_DEFAULT_IMASK (FEC_ENET_TXF | FEC_ENET_RXF | FEC_ENET_MII)
 #define FEC_RX_DISABLED_IMASK (FEC_DEFAULT_IMASK & (~FEC_ENET_RXF))
 
-/* The FEC stores dest/src/type, data, and checksum for receive packets.
+/* The FEC stores dest/src/type/vlan, data, and checksum for receive packets.
  */
-#define PKT_MAXBUF_SIZE		1518
+#define PKT_MAXBUF_SIZE		1522
 #define PKT_MINBUF_SIZE		64
-#define PKT_MAXBLR_SIZE		1520
+#define PKT_MAXBLR_SIZE		1536
 
 /* FEC receive acceleration */
 #define FEC_RACC_IPDIS		(1 << 1)
@@ -803,6 +807,9 @@ fec_enet_rx(struct net_device *ndev, int budget)
 	ushort	pkt_len;
 	__u8 *data;
 	int	pkt_received = 0;
+	struct	bufdesc_ex *ebdp = NULL;
+	bool	vlan_packet_rcvd = false;
+	u16	vlan_tag;
 
 #ifdef CONFIG_M532x
 	flush_cache_all();
@@ -866,6 +873,24 @@ fec_enet_rx(struct net_device *ndev, int budget)
 		if (id_entry->driver_data & FEC_QUIRK_SWAP_FRAME)
 			swap_buffer(data, pkt_len);
 
+		/* Extract the enhanced buffer descriptor */
+		ebdp = NULL;
+		if (fep->bufdesc_ex)
+			ebdp = (struct bufdesc_ex *)bdp;
+
+		/* If this is a VLAN packet remove the VLAN Tag */
+		vlan_packet_rcvd = false;
+		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
+				ebdp && (ebdp->cbd_esc & BD_ENET_RX_VLAN)) {
+			/* Push and remove the vlan tag */
+			struct vlan_hdr *vlan_header =
+					(struct vlan_hdr *) (data + ETH_HLEN);
+			vlan_tag = ntohs(vlan_header->h_vlan_TCI);
+			pkt_len -= VLAN_HLEN;
+
+			vlan_packet_rcvd = true;
+		}
+
 		/* This does 16 byte alignment, exactly what we need.
 		 * The packet length includes FCS, but we don't want to
 		 * include that when passing upstream as it messes up
@@ -876,18 +901,25 @@ fec_enet_rx(struct net_device *ndev, int budget)
 		if (unlikely(!skb)) {
 			ndev->stats.rx_dropped++;
 		} else {
+			int payload_offset = (2 * ETH_ALEN);
 			skb_reserve(skb, NET_IP_ALIGN);
 			skb_put(skb, pkt_len - 4);	/* Make room */
-			skb_copy_to_linear_data(skb, data, pkt_len - 4);
+
+			/* Extract the frame data without the VLAN header. */
+			skb_copy_to_linear_data(skb, data, (2 * ETH_ALEN));
+			if (vlan_packet_rcvd)
+				payload_offset = (2 * ETH_ALEN) + VLAN_HLEN;
+			skb_copy_to_linear_data_offset(skb, (2 * ETH_ALEN),
+					data + payload_offset,
+					pkt_len - 4 - (2 * ETH_ALEN));
+
 			skb->protocol = eth_type_trans(skb, ndev);
 
 			/* Get receive timestamp from the skb */
-			if (fep->hwts_rx_en && fep->bufdesc_ex) {
+			if (fep->hwts_rx_en && ebdp) {
 				struct skb_shared_hwtstamps *shhwtstamps =
 							    skb_hwtstamps(skb);
 				unsigned long flags;
-				struct bufdesc_ex *ebdp =
-					(struct bufdesc_ex *)bdp;
 
 				memset(shhwtstamps, 0, sizeof(*shhwtstamps));
 
@@ -897,10 +929,7 @@ fec_enet_rx(struct net_device *ndev, int budget)
 				spin_unlock_irqrestore(&fep->tmreg_lock, flags);
 			}
 
-			if (fep->bufdesc_ex &&
-				(fep->csum_flags & FLAG_RX_CSUM_ENABLED)) {
-				struct bufdesc_ex *ebdp =
-					(struct bufdesc_ex *)bdp;
+			if (ebdp && (fep->csum_flags & FLAG_RX_CSUM_ENABLED)) {
 				if (!(ebdp->cbd_esc & FLAG_RX_CSUM_ERROR)) {
 					/* don't check it */
 					skb->ip_summed = CHECKSUM_UNNECESSARY;
@@ -909,6 +938,11 @@ fec_enet_rx(struct net_device *ndev, int budget)
 				}
 			}
 
+			/* Handle received VLAN packets */
+			if (vlan_packet_rcvd)
+				__vlan_hwaccel_put_tag(skb,
+						htons(ETH_P_8021Q), vlan_tag);
+
 			if (!skb_defer_rx_timestamp(skb))
 				napi_gro_receive(&fep->napi, skb);
 		}
@@ -1916,6 +1950,12 @@ static int fec_enet_init(struct net_device *ndev)
 	writel(FEC_RX_DISABLED_IMASK, fep->hwp + FEC_IMASK);
 	netif_napi_add(ndev, &fep->napi, fec_enet_rx_napi, FEC_NAPI_WEIGHT);
 
+	if (id_entry->driver_data & FEC_QUIRK_HAS_VLAN) {
+		/* enable hw VLAN support */
+		ndev->features |= NETIF_F_HW_VLAN_CTAG_RX;
+		ndev->hw_features |= NETIF_F_HW_VLAN_CTAG_RX;
+	}
+
 	if (id_entry->driver_data & FEC_QUIRK_HAS_CSUM) {
 		/* enable hw accelerator */
 		ndev->features |= (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM
-- 
1.7.10.4

RE: [PATCH net-next v2 1/1] net: fec: Add VLAN receive HW support.

[Duan Fugang-B38611]

On 06/28/13 22:08, Jim Baxter wrote:

> This enables the driver to take advantage of the FEC VLAN indicator to improve performance.
>
> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
> ---
> Change from v1 to v2
> Changed Max recieve Buffer Size Register from 1540 to 1536 so it is divisible by 64.
>
>  drivers/net/ethernet/freescale/fec.h      |    3 ++
>  drivers/net/ethernet/freescale/fec_main.c |   64 +++++++++++++++++++++++------
>  2 files changed, 55 insertions(+), 12 deletions(-)
> --
> 1.7.10.4

Acked-By: Fugang Duan  <B38611@freescale.com> 

Re: [PATCH net-next v2 1/1] net: fec: Add VLAN receive HW support.

[David Miller]

From: Jim Baxter <jim_baxter@mentor.com>
Date: Fri, 28 Jun 2013 15:08:23 +0100

> @@ -803,6 +807,9 @@ fec_enet_rx(struct net_device *ndev, int budget)
>  	ushort	pkt_len;
>  	__u8 *data;
>  	int	pkt_received = 0;
> +	struct	bufdesc_ex *ebdp = NULL;
> +	bool	vlan_packet_rcvd = false;
> +	u16	vlan_tag;
>  
>  #ifdef CONFIG_M532x
>  	flush_cache_all();
> @@ -866,6 +873,24 @@ fec_enet_rx(struct net_device *ndev, int budget)
>  		if (id_entry->driver_data & FEC_QUIRK_SWAP_FRAME)
>  			swap_buffer(data, pkt_len);
>  
> +		/* Extract the enhanced buffer descriptor */
> +		ebdp = NULL;
> +		if (fep->bufdesc_ex)
> +			ebdp = (struct bufdesc_ex *)bdp;

I would use a union here, so you'd have something like:

	union {
		struct bufdesc		*bdp;
		struct bufdesc_ex	*bdp_ex;
	} *p;

Alternatively, you can always use "struct bufdesc_ex *p", along with
the boolean saying if the extended descriptors are in use.

> +		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
> +				ebdp && (ebdp->cbd_esc & BD_ENET_RX_VLAN)) {

This is not indented properly.   The "ebp" on that second line must line
up exactly with the first column after the openning parenthesis on the
first line.

I see what you're trying to do, purely using TAB characters to indent
that second line.  But you must take the care and time to use the
appropriate number of TAB and space characters to place things at the
proper column.

> +			skb_copy_to_linear_data_offset(skb, (2 * ETH_ALEN),
> +					data + payload_offset,
> +					pkt_len - 4 - (2 * ETH_ALEN));

Again, line up the first non-space character on the second and third
lines of this function call so that they line up to the first column
after the openning parenthesis of the first line.

> +				__vlan_hwaccel_put_tag(skb,
> +						htons(ETH_P_8021Q), vlan_tag);

Likewise.

Re: [PATCH net-next v2 1/1] net: fec: Add VLAN receive HW support.

[Jim Baxter]

On 02/07/13 01:09, David Miller wrote:
> From: Jim Baxter <jim_baxter@mentor.com>
> Date: Fri, 28 Jun 2013 15:08:23 +0100
> 
>> @@ -803,6 +807,9 @@ fec_enet_rx(struct net_device *ndev, int budget)
>>  	ushort	pkt_len;
>>  	__u8 *data;
>>  	int	pkt_received = 0;
>> +	struct	bufdesc_ex *ebdp = NULL;
>> +	bool	vlan_packet_rcvd = false;
>> +	u16	vlan_tag;
>>  
>>  #ifdef CONFIG_M532x
>>  	flush_cache_all();
>> @@ -866,6 +873,24 @@ fec_enet_rx(struct net_device *ndev, int budget)
>>  		if (id_entry->driver_data & FEC_QUIRK_SWAP_FRAME)
>>  			swap_buffer(data, pkt_len);
>>  
>> +		/* Extract the enhanced buffer descriptor */
>> +		ebdp = NULL;
>> +		if (fep->bufdesc_ex)
>> +			ebdp = (struct bufdesc_ex *)bdp;
> 
> I would use a union here, so you'd have something like:
> 
> 	union {
> 		struct bufdesc		*bdp;
> 		struct bufdesc_ex	*bdp_ex;
> 	} *p;
> 
> Alternatively, you can always use "struct bufdesc_ex *p", along with
> the boolean saying if the extended descriptors are in use.
> 

I see (I think) I would replace the variables:
struct bufdesc *bdp;
struct	bufdesc_ex *ebdp

with a single union variable *p and then access p.bdp where the *bdp  is
currently used and use the fep->bufdesc_ex flag to determine if p.bdp_ex
can be accessed instead of my *ebdp variable.


>> +		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
>> +				ebdp && (ebdp->cbd_esc & BD_ENET_RX_VLAN)) {
> 
> This is not indented properly.   The "ebp" on that second line must line
> up exactly with the first column after the openning parenthesis on the
> first line.
> 
> I see what you're trying to do, purely using TAB characters to indent
> that second line.  But you must take the care and time to use the
> appropriate number of TAB and space characters to place things at the
> proper column.
> 

Thank you, I will do this correctly in future.

>> +			skb_copy_to_linear_data_offset(skb, (2 * ETH_ALEN),
>> +					data + payload_offset,
>> +					pkt_len - 4 - (2 * ETH_ALEN));
> 
> Again, line up the first non-space character on the second and third
> lines of this function call so that they line up to the first column
> after the openning parenthesis of the first line.
> 
>> +				__vlan_hwaccel_put_tag(skb,
>> +						htons(ETH_P_8021Q), vlan_tag);
> 
> Likewise.
> 

Jim

[PATCH RFC net-next v1 1/1] net: fec: Fix RMON registers on imx6

[Jim Baxter]

commit 38ae92d "fec: Add support for reading
RMON registers" causes the imx6Q to crash.

This fixes it and clears it using the MIB Control
Register's MIB_CLEAR bit.

Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
---
 drivers/net/ethernet/freescale/fec_main.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index ed6180e..a1f0413 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -607,8 +607,12 @@ fec_restart(struct net_device *ndev, int duplex)
 #ifndef CONFIG_M5272
 	/* Disable, clear, and enable the MIB */
 	writel(1 << 31, fep->hwp + FEC_MIB_CTRLSTAT);
-	for (i = RMON_T_DROP; i < IEEE_R_OCTETS_OK; i++)
-		writel(0, fep->hwp + i);
+	if (strcmp(id_entry->name, "imx6q-fec") == 0) {
+		writel(1 << 29, fep->hwp + FEC_MIB_CTRLSTAT);
+	} else {
+		for (i = RMON_T_DROP; i < IEEE_R_OCTETS_OK; i++)
+			writel(0, fep->hwp + i);
+	}
 	writel(0, fep->hwp + FEC_MIB_CTRLSTAT);
 #endif
 
-- 
1.7.10.4

RE: [PATCH RFC net-next v1 1/1] net: fec: Fix RMON registers on imx6

[Duan Fugang-B38611]

On 06/28/13 23:08, Jim Baxter wrote:

> commit 38ae92d "fec: Add support for reading RMON registers" causes the imx6Q to crash.
>
> This fixes it and clears it using the MIB Control Register's MIB_CLEAR bit.
>
> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index ed6180e..a1f0413 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -607,8 +607,12 @@ fec_restart(struct net_device *ndev, int duplex)  #ifndef CONFIG_M5272
>  	/* Disable, clear, and enable the MIB */
>  	writel(1 << 31, fep->hwp + FEC_MIB_CTRLSTAT);
> -	for (i = RMON_T_DROP; i < IEEE_R_OCTETS_OK; i++)
> -		writel(0, fep->hwp + i);
> +	if (strcmp(id_entry->name, "imx6q-fec") == 0) {
> +		writel(1 << 29, fep->hwp + FEC_MIB_CTRLSTAT);
> +	} else {
> +		for (i = RMON_T_DROP; i < IEEE_R_OCTETS_OK; i++)
> +			writel(0, fep->hwp + i);
> +	}
>  	writel(0, fep->hwp + FEC_MIB_CTRLSTAT);  #endif
>  
> --
> 1.7.10.4

It is not necessary to clear MIB since fec_restart() will reset MAC which cause all MAC register auto reset to  zero.
 
 
Thanks,
Andy

Re: [PATCH RFC net-next v1 1/1] net: fec: Fix RMON registers on imx6

[Jim Baxter]

On 30/06/13 04:55, Chris Healy wrote:
> On the i.MX28, I tested just setting bit 31 to 0 and not doing any of
> the clearing work and it performed correctly.  I'd suggest we just turn
> on the MIB logic in fec_restart regardless of which i.MX platform it
> is.  (Still need to not do anything if it is one of the coldfire
> platforms though.  Jim, would you like to do this our would you like me to?
> 
> Chris
> 
> 
> On Sat, Jun 29, 2013 at 3:00 PM, Chris Healy <cphealy@gmail.com
> <mailto:cphealy@gmail.com>> wrote:
> 
>     I took a look at what was going on in the various use cases with and
>     without the i.MX6x.
> 
>     The i.MX6x AND i.MX28 both have bit 29 which clears all the RMON
>     regs to zero when written to.  I was not aware of bit 29 with the
>     i.MX28 as I originally did this function for the i.MX51 and i.MX53
>     which do not have bit 29.
> 
>     Regardless, according to Andy, clearing the RMON registers in
>     fec_restart is moot as fec_restart restarts the MAC.  This being
>     true, we could probably just enable the MIB by setting bit 31 of
>     FEC_MIB_CTRLSTAT and be done.
> 
>     I have not tested this yet though.
> 
>     Chris
> 
> 
>     On Fri, Jun 28, 2013 at 10:58 PM, Duan Fugang-B38611
>     <B38611@freescale.com <mailto:B38611@freescale.com>> wrote:
> 
>         On 06/28/13 23:08, Jim Baxter wrote:
> 
>         > commit 38ae92d "fec: Add support for reading RMON registers"
>         causes the imx6Q to crash.
>         >
>         > This fixes it and clears it using the MIB Control Register's
>         MIB_CLEAR bit.
>         >
>         > Signed-off-by: Jim Baxter <jim_baxter@mentor.com
>         <mailto:jim_baxter@mentor.com>>
>         > ---
>         >  drivers/net/ethernet/freescale/fec_main.c |    8 ++++++--
>         >  1 file changed, 6 insertions(+), 2 deletions(-)
>         >
>         > diff --git a/drivers/net/ethernet/freescale/fec_main.c
>         b/drivers/net/ethernet/freescale/fec_main.c
>         > index ed6180e..a1f0413 100644
>         > --- a/drivers/net/ethernet/freescale/fec_main.c
>         > +++ b/drivers/net/ethernet/freescale/fec_main.c
>         > @@ -607,8 +607,12 @@ fec_restart(struct net_device *ndev, int
>         duplex)  #ifndef CONFIG_M5272
>         >       /* Disable, clear, and enable the MIB */
>         >       writel(1 << 31, fep->hwp + FEC_MIB_CTRLSTAT);
>         > -     for (i = RMON_T_DROP; i < IEEE_R_OCTETS_OK; i++)
>         > -             writel(0, fep->hwp + i);
>         > +     if (strcmp(id_entry->name, "imx6q-fec") == 0) {
>         > +             writel(1 << 29, fep->hwp + FEC_MIB_CTRLSTAT);
>         > +     } else {
>         > +             for (i = RMON_T_DROP; i < IEEE_R_OCTETS_OK; i++)
>         > +                     writel(0, fep->hwp + i);
>         > +     }
>         >       writel(0, fep->hwp + FEC_MIB_CTRLSTAT);  #endif
>         >
>         > --
>         > 1.7.10.4
> 
>         It is not necessary to clear MIB since fec_restart() will reset
>         MAC which cause all MAC register auto reset to  zero.
> 
> 
>         Thanks,
>         Andy
> 
> 
> 
Hi Chris,

I have submitted another patch, please check if that is what you meant?

Thank you,
Jim

[PATCH net-next v1 1/1] net: fec: Fix RMON registers on imx6

[Jim Baxter]

commit 38ae92d "fec: Add support for reading
RMON registers" causes the imx6Q to crash.

This fixes it by only enabling the RMON registers, the
registers are already cleared by the MAC being reset.

Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
---
Change v1 to v2
Removed per processor change and just set bit 31 to 0.

 drivers/net/ethernet/freescale/fec_main.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index ed6180e..c772df1 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -606,10 +606,7 @@ fec_restart(struct net_device *ndev, int duplex)
 
 #ifndef CONFIG_M5272
 	/* Disable, clear, and enable the MIB */
-	writel(1 << 31, fep->hwp + FEC_MIB_CTRLSTAT);
-	for (i = RMON_T_DROP; i < IEEE_R_OCTETS_OK; i++)
-		writel(0, fep->hwp + i);
-	writel(0, fep->hwp + FEC_MIB_CTRLSTAT);
+	writel(0 << 31, fep->hwp + FEC_MIB_CTRLSTAT);
 #endif
 
 	/* And last, enable the transmit and receive processing */
-- 
1.7.10.4

Re: [PATCH net-next v1 1/1] net: fec: Fix RMON registers on imx6

[Jim Baxter]

On 01/07/13 14:42, Chris Healy wrote:
> The comment one line up should change to reflect the functional change. 
> Something like:  "Enable the MIB logic"
> 
> 
> On Mon, Jul 1, 2013 at 3:31 AM, Jim Baxter <jim_baxter@mentor.com
> <mailto:jim_baxter@mentor.com>> wrote:
> 
>     commit 38ae92d "fec: Add support for reading
>     RMON registers" causes the imx6Q to crash.
> 
>     This fixes it by only enabling the RMON registers, the
>     registers are already cleared by the MAC being reset.
> 
>     Signed-off-by: Jim Baxter <jim_baxter@mentor.com
>     <mailto:jim_baxter@mentor.com>>
>     ---
>     Change v1 to v2
>     Removed per processor change and just set bit 31 to 0.
> 
>      drivers/net/ethernet/freescale/fec_main.c |    5 +----
>      1 file changed, 1 insertion(+), 4 deletions(-)
> 
>     diff --git a/drivers/net/ethernet/freescale/fec_main.c
>     b/drivers/net/ethernet/freescale/fec_main.c
>     index ed6180e..c772df1 100644
>     --- a/drivers/net/ethernet/freescale/fec_main.c
>     +++ b/drivers/net/ethernet/freescale/fec_main.c
>     @@ -606,10 +606,7 @@ fec_restart(struct net_device *ndev, int duplex)
> 
>      #ifndef CONFIG_M5272
>             /* Disable, clear, and enable the MIB */
>     -       writel(1 << 31, fep->hwp + FEC_MIB_CTRLSTAT);
>     -       for (i = RMON_T_DROP; i < IEEE_R_OCTETS_OK; i++)
>     -               writel(0, fep->hwp + i);
>     -       writel(0, fep->hwp + FEC_MIB_CTRLSTAT);
>     +       writel(0 << 31, fep->hwp + FEC_MIB_CTRLSTAT);
>      #endif
> 
>             /* And last, enable the transmit and receive processing */
>     --
>     1.7.10.4
> 
> 

Hi Chris,
Does "Enable the MIB statistic event counters" sound OK?

Thank you,
Jim

Re: [PATCH net-next v1 1/1] net: fec: Fix RMON registers on imx6

[David Miller]

From: Jim Baxter <jim_baxter@mentor.com>
Date: Mon,  1 Jul 2013 11:31:59 +0100

> commit 38ae92d "fec: Add support for reading
> RMON registers" causes the imx6Q to crash.
> 
> This fixes it by only enabling the RMON registers, the
> registers are already cleared by the MAC being reset.
> 
> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>

Applied, thanks.

[PATCH net-next v3 1/1] net: fec: Fix RMON registers on imx6

[Jim Baxter]

commit 38ae92d "fec: Add support for reading
RMON registers" causes the imx6Q to crash.

This fixes it by only enabling the RMON registers, the
registers are already cleared by the MAC being reset.

Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
---
Change v1 to v2
Removed per processor change and just set bit 31 to 0.
Change v2 to v3
Fixed comment to describe operation correctly.

 drivers/net/ethernet/freescale/fec_main.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index ed6180e..68834c3 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -605,11 +605,8 @@ fec_restart(struct net_device *ndev, int duplex)
 		ecntl |= (1 << 4);
 
 #ifndef CONFIG_M5272
-	/* Disable, clear, and enable the MIB */
-	writel(1 << 31, fep->hwp + FEC_MIB_CTRLSTAT);
-	for (i = RMON_T_DROP; i < IEEE_R_OCTETS_OK; i++)
-		writel(0, fep->hwp + i);
-	writel(0, fep->hwp + FEC_MIB_CTRLSTAT);
+	/* Enable the MIB statistic event counters */
+	writel(0 << 31, fep->hwp + FEC_MIB_CTRLSTAT);
 #endif
 
 	/* And last, enable the transmit and receive processing */
-- 
1.7.10.4

[PATCH net-next v3 1/1] net: fec: Add VLAN receive HW support.

[Jim Baxter]

This enables the driver to take advantage of the FEC VLAN
indicator to improve performance.

Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
---
Change from v1 to v2
- Changed Max recieve Buffer Size Register from 1540 to 1536 so it is
divisible by 64.
Change from v2 to v3
- Fixed indentation. pkt_len - 4 - (2 * ETH_ALEN)); extends to 86 chars,
but I feel that keeping the the code within 80 chars makes it confusing
to read.
- Changed code to use a flag instead of a pointer to check for the enhanced
buffer.

 drivers/net/ethernet/freescale/fec.h      |    3 ++
 drivers/net/ethernet/freescale/fec_main.c |   62 ++++++++++++++++++++++++-----
 2 files changed, 55 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 8362a03..2b0a0ea 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -203,6 +203,9 @@ struct bufdesc_ex {
 #define BD_ENET_RX_CL           ((ushort)0x0001)
 #define BD_ENET_RX_STATS        ((ushort)0x013f)        /* All status bits */
 
+/* Enhanced buffer descriptor control/status used by Ethernet receive */
+#define BD_ENET_RX_VLAN         0x00000004
+
 /* Buffer descriptor control/status used by Ethernet transmit.
 */
 #define BD_ENET_TX_READY        ((ushort)0x8000)
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index b676882..d106fa0 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -54,6 +54,7 @@
 #include <linux/of_gpio.h>
 #include <linux/of_net.h>
 #include <linux/regulator/consumer.h>
+#include <linux/if_vlan.h>
 
 #include <asm/cacheflush.h>
 
@@ -90,6 +91,8 @@ static void set_multicast_list(struct net_device *ndev);
 #define FEC_QUIRK_HAS_BUFDESC_EX	(1 << 4)
 /* Controller has hardware checksum support */
 #define FEC_QUIRK_HAS_CSUM		(1 << 5)
+/* Controller has hardware vlan support */
+#define FEC_QUIRK_HAS_VLAN		(1 << 6)
 
 static struct platform_device_id fec_devtype[] = {
 	{
@@ -108,7 +111,8 @@ static struct platform_device_id fec_devtype[] = {
 	}, {
 		.name = "imx6q-fec",
 		.driver_data = FEC_QUIRK_ENET_MAC | FEC_QUIRK_HAS_GBIT |
-				FEC_QUIRK_HAS_BUFDESC_EX | FEC_QUIRK_HAS_CSUM,
+				FEC_QUIRK_HAS_BUFDESC_EX | FEC_QUIRK_HAS_CSUM |
+				FEC_QUIRK_HAS_VLAN,
 	}, {
 		.name = "mvf600-fec",
 		.driver_data = FEC_QUIRK_ENET_MAC,
@@ -179,11 +183,11 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
 #define FEC_DEFAULT_IMASK (FEC_ENET_TXF | FEC_ENET_RXF | FEC_ENET_MII)
 #define FEC_RX_DISABLED_IMASK (FEC_DEFAULT_IMASK & (~FEC_ENET_RXF))
 
-/* The FEC stores dest/src/type, data, and checksum for receive packets.
+/* The FEC stores dest/src/type/vlan, data, and checksum for receive packets.
  */
-#define PKT_MAXBUF_SIZE		1518
+#define PKT_MAXBUF_SIZE		1522
 #define PKT_MINBUF_SIZE		64
-#define PKT_MAXBLR_SIZE		1520
+#define PKT_MAXBLR_SIZE		1536
 
 /* FEC receive acceleration */
 #define FEC_RACC_IPDIS		(1 << 1)
@@ -802,6 +806,9 @@ fec_enet_rx(struct net_device *ndev, int budget)
 	ushort	pkt_len;
 	__u8 *data;
 	int	pkt_received = 0;
+	struct	bufdesc_ex *ebdp = NULL;
+	bool	vlan_packet_rcvd = false;
+	u16	vlan_tag;
 
 #ifdef CONFIG_M532x
 	flush_cache_all();
@@ -865,6 +872,24 @@ fec_enet_rx(struct net_device *ndev, int budget)
 		if (id_entry->driver_data & FEC_QUIRK_SWAP_FRAME)
 			swap_buffer(data, pkt_len);
 
+		/* Extract the enhanced buffer descriptor */
+		ebdp = NULL;
+		if (fep->bufdesc_ex)
+			ebdp = (struct bufdesc_ex *)bdp;
+
+		/* If this is a VLAN packet remove the VLAN Tag */
+		vlan_packet_rcvd = false;
+		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
+		    fep->bufdesc_ex && (ebdp->cbd_esc & BD_ENET_RX_VLAN)) {
+			/* Push and remove the vlan tag */
+			struct vlan_hdr *vlan_header =
+					(struct vlan_hdr *) (data + ETH_HLEN);
+			vlan_tag = ntohs(vlan_header->h_vlan_TCI);
+			pkt_len -= VLAN_HLEN;
+
+			vlan_packet_rcvd = true;
+		}
+
 		/* This does 16 byte alignment, exactly what we need.
 		 * The packet length includes FCS, but we don't want to
 		 * include that when passing upstream as it messes up
@@ -875,9 +900,18 @@ fec_enet_rx(struct net_device *ndev, int budget)
 		if (unlikely(!skb)) {
 			ndev->stats.rx_dropped++;
 		} else {
+			int payload_offset = (2 * ETH_ALEN);
 			skb_reserve(skb, NET_IP_ALIGN);
 			skb_put(skb, pkt_len - 4);	/* Make room */
-			skb_copy_to_linear_data(skb, data, pkt_len - 4);
+
+			/* Extract the frame data without the VLAN header. */
+			skb_copy_to_linear_data(skb, data, (2 * ETH_ALEN));
+			if (vlan_packet_rcvd)
+				payload_offset = (2 * ETH_ALEN) + VLAN_HLEN;
+			skb_copy_to_linear_data_offset(skb, (2 * ETH_ALEN),
+						       data + payload_offset,
+						       pkt_len - 4 - (2 * ETH_ALEN));
+
 			skb->protocol = eth_type_trans(skb, ndev);
 
 			/* Get receive timestamp from the skb */
@@ -885,8 +919,6 @@ fec_enet_rx(struct net_device *ndev, int budget)
 				struct skb_shared_hwtstamps *shhwtstamps =
 							    skb_hwtstamps(skb);
 				unsigned long flags;
-				struct bufdesc_ex *ebdp =
-					(struct bufdesc_ex *)bdp;
 
 				memset(shhwtstamps, 0, sizeof(*shhwtstamps));
 
@@ -897,9 +929,7 @@ fec_enet_rx(struct net_device *ndev, int budget)
 			}
 
 			if (fep->bufdesc_ex &&
-				(fep->csum_flags & FLAG_RX_CSUM_ENABLED)) {
-				struct bufdesc_ex *ebdp =
-					(struct bufdesc_ex *)bdp;
+			    (fep->csum_flags & FLAG_RX_CSUM_ENABLED)) {
 				if (!(ebdp->cbd_esc & FLAG_RX_CSUM_ERROR)) {
 					/* don't check it */
 					skb->ip_summed = CHECKSUM_UNNECESSARY;
@@ -908,6 +938,12 @@ fec_enet_rx(struct net_device *ndev, int budget)
 				}
 			}
 
+			/* Handle received VLAN packets */
+			if (vlan_packet_rcvd)
+				__vlan_hwaccel_put_tag(skb,
+						       htons(ETH_P_8021Q),
+						       vlan_tag);
+
 			if (!skb_defer_rx_timestamp(skb))
 				napi_gro_receive(&fep->napi, skb);
 		}
@@ -1915,6 +1951,12 @@ static int fec_enet_init(struct net_device *ndev)
 	writel(FEC_RX_DISABLED_IMASK, fep->hwp + FEC_IMASK);
 	netif_napi_add(ndev, &fep->napi, fec_enet_rx_napi, FEC_NAPI_WEIGHT);
 
+	if (id_entry->driver_data & FEC_QUIRK_HAS_VLAN) {
+		/* enable hw VLAN support */
+		ndev->features |= NETIF_F_HW_VLAN_CTAG_RX;
+		ndev->hw_features |= NETIF_F_HW_VLAN_CTAG_RX;
+	}
+
 	if (id_entry->driver_data & FEC_QUIRK_HAS_CSUM) {
 		/* enable hw accelerator */
 		ndev->features |= (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM
-- 
1.7.10.4

Re: [PATCH net-next v3 1/1] net: fec: Add VLAN receive HW support.

[David Miller]

From: Jim Baxter <jim_baxter@mentor.com>
Date: Tue,  2 Jul 2013 22:52:56 +0100

> This enables the driver to take advantage of the FEC VLAN
> indicator to improve performance.
> 
> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>

Applied, thanks.

[RFC] kbuild: Build samples directory with make modules

[Jim Baxter]

If CONFIG_SAMPLES is enabled with any of the sample
modules enabled, they are only built if the make command
includes uImage and modules (i.e. make uImage modules).

This patches allows the kernel and modules to be built
separately, this is how openembedded and yocto build
the kernel.

Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
---
 Makefile |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 29339b4..1acd69f 100644
--- a/Makefile
+++ b/Makefile
@@ -772,9 +772,6 @@ vmlinux: scripts/link-vmlinux.sh $(vmlinux-deps) FORCE
 ifdef CONFIG_HEADERS_CHECK
 	$(Q)$(MAKE) -f $(srctree)/Makefile headers_check
 endif
-ifdef CONFIG_SAMPLES
-	$(Q)$(MAKE) $(build)=samples
-endif
 ifdef CONFIG_BUILD_DOCSRC
 	$(Q)$(MAKE) $(build)=Documentation
 endif
@@ -943,6 +940,9 @@ all: modules
 
 PHONY += modules
 modules: $(vmlinux-dirs) $(if $(KBUILD_BUILTIN),vmlinux) modules.builtin
+ifdef CONFIG_SAMPLES
+	$(Q)$(MAKE) $(build)=samples
+endif
 	$(Q)$(AWK) '!x[$$0]++' $(vmlinux-dirs:%=$(objtree)/%/modules.order) > $(objtree)/modules.order
 	@$(kecho) '  Building modules, stage 2.';
 	$(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modpost
-- 
1.7.10.4

Re: [RFC] kbuild: Build samples directory with make modules

[Sam Ravnborg]

On Tue, Jul 16, 2013 at 07:29:50PM +0100, Jim Baxter wrote:
> If CONFIG_SAMPLES is enabled with any of the sample
> modules enabled, they are only built if the make command
> includes uImage and modules (i.e. make uImage modules).

With the current location samples are built when you build vmlinux.
And vmlinux is implicit when you just type "make".

With the suggestion you have samples are built when you do "make modules".
IMO this is less logical than to built them with vmlinux as we do today.

	Sam

Re: [RFC] kbuild: Build samples directory with make modules

[Jim Baxter]

On 16/07/13 20:37, Sam Ravnborg wrote:
> On Tue, Jul 16, 2013 at 07:29:50PM +0100, Jim Baxter wrote:
>> If CONFIG_SAMPLES is enabled with any of the sample
>> modules enabled, they are only built if the make command
>> includes uImage and modules (i.e. make uImage modules).
> 
> With the current location samples are built when you build vmlinux.
> And vmlinux is implicit when you just type "make".
> 
> With the suggestion you have samples are built when you do "make modules".
> IMO this is less logical than to built them with vmlinux as we do today.
> 
> 	Sam
> 

Hi,

I am building it with:
make uImage
make modules

At least bitbake in Yocto and Openembedded is.

Running make on it's own is the equivalent of:
make vmlinux modules

which builds the samples directory, however if you run:
make vmlinux
make modules

Then the Samples are not built, that is what I am trying to fix.

Thank you,
Jim

Re: [RFC] kbuild: Build samples directory with make modules

[Jim Baxter]

On 16/07/13 20:37, Sam Ravnborg wrote:
> On Tue, Jul 16, 2013 at 07:29:50PM +0100, Jim Baxter wrote:
>> If CONFIG_SAMPLES is enabled with any of the sample
>> modules enabled, they are only built if the make command
>> includes uImage and modules (i.e. make uImage modules).
> 
> With the current location samples are built when you build vmlinux.
> And vmlinux is implicit when you just type "make".
> 
> With the suggestion you have samples are built when you do "make modules".
> IMO this is less logical than to built them with vmlinux as we do today.
> 
> 	Sam
> 

Hi,

I am building it with:
make uImage
make modules

At least bitbake in Yocto and Openembedded is.

Running make on it's own is the equivalent of:
make vmlinux modules

which builds the samples directory, however if you run:
make vmlinux
make modules

Then the Samples are not built, that is what I am trying to fix.

Thank you,
Jim

Fwd: [RFC] kbuild: Build samples directory with make modules

[Syed Muhammad Mohsin Kazmi]

Is there any specific reason that this patch is ignored still yet?
Compiling the uImage and modules together add the support of samples?
Shouldn't it be compiled as a module separately and that is correct
behavior in case of if samples are compiled as loadable modules?

Thanks,
Mohsin Kazmi


On Wed, Jul 17, 2013 at 2:15 AM, Jim Baxter <jim_baxter@mentor.com> wrote:
>
> On 16/07/13 20:37, Sam Ravnborg wrote:
> > On Tue, Jul 16, 2013 at 07:29:50PM +0100, Jim Baxter wrote:
> >> If CONFIG_SAMPLES is enabled with any of the sample
> >> modules enabled, they are only built if the make command
> >> includes uImage and modules (i.e. make uImage modules).
> >
> > With the current location samples are built when you build vmlinux.
> > And vmlinux is implicit when you just type "make".
> >
> > With the suggestion you have samples are built when you do "make modules".
> > IMO this is less logical than to built them with vmlinux as we do today.
> >
> >       Sam
> >
>
> Hi,
>
> I am building it with:
> make uImage
> make modules
>
> At least bitbake in Yocto and Openembedded is.
>
> Running make on it's own is the equivalent of:
> make vmlinux modules
>
> which builds the samples directory, however if you run:
> make vmlinux
> make modules
>
> Then the Samples are not built, that is what I am trying to fix.
>
> Thank you,
> Jim
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v1 0/3] usb: gadget: NCM: Fixes and Multi-frame for TX.

[Jim Baxter]

This series adds the ability to support packaging multiple network
packets into a single 16k CDC-NCM NTB.

Patches 1 and 3 are fixes for the receive unwrap function which
previously was unable to handle an NTB with multiple NDP's and a fix
that switches from using skb_clone to creating a new packet to fix the
issue of the truesize being far to big and causing packets to be
dropped incorrectly.

Jim Baxter (3):
  usb: gadget: NCM: RX function support multiple NDPs
  usb: gadget: NCM: Add transmit multi-frame.
  usb: gadget: NCM: Stop RX TCP Bursts getting dropped.

 drivers/usb/gadget/f_ncm.c   |  480 +++++++++++++++++++++++++++++-------------
 drivers/usb/gadget/u_ether.c |   19 +-
 drivers/usb/gadget/u_ether.h |    2 +
 3 files changed, 347 insertions(+), 154 deletions(-)

-- 
1.7.9.5

[PATCH v1 1/3] usb: gadget: NCM: RX function support multiple NDPs

[Jim Baxter]

The NDP was ignoring the wNextNdpIndex in the NDP which
means that NTBs containing multiple NDPs would have missed
frames.

Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
---
 drivers/usb/gadget/f_ncm.c |  146 +++++++++++++++++++++++---------------------
 1 file changed, 78 insertions(+), 68 deletions(-)

diff --git a/drivers/usb/gadget/f_ncm.c b/drivers/usb/gadget/f_ncm.c
index a9499fd..d0ebbac 100644
--- a/drivers/usb/gadget/f_ncm.c
+++ b/drivers/usb/gadget/f_ncm.c
@@ -963,6 +963,7 @@ static int ncm_unwrap_ntb(struct gether *port,
 	struct f_ncm	*ncm = func_to_ncm(&port->func);
 	__le16		*tmp = (void *) skb->data;
 	unsigned	index, index2;
+	int		ndp_index;
 	unsigned	dg_len, dg_len2;
 	unsigned	ndp_len;
 	struct sk_buff	*skb2;
@@ -995,91 +996,100 @@ static int ncm_unwrap_ntb(struct gether *port,
 		goto err;
 	}
 
-	index = get_ncm(&tmp, opts->fp_index);
-	/* NCM 3.2 */
-	if (((index % 4) != 0) && (index < opts->nth_size)) {
-		INFO(port->func.config->cdev, "Bad index: %x\n",
-			index);
-		goto err;
-	}
-
-	/* walk through NDP */
-	tmp = ((void *)skb->data) + index;
-	if (get_unaligned_le32(tmp) != ncm->ndp_sign) {
-		INFO(port->func.config->cdev, "Wrong NDP SIGN\n");
-		goto err;
-	}
-	tmp += 2;
-
-	ndp_len = get_unaligned_le16(tmp++);
-	/*
-	 * NCM 3.3.1
-	 * entry is 2 items
-	 * item size is 16/32 bits, opts->dgram_item_len * 2 bytes
-	 * minimal: struct usb_cdc_ncm_ndpX + normal entry + zero entry
-	 */
-	if ((ndp_len < opts->ndp_size + 2 * 2 * (opts->dgram_item_len * 2))
-	    || (ndp_len % opts->ndplen_align != 0)) {
-		INFO(port->func.config->cdev, "Bad NDP length: %x\n", ndp_len);
-		goto err;
-	}
-	tmp += opts->reserved1;
-	tmp += opts->next_fp_index; /* skip reserved (d)wNextFpIndex */
-	tmp += opts->reserved2;
-
-	ndp_len -= opts->ndp_size;
-	index2 = get_ncm(&tmp, opts->dgram_item_len);
-	dg_len2 = get_ncm(&tmp, opts->dgram_item_len);
-	dgram_counter = 0;
+	ndp_index = get_ncm(&tmp, opts->fp_index);
 
+	/* Run through all the NDP's in the NTB */
 	do {
-		index = index2;
-		dg_len = dg_len2;
-		if (dg_len < 14 + crc_len) { /* ethernet header + crc */
-			INFO(port->func.config->cdev, "Bad dgram length: %x\n",
-			     dg_len);
+		/* NCM 3.2 */
+		if (((ndp_index % 4) != 0) &&
+				(ndp_index < opts->nth_size)) {
+			INFO(port->func.config->cdev, "Bad index: %#X\n",
+			     ndp_index);
 			goto err;
 		}
-		if (ncm->is_crc) {
-			uint32_t crc, crc2;
-
-			crc = get_unaligned_le32(skb->data +
-						 index + dg_len - crc_len);
-			crc2 = ~crc32_le(~0,
-					 skb->data + index,
-					 dg_len - crc_len);
-			if (crc != crc2) {
-				INFO(port->func.config->cdev, "Bad CRC\n");
-				goto err;
-			}
+
+		/* walk through NDP */
+		tmp = (void *)(skb->data + ndp_index);
+		if (get_unaligned_le32(tmp) != ncm->ndp_sign) {
+			INFO(port->func.config->cdev, "Wrong NDP SIGN\n");
+			goto err;
 		}
+		tmp += 2;
 
+		ndp_len = get_unaligned_le16(tmp++);
+		/*
+		 * NCM 3.3.1
+		 * entry is 2 items
+		 * item size is 16/32 bits, opts->dgram_item_len * 2 bytes
+		 * minimal: struct usb_cdc_ncm_ndpX + normal entry + zero entry
+		 * Each entry is a dgram index and a dgram length.
+		 */
+		if ((ndp_len < opts->ndp_size
+				+ 2 * 2 * (opts->dgram_item_len * 2))
+				|| (ndp_len % opts->ndplen_align != 0)) {
+			INFO(port->func.config->cdev, "Bad NDP length: %#X\n",
+			     ndp_len);
+			goto err;
+		}
+		tmp += opts->reserved1;
+		/* Check for another NDP (d)wNextNdpIndex */
+		ndp_index = get_ncm(&tmp, opts->next_fp_index);
+		tmp += opts->reserved2;
+
+		ndp_len -= opts->ndp_size;
 		index2 = get_ncm(&tmp, opts->dgram_item_len);
 		dg_len2 = get_ncm(&tmp, opts->dgram_item_len);
+		dgram_counter = 0;
+
+		do {
+			index = index2;
+			dg_len = dg_len2;
+			if (dg_len < 14 + crc_len) { /* ethernet hdr + crc */
+				INFO(port->func.config->cdev,
+				     "Bad dgram length: %#X\n", dg_len);
+				goto err;
+			}
+			if (ncm->is_crc) {
+				uint32_t crc, crc2;
+
+				crc = get_unaligned_le32(skb->data +
+							 index + dg_len -
+							 crc_len);
+				crc2 = ~crc32_le(~0,
+						 skb->data + index,
+						 dg_len - crc_len);
+				if (crc != crc2) {
+					INFO(port->func.config->cdev,
+					     "Bad CRC\n");
+					goto err;
+				}
+			}
+
+			index2 = get_ncm(&tmp, opts->dgram_item_len);
+			dg_len2 = get_ncm(&tmp, opts->dgram_item_len);
 
-		if (index2 == 0 || dg_len2 == 0) {
-			skb2 = skb;
-		} else {
 			skb2 = skb_clone(skb, GFP_ATOMIC);
 			if (skb2 == NULL)
 				goto err;
-		}
 
-		if (!skb_pull(skb2, index)) {
-			ret = -EOVERFLOW;
-			goto err;
-		}
+			if (!skb_pull(skb2, index)) {
+				ret = -EOVERFLOW;
+				goto err;
+			}
 
-		skb_trim(skb2, dg_len - crc_len);
-		skb_queue_tail(list, skb2);
+			skb_trim(skb2, dg_len - crc_len);
+			skb_queue_tail(list, skb2);
 
-		ndp_len -= 2 * (opts->dgram_item_len * 2);
+			ndp_len -= 2 * (opts->dgram_item_len * 2);
 
-		dgram_counter++;
+			dgram_counter++;
 
-		if (index2 == 0 || dg_len2 == 0)
-			break;
-	} while (ndp_len > 2 * (opts->dgram_item_len * 2)); /* zero entry */
+			if (index2 == 0 || dg_len2 == 0)
+				break;
+		} while (ndp_len > 2 * (opts->dgram_item_len * 2));
+	} while (ndp_index);
+
+	dev_kfree_skb_any(skb);
 
 	VDBG(port->func.config->cdev,
 	     "Parsed NTB with %d frames\n", dgram_counter);
-- 
1.7.9.5

Re: [PATCH v1 1/3] usb: gadget: NCM: RX function support multiple NDPs

[Bjørn Mork]

Jim Baxter <jim_baxter@mentor.com> writes:

> The NDP was ignoring the wNextNdpIndex in the NDP which
> means that NTBs containing multiple NDPs would have missed
> frames.

Well, just for the record: I believe this field was meant to be reserved
and always 0 in the CDC NCM spec.  Table 3-3, describing 16bit NDPs,
says so.  But reading the spec now, I noticed that there is an
inconsistency between table 3-3 and table 3-4 describing 32bit NDPs.  It
looks like the field is not reserved in the 32bit version.

I am pretty sure that is a specification error, but I guess it doesn't
harm to implement the support anyway.  And it paves the way for an MBIM
gadget :-)




Bjørn

Re: [PATCH v1 1/3] usb: gadget: NCM: RX function support multiple NDPs

[Jim Baxter]

On 29/05/14 19:55, Bjørn Mork wrote:
> Jim Baxter <jim_baxter@mentor.com> writes:
> 
>> The NDP was ignoring the wNextNdpIndex in the NDP which
>> means that NTBs containing multiple NDPs would have missed
>> frames.
> 
> Well, just for the record: I believe this field was meant to be reserved
> and always 0 in the CDC NCM spec.  Table 3-3, describing 16bit NDPs,
> says so.  But reading the spec now, I noticed that there is an
> inconsistency between table 3-3 and table 3-4 describing 32bit NDPs.  It
> looks like the field is not reserved in the 32bit version.
> 
> I am pretty sure that is a specification error, but I guess it doesn't
> harm to implement the support anyway.  And it paves the way for an MBIM
> gadget :-)
> 
> 
> 
> 
> Bjørn
> 

Yes I was looking at Figure 3-1, Figure 3-2 which all state it is a
link. I must have been reading Table 3-4.

I am glad to see it helps with the MBIM specification :-) .

Jim

[PATCH v1 2/3] usb: gadget: NCM: Add transmit multi-frame.

[Jim Baxter]

This adds multi-frame support to the NCM NTB's for
the gadget driver. This allows multiple network
packets to be put inside a single USB NTB with a
maximum size of 16kB.

It has a time out of 300ms to ensure that smaller
number of packets still maintain a normal latency.

Also the .fp_index and .next_fp_index have been
changed to .ndp_index and .next_ndp_index to
match the latest CDC-NCM specification and
help with maintenance.

Results transmitting from gadget to host.

Before the change:

TCP_STREAM Throughput (10^6bits/sec): 22.72
UDP_STREAM Throughput (10^6bits/sec): 25.94

Latency:
netperf -H 192.168.1.101 -v2 -l 50 -t TCP_RR -- -r 16384,16384
Trans.   RoundTrip  Throughput
Rate     Latency    10^6bits/s
per sec  usec/Tran  Outbound

100.83   9918.116   13.215

After the change:

TCP_STREAM Throughput (10^6bits/sec): 124.26
UDP_STREAM Throughput (10^6bits/sec): 227.48

Latency:
netperf -H 192.168.1.101 -v2 -l 50 -t TCP_RR -- -r 16384,16384
Trans.   RoundTrip  Throughput
Rate     Latency    10^6bits/s
per sec  usec/Tran  Outbound

156.80   6377.730   20.552

Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
---
 drivers/usb/gadget/f_ncm.c   |  335 ++++++++++++++++++++++++++++++++----------
 drivers/usb/gadget/u_ether.c |   19 ++-
 drivers/usb/gadget/u_ether.h |    2 +
 3 files changed, 269 insertions(+), 87 deletions(-)

diff --git a/drivers/usb/gadget/f_ncm.c b/drivers/usb/gadget/f_ncm.c
index d0ebbac..5452fb6 100644
--- a/drivers/usb/gadget/f_ncm.c
+++ b/drivers/usb/gadget/f_ncm.c
@@ -68,6 +68,18 @@ struct f_ncm {
 	 * callback and ethernet open/close
 	 */
 	spinlock_t			lock;
+
+	struct net_device		*netdev;
+
+	/* For multi-frame NDP TX */
+	struct sk_buff			*skb_tx_data;
+	struct sk_buff			*skb_tx_ndp;
+	u16				ndp_dgram_count;
+	bool				timer_force_tx;
+	struct tasklet_struct		tx_tasklet;
+	struct hrtimer			task_timer;
+
+	bool				timer_stopping;
 };
 
 static inline struct f_ncm *func_to_ncm(struct usb_function *f)
@@ -92,15 +104,20 @@ static inline unsigned ncm_bitrate(struct usb_gadget *g)
  * If the host can group frames, allow it to do that, 16K is selected,
  * because it's used by default by the current linux host driver
  */
-#define NTB_DEFAULT_IN_SIZE	USB_CDC_NCM_NTB_MIN_IN_SIZE
+#define NTB_DEFAULT_IN_SIZE	16384
 #define NTB_OUT_SIZE		16384
 
-/*
- * skbs of size less than that will not be aligned
- * to NCM's dwNtbInMaxSize to save bus bandwidth
+/* Allocation for storing the NDP, 32 should suffice for a
+ * 16k packet. This allows a maximum of 32 * 507 Byte packets to
+ * be transmitted in a single 16kB skb, though when sending full size
+ * packets this limit will be plenty.
+ * Smaller packets are not likely to be trying to maximize the
+ * throughput and will be mstly sending smaller infrequent frames.
  */
+#define TX_MAX_NUM_DPE		32
 
-#define	MAX_TX_NONFIXED		(512 * 3)
+/* Delay for the transmit to wait before sending an unfilled NTB frame. */
+#define TX_TIMEOUT_NSECS	300000
 
 #define FORMATS_SUPPORTED	(USB_CDC_NCM_NTB16_SUPPORTED |	\
 				 USB_CDC_NCM_NTB32_SUPPORTED)
@@ -355,14 +372,15 @@ struct ndp_parser_opts {
 	u32		ndp_sign;
 	unsigned	nth_size;
 	unsigned	ndp_size;
+	unsigned	dpe_size;
 	unsigned	ndplen_align;
 	/* sizes in u16 units */
 	unsigned	dgram_item_len; /* index or length */
 	unsigned	block_length;
-	unsigned	fp_index;
+	unsigned	ndp_index;
 	unsigned	reserved1;
 	unsigned	reserved2;
-	unsigned	next_fp_index;
+	unsigned	next_ndp_index;
 };
 
 #define INIT_NDP16_OPTS {					\
@@ -370,13 +388,14 @@ struct ndp_parser_opts {
 		.ndp_sign = USB_CDC_NCM_NDP16_NOCRC_SIGN,	\
 		.nth_size = sizeof(struct usb_cdc_ncm_nth16),	\
 		.ndp_size = sizeof(struct usb_cdc_ncm_ndp16),	\
+		.dpe_size = sizeof(struct usb_cdc_ncm_dpe16),	\
 		.ndplen_align = 4,				\
 		.dgram_item_len = 1,				\
 		.block_length = 1,				\
-		.fp_index = 1,					\
+		.ndp_index = 1,					\
 		.reserved1 = 0,					\
 		.reserved2 = 0,					\
-		.next_fp_index = 1,				\
+		.next_ndp_index = 1,				\
 	}
 
 
@@ -385,13 +404,14 @@ struct ndp_parser_opts {
 		.ndp_sign = USB_CDC_NCM_NDP32_NOCRC_SIGN,	\
 		.nth_size = sizeof(struct usb_cdc_ncm_nth32),	\
 		.ndp_size = sizeof(struct usb_cdc_ncm_ndp32),	\
+		.dpe_size = sizeof(struct usb_cdc_ncm_dpe32),	\
 		.ndplen_align = 8,				\
 		.dgram_item_len = 2,				\
 		.block_length = 2,				\
-		.fp_index = 2,					\
+		.ndp_index = 2,					\
 		.reserved1 = 1,					\
 		.reserved2 = 2,					\
-		.next_fp_index = 2,				\
+		.next_ndp_index = 2,				\
 	}
 
 static const struct ndp_parser_opts ndp16_opts = INIT_NDP16_OPTS;
@@ -803,6 +823,8 @@ static int ncm_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 
 		if (ncm->port.in_ep->driver_data) {
 			DBG(cdev, "reset ncm\n");
+			ncm->timer_stopping = true;
+			ncm->netdev = NULL;
 			gether_disconnect(&ncm->port);
 			ncm_reset_values(ncm);
 		}
@@ -839,6 +861,8 @@ static int ncm_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 			net = gether_connect(&ncm->port);
 			if (IS_ERR(net))
 				return PTR_ERR(net);
+			ncm->netdev = net;
+			ncm->timer_stopping = false;
 		}
 
 		spin_lock(&ncm->lock);
@@ -865,95 +889,232 @@ static int ncm_get_alt(struct usb_function *f, unsigned intf)
 	return ncm->port.in_ep->driver_data ? 1 : 0;
 }
 
+static struct sk_buff *package_for_tx(struct f_ncm *ncm)
+{
+	__le16		*ntb_iter;
+	struct sk_buff	*skb2 = NULL;
+	unsigned	ndp_pad;
+	unsigned	ndp_index;
+	unsigned	new_len;
+
+	const struct ndp_parser_opts *opts = ncm->parser_opts;
+	const int ndp_align = le16_to_cpu(ntb_parameters.wNdpInAlignment);
+	const int dgram_idx_len = 2 * 2 * opts->dgram_item_len;
+
+	/* Stop the timer */
+	hrtimer_try_to_cancel(&ncm->task_timer);
+
+	ndp_pad = ALIGN(ncm->skb_tx_data->len, ndp_align) -
+			ncm->skb_tx_data->len;
+	ndp_index = ncm->skb_tx_data->len + ndp_pad;
+	new_len = ndp_index + dgram_idx_len + ncm->skb_tx_ndp->len;
+
+	/* Set the final BlockLength and wNdpIndex */
+	ntb_iter = (void *) ncm->skb_tx_data->data;
+	/* Increment pointer to BlockLength */
+	ntb_iter += 2 + 1 + 1;
+	put_ncm(&ntb_iter, opts->block_length, new_len);
+	put_ncm(&ntb_iter, opts->ndp_index, ndp_index);
+
+	/* Set the final NDP wLength */
+	new_len = opts->ndp_size +
+			(ncm->ndp_dgram_count * dgram_idx_len);
+	ncm->ndp_dgram_count = 0;
+	/* Increment from start to wLength */
+	ntb_iter = (void *) ncm->skb_tx_ndp->data;
+	ntb_iter += 2;
+	put_unaligned_le16(new_len, ntb_iter);
+
+	/* Merge the skbs */
+	swap(skb2, ncm->skb_tx_data);
+	if (ncm->skb_tx_data) {
+		dev_kfree_skb_any(ncm->skb_tx_data);
+		ncm->skb_tx_data = NULL;
+	}
+
+	/* Insert NDP alignment. */
+	ntb_iter = (void *) skb_put(skb2, ndp_pad);
+	memset(ntb_iter, 0, ndp_pad);
+
+	/* Copy NTB across. */
+	ntb_iter = (void *) skb_put(skb2, ncm->skb_tx_ndp->len);
+	memcpy(ntb_iter, ncm->skb_tx_ndp->data, ncm->skb_tx_ndp->len);
+	dev_kfree_skb_any(ncm->skb_tx_ndp);
+	ncm->skb_tx_ndp = NULL;
+
+	/* Insert zero'd datagram. */
+	ntb_iter = (void *) skb_put(skb2, dgram_idx_len);
+	memset(ntb_iter, 0, dgram_idx_len);
+
+	return skb2;
+}
+
 static struct sk_buff *ncm_wrap_ntb(struct gether *port,
 				    struct sk_buff *skb)
 {
 	struct f_ncm	*ncm = func_to_ncm(&port->func);
-	struct sk_buff	*skb2;
+	struct sk_buff	*skb2 = NULL;
 	int		ncb_len = 0;
-	__le16		*tmp;
-	int		div;
-	int		rem;
-	int		pad;
-	int		ndp_align;
-	int		ndp_pad;
+	__le16		*ntb_data;
+	__le16		*ntb_ndp;
+	int		dgram_pad;
+
 	unsigned	max_size = ncm->port.fixed_in_len;
 	const struct ndp_parser_opts *opts = ncm->parser_opts;
-	unsigned	crc_len = ncm->is_crc ? sizeof(uint32_t) : 0;
-
-	div = le16_to_cpu(ntb_parameters.wNdpInDivisor);
-	rem = le16_to_cpu(ntb_parameters.wNdpInPayloadRemainder);
-	ndp_align = le16_to_cpu(ntb_parameters.wNdpInAlignment);
-
-	ncb_len += opts->nth_size;
-	ndp_pad = ALIGN(ncb_len, ndp_align) - ncb_len;
-	ncb_len += ndp_pad;
-	ncb_len += opts->ndp_size;
-	ncb_len += 2 * 2 * opts->dgram_item_len; /* Datagram entry */
-	ncb_len += 2 * 2 * opts->dgram_item_len; /* Zero datagram entry */
-	pad = ALIGN(ncb_len, div) + rem - ncb_len;
-	ncb_len += pad;
+	const int ndp_align = le16_to_cpu(ntb_parameters.wNdpInAlignment);
+	const int div = le16_to_cpu(ntb_parameters.wNdpInDivisor);
+	const int rem = le16_to_cpu(ntb_parameters.wNdpInPayloadRemainder);
+	const int dgram_idx_len = 2 * 2 * opts->dgram_item_len;
 
-	if (ncb_len + skb->len + crc_len > max_size) {
-		dev_kfree_skb_any(skb);
+	if (!skb && !ncm->skb_tx_data)
 		return NULL;
-	}
 
-	skb2 = skb_copy_expand(skb, ncb_len,
-			       max_size - skb->len - ncb_len - crc_len,
-			       GFP_ATOMIC);
-	dev_kfree_skb_any(skb);
-	if (!skb2)
-		return NULL;
+	if (skb) {
+		/* Add the CRC if required up front */
+		if (ncm->is_crc) {
+			uint32_t	crc;
+			__le16		*crc_pos;
+
+			crc = ~crc32_le(~0,
+					skb->data,
+					skb->len);
+			crc_pos = (void *) skb_put(skb, sizeof(uint32_t));
+			put_unaligned_le32(crc, crc_pos);
+		}
 
-	skb = skb2;
+		/* If the new skb is too big for the current NCM NTB then
+		 * set the current stored skb to be sent now and clear it
+		 * ready for new data.
+		 * NOTE: Assume maximum align for speed of calculation.
+		 */
+		if (ncm->skb_tx_data
+		    && (ncm->ndp_dgram_count >= TX_MAX_NUM_DPE
+		    || (ncm->skb_tx_data->len +
+		    div + rem + skb->len +
+		    ncm->skb_tx_ndp->len + ndp_align + (2 * dgram_idx_len))
+		    > max_size)) {
+			skb2 = package_for_tx(ncm);
+			if (!skb2)
+				goto err;
+		}
 
-	tmp = (void *) skb_push(skb, ncb_len);
-	memset(tmp, 0, ncb_len);
+		if (!ncm->skb_tx_data) {
+			ncb_len = opts->nth_size;
+			dgram_pad = ALIGN(ncb_len, div) + rem - ncb_len;
+			ncb_len += dgram_pad;
 
-	put_unaligned_le32(opts->nth_sign, tmp); /* dwSignature */
-	tmp += 2;
-	/* wHeaderLength */
-	put_unaligned_le16(opts->nth_size, tmp++);
-	tmp++; /* skip wSequence */
-	put_ncm(&tmp, opts->block_length, skb->len); /* (d)wBlockLength */
-	/* (d)wFpIndex */
-	/* the first pointer is right after the NTH + align */
-	put_ncm(&tmp, opts->fp_index, opts->nth_size + ndp_pad);
+			/* Create a new skb for the NTH and datagrams. */
+			ncm->skb_tx_data = alloc_skb(max_size, GFP_ATOMIC);
+			if (!ncm->skb_tx_data)
+				goto err;
 
-	tmp = (void *)tmp + ndp_pad;
+			ntb_data = (void *) skb_put(ncm->skb_tx_data, ncb_len);
+			memset(ntb_data, 0, ncb_len);
+			/* dwSignature */
+			put_unaligned_le32(opts->nth_sign, ntb_data);
+			ntb_data += 2;
+			/* wHeaderLength */
+			put_unaligned_le16(opts->nth_size, ntb_data++);
+
+			/* Allocate an skb for storing the NDP,
+			 * TX_MAX_NUM_DPE should easily suffice for a
+			 * 16k packet.
+			 */
+			ncm->skb_tx_ndp = alloc_skb((int)(opts->ndp_size
+						    + opts->dpe_size
+						    * TX_MAX_NUM_DPE),
+						    GFP_ATOMIC);
+			if (!ncm->skb_tx_ndp)
+				goto err;
+			ntb_ndp = (void *) skb_put(ncm->skb_tx_ndp,
+						    opts->ndp_size);
+			memset(ntb_ndp, 0, ncb_len);
+			/* dwSignature */
+			put_unaligned_le32(ncm->ndp_sign, ntb_ndp);
+			ntb_ndp += 2;
 
-	/* NDP */
-	put_unaligned_le32(ncm->ndp_sign, tmp); /* dwSignature */
-	tmp += 2;
-	/* wLength */
-	put_unaligned_le16(ncb_len - opts->nth_size - pad, tmp++);
+			/* There is always a zeroed entry */
+			ncm->ndp_dgram_count = 1;
 
-	tmp += opts->reserved1;
-	tmp += opts->next_fp_index; /* skip reserved (d)wNextFpIndex */
-	tmp += opts->reserved2;
+			/* Note: we skip opts->next_ndp_index */
+		}
 
-	if (ncm->is_crc) {
-		uint32_t crc;
+		/* Delay the timer. */
+		hrtimer_start(&ncm->task_timer,
+			      ktime_set(0, TX_TIMEOUT_NSECS),
+			      HRTIMER_MODE_REL);
+
+		/* Add the datagram position entries */
+		ntb_ndp = (void *) skb_put(ncm->skb_tx_ndp, dgram_idx_len);
+		memset(ntb_ndp, 0, dgram_idx_len);
+
+		ncb_len = ncm->skb_tx_data->len;
+		dgram_pad = ALIGN(ncb_len, div) + rem - ncb_len;
+		ncb_len += dgram_pad;
+
+		/* (d)wDatagramIndex */
+		put_ncm(&ntb_ndp, opts->dgram_item_len, ncb_len);
+		/* (d)wDatagramLength */
+		put_ncm(&ntb_ndp, opts->dgram_item_len, skb->len);
+		ncm->ndp_dgram_count++;
+
+		/* Add the new data to the skb */
+		ntb_data = (void *) skb_put(ncm->skb_tx_data, dgram_pad);
+		memset(ntb_data, 0, dgram_pad);
+		ntb_data = (void *) skb_put(ncm->skb_tx_data, skb->len);
+		memcpy(ntb_data, skb->data, skb->len);
+		dev_kfree_skb_any(skb);
+		skb = NULL;
 
-		crc = ~crc32_le(~0,
-				skb->data + ncb_len,
-				skb->len - ncb_len);
-		put_unaligned_le32(crc, skb->data + skb->len);
-		skb_put(skb, crc_len);
+	} else if (ncm->skb_tx_data && ncm->timer_force_tx) {
+		/* If the tx was requested because of a timeout then send */
+		skb2 = package_for_tx(ncm);
+		if (!skb2)
+			goto err;
 	}
 
-	/* (d)wDatagramIndex[0] */
-	put_ncm(&tmp, opts->dgram_item_len, ncb_len);
-	/* (d)wDatagramLength[0] */
-	put_ncm(&tmp, opts->dgram_item_len, skb->len - ncb_len);
-	/* (d)wDatagramIndex[1] and  (d)wDatagramLength[1] already zeroed */
+	return skb2;
+
+err:
+	ncm->netdev->stats.tx_dropped++;
+
+	if (skb)
+		dev_kfree_skb_any(skb);
+	if (ncm->skb_tx_data)
+		dev_kfree_skb_any(ncm->skb_tx_data);
+	if (ncm->skb_tx_ndp)
+		dev_kfree_skb_any(ncm->skb_tx_ndp);
+
+	return NULL;
+}
+
+/*
+ * This transmits the NTB if there are frames waiting.
+ */
+static void ncm_tx_tasklet(unsigned long data)
+{
+	struct f_ncm	*ncm = (void *)data;
 
-	if (skb->len > MAX_TX_NONFIXED)
-		memset(skb_put(skb, max_size - skb->len),
-		       0, max_size - skb->len);
+	if (ncm->timer_stopping)
+		return;
+
+	/* Only send if data is available. */
+	if (ncm->skb_tx_data) {
+		ncm->timer_force_tx = true;
+		ncm->netdev->netdev_ops->ndo_start_xmit(NULL, ncm->netdev);
+		ncm->timer_force_tx = false;
+	}
+}
 
-	return skb;
+/*
+ * The transmit should only be run if no skb data has been sent
+ * for a certain duration.
+ */
+static enum hrtimer_restart ncm_tx_timeout(struct hrtimer *data)
+{
+	struct f_ncm *ncm = container_of(data, struct f_ncm, task_timer);
+	tasklet_schedule(&ncm->tx_tasklet);
+	return HRTIMER_NORESTART;
 }
 
 static int ncm_unwrap_ntb(struct gether *port,
@@ -996,7 +1157,7 @@ static int ncm_unwrap_ntb(struct gether *port,
 		goto err;
 	}
 
-	ndp_index = get_ncm(&tmp, opts->fp_index);
+	ndp_index = get_ncm(&tmp, opts->ndp_index);
 
 	/* Run through all the NDP's in the NTB */
 	do {
@@ -1033,7 +1194,7 @@ static int ncm_unwrap_ntb(struct gether *port,
 		}
 		tmp += opts->reserved1;
 		/* Check for another NDP (d)wNextNdpIndex */
-		ndp_index = get_ncm(&tmp, opts->next_fp_index);
+		ndp_index = get_ncm(&tmp, opts->next_ndp_index);
 		tmp += opts->reserved2;
 
 		ndp_len -= opts->ndp_size;
@@ -1107,8 +1268,11 @@ static void ncm_disable(struct usb_function *f)
 
 	DBG(cdev, "ncm deactivated\n");
 
-	if (ncm->port.in_ep->driver_data)
+	if (ncm->port.in_ep->driver_data) {
+		ncm->timer_stopping = true;
+		ncm->netdev = NULL;
 		gether_disconnect(&ncm->port);
+	}
 
 	if (ncm->notify->driver_data) {
 		usb_ep_disable(ncm->notify);
@@ -1277,6 +1441,10 @@ static int ncm_bind(struct usb_configuration *c, struct usb_function *f)
 	ncm->port.open = ncm_open;
 	ncm->port.close = ncm_close;
 
+	tasklet_init(&ncm->tx_tasklet, ncm_tx_tasklet, (unsigned long) ncm);
+	hrtimer_init(&ncm->task_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	ncm->task_timer.function = ncm_tx_timeout;
+
 	DBG(cdev, "CDC Network: %s speed IN/%s OUT/%s NOTIFY/%s\n",
 			gadget_is_dualspeed(c->cdev->gadget) ? "dual" : "full",
 			ncm->port.in_ep->name, ncm->port.out_ep->name,
@@ -1390,6 +1558,10 @@ static void ncm_unbind(struct usb_configuration *c, struct usb_function *f)
 
 	DBG(c->cdev, "ncm unbind\n");
 
+	hrtimer_cancel(&ncm->task_timer);
+	tasklet_kill(&ncm->tx_tasklet);
+
+	ncm_string_defs[0].id = 0;
 	usb_free_all_descriptors(f);
 
 	kfree(ncm->notify_req->buf);
@@ -1426,6 +1598,7 @@ static struct usb_function *ncm_alloc(struct usb_function_instance *fi)
 	ncm->port.ioport = netdev_priv(opts->net);
 	mutex_unlock(&opts->lock);
 	ncm->port.is_fixed = true;
+	ncm->port.supports_multi_frame = true;
 
 	ncm->port.func.name = "cdc_network";
 	/* descriptors are per-instance copies */
diff --git a/drivers/usb/gadget/u_ether.c b/drivers/usb/gadget/u_ether.c
index fe0880d..b561c93 100644
--- a/drivers/usb/gadget/u_ether.c
+++ b/drivers/usb/gadget/u_ether.c
@@ -483,7 +483,7 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
 					struct net_device *net)
 {
 	struct eth_dev		*dev = netdev_priv(net);
-	int			length = skb->len;
+	int			length = 0;
 	int			retval;
 	struct usb_request	*req = NULL;
 	unsigned long		flags;
@@ -500,13 +500,13 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
 	}
 	spin_unlock_irqrestore(&dev->lock, flags);
 
-	if (!in) {
+	if (skb && !in) {
 		dev_kfree_skb_any(skb);
 		return NETDEV_TX_OK;
 	}
 
 	/* apply outgoing CDC or RNDIS filters */
-	if (!is_promisc(cdc_filter)) {
+	if (skb && !is_promisc(cdc_filter)) {
 		u8		*dest = skb->data;
 
 		if (is_multicast_ether_addr(dest)) {
@@ -557,11 +557,17 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
 		if (dev->port_usb)
 			skb = dev->wrap(dev->port_usb, skb);
 		spin_unlock_irqrestore(&dev->lock, flags);
-		if (!skb)
+		if (!skb) {
+			/* Multi frame CDC protocols may store the frame for
+			 * later which is not a dropped frame.
+			 */
+			if (dev->port_usb->supports_multi_frame)
+				goto multiframe;
 			goto drop;
-
-		length = skb->len;
+		}
 	}
+
+	length = skb->len;
 	req->buf = skb->data;
 	req->context = skb;
 	req->complete = tx_complete;
@@ -604,6 +610,7 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
 		dev_kfree_skb_any(skb);
 drop:
 		dev->net->stats.tx_dropped++;
+multiframe:
 		spin_lock_irqsave(&dev->req_lock, flags);
 		if (list_empty(&dev->tx_reqs))
 			netif_start_queue(net);
diff --git a/drivers/usb/gadget/u_ether.h b/drivers/usb/gadget/u_ether.h
index 0f0290a..334b389 100644
--- a/drivers/usb/gadget/u_ether.h
+++ b/drivers/usb/gadget/u_ether.h
@@ -18,6 +18,7 @@
 #include <linux/if_ether.h>
 #include <linux/usb/composite.h>
 #include <linux/usb/cdc.h>
+#include <linux/netdevice.h>
 
 #include "gadget_chips.h"
 
@@ -74,6 +75,7 @@ struct gether {
 	bool				is_fixed;
 	u32				fixed_out_len;
 	u32				fixed_in_len;
+	bool				supports_multi_frame;
 	struct sk_buff			*(*wrap)(struct gether *port,
 						struct sk_buff *skb);
 	int				(*unwrap)(struct gether *port,
-- 
1.7.9.5

[PATCH v1 3/3] usb: gadget: NCM: Stop RX TCP Bursts getting dropped.

[Jim Baxter]

This fixes a problem with dropped packets over 16k CDC-NCM
when the connection is being heavily used.

The issue was that the skb truesize for the unpacked NCM
packets was too high after they were cloned from the 16k
skb, this lead to the potential memory calculated by the
Kernel running out of memory earlier then it should.

Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
---
 drivers/usb/gadget/f_ncm.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/f_ncm.c b/drivers/usb/gadget/f_ncm.c
index 5452fb6..bcdc882 100644
--- a/drivers/usb/gadget/f_ncm.c
+++ b/drivers/usb/gadget/f_ncm.c
@@ -1229,16 +1229,17 @@ static int ncm_unwrap_ntb(struct gether *port,
 			index2 = get_ncm(&tmp, opts->dgram_item_len);
 			dg_len2 = get_ncm(&tmp, opts->dgram_item_len);
 
-			skb2 = skb_clone(skb, GFP_ATOMIC);
+			/*
+			 * Copy the data into a new skb.
+			 * This ensures the truesize is correct
+			 */
+			skb2 = netdev_alloc_skb_ip_align(ncm->netdev,
+							 dg_len - crc_len);
 			if (skb2 == NULL)
 				goto err;
+			memcpy(skb_put(skb2, dg_len - crc_len),
+			       skb->data + index, dg_len - crc_len);
 
-			if (!skb_pull(skb2, index)) {
-				ret = -EOVERFLOW;
-				goto err;
-			}
-
-			skb_trim(skb2, dg_len - crc_len);
 			skb_queue_tail(list, skb2);
 
 			ndp_len -= 2 * (opts->dgram_item_len * 2);
-- 
1.7.9.5

Re: [PATCH v1 3/3] usb: gadget: NCM: Stop RX TCP Bursts getting dropped.

[Eric Dumazet]

On Thu, 2014-05-29 at 18:12 +0100, Jim Baxter wrote:
> This fixes a problem with dropped packets over 16k CDC-NCM
> when the connection is being heavily used.
> 
> The issue was that the skb truesize for the unpacked NCM
> packets was too high after they were cloned from the 16k
> skb, this lead to the potential memory calculated by the
> Kernel running out of memory earlier then it should.
> 
> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
> ---

Note the patch is OK, but changelog a bit misleading ;)

Kernel was not running out of memory, because truesize was correct.

The problem here is that a frame was consuming more kernel memory than
really needed, so chances of hitting socket sk_rcvbuf limit was high.

BTW :
#define NTB_OUT_SIZE              16384

alloc_skb(size) ->
	kmalloc(16384 + sizeof(struct skb_shared_info)) ->
		roundup() => 32768

So truesize of the skb was infact ~32KB, which is really insane indeed.
After your patch, its back to ~2KB

Acked-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH v1 3/3] usb: gadget: NCM: Stop RX TCP Bursts getting dropped.

[Jim Baxter]

On 29/05/14 20:04, Eric Dumazet wrote:
> On Thu, 2014-05-29 at 18:12 +0100, Jim Baxter wrote:
>> This fixes a problem with dropped packets over 16k CDC-NCM
>> when the connection is being heavily used.
>>
>> The issue was that the skb truesize for the unpacked NCM
>> packets was too high after they were cloned from the 16k
>> skb, this lead to the potential memory calculated by the
>> Kernel running out of memory earlier then it should.
>>
>> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
>> ---
> 
> Note the patch is OK, but changelog a bit misleading ;)
> 
> Kernel was not running out of memory, because truesize was correct.
> 
> The problem here is that a frame was consuming more kernel memory than
> really needed, so chances of hitting socket sk_rcvbuf limit was high.
> 
> BTW :
> #define NTB_OUT_SIZE              16384
> 
> alloc_skb(size) ->
> 	kmalloc(16384 + sizeof(struct skb_shared_info)) ->
> 		roundup() => 32768
> 
> So truesize of the skb was infact ~32KB, which is really insane indeed.
> After your patch, its back to ~2KB
> 
> Acked-by: Eric Dumazet <edumazet@google.com>
> 
> 
> 

Thank you, I will clarify the description.

Jim

Re: [PATCH v1 3/3] usb: gadget: NCM: Stop RX TCP Bursts getting dropped.

[Jim Baxter]

On 30/05/14 12:25, Jim Baxter wrote:
> On 29/05/14 20:04, Eric Dumazet wrote:
>> On Thu, 2014-05-29 at 18:12 +0100, Jim Baxter wrote:
>>
>> Note the patch is OK, but changelog a bit misleading ;)
>>
>> Kernel was not running out of memory, because truesize was correct.
>>
>> The problem here is that a frame was consuming more kernel memory than
>> really needed, so chances of hitting socket sk_rcvbuf limit was high.
>>
>> BTW :
>> #define NTB_OUT_SIZE              16384
>>
>> alloc_skb(size) ->
>> 	kmalloc(16384 + sizeof(struct skb_shared_info)) ->
>> 		roundup() => 32768
>>
>> So truesize of the skb was infact ~32KB, which is really insane indeed.
>> After your patch, its back to ~2KB
>>
>> Acked-by: Eric Dumazet <edumazet@google.com>
>>
>>
>>
> 
> Thank you, I will clarify the description.
> 
> Jim
> 

Hi Eric,

Is this commit log more correct?


This fixes a problem with dropped packets over 16k CDC-NCM
when the connection is being heavily used.

The issue was that the extracted frames cloned from the
received frame were consuming more memory than necessary
due to the truesize being ~32KB instead of ~2KB, this
meant there was a high chance of reaching the sk_rcvbuf
limit.

Acked-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Jim Baxter <jim_baxter@mentor.com>

Thank you,
Jim

RE: [PATCH v1 3/3] usb: gadget: NCM: Stop RX TCP Bursts getting dropped.

[David Laight]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1614 bytes --]

From: Jim Baxter
> On 30/05/14 12:25, Jim Baxter wrote:
> > On 29/05/14 20:04, Eric Dumazet wrote:
> >> On Thu, 2014-05-29 at 18:12 +0100, Jim Baxter wrote:
> >>
> >> Note the patch is OK, but changelog a bit misleading ;)
> >>
> >> Kernel was not running out of memory, because truesize was correct.
> >>
> >> The problem here is that a frame was consuming more kernel memory than
> >> really needed, so chances of hitting socket sk_rcvbuf limit was high.
> >>
> >> BTW :
> >> #define NTB_OUT_SIZE              16384
> >>
> >> alloc_skb(size) ->
> >> 	kmalloc(16384 + sizeof(struct skb_shared_info)) ->
> >> 		roundup() => 32768
> >>
> >> So truesize of the skb was infact ~32KB, which is really insane indeed.
> >> After your patch, its back to ~2KB
> >>
> >> Acked-by: Eric Dumazet <edumazet@google.com>
> >>
> >>
> >>
> >
> > Thank you, I will clarify the description.
> >
> > Jim
> >
> 
> Hi Eric,
> 
> Is this commit log more correct?
> 
> 
> This fixes a problem with dropped packets over 16k CDC-NCM
> when the connection is being heavily used.
> 
> The issue was that the extracted frames cloned from the
> received frame were consuming more memory than necessary
> due to the truesize being ~32KB instead of ~2KB, this
> meant there was a high chance of reaching the sk_rcvbuf
> limit.

s/due to/resulting in/

	David

> 
> Acked-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
> 
> Thank you,
> Jim

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH v1 3/3] usb: gadget: NCM: Stop RX TCP Bursts getting dropped.

[David Laight]

From: Jim Baxter
> On 30/05/14 12:25, Jim Baxter wrote:
> > On 29/05/14 20:04, Eric Dumazet wrote:
> >> On Thu, 2014-05-29 at 18:12 +0100, Jim Baxter wrote:
> >>
> >> Note the patch is OK, but changelog a bit misleading ;)
> >>
> >> Kernel was not running out of memory, because truesize was correct.
> >>
> >> The problem here is that a frame was consuming more kernel memory than
> >> really needed, so chances of hitting socket sk_rcvbuf limit was high.
> >>
> >> BTW :
> >> #define NTB_OUT_SIZE              16384
> >>
> >> alloc_skb(size) ->
> >> 	kmalloc(16384 + sizeof(struct skb_shared_info)) ->
> >> 		roundup() => 32768
> >>
> >> So truesize of the skb was infact ~32KB, which is really insane indeed.
> >> After your patch, its back to ~2KB
> >>
> >> Acked-by: Eric Dumazet <edumazet@google.com>
> >>
> >>
> >>
> >
> > Thank you, I will clarify the description.
> >
> > Jim
> >
> 
> Hi Eric,
> 
> Is this commit log more correct?
> 
> 
> This fixes a problem with dropped packets over 16k CDC-NCM
> when the connection is being heavily used.
> 
> The issue was that the extracted frames cloned from the
> received frame were consuming more memory than necessary
> due to the truesize being ~32KB instead of ~2KB, this
> meant there was a high chance of reaching the sk_rcvbuf
> limit.

s/due to/resulting in/

	David

> 
> Acked-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
> 
> Thank you,
> Jim

[PATCH v2 0/3] usb: gadget: NCM: Fixes and Multi-frame for TX.

[Jim Baxter]

This series adds the ability to support packaging multiple network
packets into a single 16k CDC-NCM NTB.

Patches 1 and 3 are fixes for the receive unwrap function which
previously was unable to handle an NTB with multiple NDP's and a fix
that switches from using skb_clone to creating a new packet to fix the
issue of the truesize being far to big and causing packets to be
dropped incorrectly.

Jim Baxter (3):
  usb: gadget: NCM: RX function support multiple NDPs
  usb: gadget: NCM: Add transmit multi-frame.
  usb: gadget: NCM: Stop RX TCP Bursts getting dropped.

 drivers/usb/gadget/f_ncm.c   |  480 +++++++++++++++++++++++++++++-------------
 drivers/usb/gadget/u_ether.c |   19 +-
 drivers/usb/gadget/u_ether.h |    2 +
 3 files changed, 347 insertions(+), 154 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/3] usb: gadget: NCM: RX function support multiple NDPs

[Jim Baxter]

The NDP was ignoring the wNextNdpIndex in the NDP which
means that NTBs containing multiple NDPs would have missed
frames.

Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
---

This patch originally came about because I read the CDC-NCM specification
Figures 3-1, 3-2 and table 3-4 which specifies 32bit NDPs as a link.
It has since been noted that table 3-3 states it is reserved so there is
an inconsistancy in the specfification which this patch covers either
version of.


 drivers/usb/gadget/f_ncm.c |  146 +++++++++++++++++++++++---------------------
 1 file changed, 78 insertions(+), 68 deletions(-)

diff --git a/drivers/usb/gadget/f_ncm.c b/drivers/usb/gadget/f_ncm.c
index a9499fd..d0ebbac 100644
--- a/drivers/usb/gadget/f_ncm.c
+++ b/drivers/usb/gadget/f_ncm.c
@@ -963,6 +963,7 @@ static int ncm_unwrap_ntb(struct gether *port,
 	struct f_ncm	*ncm = func_to_ncm(&port->func);
 	__le16		*tmp = (void *) skb->data;
 	unsigned	index, index2;
+	int		ndp_index;
 	unsigned	dg_len, dg_len2;
 	unsigned	ndp_len;
 	struct sk_buff	*skb2;
@@ -995,91 +996,100 @@ static int ncm_unwrap_ntb(struct gether *port,
 		goto err;
 	}
 
-	index = get_ncm(&tmp, opts->fp_index);
-	/* NCM 3.2 */
-	if (((index % 4) != 0) && (index < opts->nth_size)) {
-		INFO(port->func.config->cdev, "Bad index: %x\n",
-			index);
-		goto err;
-	}
-
-	/* walk through NDP */
-	tmp = ((void *)skb->data) + index;
-	if (get_unaligned_le32(tmp) != ncm->ndp_sign) {
-		INFO(port->func.config->cdev, "Wrong NDP SIGN\n");
-		goto err;
-	}
-	tmp += 2;
-
-	ndp_len = get_unaligned_le16(tmp++);
-	/*
-	 * NCM 3.3.1
-	 * entry is 2 items
-	 * item size is 16/32 bits, opts->dgram_item_len * 2 bytes
-	 * minimal: struct usb_cdc_ncm_ndpX + normal entry + zero entry
-	 */
-	if ((ndp_len < opts->ndp_size + 2 * 2 * (opts->dgram_item_len * 2))
-	    || (ndp_len % opts->ndplen_align != 0)) {
-		INFO(port->func.config->cdev, "Bad NDP length: %x\n", ndp_len);
-		goto err;
-	}
-	tmp += opts->reserved1;
-	tmp += opts->next_fp_index; /* skip reserved (d)wNextFpIndex */
-	tmp += opts->reserved2;
-
-	ndp_len -= opts->ndp_size;
-	index2 = get_ncm(&tmp, opts->dgram_item_len);
-	dg_len2 = get_ncm(&tmp, opts->dgram_item_len);
-	dgram_counter = 0;
+	ndp_index = get_ncm(&tmp, opts->fp_index);
 
+	/* Run through all the NDP's in the NTB */
 	do {
-		index = index2;
-		dg_len = dg_len2;
-		if (dg_len < 14 + crc_len) { /* ethernet header + crc */
-			INFO(port->func.config->cdev, "Bad dgram length: %x\n",
-			     dg_len);
+		/* NCM 3.2 */
+		if (((ndp_index % 4) != 0) &&
+				(ndp_index < opts->nth_size)) {
+			INFO(port->func.config->cdev, "Bad index: %#X\n",
+			     ndp_index);
 			goto err;
 		}
-		if (ncm->is_crc) {
-			uint32_t crc, crc2;
-
-			crc = get_unaligned_le32(skb->data +
-						 index + dg_len - crc_len);
-			crc2 = ~crc32_le(~0,
-					 skb->data + index,
-					 dg_len - crc_len);
-			if (crc != crc2) {
-				INFO(port->func.config->cdev, "Bad CRC\n");
-				goto err;
-			}
+
+		/* walk through NDP */
+		tmp = (void *)(skb->data + ndp_index);
+		if (get_unaligned_le32(tmp) != ncm->ndp_sign) {
+			INFO(port->func.config->cdev, "Wrong NDP SIGN\n");
+			goto err;
 		}
+		tmp += 2;
 
+		ndp_len = get_unaligned_le16(tmp++);
+		/*
+		 * NCM 3.3.1
+		 * entry is 2 items
+		 * item size is 16/32 bits, opts->dgram_item_len * 2 bytes
+		 * minimal: struct usb_cdc_ncm_ndpX + normal entry + zero entry
+		 * Each entry is a dgram index and a dgram length.
+		 */
+		if ((ndp_len < opts->ndp_size
+				+ 2 * 2 * (opts->dgram_item_len * 2))
+				|| (ndp_len % opts->ndplen_align != 0)) {
+			INFO(port->func.config->cdev, "Bad NDP length: %#X\n",
+			     ndp_len);
+			goto err;
+		}
+		tmp += opts->reserved1;
+		/* Check for another NDP (d)wNextNdpIndex */
+		ndp_index = get_ncm(&tmp, opts->next_fp_index);
+		tmp += opts->reserved2;
+
+		ndp_len -= opts->ndp_size;
 		index2 = get_ncm(&tmp, opts->dgram_item_len);
 		dg_len2 = get_ncm(&tmp, opts->dgram_item_len);
+		dgram_counter = 0;
+
+		do {
+			index = index2;
+			dg_len = dg_len2;
+			if (dg_len < 14 + crc_len) { /* ethernet hdr + crc */
+				INFO(port->func.config->cdev,
+				     "Bad dgram length: %#X\n", dg_len);
+				goto err;
+			}
+			if (ncm->is_crc) {
+				uint32_t crc, crc2;
+
+				crc = get_unaligned_le32(skb->data +
+							 index + dg_len -
+							 crc_len);
+				crc2 = ~crc32_le(~0,
+						 skb->data + index,
+						 dg_len - crc_len);
+				if (crc != crc2) {
+					INFO(port->func.config->cdev,
+					     "Bad CRC\n");
+					goto err;
+				}
+			}
+
+			index2 = get_ncm(&tmp, opts->dgram_item_len);
+			dg_len2 = get_ncm(&tmp, opts->dgram_item_len);
 
-		if (index2 == 0 || dg_len2 == 0) {
-			skb2 = skb;
-		} else {
 			skb2 = skb_clone(skb, GFP_ATOMIC);
 			if (skb2 == NULL)
 				goto err;
-		}
 
-		if (!skb_pull(skb2, index)) {
-			ret = -EOVERFLOW;
-			goto err;
-		}
+			if (!skb_pull(skb2, index)) {
+				ret = -EOVERFLOW;
+				goto err;
+			}
 
-		skb_trim(skb2, dg_len - crc_len);
-		skb_queue_tail(list, skb2);
+			skb_trim(skb2, dg_len - crc_len);
+			skb_queue_tail(list, skb2);
 
-		ndp_len -= 2 * (opts->dgram_item_len * 2);
+			ndp_len -= 2 * (opts->dgram_item_len * 2);
 
-		dgram_counter++;
+			dgram_counter++;
 
-		if (index2 == 0 || dg_len2 == 0)
-			break;
-	} while (ndp_len > 2 * (opts->dgram_item_len * 2)); /* zero entry */
+			if (index2 == 0 || dg_len2 == 0)
+				break;
+		} while (ndp_len > 2 * (opts->dgram_item_len * 2));
+	} while (ndp_index);
+
+	dev_kfree_skb_any(skb);
 
 	VDBG(port->func.config->cdev,
 	     "Parsed NTB with %d frames\n", dgram_counter);
-- 
1.7.9.5

[PATCH v2 2/3] usb: gadget: NCM: Add transmit multi-frame.

[Jim Baxter]

This adds multi-frame support to the NCM NTB's for
the gadget driver. This allows multiple network
packets to be put inside a single USB NTB with a
maximum size of 16kB.

It has a time out of 300ms to ensure that smaller
number of packets still maintain a normal latency.

Also the .fp_index and .next_fp_index have been
changed to .ndp_index and .next_ndp_index to
match the latest CDC-NCM specification and
help with maintenance.

Results transmitting from gadget to host.

Before the change:

TCP_STREAM Throughput (10^6bits/sec): 22.72
UDP_STREAM Throughput (10^6bits/sec): 25.94

Latency:
netperf -H 192.168.1.101 -v2 -l 50 -t TCP_RR -- -r 16384,16384
Trans.   RoundTrip  Throughput
Rate     Latency    10^6bits/s
per sec  usec/Tran  Outbound

100.83   9918.116   13.215

After the change:

TCP_STREAM Throughput (10^6bits/sec): 124.26
UDP_STREAM Throughput (10^6bits/sec): 227.48

Latency:
netperf -H 192.168.1.101 -v2 -l 50 -t TCP_RR -- -r 16384,16384
Trans.   RoundTrip  Throughput
Rate     Latency    10^6bits/s
per sec  usec/Tran  Outbound

156.80   6377.730   20.552

Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
---
 drivers/usb/gadget/f_ncm.c   |  335 ++++++++++++++++++++++++++++++++----------
 drivers/usb/gadget/u_ether.c |   19 ++-
 drivers/usb/gadget/u_ether.h |    2 +
 3 files changed, 269 insertions(+), 87 deletions(-)

diff --git a/drivers/usb/gadget/f_ncm.c b/drivers/usb/gadget/f_ncm.c
index d0ebbac..5452fb6 100644
--- a/drivers/usb/gadget/f_ncm.c
+++ b/drivers/usb/gadget/f_ncm.c
@@ -68,6 +68,18 @@ struct f_ncm {
 	 * callback and ethernet open/close
 	 */
 	spinlock_t			lock;
+
+	struct net_device		*netdev;
+
+	/* For multi-frame NDP TX */
+	struct sk_buff			*skb_tx_data;
+	struct sk_buff			*skb_tx_ndp;
+	u16				ndp_dgram_count;
+	bool				timer_force_tx;
+	struct tasklet_struct		tx_tasklet;
+	struct hrtimer			task_timer;
+
+	bool				timer_stopping;
 };
 
 static inline struct f_ncm *func_to_ncm(struct usb_function *f)
@@ -92,15 +104,20 @@ static inline unsigned ncm_bitrate(struct usb_gadget *g)
  * If the host can group frames, allow it to do that, 16K is selected,
  * because it's used by default by the current linux host driver
  */
-#define NTB_DEFAULT_IN_SIZE	USB_CDC_NCM_NTB_MIN_IN_SIZE
+#define NTB_DEFAULT_IN_SIZE	16384
 #define NTB_OUT_SIZE		16384
 
-/*
- * skbs of size less than that will not be aligned
- * to NCM's dwNtbInMaxSize to save bus bandwidth
+/* Allocation for storing the NDP, 32 should suffice for a
+ * 16k packet. This allows a maximum of 32 * 507 Byte packets to
+ * be transmitted in a single 16kB skb, though when sending full size
+ * packets this limit will be plenty.
+ * Smaller packets are not likely to be trying to maximize the
+ * throughput and will be mstly sending smaller infrequent frames.
  */
+#define TX_MAX_NUM_DPE		32
 
-#define	MAX_TX_NONFIXED		(512 * 3)
+/* Delay for the transmit to wait before sending an unfilled NTB frame. */
+#define TX_TIMEOUT_NSECS	300000
 
 #define FORMATS_SUPPORTED	(USB_CDC_NCM_NTB16_SUPPORTED |	\
 				 USB_CDC_NCM_NTB32_SUPPORTED)
@@ -355,14 +372,15 @@ struct ndp_parser_opts {
 	u32		ndp_sign;
 	unsigned	nth_size;
 	unsigned	ndp_size;
+	unsigned	dpe_size;
 	unsigned	ndplen_align;
 	/* sizes in u16 units */
 	unsigned	dgram_item_len; /* index or length */
 	unsigned	block_length;
-	unsigned	fp_index;
+	unsigned	ndp_index;
 	unsigned	reserved1;
 	unsigned	reserved2;
-	unsigned	next_fp_index;
+	unsigned	next_ndp_index;
 };
 
 #define INIT_NDP16_OPTS {					\
@@ -370,13 +388,14 @@ struct ndp_parser_opts {
 		.ndp_sign = USB_CDC_NCM_NDP16_NOCRC_SIGN,	\
 		.nth_size = sizeof(struct usb_cdc_ncm_nth16),	\
 		.ndp_size = sizeof(struct usb_cdc_ncm_ndp16),	\
+		.dpe_size = sizeof(struct usb_cdc_ncm_dpe16),	\
 		.ndplen_align = 4,				\
 		.dgram_item_len = 1,				\
 		.block_length = 1,				\
-		.fp_index = 1,					\
+		.ndp_index = 1,					\
 		.reserved1 = 0,					\
 		.reserved2 = 0,					\
-		.next_fp_index = 1,				\
+		.next_ndp_index = 1,				\
 	}
 
 
@@ -385,13 +404,14 @@ struct ndp_parser_opts {
 		.ndp_sign = USB_CDC_NCM_NDP32_NOCRC_SIGN,	\
 		.nth_size = sizeof(struct usb_cdc_ncm_nth32),	\
 		.ndp_size = sizeof(struct usb_cdc_ncm_ndp32),	\
+		.dpe_size = sizeof(struct usb_cdc_ncm_dpe32),	\
 		.ndplen_align = 8,				\
 		.dgram_item_len = 2,				\
 		.block_length = 2,				\
-		.fp_index = 2,					\
+		.ndp_index = 2,					\
 		.reserved1 = 1,					\
 		.reserved2 = 2,					\
-		.next_fp_index = 2,				\
+		.next_ndp_index = 2,				\
 	}
 
 static const struct ndp_parser_opts ndp16_opts = INIT_NDP16_OPTS;
@@ -803,6 +823,8 @@ static int ncm_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 
 		if (ncm->port.in_ep->driver_data) {
 			DBG(cdev, "reset ncm\n");
+			ncm->timer_stopping = true;
+			ncm->netdev = NULL;
 			gether_disconnect(&ncm->port);
 			ncm_reset_values(ncm);
 		}
@@ -839,6 +861,8 @@ static int ncm_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 			net = gether_connect(&ncm->port);
 			if (IS_ERR(net))
 				return PTR_ERR(net);
+			ncm->netdev = net;
+			ncm->timer_stopping = false;
 		}
 
 		spin_lock(&ncm->lock);
@@ -865,95 +889,232 @@ static int ncm_get_alt(struct usb_function *f, unsigned intf)
 	return ncm->port.in_ep->driver_data ? 1 : 0;
 }
 
+static struct sk_buff *package_for_tx(struct f_ncm *ncm)
+{
+	__le16		*ntb_iter;
+	struct sk_buff	*skb2 = NULL;
+	unsigned	ndp_pad;
+	unsigned	ndp_index;
+	unsigned	new_len;
+
+	const struct ndp_parser_opts *opts = ncm->parser_opts;
+	const int ndp_align = le16_to_cpu(ntb_parameters.wNdpInAlignment);
+	const int dgram_idx_len = 2 * 2 * opts->dgram_item_len;
+
+	/* Stop the timer */
+	hrtimer_try_to_cancel(&ncm->task_timer);
+
+	ndp_pad = ALIGN(ncm->skb_tx_data->len, ndp_align) -
+			ncm->skb_tx_data->len;
+	ndp_index = ncm->skb_tx_data->len + ndp_pad;
+	new_len = ndp_index + dgram_idx_len + ncm->skb_tx_ndp->len;
+
+	/* Set the final BlockLength and wNdpIndex */
+	ntb_iter = (void *) ncm->skb_tx_data->data;
+	/* Increment pointer to BlockLength */
+	ntb_iter += 2 + 1 + 1;
+	put_ncm(&ntb_iter, opts->block_length, new_len);
+	put_ncm(&ntb_iter, opts->ndp_index, ndp_index);
+
+	/* Set the final NDP wLength */
+	new_len = opts->ndp_size +
+			(ncm->ndp_dgram_count * dgram_idx_len);
+	ncm->ndp_dgram_count = 0;
+	/* Increment from start to wLength */
+	ntb_iter = (void *) ncm->skb_tx_ndp->data;
+	ntb_iter += 2;
+	put_unaligned_le16(new_len, ntb_iter);
+
+	/* Merge the skbs */
+	swap(skb2, ncm->skb_tx_data);
+	if (ncm->skb_tx_data) {
+		dev_kfree_skb_any(ncm->skb_tx_data);
+		ncm->skb_tx_data = NULL;
+	}
+
+	/* Insert NDP alignment. */
+	ntb_iter = (void *) skb_put(skb2, ndp_pad);
+	memset(ntb_iter, 0, ndp_pad);
+
+	/* Copy NTB across. */
+	ntb_iter = (void *) skb_put(skb2, ncm->skb_tx_ndp->len);
+	memcpy(ntb_iter, ncm->skb_tx_ndp->data, ncm->skb_tx_ndp->len);
+	dev_kfree_skb_any(ncm->skb_tx_ndp);
+	ncm->skb_tx_ndp = NULL;
+
+	/* Insert zero'd datagram. */
+	ntb_iter = (void *) skb_put(skb2, dgram_idx_len);
+	memset(ntb_iter, 0, dgram_idx_len);
+
+	return skb2;
+}
+
 static struct sk_buff *ncm_wrap_ntb(struct gether *port,
 				    struct sk_buff *skb)
 {
 	struct f_ncm	*ncm = func_to_ncm(&port->func);
-	struct sk_buff	*skb2;
+	struct sk_buff	*skb2 = NULL;
 	int		ncb_len = 0;
-	__le16		*tmp;
-	int		div;
-	int		rem;
-	int		pad;
-	int		ndp_align;
-	int		ndp_pad;
+	__le16		*ntb_data;
+	__le16		*ntb_ndp;
+	int		dgram_pad;
+
 	unsigned	max_size = ncm->port.fixed_in_len;
 	const struct ndp_parser_opts *opts = ncm->parser_opts;
-	unsigned	crc_len = ncm->is_crc ? sizeof(uint32_t) : 0;
-
-	div = le16_to_cpu(ntb_parameters.wNdpInDivisor);
-	rem = le16_to_cpu(ntb_parameters.wNdpInPayloadRemainder);
-	ndp_align = le16_to_cpu(ntb_parameters.wNdpInAlignment);
-
-	ncb_len += opts->nth_size;
-	ndp_pad = ALIGN(ncb_len, ndp_align) - ncb_len;
-	ncb_len += ndp_pad;
-	ncb_len += opts->ndp_size;
-	ncb_len += 2 * 2 * opts->dgram_item_len; /* Datagram entry */
-	ncb_len += 2 * 2 * opts->dgram_item_len; /* Zero datagram entry */
-	pad = ALIGN(ncb_len, div) + rem - ncb_len;
-	ncb_len += pad;
+	const int ndp_align = le16_to_cpu(ntb_parameters.wNdpInAlignment);
+	const int div = le16_to_cpu(ntb_parameters.wNdpInDivisor);
+	const int rem = le16_to_cpu(ntb_parameters.wNdpInPayloadRemainder);
+	const int dgram_idx_len = 2 * 2 * opts->dgram_item_len;
 
-	if (ncb_len + skb->len + crc_len > max_size) {
-		dev_kfree_skb_any(skb);
+	if (!skb && !ncm->skb_tx_data)
 		return NULL;
-	}
 
-	skb2 = skb_copy_expand(skb, ncb_len,
-			       max_size - skb->len - ncb_len - crc_len,
-			       GFP_ATOMIC);
-	dev_kfree_skb_any(skb);
-	if (!skb2)
-		return NULL;
+	if (skb) {
+		/* Add the CRC if required up front */
+		if (ncm->is_crc) {
+			uint32_t	crc;
+			__le16		*crc_pos;
+
+			crc = ~crc32_le(~0,
+					skb->data,
+					skb->len);
+			crc_pos = (void *) skb_put(skb, sizeof(uint32_t));
+			put_unaligned_le32(crc, crc_pos);
+		}
 
-	skb = skb2;
+		/* If the new skb is too big for the current NCM NTB then
+		 * set the current stored skb to be sent now and clear it
+		 * ready for new data.
+		 * NOTE: Assume maximum align for speed of calculation.
+		 */
+		if (ncm->skb_tx_data
+		    && (ncm->ndp_dgram_count >= TX_MAX_NUM_DPE
+		    || (ncm->skb_tx_data->len +
+		    div + rem + skb->len +
+		    ncm->skb_tx_ndp->len + ndp_align + (2 * dgram_idx_len))
+		    > max_size)) {
+			skb2 = package_for_tx(ncm);
+			if (!skb2)
+				goto err;
+		}
 
-	tmp = (void *) skb_push(skb, ncb_len);
-	memset(tmp, 0, ncb_len);
+		if (!ncm->skb_tx_data) {
+			ncb_len = opts->nth_size;
+			dgram_pad = ALIGN(ncb_len, div) + rem - ncb_len;
+			ncb_len += dgram_pad;
 
-	put_unaligned_le32(opts->nth_sign, tmp); /* dwSignature */
-	tmp += 2;
-	/* wHeaderLength */
-	put_unaligned_le16(opts->nth_size, tmp++);
-	tmp++; /* skip wSequence */
-	put_ncm(&tmp, opts->block_length, skb->len); /* (d)wBlockLength */
-	/* (d)wFpIndex */
-	/* the first pointer is right after the NTH + align */
-	put_ncm(&tmp, opts->fp_index, opts->nth_size + ndp_pad);
+			/* Create a new skb for the NTH and datagrams. */
+			ncm->skb_tx_data = alloc_skb(max_size, GFP_ATOMIC);
+			if (!ncm->skb_tx_data)
+				goto err;
 
-	tmp = (void *)tmp + ndp_pad;
+			ntb_data = (void *) skb_put(ncm->skb_tx_data, ncb_len);
+			memset(ntb_data, 0, ncb_len);
+			/* dwSignature */
+			put_unaligned_le32(opts->nth_sign, ntb_data);
+			ntb_data += 2;
+			/* wHeaderLength */
+			put_unaligned_le16(opts->nth_size, ntb_data++);
+
+			/* Allocate an skb for storing the NDP,
+			 * TX_MAX_NUM_DPE should easily suffice for a
+			 * 16k packet.
+			 */
+			ncm->skb_tx_ndp = alloc_skb((int)(opts->ndp_size
+						    + opts->dpe_size
+						    * TX_MAX_NUM_DPE),
+						    GFP_ATOMIC);
+			if (!ncm->skb_tx_ndp)
+				goto err;
+			ntb_ndp = (void *) skb_put(ncm->skb_tx_ndp,
+						    opts->ndp_size);
+			memset(ntb_ndp, 0, ncb_len);
+			/* dwSignature */
+			put_unaligned_le32(ncm->ndp_sign, ntb_ndp);
+			ntb_ndp += 2;
 
-	/* NDP */
-	put_unaligned_le32(ncm->ndp_sign, tmp); /* dwSignature */
-	tmp += 2;
-	/* wLength */
-	put_unaligned_le16(ncb_len - opts->nth_size - pad, tmp++);
+			/* There is always a zeroed entry */
+			ncm->ndp_dgram_count = 1;
 
-	tmp += opts->reserved1;
-	tmp += opts->next_fp_index; /* skip reserved (d)wNextFpIndex */
-	tmp += opts->reserved2;
+			/* Note: we skip opts->next_ndp_index */
+		}
 
-	if (ncm->is_crc) {
-		uint32_t crc;
+		/* Delay the timer. */
+		hrtimer_start(&ncm->task_timer,
+			      ktime_set(0, TX_TIMEOUT_NSECS),
+			      HRTIMER_MODE_REL);
+
+		/* Add the datagram position entries */
+		ntb_ndp = (void *) skb_put(ncm->skb_tx_ndp, dgram_idx_len);
+		memset(ntb_ndp, 0, dgram_idx_len);
+
+		ncb_len = ncm->skb_tx_data->len;
+		dgram_pad = ALIGN(ncb_len, div) + rem - ncb_len;
+		ncb_len += dgram_pad;
+
+		/* (d)wDatagramIndex */
+		put_ncm(&ntb_ndp, opts->dgram_item_len, ncb_len);
+		/* (d)wDatagramLength */
+		put_ncm(&ntb_ndp, opts->dgram_item_len, skb->len);
+		ncm->ndp_dgram_count++;
+
+		/* Add the new data to the skb */
+		ntb_data = (void *) skb_put(ncm->skb_tx_data, dgram_pad);
+		memset(ntb_data, 0, dgram_pad);
+		ntb_data = (void *) skb_put(ncm->skb_tx_data, skb->len);
+		memcpy(ntb_data, skb->data, skb->len);
+		dev_kfree_skb_any(skb);
+		skb = NULL;
 
-		crc = ~crc32_le(~0,
-				skb->data + ncb_len,
-				skb->len - ncb_len);
-		put_unaligned_le32(crc, skb->data + skb->len);
-		skb_put(skb, crc_len);
+	} else if (ncm->skb_tx_data && ncm->timer_force_tx) {
+		/* If the tx was requested because of a timeout then send */
+		skb2 = package_for_tx(ncm);
+		if (!skb2)
+			goto err;
 	}
 
-	/* (d)wDatagramIndex[0] */
-	put_ncm(&tmp, opts->dgram_item_len, ncb_len);
-	/* (d)wDatagramLength[0] */
-	put_ncm(&tmp, opts->dgram_item_len, skb->len - ncb_len);
-	/* (d)wDatagramIndex[1] and  (d)wDatagramLength[1] already zeroed */
+	return skb2;
+
+err:
+	ncm->netdev->stats.tx_dropped++;
+
+	if (skb)
+		dev_kfree_skb_any(skb);
+	if (ncm->skb_tx_data)
+		dev_kfree_skb_any(ncm->skb_tx_data);
+	if (ncm->skb_tx_ndp)
+		dev_kfree_skb_any(ncm->skb_tx_ndp);
+
+	return NULL;
+}
+
+/*
+ * This transmits the NTB if there are frames waiting.
+ */
+static void ncm_tx_tasklet(unsigned long data)
+{
+	struct f_ncm	*ncm = (void *)data;
 
-	if (skb->len > MAX_TX_NONFIXED)
-		memset(skb_put(skb, max_size - skb->len),
-		       0, max_size - skb->len);
+	if (ncm->timer_stopping)
+		return;
+
+	/* Only send if data is available. */
+	if (ncm->skb_tx_data) {
+		ncm->timer_force_tx = true;
+		ncm->netdev->netdev_ops->ndo_start_xmit(NULL, ncm->netdev);
+		ncm->timer_force_tx = false;
+	}
+}
 
-	return skb;
+/*
+ * The transmit should only be run if no skb data has been sent
+ * for a certain duration.
+ */
+static enum hrtimer_restart ncm_tx_timeout(struct hrtimer *data)
+{
+	struct f_ncm *ncm = container_of(data, struct f_ncm, task_timer);
+	tasklet_schedule(&ncm->tx_tasklet);
+	return HRTIMER_NORESTART;
 }
 
 static int ncm_unwrap_ntb(struct gether *port,
@@ -996,7 +1157,7 @@ static int ncm_unwrap_ntb(struct gether *port,
 		goto err;
 	}
 
-	ndp_index = get_ncm(&tmp, opts->fp_index);
+	ndp_index = get_ncm(&tmp, opts->ndp_index);
 
 	/* Run through all the NDP's in the NTB */
 	do {
@@ -1033,7 +1194,7 @@ static int ncm_unwrap_ntb(struct gether *port,
 		}
 		tmp += opts->reserved1;
 		/* Check for another NDP (d)wNextNdpIndex */
-		ndp_index = get_ncm(&tmp, opts->next_fp_index);
+		ndp_index = get_ncm(&tmp, opts->next_ndp_index);
 		tmp += opts->reserved2;
 
 		ndp_len -= opts->ndp_size;
@@ -1107,8 +1268,11 @@ static void ncm_disable(struct usb_function *f)
 
 	DBG(cdev, "ncm deactivated\n");
 
-	if (ncm->port.in_ep->driver_data)
+	if (ncm->port.in_ep->driver_data) {
+		ncm->timer_stopping = true;
+		ncm->netdev = NULL;
 		gether_disconnect(&ncm->port);
+	}
 
 	if (ncm->notify->driver_data) {
 		usb_ep_disable(ncm->notify);
@@ -1277,6 +1441,10 @@ static int ncm_bind(struct usb_configuration *c, struct usb_function *f)
 	ncm->port.open = ncm_open;
 	ncm->port.close = ncm_close;
 
+	tasklet_init(&ncm->tx_tasklet, ncm_tx_tasklet, (unsigned long) ncm);
+	hrtimer_init(&ncm->task_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	ncm->task_timer.function = ncm_tx_timeout;
+
 	DBG(cdev, "CDC Network: %s speed IN/%s OUT/%s NOTIFY/%s\n",
 			gadget_is_dualspeed(c->cdev->gadget) ? "dual" : "full",
 			ncm->port.in_ep->name, ncm->port.out_ep->name,
@@ -1390,6 +1558,10 @@ static void ncm_unbind(struct usb_configuration *c, struct usb_function *f)
 
 	DBG(c->cdev, "ncm unbind\n");
 
+	hrtimer_cancel(&ncm->task_timer);
+	tasklet_kill(&ncm->tx_tasklet);
+
+	ncm_string_defs[0].id = 0;
 	usb_free_all_descriptors(f);
 
 	kfree(ncm->notify_req->buf);
@@ -1426,6 +1598,7 @@ static struct usb_function *ncm_alloc(struct usb_function_instance *fi)
 	ncm->port.ioport = netdev_priv(opts->net);
 	mutex_unlock(&opts->lock);
 	ncm->port.is_fixed = true;
+	ncm->port.supports_multi_frame = true;
 
 	ncm->port.func.name = "cdc_network";
 	/* descriptors are per-instance copies */
diff --git a/drivers/usb/gadget/u_ether.c b/drivers/usb/gadget/u_ether.c
index 97b0277..d50adda 100644
--- a/drivers/usb/gadget/u_ether.c
+++ b/drivers/usb/gadget/u_ether.c
@@ -483,7 +483,7 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
 					struct net_device *net)
 {
 	struct eth_dev		*dev = netdev_priv(net);
-	int			length = skb->len;
+	int			length = 0;
 	int			retval;
 	struct usb_request	*req = NULL;
 	unsigned long		flags;
@@ -500,13 +500,13 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
 	}
 	spin_unlock_irqrestore(&dev->lock, flags);
 
-	if (!in) {
+	if (skb && !in) {
 		dev_kfree_skb_any(skb);
 		return NETDEV_TX_OK;
 	}
 
 	/* apply outgoing CDC or RNDIS filters */
-	if (!is_promisc(cdc_filter)) {
+	if (skb && !is_promisc(cdc_filter)) {
 		u8		*dest = skb->data;
 
 		if (is_multicast_ether_addr(dest)) {
@@ -557,11 +557,17 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
 		if (dev->port_usb)
 			skb = dev->wrap(dev->port_usb, skb);
 		spin_unlock_irqrestore(&dev->lock, flags);
-		if (!skb)
+		if (!skb) {
+			/* Multi frame CDC protocols may store the frame for
+			 * later which is not a dropped frame.
+			 */
+			if (dev->port_usb->supports_multi_frame)
+				goto multiframe;
 			goto drop;
-
-		length = skb->len;
+		}
 	}
+
+	length = skb->len;
 	req->buf = skb->data;
 	req->context = skb;
 	req->complete = tx_complete;
@@ -604,6 +610,7 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
 		dev_kfree_skb_any(skb);
 drop:
 		dev->net->stats.tx_dropped++;
+multiframe:
 		spin_lock_irqsave(&dev->req_lock, flags);
 		if (list_empty(&dev->tx_reqs))
 			netif_start_queue(net);
diff --git a/drivers/usb/gadget/u_ether.h b/drivers/usb/gadget/u_ether.h
index 0f0290a..334b389 100644
--- a/drivers/usb/gadget/u_ether.h
+++ b/drivers/usb/gadget/u_ether.h
@@ -18,6 +18,7 @@
 #include <linux/if_ether.h>
 #include <linux/usb/composite.h>
 #include <linux/usb/cdc.h>
+#include <linux/netdevice.h>
 
 #include "gadget_chips.h"
 
@@ -74,6 +75,7 @@ struct gether {
 	bool				is_fixed;
 	u32				fixed_out_len;
 	u32				fixed_in_len;
+	bool				supports_multi_frame;
 	struct sk_buff			*(*wrap)(struct gether *port,
 						struct sk_buff *skb);
 	int				(*unwrap)(struct gether *port,
-- 
1.7.9.5

[PATCH v2 3/3] usb: gadget: NCM: Stop RX TCP Bursts getting dropped.

[Jim Baxter]

This fixes a problem with dropped packets over 16k CDC-NCM
when the connection is being heavily used.

The issue was that the extracted frames cloned from the
received frame were consuming more memory than necessary
resulting in the truesize being ~32KB instead of ~2KB, this
meant there was a high chance of reaching the sk_rcvbuf
limit.

Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
Acked-by: Eric Dumazet <edumazet@google.com>
---

Updated the commit message to accurately describe the issue.

 drivers/usb/gadget/f_ncm.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/f_ncm.c b/drivers/usb/gadget/f_ncm.c
index 5452fb6..bcdc882 100644
--- a/drivers/usb/gadget/f_ncm.c
+++ b/drivers/usb/gadget/f_ncm.c
@@ -1229,16 +1229,17 @@ static int ncm_unwrap_ntb(struct gether *port,
 			index2 = get_ncm(&tmp, opts->dgram_item_len);
 			dg_len2 = get_ncm(&tmp, opts->dgram_item_len);
 
-			skb2 = skb_clone(skb, GFP_ATOMIC);
+			/*
+			 * Copy the data into a new skb.
+			 * This ensures the truesize is correct
+			 */
+			skb2 = netdev_alloc_skb_ip_align(ncm->netdev,
+							 dg_len - crc_len);
 			if (skb2 == NULL)
 				goto err;
+			memcpy(skb_put(skb2, dg_len - crc_len),
+			       skb->data + index, dg_len - crc_len);
 
-			if (!skb_pull(skb2, index)) {
-				ret = -EOVERFLOW;
-				goto err;
-			}
-
-			skb_trim(skb2, dg_len - crc_len);
 			skb_queue_tail(list, skb2);
 
 			ndp_len -= 2 * (opts->dgram_item_len * 2);
-- 
1.7.9.5