diff mbox

[10/27] ixgb: Don't check for vlan group on transmit

Message ID 1292048241-22026-11-git-send-email-jeffrey.t.kirsher@intel.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Kirsher, Jeffrey T Dec. 11, 2010, 6:17 a.m. UTC 
From: Emil Tantilov <emil.s.tantilov@intel.com>

Based on a patch from Jesse Gross.

Enable vlan tag insertion even when vlan group is not configured.

For ixgb HW both CTRL0.VME and VLE bit in the Tx descriptor need to be set
in order to enable HW acceleration.

Introduced separate functions for enabling/disabling of vlan tag stripping
similar to ixgbe.

CC: Jesse Gross <jesse@nicira.com>
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
Tested-by: Jeff Pieper <jeffrey.e.pieper@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ixgb/ixgb_main.c |   51 ++++++++++++++++++++++++-----------------
 1 files changed, 30 insertions(+), 21 deletions(-)

Comments

Jesse Gross Dec. 13, 2010, 1 a.m. UTC | #1 
On Fri, Dec 10, 2010 at 10:17 PM, Jeff Kirsher
<jeffrey.t.kirsher@intel.com> wrote:
> From: Emil Tantilov <emil.s.tantilov@intel.com>
>
> Based on a patch from Jesse Gross.
>
> Enable vlan tag insertion even when vlan group is not configured.
>
> For ixgb HW both CTRL0.VME and VLE bit in the Tx descriptor need to be set
> in order to enable HW acceleration.
>
> Introduced separate functions for enabling/disabling of vlan tag stripping
> similar to ixgbe.

Thanks for working on this.  However, I don't think that this patch
actually does what it says.

In ixgb_xmit_frame() it's still checking whether adapter->vlgrp is
non-null before inserting a tag, so it will drop tags unless a vlan
group is configured.  Also, since it's not currently possible to
toggle NETIF_F_HW_VLAN_RX, vlan stripping will never get disabled.
This is actually a regression since before vlan stripping would get
disabled if no vlan group was configured.  Now, vlan headers will get
silently dropped if there is no vlan group.

Regardless of that, I still think this is a useful change on the road
towards adopting the new vlan interfaces, the problem is just that
currently it's halfway in between the old and the new.  Given that, it
would obviously be much better to move all the way over the new when
addressing this.

Out of curiosity, is the implication of this that vlan insertion on
transmit and stripping on receive are always configured together?  I
don't have hardware supported by this driver but when I worked on
ixgbe (which is at least superficially similar in this area) I didn't
have any problems configuring them independently.

Thanks.
--
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
Tantilov, Emil S Dec. 13, 2010, 6:43 p.m. UTC | #2 
>-----Original Message-----
>From: Jesse Gross [mailto:jesse@nicira.com]
>Sent: Sunday, December 12, 2010 5:01 PM
>To: Kirsher, Jeffrey T
>Cc: davem@davemloft.net; Tantilov, Emil S; netdev@vger.kernel.org;
>gospo@redhat.com; bphilips@novell.com
>Subject: Re: [PATCH 10/27] ixgb: Don't check for vlan group on transmit
>
>On Fri, Dec 10, 2010 at 10:17 PM, Jeff Kirsher
><jeffrey.t.kirsher@intel.com> wrote:
>> From: Emil Tantilov <emil.s.tantilov@intel.com>
>>
>> Based on a patch from Jesse Gross.
>>
>> Enable vlan tag insertion even when vlan group is not configured.
>>
>> For ixgb HW both CTRL0.VME and VLE bit in the Tx descriptor need to be
>set
>> in order to enable HW acceleration.
>>
>> Introduced separate functions for enabling/disabling of vlan tag
>stripping
>> similar to ixgbe.
>
>Thanks for working on this.  However, I don't think that this patch
>actually does what it says.
>
>In ixgb_xmit_frame() it's still checking whether adapter->vlgrp is
>non-null before inserting a tag, so it will drop tags unless a vlan
>group is configured.  Also, since it's not currently possible to
>toggle NETIF_F_HW_VLAN_RX, vlan stripping will never get disabled.
>This is actually a regression since before vlan stripping would get
>disabled if no vlan group was configured.  Now, vlan headers will get
>silently dropped if there is no vlan group.
I'm sorry. This patch was supposed to include your original patch that 
removed the vlgrp check on Tx. Somehow that didn't make it (I may have generated the patch from the wrong branch).

