[PATCH 1/2] mv643xx_eth: disable csum offloading if the hw limited by max size

[PATCH 1/2] mv643xx_eth: disable csum offloading if the hw limited by max size

[Saeed Bishara]

Some controllers (KW, Dove) limits the TX IP/layer4 checksum offloading to a max size.

Signed-off-by: Saeed Bishara <saeed@marvell.com>
---
 drivers/net/mv643xx_eth.c   |   10 ++++++++++
 include/linux/mv643xx_eth.h |    4 ++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
index e345ec8..79243dd 100644
--- a/drivers/net/mv643xx_eth.c
+++ b/drivers/net/mv643xx_eth.c
@@ -289,6 +289,7 @@ struct mv643xx_eth_shared_private {
 	unsigned int t_clk;
 	int extended_rx_coal_limit;
 	int tx_bw_control;
+	int tx_csum_limit;
 };
 
 #define TX_BW_CONTROL_ABSENT		0
@@ -2468,6 +2469,14 @@ static int mv643xx_eth_change_mtu(struct net_device *dev, int new_mtu)
 	mv643xx_eth_recalc_skb_size(mp);
 	tx_set_rate(mp, 1000000000, 16777216);
 
+	if (mp->shared->tx_csum_limit && dev->mtu > mp->shared->tx_csum_limit) {
+		dev->features &= ~NETIF_F_IP_CSUM;
+		dev->vlan_features &= ~NETIF_F_IP_CSUM;
+	} else {
+		dev->features |= NETIF_F_IP_CSUM;
+		dev->vlan_features |= NETIF_F_IP_CSUM;
+	}
+
 	if (!netif_running(dev))
 		return 0;
 
@@ -2666,6 +2675,7 @@ static int mv643xx_eth_shared_probe(struct platform_device *pdev)
 	 * Detect hardware parameters.
 	 */
 	msp->t_clk = (pd != NULL && pd->t_clk != 0) ? pd->t_clk : 133000000;
+	msp->tx_csum_limit = pd->tx_csum_limit;
 	infer_hw_params(msp);
 
 	platform_set_drvdata(pdev, msp);
diff --git a/include/linux/mv643xx_eth.h b/include/linux/mv643xx_eth.h
index cbbbe9b..7402718 100644
--- a/include/linux/mv643xx_eth.h
+++ b/include/linux/mv643xx_eth.h
@@ -19,6 +19,10 @@ struct mv643xx_eth_shared_platform_data {
 	struct mbus_dram_target_info	*dram;
 	struct platform_device	*shared_smi;
 	unsigned int		t_clk;
+	/*
+	 * Max packet size for Tx IP/Layer 4 checksum (0 -> no limit)
+	 */
+	int			tx_csum_limit;
 };
 
 #define MV643XX_ETH_PHY_ADDR_DEFAULT	0
-- 
1.6.0.4

[PATCH 2/2] set packet size limit for the mv643xx csum offloading

[Saeed Bishara]

The mv643xx ethernet controller limits the packet size for the TX
checksum offloading. this patch sets this limits for the needed
devices.

Signed-off-by: Saeed Bishara <saeed@marvell.com>
---
 arch/arm/mach-dove/common.c     |    1 +
 arch/arm/mach-kirkwood/common.c |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c
