[PATCH 05/34]bnx2x: Setting the GSO_TYPE with LRO

[PATCH 05/34]bnx2x: Setting the GSO_TYPE with LRO

[Eilon Greenstein]

When TPA (HW GRO) is used, the GSO_TYPE should be set

Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
 drivers/net/bnx2x.h      |    9 ++++++---
 drivers/net/bnx2x_main.c |    7 ++++++-
 drivers/net/bnx2x_reg.h  |    1 +
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/net/bnx2x.h b/drivers/net/bnx2x.h
index 73f6c21..b9bc285 100644
--- a/drivers/net/bnx2x.h
+++ b/drivers/net/bnx2x.h
@@ -401,10 +401,13 @@ struct bnx2x_fastpath {
 #define BNX2X_RX_CSUM_OK(cqe) \
 			(!(BNX2X_L4_CSUM_ERR(cqe) || BNX2X_IP_CSUM_ERR(cqe)))
 
+#define BNX2X_PRS_FLAG_OVERETH_IPV4(flags) \
+				(((le16_to_cpu(flags) & \
+				   PARSING_FLAGS_OVER_ETHERNET_PROTOCOL) >> \
+				  PARSING_FLAGS_OVER_ETHERNET_PROTOCOL_SHIFT) \
+				 == PRS_FLAG_OVERETH_IPV4)
 #define BNX2X_RX_SUM_FIX(cqe) \
-			((le16_to_cpu(cqe->fast_path_cqe.pars_flags.flags) & \
-			  PARSING_FLAGS_OVER_ETHERNET_PROTOCOL) == \
-			 (1 << PARSING_FLAGS_OVER_ETHERNET_PROTOCOL_SHIFT))
+	BNX2X_PRS_FLAG_OVERETH_IPV4(cqe->fast_path_cqe.pars_flags.flags)
 
 
 #define FP_USB_FUNC_OFF			(2 + 2*HC_USTORM_SB_NUM_INDICES)
diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c
index e811ecb..da18528 100644
--- a/drivers/net/bnx2x_main.c
+++ b/drivers/net/bnx2x_main.c
@@ -1225,9 +1225,14 @@ static int bnx2x_fill_frag_skb(struct bnx2x *bp, struct bnx2x_fastpath *fp,
 	pages = SGE_PAGE_ALIGN(frag_size) >> SGE_PAGE_SHIFT;
 
 	/* This is needed in order to enable forwarding support */
-	if (frag_size)
+	if (frag_size) {
 		skb_shinfo(skb)->gso_size = min((u32)SGE_PAGE_SIZE,
 					       max(frag_size, (u32)len_on_bd));
+		if (BNX2X_PRS_FLAG_OVERETH_IPV4(fp_cqe->pars_flags.flags))
+			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
+		else
+			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
+	}
 
 #ifdef BNX2X_STOP_ON_ERROR
 	if (pages >
diff --git a/drivers/net/bnx2x_reg.h b/drivers/net/bnx2x_reg.h
index a67b0c3..ed0d19a 100644
--- a/drivers/net/bnx2x_reg.h
+++ b/drivers/net/bnx2x_reg.h
@@ -5020,6 +5020,7 @@
 #define HW_LOCK_RESOURCE_PORT0_ATT_MASK 			 3
 #define HW_LOCK_RESOURCE_SPIO					 2
 #define HW_LOCK_RESOURCE_UNDI					 5
+#define PRS_FLAG_OVERETH_IPV4					 1
 #define AEU_INPUTS_ATTN_BITS_BRB_PARITY_ERROR		      (1<<18)
 #define AEU_INPUTS_ATTN_BITS_CCM_HW_INTERRUPT		      (1<<31)
 #define AEU_INPUTS_ATTN_BITS_CDU_HW_INTERRUPT		      (1<<9)
-- 
1.5.4.3

Re: [PATCH 05/34]bnx2x: Setting the GSO_TYPE with LRO

[Ben Hutchings]

On Wed, 2009-01-14 at 18:42 +0200, Eilon Greenstein wrote:
> When TPA (HW GRO) is used, the GSO_TYPE should be set
[...]

No it shouldn't.  That is for the output path only.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
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 05/34]bnx2x: Setting the GSO_TYPE with LRO

[Eilon Greenstein]

On Wed, 2009-01-14 at 08:57 -0800, Ben Hutchings wrote:
> On Wed, 2009-01-14 at 18:42 +0200, Eilon Greenstein wrote:
> > When TPA (HW GRO) is used, the GSO_TYPE should be set
> [...]
> 
> No it shouldn't.  That is for the output path only.
> 

Right - but it was an issue with forwarding

Eilon

Re: [PATCH 05/34]bnx2x: Setting the GSO_TYPE with LRO

[Ben Hutchings]

On Wed, 2009-01-14 at 19:42 +0200, Eilon Greenstein wrote:
> On Wed, 2009-01-14 at 08:57 -0800, Ben Hutchings wrote:
> > On Wed, 2009-01-14 at 18:42 +0200, Eilon Greenstein wrote:
> > > When TPA (HW GRO) is used, the GSO_TYPE should be set
> > [...]
> > 
> > No it shouldn't.  That is for the output path only.
> > 
> 
> Right - but it was an issue with forwarding

You can't use LRO together with forwarding/bridging unless you replicate
what Herbert Xu has done to allow reconstruction of the original frames.
Setting gso_type is a hack which you may or may not get away with,
depending on the output device's capabilities.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
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 05/34]bnx2x: Setting the GSO_TYPE with LRO

[Eilon Greenstein]

On Wed, 2009-01-14 at 10:00 -0800, Ben Hutchings wrote:
> On Wed, 2009-01-14 at 19:42 +0200, Eilon Greenstein wrote:
> > On Wed, 2009-01-14 at 08:57 -0800, Ben Hutchings wrote:
> > > On Wed, 2009-01-14 at 18:42 +0200, Eilon Greenstein wrote:
> > > > When TPA (HW GRO) is used, the GSO_TYPE should be set
> > > [...]
> > > 
> > > No it shouldn't.  That is for the output path only.
> > > 
> > 
> > Right - but it was an issue with forwarding
> 
> You can't use LRO together with forwarding/bridging unless you replicate
> what Herbert Xu has done to allow reconstruction of the original frames.
> Setting gso_type is a hack which you may or may not get away with,
> depending on the output device's capabilities.
> 
This is far from bullet proof solution, but IMHO it is better set then
not  this way, you stand a chance (depending on the output device
capabilities)

Eilon

Re: [PATCH 05/34]bnx2x: Setting the GSO_TYPE with LRO

[Herbert Xu]

Eilon Greenstein <eilong@broadcom.com> wrote:
>> 
> This is far from bullet proof solution, but IMHO it is better set then
> not  this way, you stand a chance (depending on the output device
> capabilities)

The question is simple, does your hardware guarantee that on
output GSO will turn the packet into exactly the same sequence
of packets that was seen on input, with no changes (apart from
what the stack would have done to them anyway, e.g., TTL update)
whatsoever?

If not then you mustn't set this and also you must disable it
if forwarding/bridging is used.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 05/34]bnx2x: Setting the GSO_TYPE with LRO

