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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 13 Dec 2010 19:40:53 -0800
From:	Jesse Gross <jesse@...ira.com>
To:	"Tantilov, Emil S" <emil.s.tantilov@...el.com>
Cc:	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"gospo@...hat.com" <gospo@...hat.com>,
	"bphilips@...ell.com" <bphilips@...ell.com>
Subject: Re: [PATCH 10/27] ixgb: Don't check for vlan group on transmit

On Mon, Dec 13, 2010 at 10:43 AM, Tantilov, Emil S
<emil.s.tantilov@...el.com> wrote:
>>-----Original Message-----
>>From: Jesse Gross [mailto:jesse@...ira.com]
>>Sent: Sunday, December 12, 2010 5:01 PM
>>To: Kirsher, Jeffrey T
>>Cc: davem@...emloft.net; Tantilov, Emil S; netdev@...r.kernel.org;
>>gospo@...hat.com; bphilips@...ell.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@...el.com> wrote:
>>> From: Emil Tantilov <emil.s.tantilov@...el.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html