RE: [PATCH] vlan_dev: VLAN 0 should be treated as "no vlan tag" (802.1p packet)

["Vladislav Zolotarov" <vladz@broadcom.com>]

I've checked your patch on bnx2x HW acceleration capable device in both HW accelerated and none-accelerated modes and it looks good.

Thanks,
vlad

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> Behalf Of Pedro Garcia
> Sent: Wednesday, June 16, 2010 11:49 AM
> To: netdev@vger.kernel.org
> Cc: Patrick McHardy; Ben Hutchings; Eric Dumazet
> Subject: Re: [PATCH] vlan_dev: VLAN 0 should be treated as "no vlan tag"
> (802.1p packet)
> 
> On Mon, 14 Jun 2010 21:12:52 +0200, Eric Dumazet <eric.dumazet@gmail.com>
> wrote:
> > Le lundi 14 juin 2010 à 19:11 +0200, Patrick McHardy a écrit :
> >> Ben Hutchings wrote:
> >> > On Mon, 2010-06-14 at 18:49 +0200, Pedro Garcia wrote:
> >> >
> >> >> On Sun, 13 Jun 2010 22:56:30 +0100, Ben Hutchings
> >> >> <bhutchings@solarflare.com> wrote:
> >> >>
> >> >>> I have no particular opinion on this change, but you need to read and
> >> >>> follow Documentation/SubmittingPatches.
> >> >>>
> >> >>> Ben.
> >> >>>
> >> >> Sorry, first kernel patch, and I did not know about it. I resubmit
> >> >> with
> >> >> the correct style / format:
> >> >>
> >> > [...]
> >> >
> >> > Sorry, no you haven't.
> >> >
> >> > - Networking changes go through David Miller's net-next-2.6 tree so you
> >> > need to use that as the baseline, not 2.6.26
> >> > - Patches should be applicable with -p1, not -p0 (so if you use diff,
> >> > you should run it from one directory level up)
> >> > - The patch was word-wrapped
> >>
> >> Additionally:
> >>
> >> - please use the proper comment style, meaning each line begins
> >>   with a '*'
> >>
> >> - the pr_debug statements may be useful for debugging, but are
> >>   a bit excessive for the final version
> >>
> >> - + /* 2010-06-13: Pedro Garcia
> >>
> >>    We have changelogs for this, simply explaining what the code
> >>    does is enough.
> >>
> >> - Please CC the maintainer (which is me)
> >> --
> >
> > Pedro, we have two kind of vlan setups :
> >
> > accelerated and non accelerated ones.
> >
> > Your patch address non accelated ones only, since you only touch
> > vlan_skb_recv()
> >
> > Accelerated vlan can follow these paths :
> >
> > 1) NAPI devices
> >
> > vlan_gro_receive() -> vlan_gro_common()
> >
> > 2) non NAPI devices
> >
> > __vlan_hwaccel_rx()
> >
> > So you might also patch __vlan_hwaccel_rx() and vlan_gro_common()
> >
> > Please merge following bits to your patch submission :
> >
> > http://kerneltrap.org/mailarchive/linux-netdev/2010/5/23/6277868
> >
> >
> > Good luck for your first patch !
> 
> Here it is again. I added the modifications in
> http://kerneltrap.org/mailarchive/linux-netdev/2010/5/23/6277868 for HW
> accelerated incoming packets (it did not apply cleanly on the last version of
> the kernel, so I applied manually). Now, if the VLAN 0 is not explicitly
> created by the user, VLAN 0 packets will be treated as no VLAN (802.1p
> packets), instead of dropping them.
> 
> The patch is now for two files: vlan_core (accel) and vlan_dev (non accel)
> 
> I can not test on HW accelerated devices, so if someone can check it I will
> appreciate (even though in the thread above it looked like yes). For non
> accel I tessted in 2.6.26. Now the patch is for
> net-next-2.6, and it compiles OK, but I a have to setup a test environment to
> check it is still OK (should, but better to test).
> 
> Signed-off-by: Pedro Garcia <pedro.netdev@dondevamos.com>
> --
> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> index 50f58f5..daaca31 100644
> --- a/net/8021q/vlan_core.c
> +++ b/net/8021q/vlan_core.c
> @@ -8,6 +8,9 @@
>  int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
>                       u16 vlan_tci, int polling)
>  {
> +       struct net_device *vlan_dev;
> +       u16 vlan_id;
> +
>         if (netpoll_rx(skb))
>                 return NET_RX_DROP;
> 
> @@ -16,10 +19,14 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct
> vlan_group *grp,
> 
>         skb->skb_iif = skb->dev->ifindex;
>         __vlan_hwaccel_put_tag(skb, vlan_tci);
> -       skb->dev = vlan_group_get_device(grp, vlan_tci & VLAN_VID_MASK);
> +       vlan_id = vlan_tci & VLAN_VID_MASK;
> +       vlan_dev = vlan_group_get_device(grp, vlan_id);
> 
> -       if (!skb->dev)
> -               goto drop;
> +       if (vlan_dev)
> +               skb->dev = vlan_dev;
> +       else
> +               if (vlan_id)
> +                       goto drop;
> 
>         return (polling ? netif_receive_skb(skb) : netif_rx(skb));
> 
> @@ -82,16 +89,22 @@ vlan_gro_common(struct napi_struct *napi, struct
> vlan_group *grp,
>                 unsigned int vlan_tci, struct sk_buff *skb)
>  {
>         struct sk_buff *p;
> +       struct net_device *vlan_dev;
> +       u16 vlan_id;
> 
>         if (skb_bond_should_drop(skb, ACCESS_ONCE(skb->dev->master)))
>                 skb->deliver_no_wcard = 1;
> 
>         skb->skb_iif = skb->dev->ifindex;
>         __vlan_hwaccel_put_tag(skb, vlan_tci);
> -       skb->dev = vlan_group_get_device(grp, vlan_tci & VLAN_VID_MASK);
> -
> -       if (!skb->dev)
> -               goto drop;
> +       vlan_id = vlan_tci & VLAN_VID_MASK;
> +       vlan_dev = vlan_group_get_device(grp, vlan_id);
> +
> +       if (vlan_dev)
> +               skb->dev = vlan_dev;
> +       else
> +               if (vlan_id)
> +                       goto drop;
> 
>         for (p = napi->gro_list; p; p = p->next) {
>                 NAPI_GRO_CB(p)->same_flow =
> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> index 5298426..65512c3 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c
> @@ -142,6 +142,7 @@ int vlan_skb_recv(struct sk_buff *skb, struct net_device
> *dev,
>  {
>         struct vlan_hdr *vhdr;
>         struct vlan_rx_stats *rx_stats;
> +       struct net_device *vlan_dev;
>         u16 vlan_id;
>         u16 vlan_tci;
> 
> @@ -157,8 +158,18 @@ int vlan_skb_recv(struct sk_buff *skb, struct net_device
> *dev,
>         vlan_id = vlan_tci & VLAN_VID_MASK;
> 
>         rcu_read_lock();
> -       skb->dev = __find_vlan_dev(dev, vlan_id);
> -       if (!skb->dev) {
> +       vlan_dev = __find_vlan_dev(dev, vlan_id);
> +
> +       /* If the VLAN device is defined, we use it.
> +        * If not, and the VID is 0, it is a 802.1p packet (not
> +        * really a VLAN), so we will just netif_rx it later to the
> +        * original interface, but with the skb->proto set to the
> +        * wrapped proto: we do nothing here.
> +        */
> +
> +       if (vlan_dev) {
> +               skb->dev = vlan_dev;
> +       } else if (vlan_id) {
>                 pr_debug("%s: ERROR: No net_device for VID: %u on dev: %s\n",
>                          __func__, vlan_id, dev->name);
>                 goto err_unlock;
> 
> 
> --
> 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