Re: [PATCH net-next v2 2/3] bonding: support QinQ for bond arp interval

[Jay Vosburgh <fubar@us.ibm.com>]

Veaceslav Falico <vfalico@redhat.com> wrote:

>On Tue, Mar 18, 2014 at 06:43:52PM +0800, Ding Tianhong wrote:
>...snip...
>>-	if (vlan_id) {
>>-		skb = vlan_put_tag(skb, htons(ETH_P_8021Q), vlan_id);
>>+	if (outer->vlan_id) {
>>+		if (inner->vlan_id) {
>>+			pr_debug("inner tag: proto %X vid %X\n",
>>+				 ntohs(inner->vlan_proto), inner->vlan_id);
>>+			skb = __vlan_put_tag(skb, inner->vlan_proto, inner->vlan_id);
>>+			if (!skb) {
>>+				pr_err("failed to insert inner VLAN tag\n");
>>+				return;
>>+			}
>>+		}
>>+
>>+		pr_debug("outer reg: proto %X vid %X\n",
>>+			 ntohs(outer->vlan_proto), outer->vlan_id);
>>+		skb = vlan_put_tag(skb, outer->vlan_proto, outer->vlan_id);
>
>If I read correctly then the inner->vlan_proto will always be ETH_P_8021AD,
>whilst the outer will also always be ETH_P_8021Q. So I think it'd be a lot
>easier (and more readable) to just pass 2 vlan ids, and set those protos
>statically - that will save you from adding that new function to vlan core,
>fro madding a new struct that you've added here and make it several lines
>less.

	Do you mean that the outer will always be 8021AD and the inner
8021Q?  The inner/outer terminology is making my brain hurt, since the
ip commands to configure them look backwards to me.  I think of it as
the outer tag is the first one sequentially in the ethernet header, and
the inner tag is second in the header.

	Anyway, in the past, I've seen configurations with 802.1q VLANs
nested such that the inner and outer tags were both 802.1q 0x8100
ethertype, so I'm not sure that hard-coding these is necessarily a good
idea.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com