Re: [PATCH net-next 13/27] bridge: use __vlan_hwaccel helpers

[Toshiaki Makita]

On 2016/12/14 0:11, Michał Mirosław wrote:
> On Tue, Dec 13, 2016 at 03:59:46PM +0300, Sergei Shtylyov wrote:
>> Hello!
>>
>> On 12/13/2016 3:12 AM, Michał Mirosław wrote:
>>
>>> This removes assumption than vlan_tci != 0 when tag is present.
>>>
>>> Signed-off-by: Michał Mirosław 
>>> ---
>>>  net/bridge/br_netfilter_hooks.c | 14 --
>>>  net/bridge/br_private.h |  2 +-
>>>  net/bridge/br_vlan.c|  6 +++---
>>>  3 files changed, 12 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/net/bridge/br_netfilter_hooks.c 
>>> b/net/bridge/br_netfilter_hooks.c
>>> index b12501a..2cc0747 100644
>>> --- a/net/bridge/br_netfilter_hooks.c
>>> +++ b/net/bridge/br_netfilter_hooks.c
>> [...]
>>> @@ -749,8 +747,12 @@ static int br_nf_dev_queue_xmit(struct net *net, 
>>> struct sock *sk, struct sk_buff
>>>
>>> data = this_cpu_ptr(_frag_data_storage);
>>>
>>> -   data->vlan_tci = skb->vlan_tci;
>>> -   data->vlan_proto = skb->vlan_proto;
>>> +   if (skb_vlan_tag_present(skb)) {
>>> +   data->vlan_tci = skb->vlan_tci;
>>> +   data->vlan_proto = skb->vlan_proto;
>>> +   } else
>>> +   data->vlan_proto = 0;
>>
>>CodingStyle: should use {} in all branches of *if* if at least one branch
>> has {}.
>>
>> [...]
>>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>>> index b6de4f4..ef94664 100644
>>> --- a/net/bridge/br_vlan.c
>>> +++ b/net/bridge/br_vlan.c
>>
>>> @@ -444,8 +444,8 @@ static bool __allowed_ingress(const struct net_bridge 
>>> *br,
>>> __vlan_hwaccel_put_tag(skb, br->vlan_proto, pvid);
>>> else
>>> /* Priority-tagged Frame.
>>> -* At this point, We know that skb->vlan_tci had
>>> -* VLAN_TAG_PRESENT bit and its VID field was 0x000.
>>> +* At this point, We know that skb->vlan_tci VID
>>
>>s/We/we/.
>>
>>> +* field was 0x000.
>>
>>Simply 0, maybe?

I originally wrote it like this because we are playing with bit field here.
I meant that all of 12 bits are 0 thus we can safely perform bitwise-OR
to update the VID field.

Thanks,
Toshiaki Makita

Re: [PATCH net-next 13/27] bridge: use __vlan_hwaccel helpers

[Michał Mirosław]

On Tue, Dec 13, 2016 at 03:59:46PM +0300, Sergei Shtylyov wrote:
> Hello!
> 
> On 12/13/2016 3:12 AM, Michał Mirosław wrote:
> 
> > This removes assumption than vlan_tci != 0 when tag is present.
> > 
> > Signed-off-by: Michał Mirosław 
> > ---
> >  net/bridge/br_netfilter_hooks.c | 14 --
> >  net/bridge/br_private.h |  2 +-
> >  net/bridge/br_vlan.c|  6 +++---
> >  3 files changed, 12 insertions(+), 10 deletions(-)
> > 
> > diff --git a/net/bridge/br_netfilter_hooks.c 
> > b/net/bridge/br_netfilter_hooks.c
> > index b12501a..2cc0747 100644
> > --- a/net/bridge/br_netfilter_hooks.c
> > +++ b/net/bridge/br_netfilter_hooks.c
> [...]
> > @@ -749,8 +747,12 @@ static int br_nf_dev_queue_xmit(struct net *net, 
> > struct sock *sk, struct sk_buff
> > 
> > data = this_cpu_ptr(_frag_data_storage);
> > 
> > -   data->vlan_tci = skb->vlan_tci;
> > -   data->vlan_proto = skb->vlan_proto;
> > +   if (skb_vlan_tag_present(skb)) {
> > +   data->vlan_tci = skb->vlan_tci;
> > +   data->vlan_proto = skb->vlan_proto;
> > +   } else
> > +   data->vlan_proto = 0;
> 
>CodingStyle: should use {} in all branches of *if* if at least one branch
> has {}.
> 
> [...]
> > diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> > index b6de4f4..ef94664 100644
> > --- a/net/bridge/br_vlan.c
> > +++ b/net/bridge/br_vlan.c
> 
> > @@ -444,8 +444,8 @@ static bool __allowed_ingress(const struct net_bridge 
> > *br,
> > __vlan_hwaccel_put_tag(skb, br->vlan_proto, pvid);
> > else
> > /* Priority-tagged Frame.
> > -* At this point, We know that skb->vlan_tci had
> > -* VLAN_TAG_PRESENT bit and its VID field was 0x000.
> > +* At this point, We know that skb->vlan_tci VID
> 
>s/We/we/.
> 
> > +* field was 0x000.
> 
>Simply 0, maybe?

Thanks, fixed.
-- Michał Mirosław

Re: [PATCH net-next 13/27] bridge: use __vlan_hwaccel helpers

[Sergei Shtylyov]

Hello!

On 12/13/2016 3:12 AM, Michał Mirosław wrote:


This removes assumption than vlan_tci != 0 when tag is present.

Signed-off-by: Michał Mirosław 
---
 net/bridge/br_netfilter_hooks.c | 14 --
 net/bridge/br_private.h |  2 +-
 net/bridge/br_vlan.c|  6 +++---
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
index b12501a..2cc0747 100644
--- a/net/bridge/br_netfilter_hooks.c
+++ b/net/bridge/br_netfilter_hooks.c

[...]

@@ -749,8 +747,12 @@ static int br_nf_dev_queue_xmit(struct net *net, struct 
sock *sk, struct sk_buff

data = this_cpu_ptr(_frag_data_storage);

-   data->vlan_tci = skb->vlan_tci;
-   data->vlan_proto = skb->vlan_proto;
+   if (skb_vlan_tag_present(skb)) {
+   data->vlan_tci = skb->vlan_tci;
+   data->vlan_proto = skb->vlan_proto;
+   } else
+   data->vlan_proto = 0;


   CodingStyle: should use {} in all branches of *if* if at least one branch 
has {}.


[...]

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index b6de4f4..ef94664 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c



@@ -444,8 +444,8 @@ static bool __allowed_ingress(const struct net_bridge *br,
__vlan_hwaccel_put_tag(skb, br->vlan_proto, pvid);
else
/* Priority-tagged Frame.
-* At this point, We know that skb->vlan_tci had
-* VLAN_TAG_PRESENT bit and its VID field was 0x000.
+* At this point, We know that skb->vlan_tci VID


   s/We/we/.


+* field was 0x000.


   Simply 0, maybe?

[...]

MBR, Sergei

[PATCH net-next 13/27] bridge: use __vlan_hwaccel helpers

[Michał Mirosław]

This removes assumption than vlan_tci != 0 when tag is present.

Signed-off-by: Michał Mirosław 
---
 net/bridge/br_netfilter_hooks.c | 14 --
 net/bridge/br_private.h |  2 +-
 net/bridge/br_vlan.c|  6 +++---
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
index b12501a..2cc0747 100644
--- a/net/bridge/br_netfilter_hooks.c
+++ b/net/bridge/br_netfilter_hooks.c
@@ -682,10 +682,8 @@ static int br_nf_push_frag_xmit(struct net *net, struct 
sock *sk, struct sk_buff
return 0;
}
 
-   if (data->vlan_tci) {
-   skb->vlan_tci = data->vlan_tci;
-   skb->vlan_proto = data->vlan_proto;
-   }
+   if (data->vlan_proto)
+   __vlan_hwaccel_put_tag(skb, data->vlan_proto, data->vlan_tci);
 
skb_copy_to_linear_data_offset(skb, -data->size, data->mac, data->size);
__skb_push(skb, data->encap_size);
@@ -749,8 +747,12 @@ static int br_nf_dev_queue_xmit(struct net *net, struct 
sock *sk, struct sk_buff
 
data = this_cpu_ptr(_frag_data_storage);
 
-   data->vlan_tci = skb->vlan_tci;
-   data->vlan_proto = skb->vlan_proto;
+   if (skb_vlan_tag_present(skb)) {
+   data->vlan_tci = skb->vlan_tci;
+   data->vlan_proto = skb->vlan_proto;
+   } else
+   data->vlan_proto = 0;
+
data->encap_size = nf_bridge_encap_header_len(skb);
data->size = ETH_HLEN + data->encap_size;
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 8ce621e..2efbdaf 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -819,7 +819,7 @@ static inline int br_vlan_get_tag(const struct sk_buff 
*skb, u16 *vid)
int err = 0;
 
if (skb_vlan_tag_present(skb)) {
-   *vid = skb_vlan_tag_get(skb) & VLAN_VID_MASK;
+   *vid = skb_vlan_tag_get_id(skb);
} else {
*vid = 0;
err = -EINVAL;
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index b6de4f4..ef94664 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -377,7 +377,7 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
}
 
if (v->flags & BRIDGE_VLAN_INFO_UNTAGGED)
-   skb->vlan_tci = 0;
+   __vlan_hwaccel_clear_tag(skb);
 out:
return skb;
 }
@@ -444,8 +444,8 @@ static bool __allowed_ingress(const struct net_bridge *br,
__vlan_hwaccel_put_tag(skb, br->vlan_proto, pvid);
else
/* Priority-tagged Frame.
-* At this point, We know that skb->vlan_tci had
-* VLAN_TAG_PRESENT bit and its VID field was 0x000.
+* At this point, We know that skb->vlan_tci VID
+* field was 0x000.
 * We update only VID field and preserve PCP field.
 */
skb->vlan_tci |= pvid;
-- 
2.10.2