[Ben Hutchings]

On Thu, 2009-01-15 at 07:18 +1100, Herbert Xu wrote:
> Eilon Greenstein <eilong@broadcom.com> wrote:
> >> 
> > This is far from bullet proof solution, but IMHO it is better set then
> > not  this way, you stand a chance (depending on the output device
> > capabilities)
> 
> The question is simple, does your hardware guarantee that on
> output GSO will turn the packet into exactly the same sequence
> of packets that was seen on input, with no changes (apart from
> what the stack would have done to them anyway, e.g., TTL update)
> whatsoever?
> 
> If not then you mustn't set this and also you must disable it
> if forwarding/bridging is used.

The driver just has to cooperate with dev_disable_lro() in order for the
forwarding/bridging code to disable it.  Then skb_warn_if_lro() should
catch the case where the user mistakenly turns it back on.  However,
setting gso_type on input subverts this check.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
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 05/34]bnx2x: Setting the GSO_TYPE with LRO

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Wed, 14 Jan 2009 20:27:22 +0000

> On Thu, 2009-01-15 at 07:18 +1100, Herbert Xu wrote:
> > Eilon Greenstein <eilong@broadcom.com> wrote:
> > >> 
> > > This is far from bullet proof solution, but IMHO it is better set then
> > > not  this way, you stand a chance (depending on the output device
> > > capabilities)
> > 
> > The question is simple, does your hardware guarantee that on
> > output GSO will turn the packet into exactly the same sequence
> > of packets that was seen on input, with no changes (apart from
> > what the stack would have done to them anyway, e.g., TTL update)
> > whatsoever?
> > 
> > If not then you mustn't set this and also you must disable it
> > if forwarding/bridging is used.
> 
> The driver just has to cooperate with dev_disable_lro() in order for the
> forwarding/bridging code to disable it.  Then skb_warn_if_lro() should
> catch the case where the user mistakenly turns it back on.  However,
> setting gso_type on input subverts this check.

Right, setting gso_type here is absolutely wrong.