index 5da2cf4..473ce0b 100644
--- a/arch/arm/mach-dove/common.c
+++ b/arch/arm/mach-dove/common.c
@@ -149,6 +149,7 @@ void __init dove_ehci1_init(void)
 struct mv643xx_eth_shared_platform_data dove_ge00_shared_data = {
 	.t_clk		= 0,
 	.dram		= &dove_mbus_dram_info,
+	.tx_csum_limit	= 1600,
 };
 
 static struct resource dove_ge00_shared_resources[] = {
diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c
index 4ccfdf9..c4bd8cb 100644
--- a/arch/arm/mach-kirkwood/common.c
+++ b/arch/arm/mach-kirkwood/common.c
@@ -114,6 +114,7 @@ void __init kirkwood_ehci_init(void)
  ****************************************************************************/
 struct mv643xx_eth_shared_platform_data kirkwood_ge00_shared_data = {
 	.dram		= &kirkwood_mbus_dram_info,
+	.tx_csum_limit	= 1600,
 };
 
 static struct resource kirkwood_ge00_shared_resources[] = {
-- 
1.6.0.4

[PATCH 1/2] mv643xx_eth: disable csum offloading if the hw limited by max size

[Lennert Buytenhek]

On Sun, Jun 06, 2010 at 03:28:55PM +0300, Saeed Bishara wrote:

> diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
> index e345ec8..79243dd 100644
> --- a/drivers/net/mv643xx_eth.c
> +++ b/drivers/net/mv643xx_eth.c
> @@ -289,6 +289,7 @@ struct mv643xx_eth_shared_private {
>  	unsigned int t_clk;
>  	int extended_rx_coal_limit;
>  	int tx_bw_control;
> +	int tx_csum_limit;
>  };
>  
>  #define TX_BW_CONTROL_ABSENT		0
> @@ -2468,6 +2469,14 @@ static int mv643xx_eth_change_mtu(struct net_device *dev, int new_mtu)
>  	mv643xx_eth_recalc_skb_size(mp);
>  	tx_set_rate(mp, 1000000000, 16777216);
>  
> +	if (mp->shared->tx_csum_limit && dev->mtu > mp->shared->tx_csum_limit) {
> +		dev->features &= ~NETIF_F_IP_CSUM;
> +		dev->vlan_features &= ~NETIF_F_IP_CSUM;
> +	} else {
> +		dev->features |= NETIF_F_IP_CSUM;
> +		dev->vlan_features |= NETIF_F_IP_CSUM;
> +	}

I wonder if it would be better to still advertise NETIF_F_IP_CSUM in
this case, but just call skb_checksum_help() if skb->len is > dev->mtu
in txq_submit_skb(), like we do for the case where the ethernet header
is of an unsupported length -- that way, we'd still have hardware
checksum offload for TX packets that _do_ fit inside the hardware TX
fifo, e.g. when communicating with hosts on another subnet where the
default MTU is used.


> diff --git a/include/linux/mv643xx_eth.h b/include/linux/mv643xx_eth.h
> index cbbbe9b..7402718 100644
> --- a/include/linux/mv643xx_eth.h
> +++ b/include/linux/mv643xx_eth.h
> @@ -19,6 +19,10 @@ struct mv643xx_eth_shared_platform_data {
>  	struct mbus_dram_target_info	*dram;
>  	struct platform_device	*shared_smi;
>  	unsigned int		t_clk;
> +	/*
> +	 * Max packet size for Tx IP/Layer 4 checksum (0 -> no limit)

There's always a limit. :-)  You might as well just fill in the right
values for orion5x/kw/dd/loki while you're at it.


thanks,
Lennert

[PATCH 2/2] set packet size limit for the mv643xx csum offloading

[Lennert Buytenhek]

On Sun, Jun 06, 2010 at 03:28:56PM +0300, Saeed Bishara wrote:

> The mv643xx ethernet controller limits the packet size for the TX
> checksum offloading. this patch sets this limits for the needed
> devices.
> 
> Signed-off-by: Saeed Bishara <saeed@marvell.com>
> ---
>  arch/arm/mach-dove/common.c     |    1 +
>  arch/arm/mach-kirkwood/common.c |    1 +
>  2 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c
> index 5da2cf4..473ce0b 100644
> --- a/arch/arm/mach-dove/common.c
> +++ b/arch/arm/mach-dove/common.c
> @@ -149,6 +149,7 @@ void __init dove_ehci1_init(void)
>  struct mv643xx_eth_shared_platform_data dove_ge00_shared_data = {
>  	.t_clk		= 0,
>  	.dram		= &dove_mbus_dram_info,
> +	.tx_csum_limit	= 1600,

Assuming that 1600 is exactly the right value (and not 1599 or 1601 etc),

Acked-by: Lennert Buytenhek <buytenh@wantstofly.org>

Can you fill in for orion5x/dd/loki as well?

[PATCH 1/2] mv643xx_eth: disable csum offloading if the hw limited by max size

[Saeed Bishara]

 

>-----Original Message-----
>From: Lennert Buytenhek [mailto:buytenh at wantstofly.org] 
>Sent: Tuesday, June 08, 2010 8:09 PM
>To: Saeed Bishara
>Cc: linux-net at vger.kernel.org; linux-arm-kernel at lists.infradead.org
>Subject: Re: [PATCH 1/2] mv643xx_eth: disable csum offloading 
>if the hw limited by max size
>
>On Sun, Jun 06, 2010 at 03:28:55PM +0300, Saeed Bishara wrote:
>
>> diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
>> index e345ec8..79243dd 100644
>> --- a/drivers/net/mv643xx_eth.c
>> +++ b/drivers/net/mv643xx_eth.c
>> @@ -289,6 +289,7 @@ struct mv643xx_eth_shared_private {
>>  	unsigned int t_clk;
>>  	int extended_rx_coal_limit;
>>  	int tx_bw_control;
>> +	int tx_csum_limit;
>>  };
>>  
>>  #define TX_BW_CONTROL_ABSENT		0
>> @@ -2468,6 +2469,14 @@ static int 
>mv643xx_eth_change_mtu(struct net_device *dev, int new_mtu)
>>  	mv643xx_eth_recalc_skb_size(mp);
>>  	tx_set_rate(mp, 1000000000, 16777216);
>>  
>> +	if (mp->shared->tx_csum_limit && dev->mtu > 
>mp->shared->tx_csum_limit) {
>> +		dev->features &= ~NETIF_F_IP_CSUM;
>> +		dev->vlan_features &= ~NETIF_F_IP_CSUM;
>> +	} else {
>> +		dev->features |= NETIF_F_IP_CSUM;
>> +		dev->vlan_features |= NETIF_F_IP_CSUM;
>> +	}
>
>I wonder if it would be better to still advertise NETIF_F_IP_CSUM in
>this case, but just call skb_checksum_help() if skb->len is > dev->mtu
>in txq_submit_skb(), like we do for the case where the ethernet header
>is of an unsupported length -- that way, we'd still have hardware
>checksum offload for TX packets that _do_ fit inside the hardware TX
>fifo, e.g. when communicating with hosts on another subnet where the
>default MTU is used.
Goog idea, I'll add a check to skb->len > mp->shared->tx_csum_limit.
>
>
>> diff --git a/include/linux/mv643xx_eth.h 
>b/include/linux/mv643xx_eth.h
>> index cbbbe9b..7402718 100644
>> --- a/include/linux/mv643xx_eth.h
>> +++ b/include/linux/mv643xx_eth.h
>> @@ -19,6 +19,10 @@ struct mv643xx_eth_shared_platform_data {
>>  	struct mbus_dram_target_info	*dram;
>>  	struct platform_device	*shared_smi;
>>  	unsigned int		t_clk;
>> +	/*
>> +	 * Max packet size for Tx IP/Layer 4 checksum (0 -> no limit)
>
>There's always a limit. :-)  You might as well just fill in the right
>values for orion5x/kw/dd/loki while you're at it.
Right, kirkwood and dove are limited to 1600, the others to 9*1024. Don't know about loki.
>
>
>thanks,
>Lennert
>

[PATCH 1/2 v2] mv643xx_eth: use sw csum for big packets

[Saeed Bishara]

Some controllers (KW, Dove) limits the TX IP/layer4 checksum offloading to a max size.

Signed-off-by: Saeed Bishara <saeed@marvell.com>
---
 drivers/net/mv643xx_eth.c   |    7 ++++++-
 include/linux/mv643xx_eth.h |    5 +++++
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
index e345ec8..2d0e06b 100644
--- a/drivers/net/mv643xx_eth.c
+++ b/drivers/net/mv643xx_eth.c
@@ -289,6 +289,7 @@ struct mv643xx_eth_shared_private {
 	unsigned int t_clk;
 	int extended_rx_coal_limit;
 	int tx_bw_control;
+	int tx_csum_limit;
 };
 
 #define TX_BW_CONTROL_ABSENT		0
@@ -782,7 +783,8 @@ static int txq_submit_skb(struct tx_queue *txq, struct sk_buff *skb)
 		       skb->protocol != htons(ETH_P_8021Q));
 
 		tag_bytes = (void *)ip_hdr(skb) - (void *)skb->data - ETH_HLEN;
-		if (unlikely(tag_bytes & ~12)) {
+		if (unlikely(tag_bytes & ~12) ||
+			skb->len > mp->shared->tx_csum_limit) {
 			if (skb_checksum_help(skb) == 0)
 				goto no_csum;
 			kfree_skb(skb);
@@ -2666,6 +2668,9 @@ static int mv643xx_eth_shared_probe(struct platform_device *pdev)
 	 * Detect hardware parameters.
 	 */
 	msp->t_clk = (pd != NULL && pd->t_clk != 0) ? pd->t_clk : 133000000;
+	msp->tx_csum_limit = pd->tx_csum_limit ? pd->tx_csum_limit : 9 * 1024;
+	/* add header count so we can compare against skb->len */
+	msp->tx_csum_limit += ETH_HLEN;
 	infer_hw_params(msp);
 
 	platform_set_drvdata(pdev, msp);
diff --git a/include/linux/mv643xx_eth.h b/include/linux/mv643xx_eth.h
index cbbbe9b..30b0c4e 100644
--- a/include/linux/mv643xx_eth.h
+++ b/include/linux/mv643xx_eth.h
@@ -19,6 +19,11 @@ struct mv643xx_eth_shared_platform_data {
 	struct mbus_dram_target_info	*dram;
 	struct platform_device	*shared_smi;
 	unsigned int		t_clk;
+	/*
+	 * Max packet size for Tx IP/Layer 4 checksum, when set to 0, default
+	 * limit of 9KiB will be used.
+	 */
+	int			tx_csum_limit;
 };
 
 #define MV643XX_ETH_PHY_ADDR_DEFAULT	0
-- 
1.6.0.4

[PATCH 1/2] mv643xx_eth: disable csum offloading if the hw limited by max size

[Lennert Buytenhek]

On Wed, Jun 09, 2010 at 11:21:47AM +0300, Saeed Bishara wrote:

> >> diff --git a/include/linux/mv643xx_eth.h 
> >b/include/linux/mv643xx_eth.h
> >> index cbbbe9b..7402718 100644
> >> --- a/include/linux/mv643xx_eth.h
> >> +++ b/include/linux/mv643xx_eth.h
> >> @@ -19,6 +19,10 @@ struct mv643xx_eth_shared_platform_data {
> >>  	struct mbus_dram_target_info	*dram;
> >>  	struct platform_device	*shared_smi;
> >>  	unsigned int		t_clk;
> >> +	/*
> >> +	 * Max packet size for Tx IP/Layer 4 checksum (0 -> no limit)
> >
> >There's always a limit. :-)  You might as well just fill in the right
> >values for orion5x/kw/dd/loki while you're at it.
>
> Right, kirkwood and dove are limited to 1600, the others to 9*1024. Don't know about loki.

As I know Loki to be able to tx jumbo frames with hw checksum
offload, it is likely 9*1024 also.

[PATCH 1/2] mv643xx_eth: disable csum offloading if the hw limited by max size

[Abhijeet Gole]

Yes, Loki is 9*1024.

-----Original Message-----
From: Lennert Buytenhek [mailto:buytenh at wantstofly.org] 
Sent: Wednesday, June 09, 2010 12:11 PM
To: Saeed Bishara
Cc: linux-net at vger.kernel.org; linux-arm-kernel at lists.infradead.org; Abhijeet Gole
Subject: Re: [PATCH 1/2] mv643xx_eth: disable csum offloading if the hw limited by max size

On Wed, Jun 09, 2010 at 11:21:47AM +0300, Saeed Bishara wrote:

> >> diff --git a/include/linux/mv643xx_eth.h 
> >b/include/linux/mv643xx_eth.h
> >> index cbbbe9b..7402718 100644
> >> --- a/include/linux/mv643xx_eth.h
> >> +++ b/include/linux/mv643xx_eth.h
> >> @@ -19,6 +19,10 @@ struct mv643xx_eth_shared_platform_data {
> >>  	struct mbus_dram_target_info	*dram;
> >>  	struct platform_device	*shared_smi;
> >>  	unsigned int		t_clk;
> >> +	/*
> >> +	 * Max packet size for Tx IP/Layer 4 checksum (0 -> no limit)
> >
> >There's always a limit. :-)  You might as well just fill in the right
> >values for orion5x/kw/dd/loki while you're at it.
>
> Right, kirkwood and dove are limited to 1600, the others to 9*1024. Don't know about loki.

As I know Loki to be able to tx jumbo frames with hw checksum
offload, it is likely 9*1024 also.

[PATCH 1/2 v2] mv643xx_eth: use sw csum for big packets

[Lennert Buytenhek]

On Wed, Jun 09, 2010 at 11:51:23AM +0300, Saeed Bishara wrote:

> diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
> index e345ec8..2d0e06b 100644
> --- a/drivers/net/mv643xx_eth.c
> +++ b/drivers/net/mv643xx_eth.c
> @@ -289,6 +289,7 @@ struct mv643xx_eth_shared_private {
>  	unsigned int t_clk;
>  	int extended_rx_coal_limit;
>  	int tx_bw_control;
> +	int tx_csum_limit;
>  };
>  
>  #define TX_BW_CONTROL_ABSENT		0
> @@ -782,7 +783,8 @@ static int txq_submit_skb(struct tx_queue *txq, struct sk_buff *skb)
>  		       skb->protocol != htons(ETH_P_8021Q));
>  
>  		tag_bytes = (void *)ip_hdr(skb) - (void *)skb->data - ETH_HLEN;
> -		if (unlikely(tag_bytes & ~12)) {
> +		if (unlikely(tag_bytes & ~12) ||
> +			skb->len > mp->shared->tx_csum_limit) {

Please line up skb->len with unlikely() on the line above it.


> @@ -2666,6 +2668,9 @@ static int mv643xx_eth_shared_probe(struct platform_device *pdev)
>  	 * Detect hardware parameters.
>  	 */
>  	msp->t_clk = (pd != NULL && pd->t_clk != 0) ? pd->t_clk : 133000000;
> +	msp->tx_csum_limit = pd->tx_csum_limit ? pd->tx_csum_limit : 9 * 1024;
> +	/* add header count so we can compare against skb->len */
> +	msp->tx_csum_limit += ETH_HLEN;
>  	infer_hw_params(msp);

Is the limit 9 * 1024 + 14 for the whole packet, or 9 * 1024 for the IP
part?

I.e. what happens if skb->len is 9 * 1024 + 15, but there is one VLAN
tag present -- will the hardware be able to do the checksum offload or
not?

[PATCH 1/2 v2] mv643xx_eth: use sw csum for big packets

[Saeed Bishara]

 

>-----Original Message-----
>From: Lennert Buytenhek [mailto:buytenh at wantstofly.org] 
>Sent: Thursday, June 10, 2010 9:44 AM
>To: Saeed Bishara
>Cc: linux-net at vger.kernel.org; linux-arm-kernel at lists.infradead.org
>Subject: Re: [PATCH 1/2 v2] mv643xx_eth: use sw csum for big packets
>
>On Wed, Jun 09, 2010 at 11:51:23AM +0300, Saeed Bishara wrote:
>
>> diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
>> index e345ec8..2d0e06b 100644
>> --- a/drivers/net/mv643xx_eth.c
>> +++ b/drivers/net/mv643xx_eth.c
>> @@ -289,6 +289,7 @@ struct mv643xx_eth_shared_private {
>>  	unsigned int t_clk;
>>  	int extended_rx_coal_limit;
>>  	int tx_bw_control;
>> +	int tx_csum_limit;
>>  };
>>  
>>  #define TX_BW_CONTROL_ABSENT		0
>> @@ -782,7 +783,8 @@ static int txq_submit_skb(struct 
>tx_queue *txq, struct sk_buff *skb)
>>  		       skb->protocol != htons(ETH_P_8021Q));
>>  
>>  		tag_bytes = (void *)ip_hdr(skb) - (void 
>*)skb->data - ETH_HLEN;
>> -		if (unlikely(tag_bytes & ~12)) {
>> +		if (unlikely(tag_bytes & ~12) ||
>> +			skb->len > mp->shared->tx_csum_limit) {
>
>Please line up skb->len with unlikely() on the line above it.
>
>
>> @@ -2666,6 +2668,9 @@ static int 
>mv643xx_eth_shared_probe(struct platform_device *pdev)
>>  	 * Detect hardware parameters.
>>  	 */
>>  	msp->t_clk = (pd != NULL && pd->t_clk != 0) ? pd->t_clk 
>: 133000000;
>> +	msp->tx_csum_limit = pd->tx_csum_limit ? 
>pd->tx_csum_limit : 9 * 1024;
>> +	/* add header count so we can compare against skb->len */
>> +	msp->tx_csum_limit += ETH_HLEN;
>>  	infer_hw_params(msp);
>
>Is the limit 9 * 1024 + 14 for the whole packet, or 9 * 1024 for the IP
>part?
the limit is for the IP part, but I thought that adding the header length, then comparing agains skb->len will be the same. What do you suggest?

>
>I.e. what happens if skb->len is 9 * 1024 + 15, but there is one VLAN
>tag present -- will the hardware be able to do the checksum offload or
>not?
>

[PATCH 1/2 v2] mv643xx_eth: use sw csum for big packets

[Lennert Buytenhek]

On Thu, Jun 10, 2010 at 10:38:05AM +0300, Saeed Bishara wrote:

> >> diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
> >> index e345ec8..2d0e06b 100644
> >> --- a/drivers/net/mv643xx_eth.c
> >> +++ b/drivers/net/mv643xx_eth.c
> >> @@ -289,6 +289,7 @@ struct mv643xx_eth_shared_private {
> >>  	unsigned int t_clk;
> >>  	int extended_rx_coal_limit;
> >>  	int tx_bw_control;
> >> +	int tx_csum_limit;
> >>  };
> >>  
> >>  #define TX_BW_CONTROL_ABSENT		0
> >> @@ -782,7 +783,8 @@ static int txq_submit_skb(struct 
> >tx_queue *txq, struct sk_buff *skb)
> >>  		       skb->protocol != htons(ETH_P_8021Q));
> >>  
> >>  		tag_bytes = (void *)ip_hdr(skb) - (void 
> >*)skb->data - ETH_HLEN;
> >> -		if (unlikely(tag_bytes & ~12)) {
> >> +		if (unlikely(tag_bytes & ~12) ||
> >> +			skb->len > mp->shared->tx_csum_limit) {
> >
> >Please line up skb->len with unlikely() on the line above it.
> >
> >
> >> @@ -2666,6 +2668,9 @@ static int 
> >mv643xx_eth_shared_probe(struct platform_device *pdev)
> >>  	 * Detect hardware parameters.
> >>  	 */
> >>  	msp->t_clk = (pd != NULL && pd->t_clk != 0) ? pd->t_clk 
> >: 133000000;
> >> +	msp->tx_csum_limit = pd->tx_csum_limit ? 
> >pd->tx_csum_limit : 9 * 1024;
> >> +	/* add header count so we can compare against skb->len */
> >> +	msp->tx_csum_limit += ETH_HLEN;
> >>  	infer_hw_params(msp);
> >
> >Is the limit 9 * 1024 + 14 for the whole packet, or 9 * 1024 for the IP
> >part?
>
> the limit is for the IP part, but I thought that adding the header length, then comparing agains skb->len will be the same. What do you suggest?

Right, but for the header length you take 14, while if to include VLAN
tags or DSA tags the header might actually be longer -- how does that
affect the ability of the hardware to compute the checksum?

I.e. is the restriction "total packet length must be < N + 14 bytes"
or is it "the IP part must be < N bytes and it doesn't matter whether
there are VLAN tags or not"?

I.e.:

> >I.e. what happens if skb->len is 9 * 1024 + 15, but there is one VLAN
> >tag present -- will the hardware be able to do the checksum offload or
> >not?

[PATCH 1/2 v2] mv643xx_eth: use sw csum for big packets

[Saeed Bishara]

 

>-----Original Message-----
>From: Lennert Buytenhek [mailto:buytenh at wantstofly.org] 
>Sent: Saturday, June 12, 2010 11:32 AM
>To: Saeed Bishara
>Cc: linux-net at vger.kernel.org; linux-arm-kernel at lists.infradead.org
>Subject: Re: [PATCH 1/2 v2] mv643xx_eth: use sw csum for big packets
>
>On Thu, Jun 10, 2010 at 10:38:05AM +0300, Saeed Bishara wrote:
>
>> >> diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
>> >> index e345ec8..2d0e06b 100644
>> >> --- a/drivers/net/mv643xx_eth.c
>> >> +++ b/drivers/net/mv643xx_eth.c
>> >> @@ -289,6 +289,7 @@ struct mv643xx_eth_shared_private {
>> >>  	unsigned int t_clk;
>> >>  	int extended_rx_coal_limit;
>> >>  	int tx_bw_control;
>> >> +	int tx_csum_limit;
>> >>  };
>> >>  
>> >>  #define TX_BW_CONTROL_ABSENT		0
>> >> @@ -782,7 +783,8 @@ static int txq_submit_skb(struct 
>> >tx_queue *txq, struct sk_buff *skb)
>> >>  		       skb->protocol != htons(ETH_P_8021Q));
>> >>  
>> >>  		tag_bytes = (void *)ip_hdr(skb) - (void 
>> >*)skb->data - ETH_HLEN;
>> >> -		if (unlikely(tag_bytes & ~12)) {
>> >> +		if (unlikely(tag_bytes & ~12) ||
>> >> +			skb->len > mp->shared->tx_csum_limit) {
>> >
>> >Please line up skb->len with unlikely() on the line above it.
>> >
>> >
>> >> @@ -2666,6 +2668,9 @@ static int 
>> >mv643xx_eth_shared_probe(struct platform_device *pdev)
>> >>  	 * Detect hardware parameters.
>> >>  	 */
>> >>  	msp->t_clk = (pd != NULL && pd->t_clk != 0) ? pd->t_clk 
>> >: 133000000;
>> >> +	msp->tx_csum_limit = pd->tx_csum_limit ? 
>> >pd->tx_csum_limit : 9 * 1024;
>> >> +	/* add header count so we can compare against skb->len */
>> >> +	msp->tx_csum_limit += ETH_HLEN;
>> >>  	infer_hw_params(msp);
>> >
>> >Is the limit 9 * 1024 + 14 for the whole packet, or 9 * 
>1024 for the IP
>> >part?
>>
>> the limit is for the IP part, but I thought that adding the 
>header length, then comparing agains skb->len will be the 
>same. What do you suggest?
>
>Right, but for the header length you take 14, while if to include VLAN
>tags or DSA tags the header might actually be longer -- how does that
>affect the ability of the hardware to compute the checksum?
>
>I.e. is the restriction "total packet length must be < N + 14 bytes"
>or is it "the IP part must be < N bytes and it doesn't matter whether
>there are VLAN tags or not"?
It's the second option: "the IP part must be < N" regardless to the L2 header.
saeed
>
>I.e.:
>
>> >I.e. what happens if skb->len is 9 * 1024 + 15, but there 
>is one VLAN
>> >tag present -- will the hardware be able to do the checksum 
>offload or
>> >not?
>

[PATCH 1/2 v2] mv643xx_eth: use sw csum for big packets

[Lennert Buytenhek]

On Sun, Jun 13, 2010 at 10:38:10AM +0300, Saeed Bishara wrote:

> >> >> @@ -2666,6 +2668,9 @@ static int 
> >> >mv643xx_eth_shared_probe(struct platform_device *pdev)
> >> >>  	 * Detect hardware parameters.
> >> >>  	 */
> >> >>  	msp->t_clk = (pd != NULL && pd->t_clk != 0) ? pd->t_clk 
> >> >: 133000000;
> >> >> +	msp->tx_csum_limit = pd->tx_csum_limit ? 
> >> >pd->tx_csum_limit : 9 * 1024;
> >> >> +	/* add header count so we can compare against skb->len */
> >> >> +	msp->tx_csum_limit += ETH_HLEN;
> >> >>  	infer_hw_params(msp);
> >> >
> >> >Is the limit 9 * 1024 + 14 for the whole packet, or 9 * 
> >1024 for the IP
> >> >part?
> >>
> >> the limit is for the IP part, but I thought that adding the 
> >header length, then comparing agains skb->len will be the 
> >same. What do you suggest?
> >
> >Right, but for the header length you take 14, while if to include VLAN
> >tags or DSA tags the header might actually be longer -- how does that
> >affect the ability of the hardware to compute the checksum?
> >
> >I.e. is the restriction "total packet length must be < N + 14 bytes"
> >or is it "the IP part must be < N bytes and it doesn't matter whether
> >there are VLAN tags or not"?
>
> It's the second option: "the IP part must be < N" regardless to the L2 header.

Then I suppose you want to do something like:

	int hdr_len;
	int tag_bytes;

	hdr_len = (void *)ip_hdr(skb) - (void *)skb->data;
	tag_bytes = hdr_len - 4;
	if (skb->len - hdr_len > the limit || unlikely(tag_bytes & ~12)) {

(And then keep the limit as maximum number of bytes in the packet
starting from the IP header part.)

[PATCH 1/2 v2] mv643xx_eth: use sw csum for big packets

[saeed bishara]

On Mon, Jun 14, 2010 at 4:32 AM, Lennert Buytenhek
<buytenh@wantstofly.org> wrote:
> On Sun, Jun 13, 2010 at 10:38:10AM +0300, Saeed Bishara wrote:
>
>> >> >> @@ -2666,6 +2668,9 @@ static int
>> >> >mv643xx_eth_shared_probe(struct platform_device *pdev)
>> >> >> ? ? ? ? ?* Detect hardware parameters.
>> >> >> ? ? ? ? ?*/
>> >> >> ? ? ? ? msp->t_clk = (pd != NULL && pd->t_clk != 0) ? pd->t_clk
>> >> >: 133000000;
>> >> >> + ? ? ? msp->tx_csum_limit = pd->tx_csum_limit ?
>> >> >pd->tx_csum_limit : 9 * 1024;
>> >> >> + ? ? ? /* add header count so we can compare against skb->len */
>> >> >> + ? ? ? msp->tx_csum_limit += ETH_HLEN;
>> >> >> ? ? ? ? infer_hw_params(msp);
>> >> >
>> >> >Is the limit 9 * 1024 + 14 for the whole packet, or 9 *
>> >1024 for the IP
>> >> >part?
>> >>
>> >> the limit is for the IP part, but I thought that adding the
>> >header length, then comparing agains skb->len will be the
>> >same. What do you suggest?
>> >
>> >Right, but for the header length you take 14, while if to include VLAN
>> >tags or DSA tags the header might actually be longer -- how does that
>> >affect the ability of the hardware to compute the checksum?
>> >
>> >I.e. is the restriction "total packet length must be < N + 14 bytes"
>> >or is it "the IP part must be < N bytes and it doesn't matter whether
>> >there are VLAN tags or not"?
>>
>> It's the second option: "the IP part must be < N" regardless to the L2 header.
>
> Then I suppose you want to do something like:
>
> ? ? ? ?int hdr_len;
> ? ? ? ?int tag_bytes;
>
> ? ? ? ?hdr_len = (void *)ip_hdr(skb) - (void *)skb->data;
> ? ? ? ?tag_bytes = hdr_len - 4;
that should be 14 instead of 4, right?
> ? ? ? ?if (skb->len - hdr_len > the limit || unlikely(tag_bytes & ~12)) {
>
> (And then keep the limit as maximum number of bytes in the packet
> starting from the IP header part.)
ok. here is the updated patch:

[PATCH 1/2 v2] mv643xx_eth: use sw csum for big packets

[Lennert Buytenhek]

On Sun, Jun 20, 2010 at 06:49:56AM -0500, saeed bishara wrote:

> >> >> >> @@ -2666,6 +2668,9 @@ static int
> >> >> >mv643xx_eth_shared_probe(struct platform_device *pdev)
> >> >> >> ? ? ? ? ?* Detect hardware parameters.
> >> >> >> ? ? ? ? ?*/
> >> >> >> ? ? ? ? msp->t_clk = (pd != NULL && pd->t_clk != 0) ? pd->t_clk
> >> >> >: 133000000;
> >> >> >> + ? ? ? msp->tx_csum_limit = pd->tx_csum_limit ?
> >> >> >pd->tx_csum_limit : 9 * 1024;
> >> >> >> + ? ? ? /* add header count so we can compare against skb->len */
> >> >> >> + ? ? ? msp->tx_csum_limit += ETH_HLEN;
> >> >> >> ? ? ? ? infer_hw_params(msp);
> >> >> >
> >> >> >Is the limit 9 * 1024 + 14 for the whole packet, or 9 *
> >> >1024 for the IP
> >> >> >part?
> >> >>
> >> >> the limit is for the IP part, but I thought that adding the
> >> >header length, then comparing agains skb->len will be the
> >> >same. What do you suggest?
> >> >
> >> >Right, but for the header length you take 14, while if to include VLAN
> >> >tags or DSA tags the header might actually be longer -- how does that
> >> >affect the ability of the hardware to compute the checksum?
> >> >
> >> >I.e. is the restriction "total packet length must be < N + 14 bytes"
> >> >or is it "the IP part must be < N bytes and it doesn't matter whether
> >> >there are VLAN tags or not"?
> >>
> >> It's the second option: "the IP part must be < N" regardless to the L2 header.
> >
> > Then I suppose you want to do something like:
> >
> > ? ? ? ?int hdr_len;
> > ? ? ? ?int tag_bytes;
> >
> > ? ? ? ?hdr_len = (void *)ip_hdr(skb) - (void *)skb->data;
> > ? ? ? ?tag_bytes = hdr_len - 4;
>
> that should be 14 instead of 4, right?

Yes, sorry.


> > ? ? ? ?if (skb->len - hdr_len > the limit || unlikely(tag_bytes & ~12)) {
> >
> > (And then keep the limit as maximum number of bytes in the packet
> > starting from the IP header part.)
>
> ok. here is the updated patch:
> >From 009530d4220dd710737d5a357629edcbc102727a Mon Sep 17 00:00:00 2001
> From: Saeed Bishara <saeed@marvell.com>
> Date: Sun, 6 Jun 2010 14:40:49 +0300
> Subject: [PATCH 1/2 v3] mv643xx_eth: use sw csum for big packets
> 
> Some controllers (KW, Dove) limits the TX IP/layer4 checksum
> offloading to a max size.
> 
> Signed-off-by: Saeed Bishara <saeed@marvell.com>

Looks good!

Acked-by: Lennert Buytenhek <buytenh@wantstofly.org>

[PATCH 1/2 v2] mv643xx_eth: use sw csum for big packets

[David Miller]

From: Lennert Buytenhek <buytenh@wantstofly.org>
Date: Sun, 20 Jun 2010 13:58:59 +0200

> Acked-by: Lennert Buytenhek <buytenh@wantstofly.org>

If the desire is to get this into one of the networking GIT
trees, sending it to linux-net isn't going to accomplish that.

It needs to go to netdev at vger.kernel.org with all signoffs
and ACKs.

Thanks.

[PATCH 2/2] set packet size limit for the mv643xx csum offloading

[Martin Michlmayr]

* Saeed Bishara <saeed@marvell.com> [2010-06-06 15:28]:
> The mv643xx ethernet controller limits the packet size for the TX
> checksum offloading. this patch sets this limits for the needed
> devices.

I noticed that patch 1/2 has gone into mainline but I cannot find 2/2
in any tree.  Saeed, can you add the definition for Loki that someone
pointed out (9*1024) and resubmit?


> Signed-off-by: Saeed Bishara <saeed@marvell.com>
> ---
>  arch/arm/mach-dove/common.c     |    1 +
>  arch/arm/mach-kirkwood/common.c |    1 +
>  2 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c
> index 5da2cf4..473ce0b 100644
> --- a/arch/arm/mach-dove/common.c
> +++ b/arch/arm/mach-dove/common.c
> @@ -149,6 +149,7 @@ void __init dove_ehci1_init(void)
>  struct mv643xx_eth_shared_platform_data dove_ge00_shared_data = {
>  	.t_clk		= 0,
>  	.dram		= &dove_mbus_dram_info,
> +	.tx_csum_limit	= 1600,
>  };
>  
>  static struct resource dove_ge00_shared_resources[] = {
> diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c
> index 4ccfdf9..c4bd8cb 100644
> --- a/arch/arm/mach-kirkwood/common.c
> +++ b/arch/arm/mach-kirkwood/common.c
> @@ -114,6 +114,7 @@ void __init kirkwood_ehci_init(void)
>   ****************************************************************************/
>  struct mv643xx_eth_shared_platform_data kirkwood_ge00_shared_data = {
>  	.dram		= &kirkwood_mbus_dram_info,
> +	.tx_csum_limit	= 1600,
>  };
>  
>  static struct resource kirkwood_ge00_shared_resources[] = {
> -- 
> 1.6.0.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Martin Michlmayr
http://www.cyrius.com/

[PATCH 2/2] set packet size limit for the mv643xx csum offloading

[Lennert Buytenhek]

On Thu, Jul 15, 2010 at 09:26:21PM +0100, Martin Michlmayr wrote:

> > The mv643xx ethernet controller limits the packet size for the TX
> > checksum offloading. this patch sets this limits for the needed
> > devices.
> 
> I noticed that patch 1/2 has gone into mainline but I cannot find 2/2
> in any tree.  Saeed, can you add the definition for Loki that someone
> pointed out (9*1024) and resubmit?

If you're going to supply the value explicitly for all platforms, you
might as well supply it explicitly for mv78xx0 (9*1024) and orion5x
(9*1024) as well.

[PATCH 2/2] set packet size limit for the mv643xx csum offloading

[Maxime Bizon]

On Sun, 2010-06-06 at 15:28 +0300, Saeed Bishara wrote:

> diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c
> index 4ccfdf9..c4bd8cb 100644
> --- a/arch/arm/mach-kirkwood/common.c
> +++ b/arch/arm/mach-kirkwood/common.c
> @@ -114,6 +114,7 @@ void __init kirkwood_ehci_init(void)
>   ****************************************************************************/
>  struct mv643xx_eth_shared_platform_data kirkwood_ge00_shared_data = {
>  	.dram		= &kirkwood_mbus_dram_info,
> +	.tx_csum_limit	= 1600,
>  };
>  
>  static struct resource kirkwood_ge00_shared_resources[] = {

Kirkwood (at least) has a second ethernet mac, please add the definition
for it too.

Thanks,

-- 
Maxime

[PATCH 2/2] set packet size limit for the mv643xx csum offloading

[saeed bishara]

On Fri, Jul 16, 2010 at 9:08 AM, Lennert Buytenhek
<buytenh@wantstofly.org>wrote:

> On Thu, Jul 15, 2010 at 09:26:21PM +0100, Martin Michlmayr wrote:
>
> > > The mv643xx ethernet controller limits the packet size for the TX
> > > checksum offloading. this patch sets this limits for the needed
> > > devices.
> >
> > I noticed that patch 1/2 has gone into mainline but I cannot find 2/2
> > in any tree.  Saeed, can you add the definition for Loki that someone
> > pointed out (9*1024) and resubmit?
>
> If you're going to supply the value explicitly for all platforms, you
> might as well supply it explicitly for mv78xx0 (9*1024) and orion5x
> (9*1024) as well.
>
The default value is 9*1024, so I see there is need to supply it for mv78xx0
and Loki. agree?
saeed

> --
> To unsubscribe from this list: send the line "unsubscribe linux-net" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100831/5a9974d1/attachment-0001.html>

[PATCH 2/2] set packet size limit for the mv643xx csum offloading

[saeed bishara]

On Sat, Aug 14, 2010 at 1:28 AM, Maxime Bizon <mbizon@freebox.fr> wrote:

>
> On Sun, 2010-06-06 at 15:28 +0300, Saeed Bishara wrote:
>
> > diff --git a/arch/arm/mach-kirkwood/common.c
> b/arch/arm/mach-kirkwood/common.c
> > index 4ccfdf9..c4bd8cb 100644
> > --- a/arch/arm/mach-kirkwood/common.c
> > +++ b/arch/arm/mach-kirkwood/common.c
> > @@ -114,6 +114,7 @@ void __init kirkwood_ehci_init(void)
> >
> ****************************************************************************/
> >  struct mv643xx_eth_shared_platform_data kirkwood_ge00_shared_data = {
> >       .dram           = &kirkwood_mbus_dram_info,
> > +     .tx_csum_limit  = 1600,
> >  };
> >
> >  static struct resource kirkwood_ge00_shared_resources[] = {
>
> Kirkwood (at least) has a second ethernet mac, please add the definition
> for it too.
>
ok

>
> Thanks,
>
> --
> Maxime
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-net" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100831/793b6520/attachment-0001.html>

[PATCH 2/2] set packet size limit for the mv643xx csum offloading

[saeed bishara]

> > I noticed that patch 1/2 has gone into mainline but I cannot find 2/2
> > in any tree. ?Saeed, can you add the definition for Loki that someone
> > pointed out (9*1024) and resubmit?
>
> If you're going to supply the value explicitly for all platforms, you
> might as well supply it explicitly for mv78xx0 (9*1024) and orion5x
> (9*1024) as well.
The default value is 9*1024, so I see there is need to supply it for
mv78xx0 and Loki. agree?
saeed

[PATCH 2/2] set packet size limit for the mv643xx csum offloading

[saeed bishara]

On Sat, Aug 14, 2010 at 1:28 AM, Maxime Bizon <mbizon@freebox.fr> wrote:
>
> On Sun, 2010-06-06 at 15:28 +0300, Saeed Bishara wrote:
>
>> diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c
>> index 4ccfdf9..c4bd8cb 100644
>> --- a/arch/arm/mach-kirkwood/common.c
>> +++ b/arch/arm/mach-kirkwood/common.c
>> @@ -114,6 +114,7 @@ void __init kirkwood_ehci_init(void)
>> ? ****************************************************************************/
>> ?struct mv643xx_eth_shared_platform_data kirkwood_ge00_shared_data = {
>> ? ? ? .dram ? ? ? ? ? = &kirkwood_mbus_dram_info,
>> + ? ? .tx_csum_limit ?= 1600,
>> ?};
>>
>> ?static struct resource kirkwood_ge00_shared_resources[] = {
>
> Kirkwood (at least) has a second ethernet mac, please add the definition
> for it too.
ok, I'll
>
> Thanks,
>
> --
> Maxime
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-net" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>

[PATCH 2/2] set packet size limit for the mv643xx csum offloading

[Lennert Buytenhek]

On Tue, Aug 31, 2010 at 04:13:28PM +0300, saeed bishara wrote:

> > > I noticed that patch 1/2 has gone into mainline but I cannot find 2/2
> > > in any tree. ?Saeed, can you add the definition for Loki that someone
> > > pointed out (9*1024) and resubmit?
> >
> > If you're going to supply the value explicitly for all platforms, you
> > might as well supply it explicitly for mv78xx0 (9*1024) and orion5x
> > (9*1024) as well.
>
> The default value is 9*1024, so I see there is need to supply it for
> mv78xx0 and Loki. agree?

I would supply it in any case, for clarity -- and if there are two
(or more) different values out there, then neither value really has
more right to be the default than the other.

[PATCH 2/2] set packet size limit for the mv643xx csum offloading

[saeed bishara]

On Tue, Aug 31, 2010 at 7:48 PM, Lennert Buytenhek
<buytenh@wantstofly.org> wrote:
> On Tue, Aug 31, 2010 at 04:13:28PM +0300, saeed bishara wrote:
>
>> > > I noticed that patch 1/2 has gone into mainline but I cannot find 2/2
>> > > in any tree. ?Saeed, can you add the definition for Loki that someone
>> > > pointed out (9*1024) and resubmit?
>> >
>> > If you're going to supply the value explicitly for all platforms, you
>> > might as well supply it explicitly for mv78xx0 (9*1024) and orion5x
>> > (9*1024) as well.
>>
>> The default value is 9*1024, so I see there is need to supply it for
>> mv78xx0 and Loki. agree?
>
> I would supply it in any case, for clarity -- and if there are two
> (or more) different values out there, then neither value really has
> more right to be the default than the other.
ok, attached the updated version of this patch.
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-set-packet-size-limit-for-the-mv643xx-csum-offloadin.patch
Type: application/octet-stream
Size: 4511 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100901/c629e319/attachment-0001.obj>

[PATCH 2/2] set packet size limit for the mv643xx csum offloading

[Lennert Buytenhek]

On Wed, Sep 01, 2010 at 10:42:15AM +0300, saeed bishara wrote:

> >> > > I noticed that patch 1/2 has gone into mainline but I cannot find 2/2
> >> > > in any tree. ?Saeed, can you add the definition for Loki that someone
> >> > > pointed out (9*1024) and resubmit?
> >> >
> >> > If you're going to supply the value explicitly for all platforms, you
> >> > might as well supply it explicitly for mv78xx0 (9*1024) and orion5x
> >> > (9*1024) as well.
> >>
> >> The default value is 9*1024, so I see there is need to supply it for
> >> mv78xx0 and Loki. agree?
> >
> > I would supply it in any case, for clarity -- and if there are two
> > (or more) different values out there, then neither value really has
> > more right to be the default than the other.
>
> ok, attached the updated version of this patch.

Acked-by: Lennert Buytenhek <buytenh@wantstofly.org>