>Regardless of that, I still think this is a useful change on the road
>towards adopting the new vlan interfaces, the problem is just that
>currently it's halfway in between the old and the new.  Given that, it
>would obviously be much better to move all the way over the new when
>addressing this.
Since this patch is already applied can you submit your change again?

>Out of curiosity, is the implication of this that vlan insertion on
>transmit and stripping on receive are always configured together?  I
>don't have hardware supported by this driver but when I worked on
>ixgbe (which is at least superficially similar in this area) I didn't
>have any problems configuring them independently.

Yes - the design of the original 10GB HW supported in ixgb does not allow 
setting Tx and Rx independently. As you mentioned ixgbe does not have 
this problem.

>
>Thanks.

Thanks,
Emil

--
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
Jesse Gross Dec. 14, 2010, 3:40 a.m. UTC | #3 
On Mon, Dec 13, 2010 at 10:43 AM, Tantilov, Emil S
<emil.s.tantilov@intel.com> wrote:
>>-----Original Message-----
>>From: Jesse Gross [mailto:jesse@nicira.com]
>>Sent: Sunday, December 12, 2010 5:01 PM
>>To: Kirsher, Jeffrey T
>>Cc: davem@davemloft.net; Tantilov, Emil S; netdev@vger.kernel.org;
>>gospo@redhat.com; bphilips@novell.com
>>Subject: Re: [PATCH 10/27] ixgb: Don't check for vlan group on transmit
>>
>>On Fri, Dec 10, 2010 at 10:17 PM, Jeff Kirsher
>><jeffrey.t.kirsher@intel.com> wrote:
>>> From: Emil Tantilov <emil.s.tantilov@intel.com>
>>>
>>> Based on a patch from Jesse Gross.
>>>
>>> Enable vlan tag insertion even when vlan group is not configured.
>>>
>>> For ixgb HW both CTRL0.VME and VLE bit in the Tx descriptor need to be
>>set
>>> in order to enable HW acceleration.
>>>
>>> Introduced separate functions for enabling/disabling of vlan tag
>>stripping
>>> similar to ixgbe.
>>
>>Thanks for working on this.  However, I don't think that this patch
>>actually does what it says.
>>
>>In ixgb_xmit_frame() it's still checking whether adapter->vlgrp is
>>non-null before inserting a tag, so it will drop tags unless a vlan
>>group is configured.  Also, since it's not currently possible to
>>toggle NETIF_F_HW_VLAN_RX, vlan stripping will never get disabled.
>>This is actually a regression since before vlan stripping would get
>>disabled if no vlan group was configured.  Now, vlan headers will get
>>silently dropped if there is no vlan group.
> I'm sorry. This patch was supposed to include your original patch that
> removed the vlgrp check on Tx. Somehow that didn't make it (I may have generated the patch from the wrong branch).
>
>>Regardless of that, I still think this is a useful change on the road
>>towards adopting the new vlan interfaces, the problem is just that
>>currently it's halfway in between the old and the new.  Given that, it
>>would obviously be much better to move all the way over the new when
>>addressing this.
> Since this patch is already applied can you submit your change again?

Sure.  I think it's probably best to just complete the conversion to
the new vlan interfaces, so I went ahead and did that.  It's much
easier now that you've pulled the hardware specific bits out for
enabling/disabling vlan offloading.  I'll send out that patch shortly
- please take a look since I don't have the hardware to test it.

