[net PATCH] be2net: take care of __vlan_put_tag return value

[net PATCH] be2net: take care of __vlan_put_tag return value

[Ivan Vecera]

The driver should use return value of __vlan_put_tag with appropriate
NULL-check instead of old skb pointer.

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/net/ethernet/emulex/benet/be_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index 4d6f3c5..a4e4626 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -759,8 +759,9 @@ static struct sk_buff *be_insert_vlan_in_pkt(struct be_adapter *adapter,
 
 	if (vlan_tx_tag_present(skb)) {
 		vlan_tag = be_get_tx_vlan_tag(adapter, skb);
-		__vlan_put_tag(skb, vlan_tag);
-		skb->vlan_tci = 0;
+		skb = __vlan_put_tag(skb, vlan_tag);
+		if (skb)
+			skb->vlan_tci = 0;
 	}
 
 	return skb;
-- 
1.8.1.5

Re: [net PATCH] be2net: take care of __vlan_put_tag return value

[David Miller]

From: Ivan Vecera <ivecera@redhat.com>
Date: Fri, 12 Apr 2013 16:49:24 +0200

> The driver should use return value of __vlan_put_tag with appropriate
> NULL-check instead of old skb pointer.
> 
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>

I'll apply this patch but there is some strangeness with the
logic that controls these code paths.

The call to be_insert_vlan_in_pkt() is guarded by:

	be_vlan_tag_chk(adapter, skb)

which evaluates to:

	vlan_tx_tag_present(skb) || adapter->pvid

But the __vlan_put_tag() call is guarded by only one of those two
conditions:

	vlan_tx_tag_present(skb)

One of these two is wrong and should be corrected.

I suspect that we have several layers of bugs here, in that
we need to do __vlan_put_tag() in the adapter->pvid case but
that means that tag determination needs to have some
adapter->pvid logic added to it.

Or, if for some reason the adapter->pvid case doesn't apply to
this HW bug, the test guarding the be_insert_vlan_in_pkt() call
should be changed to vlan_tx_tag_present() and a big comment
added explaining why adapter->pvid is not being considered.

RE: [net PATCH] be2net: take care of __vlan_put_tag return value

[Perla, Sathya]

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> 
> From: Ivan Vecera <ivecera@redhat.com>
> 
> I'll apply this patch but there is some strangeness with the logic that controls
> these code paths.
> 
> The call to be_insert_vlan_in_pkt() is guarded by:
> 
> 	be_vlan_tag_chk(adapter, skb)
> 
> which evaluates to:
> 
> 	vlan_tx_tag_present(skb) || adapter->pvid
> 
> But the __vlan_put_tag() call is guarded by only one of those two
> conditions:
> 
> 	vlan_tx_tag_present(skb)
> 
> One of these two is wrong and should be corrected.

David, yes the pvid check here is wrong.

The pvid insertion by the driver (to hack around some HW bugs) requires some support from the FW and that
requires some more code/logic. That fix seems to be missing in the netdev tree.

We'll send out a patch-set to handle these issues. Thanks!

-Sathya