[PATCH net] bnx2x: really disable TPA if 'disable_tpa' option is set

[PATCH net] bnx2x: really disable TPA if 'disable_tpa' option is set

[Michal Schmidt]

bnx2x's 'disable_tpa=1' module option is not respected properly and TPA
(transparent packet aggregation) remains enabled. Even though the
module option causes LRO to be disabled, TPA is enabled in GRO mode.

Additionally, disabling GRO via ethtool then has no effect. One can
still observe tpa_* statistics increase and large packets being received
in tcpdump.

The bug was an unintended consequence of commit aebf6244cd39 "bnx2x: Be
more forgiving toward SW GRO".

Fix it by following the bp->disable_tpa flag when initializing fp's.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index 6f7dc81581..3558a36b1c 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -2485,8 +2485,10 @@ static void bnx2x_bz_fp(struct bnx2x *bp, int index)
 	else if (bp->flags & GRO_ENABLE_FLAG)
 		fp->mode = TPA_MODE_GRO;
 
-	/* We don't want TPA on an FCoE L2 ring */
-	if (IS_FCOE_FP(fp))
+	/* We don't want TPA if it's disabled in bp
+	 * or if this is an FCoE L2 ring.
+	 */
+	if (bp->disable_tpa || IS_FCOE_FP(fp))
 		fp->disable_tpa = 1;
 }
 
-- 
2.1.0

Re: [PATCH net] bnx2x: really disable TPA if 'disable_tpa' option is set

[David Miller]

From: Michal Schmidt <mschmidt@redhat.com>
Date: Mon, 27 Apr 2015 17:20:38 +0200

> bnx2x's 'disable_tpa=1' module option is not respected properly and TPA
> (transparent packet aggregation) remains enabled. Even though the
> module option causes LRO to be disabled, TPA is enabled in GRO mode.
> 
> Additionally, disabling GRO via ethtool then has no effect. One can
> still observe tpa_* statistics increase and large packets being received
> in tcpdump.
> 
> The bug was an unintended consequence of commit aebf6244cd39 "bnx2x: Be
> more forgiving toward SW GRO".
> 
> Fix it by following the bp->disable_tpa flag when initializing fp's.
> 
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>

Applied, thanks.

RE: [PATCH net] bnx2x: really disable TPA if 'disable_tpa' option is set

[Yuval Mintz]

>> bnx2x's 'disable_tpa=1' module option is not respected properly and TPA
>> (transparent packet aggregation) remains enabled. Even though the
>> module option causes LRO to be disabled, TPA is enabled in GRO mode.
>>
>> Additionally, disabling GRO via ethtool then has no effect. One can
>> still observe tpa_* statistics increase and large packets being received
>> in tcpdump.
>>
>> The bug was an unintended consequence of commit aebf6244cd39 "bnx2x: Be
>> more forgiving toward SW GRO".
>>
>> Fix it by following the bp->disable_tpa flag when initializing fp's.
>>
>> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
>
>Applied, thanks.

Michal - thanks, but Dave -is it possible you gave us only 2.5 hours to
review this one?

I agree that this fixes an incorrect logic, but its incomplete as the bnx2x
'bp->flags' no longer represent the correct logic. I.e., it might cause
additional issues down the road, as the 'fp->mode' and 'fp->disable_tpa'
are no longer in sync.

We already have a fix for this issue, we just haven't sent it to 'net' yet.
Are you willing to give us an additional day to send an alternative fix?
Or do you prefer to take this, and possibly later we'll remove the logic
added by this patch in our own?

Re: [PATCH net] bnx2x: really disable TPA if 'disable_tpa' option is set

[David Miller]

From: Yuval Mintz <Yuval.Mintz@qlogic.com>
Date: Mon, 27 Apr 2015 18:15:44 +0000

>>> bnx2x's 'disable_tpa=1' module option is not respected properly and TPA
>>> (transparent packet aggregation) remains enabled. Even though the
>>> module option causes LRO to be disabled, TPA is enabled in GRO mode.
>>>
>>> Additionally, disabling GRO via ethtool then has no effect. One can
>>> still observe tpa_* statistics increase and large packets being received
>>> in tcpdump.
>>>
>>> The bug was an unintended consequence of commit aebf6244cd39 "bnx2x: Be
>>> more forgiving toward SW GRO".
>>>
>>> Fix it by following the bp->disable_tpa flag when initializing fp's.
>>>
>>> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
>>
>>Applied, thanks.
> 
> Michal - thanks, but Dave -is it possible you gave us only 2.5 hours to
> review this one?

You'll need to send me a revert or changes relative to this one as
the commit is not only permanent when it reaches my tree, it's also
made it's way into Linus's tree now as well.

Re: [PATCH net] bnx2x: really disable TPA if 'disable_tpa' option is set

[Michal Schmidt]

On 04/27/2015 08:15 PM, Yuval Mintz wrote:
> I agree that this fixes an incorrect logic, but its incomplete as the bnx2x
> 'bp->flags' no longer represent the correct logic. I.e., it might cause
> additional issues down the road, as the 'fp->mode' and 'fp->disable_tpa'
> are no longer in sync.

> We already have a fix for this issue, we just haven't sent it to 'net'
> yet. Are you willing to give us an additional day to send an
> alternative fix?  Or do you prefer to take this, and possibly later
> we'll remove the logic added by this patch in our own?

Yuval,

I also have a patch that removes fp->disable_tpa by merging it into
fp->mode (adding a 3rd mode: TPA_MODE_DISABLED). And another patch that
removes {TPA,GRO}_ENABLE_FLAG by using dev->features everywhere.
I'll wait and see what changes you have.

Regards,
Michal

RE: [PATCH net] bnx2x: really disable TPA if 'disable_tpa' option is set

[Yuval Mintz]

> > I agree that this fixes an incorrect logic, but its incomplete as the
> > bnx2x 'bp->flags' no longer represent the correct logic. I.e., it
> > might cause additional issues down the road, as the 'fp->mode' and 'fp-
> >disable_tpa'
> > are no longer in sync.
> 
> > We already have a fix for this issue, we just haven't sent it to 'net'
> > yet. Are you willing to give us an additional day to send an
> > alternative fix?  Or do you prefer to take this, and possibly later
> > we'll remove the logic added by this patch in our own?
> 
> Yuval,
> 
> I also have a patch that removes fp->disable_tpa by merging it into
> fp->mode (adding a 3rd mode: TPA_MODE_DISABLED). And another patch that
> removes {TPA,GRO}_ENABLE_FLAG by using dev->features everywhere.
> I'll wait and see what changes you have.
> 
> Regards,
> Michal

Ours is merely a point fix that differentiate GRO_ENABLE_FLAG from
NETIF_F_GRO.
If you have a ready patch that removes the flags altogether I think
we can skip ours and review that one instead.

Thanks,
Yuval