2013/2/3 Michał Mirosław <mirqus@xxxxxxxxx>:
> 2013/2/2 Vlad Yasevich <vyasevic@xxxxxxxxxx>:
>> On 02/01/2013 08:15 PM, Michał Mirosław wrote:
>>> 2013/2/2 Michał Mirosław <mirqus@xxxxxxxxx>:
>>>> 2013/2/1 Vlad Yasevich <vyasevic@xxxxxxxxxx>:
>>>>> A user may designate a certain vlan as PVID. This means that
>>>>> any ingress frame that does not contain a vlan tag is assigned to
>>>>> this vlan and any forwarding decisions are made with this vlan in mind.
>>>>
>>>> [...]
>>>>>
>>>>> struct net_port_vlans {
>>>>> u16 port_idx;
>>>>> + u16 pvid;
>>>>
>>>>
>>>> I'm confused about the implementation. I would expect pvid field in
>>>> net_bridge_port and adding a tag if it isn't there on ingress path.
>>>> The rest would be just like without PVIDs. But here you pvid field to
>>>> net_port_vlans, and don't do anything with it in receive nor transmit
>>>> path. Does it work? What am I missing?
>>> Found the answer in next patch (you should merge #5 and #6).
>> It was split for incremental testing. #5 added the ability to set and
>> delete it without impacting anything. #6 added the actual work that pvid
>> does.
>>
>>> Still,
>>> the implementation looks overly complicated. If you force the packet
>>> to canonical form on ingress (keeping outer tag in skb->vlan_tci, and
>>> setting skb->vlan_tci = pvid if there is no tag) the code should get
>>> simpler.
>>
>>
>> What if there is no outer tag? That's what the ingress code is doing.
>> If there is no outer tag, pvid is written to vlan_tci. If there was
>> outer tag in vlan_tci, it's left alone. This way at the end of ingress
>> vlan_tci is always set.
>> At egress, we grab that tag and compare it against pvid if any. If it
>> matches, it's stripped. If it doesn't, we output with the tag thus
>> adding the header.
>>
>> The only thing I can simplify is grab the tci directly at egress, but
>> that's what the code will do anyway.
>
> ... br_allowed_input(..., struct sk_buff **pskb)
> {
> *pskb = vlan_untag(*pskb);
> skb = *pskb;
> if (!skb)
> return 0;
>
> if (!skb->vlan_tci && (pvid & VLAN_TAG_PRESENT))
> skb->vlan_tci = pvid;
> return check_vlan_allowed(skb->vlan_tci);
> }
Or maybe some cheating to save unconditional vlan_untag()ging: since
vlan_tci is used only when vlan_tx_tag_present() returns true, we can
cache VID there for the bridge code when VLANs are not HW-accelerated.
I'm not sure of this one as I don't know if the netfilter part keeps
vlan_tci intact (quick grep suggests it does).
... br_allowed_input(..., struct sk_buff **pskb)
{
skb = *pskb;
if (!vlan_tx_tag_present(skb)) {
if (!__vlan_get_tag(skb, &skb->vlan_tci))
skb->vlan_tci &= VLAN_VID_MASK;
else
skb->vlan_tci = 0;
}
if (!skb->vlan_tci && pvid & VLAN_TAG_PRESENT) // FIXME:
802.1p-tagged is put in VLAN 0
skb->vlan_tci = pvid;
return check_vlan_allowed(skb, skb->vlan_tci & VLAN_VID_MASK);
}
struct sk_buff *br_handle_vlan(..., struct sk_buff *skb)
{
/* we guaranteed in br_allowed_input() that all packets processed
/* in bridge code will be like received with NETIF_F_HW_VLAN_RX
* feature enabled or have VID cached in vlan_tci without
* VLAN_TAG_PRESENT bit set. */
if ((skb->vlan_tci & VLAN_VID_MASK) | VLAN_TAG_PRESENT != pvid)
return skb;
if (!vlan_tx_tag_present(skb)) {
skb = vlan_untag(skb);
if (!skb)
return skb;
}
skb->vlan_tci = 0; // FIXME: what about 802.1p?
return skb;
}
Best Regards,
Michał Mirosław