>
>>Out of curiosity, is the implication of this that vlan insertion on
>>transmit and stripping on receive are always configured together?  I
>>don't have hardware supported by this driver but when I worked on
>>ixgbe (which is at least superficially similar in this area) I didn't
>>have any problems configuring them independently.
>
> Yes - the design of the original 10GB HW supported in ixgb does not allow
> setting Tx and Rx independently. As you mentioned ixgbe does not have
> this problem.

OK, thanks for the explanation.
--
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
Tantilov, Emil S Dec. 14, 2010, 6 p.m. UTC | #4 
Jesse Gross wrote:
> On Mon, Dec 13, 2010 at 10:43 AM, Tantilov, Emil S
> <emil.s.tantilov@intel.com> wrote:
>>> -----Original Message-----
>>> From: Jesse Gross [mailto:jesse@nicira.com]
>>> Sent: Sunday, December 12, 2010 5:01 PM
>>> To: Kirsher, Jeffrey T
>>> Cc: davem@davemloft.net; Tantilov, Emil S; netdev@vger.kernel.org;
>>> gospo@redhat.com; bphilips@novell.com
>>> Subject: Re: [PATCH 10/27] ixgb: Don't check for vlan group on
>>> transmit 
>>> 
>>> On Fri, Dec 10, 2010 at 10:17 PM, Jeff Kirsher
>>> <jeffrey.t.kirsher@intel.com> wrote:
>>>> From: Emil Tantilov <emil.s.tantilov@intel.com>
>>>> 
>>>> Based on a patch from Jesse Gross.
>>>> 
>>>> Enable vlan tag insertion even when vlan group is not configured.
>>>> 
>>>> For ixgb HW both CTRL0.VME and VLE bit in the Tx descriptor need
>>>> to be set in order to enable HW acceleration.
>>>> 
>>>> Introduced separate functions for enabling/disabling of vlan tag
>>>> stripping similar to ixgbe.
>>> 
>>> Thanks for working on this.  However, I don't think that this patch
>>> actually does what it says. 
>>> 
>>> In ixgb_xmit_frame() it's still checking whether adapter->vlgrp is
>>> non-null before inserting a tag, so it will drop tags unless a vlan
>>> group is configured.  Also, since it's not currently possible to
>>> toggle NETIF_F_HW_VLAN_RX, vlan stripping will never get disabled.
>>> This is actually a regression since before vlan stripping would get
>>> disabled if no vlan group was configured.  Now, vlan headers will
>>> get silently dropped if there is no vlan group.
>> I'm sorry. This patch was supposed to include your original patch
>> that 
>> removed the vlgrp check on Tx. Somehow that didn't make it (I may
>> have generated the patch from the wrong branch). 
>> 
>>> Regardless of that, I still think this is a useful change on the
>>> road towards adopting the new vlan interfaces, the problem is just
>>> that currently it's halfway in between the old and the new.  Given
>>> that, it would obviously be much better to move all the way over
>>> the new when addressing this.
>> Since this patch is already applied can you submit your change again?
> 
> Sure.  I think it's probably best to just complete the conversion to
> the new vlan interfaces, so I went ahead and did that.  It's much
> easier now that you've pulled the hardware specific bits out for
> enabling/disabling vlan offloading.  I'll send out that patch shortly
> - please take a look since I don't have the hardware to test it.

Thanks Jesse!

I'll ask Jeff to pick the patch in his tree and we'll run some tests.

Emil
--
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
diff mbox

Patch

diff --git a/drivers/net/ixgb/ixgb_main.c b/drivers/net/ixgb/ixgb_main.c
index 211a169..2e98506 100644
--- a/drivers/net/ixgb/ixgb_main.c
+++ b/drivers/net/ixgb/ixgb_main.c
@@ -98,6 +98,8 @@  static void ixgb_alloc_rx_buffers(struct ixgb_adapter *, int);
 static void ixgb_tx_timeout(struct net_device *dev);
 static void ixgb_tx_timeout_task(struct work_struct *work);
 
+static void ixgb_vlan_strip_enable(struct ixgb_adapter *adapter);
+static void ixgb_vlan_strip_disable(struct ixgb_adapter *adapter);
 static void ixgb_vlan_rx_register(struct net_device *netdev,
                                   struct vlan_group *grp);
 static void ixgb_vlan_rx_add_vid(struct net_device *netdev, u16 vid);
@@ -1076,6 +1078,8 @@  ixgb_set_multi(struct net_device *netdev)
 
 	if (netdev->flags & IFF_PROMISC) {
 		rctl |= (IXGB_RCTL_UPE | IXGB_RCTL_MPE);
+		/* disable VLAN filtering */
+		rctl &= ~IXGB_RCTL_CFIEN;
 		rctl &= ~IXGB_RCTL_VFE;
 	} else {
 		if (netdev->flags & IFF_ALLMULTI) {
@@ -1084,7 +1088,9 @@  ixgb_set_multi(struct net_device *netdev)
 		} else {
 			rctl &= ~(IXGB_RCTL_UPE | IXGB_RCTL_MPE);
 		}
+		/* enable VLAN filtering */
 		rctl |= IXGB_RCTL_VFE;
+		rctl &= ~IXGB_RCTL_CFIEN;
 	}
 
 	if (netdev_mc_count(netdev) > IXGB_MAX_NUM_MULTICAST_ADDRESSES) {
@@ -1103,6 +1109,12 @@  ixgb_set_multi(struct net_device *netdev)
 
 		ixgb_mc_addr_list_update(hw, mta, netdev_mc_count(netdev), 0);
 	}
+
+	if (netdev->features & NETIF_F_HW_VLAN_RX)
+		ixgb_vlan_strip_enable(adapter);
+	else
+		ixgb_vlan_strip_disable(adapter);
+
 }
 
 /**
@@ -2150,33 +2162,30 @@  static void
 ixgb_vlan_rx_register(struct net_device *netdev, struct vlan_group *grp)
 {
 	struct ixgb_adapter *adapter = netdev_priv(netdev);
-	u32 ctrl, rctl;
 
-	ixgb_irq_disable(adapter);
 	adapter->vlgrp = grp;
+}
 
-	if (grp) {
-		/* enable VLAN tag insert/strip */
-		ctrl = IXGB_READ_REG(&adapter->hw, CTRL0);
-		ctrl |= IXGB_CTRL0_VME;
-		IXGB_WRITE_REG(&adapter->hw, CTRL0, ctrl);
-
-		/* enable VLAN receive filtering */
+static void
+ixgb_vlan_strip_enable(struct ixgb_adapter *adapter)
+{
+	u32 ctrl;
 
-		rctl = IXGB_READ_REG(&adapter->hw, RCTL);
-		rctl &= ~IXGB_RCTL_CFIEN;
-		IXGB_WRITE_REG(&adapter->hw, RCTL, rctl);
-	} else {
-		/* disable VLAN tag insert/strip */
+	/* enable VLAN tag insert/strip */
+	ctrl = IXGB_READ_REG(&adapter->hw, CTRL0);
+	ctrl |= IXGB_CTRL0_VME;
+	IXGB_WRITE_REG(&adapter->hw, CTRL0, ctrl);
+}
 
-		ctrl = IXGB_READ_REG(&adapter->hw, CTRL0);
-		ctrl &= ~IXGB_CTRL0_VME;
-		IXGB_WRITE_REG(&adapter->hw, CTRL0, ctrl);
-	}
+static void
+ixgb_vlan_strip_disable(struct ixgb_adapter *adapter)
+{
+	u32 ctrl;
 
-	/* don't enable interrupts unless we are UP */
-	if (adapter->netdev->flags & IFF_UP)
-		ixgb_irq_enable(adapter);
+	/* disable VLAN tag insert/strip */
+	ctrl = IXGB_READ_REG(&adapter->hw, CTRL0);
+	ctrl &= ~IXGB_CTRL0_VME;
+	IXGB_WRITE_REG(&adapter->hw, CTRL0, ctrl);
 }
 
 static void