diff mbox

=?UTF-8?Q?vlan=5Fdev=3A=20VLAN=20=30=20should=20be=20treated?= =?UTF-8?Q?=20as=20=22no=20vlan=20tag=22=20=28=38=30=32=2E=31p=20packet=29?=

Message ID 5c6d1ac43fd8ad25661ebfba57c02174@dondevamos.com
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Pedro Garcia June 14, 2010, 4:49 p.m. UTC 
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:

I am using kernel 2.6.26 in a linux box, and I have another box in the
network using 802.1p (priority tagging, but no VLAN).

Without the 8021q module loaded in the kernel, all 802.1p packets are
silently discarded (probably as expected, as the protocol is not loaded in
the kernel).

When I load 8021q module, these packets are forwarded to the module, but
they are discarded also as VLAN 0 is not configured.

I think this should not be the default behaviour, as VLAN 0 is not really
a VLAN, so it should be treated differently.

I could define the VLAN 0 (ip link add link eth0 name eth0.dot1p type vlan
id 0), but then I have a lot of issues with the ARP table entries, as to
ping the other box, outgoing traffic goes through eth0, but incoming arp
reply ends up in eth0.dot1p. In the end this means I can not communicate
with the box using 802.1p unless I use 802.1p tagging for all traffic in
the network (the linux box and all other), which is not a must of the
spec.

I have developed a patch for vlan_dev.c which makes VLAN 0 to be just
reintroduced to netif_rx but with no VLAN tagging if VLAN 0 has not been
defined, so the default behaviour is to ignore the VLAN tagging and accept
the packet as if it was not tagged, and one can still define something
+               pr_debug("%s: INFO: VLAN 0 used as default VLAN on dev:
%s\n",
+                        __func__, dev->name);
        }
 
        skb->dev->last_rx = jiffies;

--
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

Comments

Ben Hutchings June 14, 2010, 5:02 p.m. UTC | #1 
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

Ben.
Patrick McHardy June 14, 2010, 5:11 p.m. UTC | #2 
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)
--
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
Eric Dumazet June 14, 2010, 7:12 p.m. UTC | #3 
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 !


--
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
Joe Perches June 14, 2010, 7:42 p.m. UTC | #4 
On Mon, 2010-06-14 at 19:11 +0200, Patrick McHardy wrote:
> 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.
> >> 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

Pedro, you could use git format-patch and git send-email
http://linux.yyz.us/git-howto.html
http://www.kernel.org/pub/software/scm/git/docs/git-format-patch.html

> Additionally:
> - the pr_debug statements may be useful for debugging, but are
>   a bit excessive for the final version

Patrick, what's wrong with pr_debug?
Do you prefer pr_devel?

> - Please CC the maintainer (which is me)

scripts/get_maintainer.pl 

--
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
Eric Dumazet June 14, 2010, 8:03 p.m. UTC | #5 
Le lundi 14 juin 2010 à 12:42 -0700, Joe Perches a écrit :
> On Mon, 2010-06-14 at 19:11 +0200, Patrick McHardy wrote:

> > Additionally:
> > - the pr_debug statements may be useful for debugging, but are
> >   a bit excessive for the final version
> 
> Patrick, what's wrong with pr_debug?
> Do you prefer pr_devel?

In the patch context, this pr_debug() is not necessary.

Just remove it, since its a normal situation, no need to log anything,
ever.



--
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
diff mbox

Patch

different for VLAN 0 if desired (so it is backwards compatible).

Signed-off-by: Pedro Garcia <pedro.netdev@dondevamos.com>
--- net/8021q/vlan_dev.c.orig   2008-07-13 23:51:29.000000000 +0200
+++ net/8021q/vlan_dev.c        2010-06-14 18:07:35.000000000 +0200
@@ -151,6 +151,7 @@  int vlan_skb_recv(struct sk_buff *skb, s
        struct vlan_hdr *vhdr;
        unsigned short vid;
        struct net_device_stats *stats;
+       struct net_device *vlan_dev;
        unsigned short vlan_TCI;
 
        skb = skb_share_check(skb, GFP_ATOMIC);
@@ -165,11 +166,23 @@  int vlan_skb_recv(struct sk_buff *skb, s
        vid = (vlan_TCI & VLAN_VID_MASK);
 
        rcu_read_lock();
-       skb->dev = __find_vlan_dev(dev, vid);
-       if (!skb->dev) {
+       vlan_dev = __find_vlan_dev(dev, vid);
+       if (vlan_dev) {
+               skb->dev = vlan_dev;
+       } else if (vid) {
                pr_debug("%s: ERROR: No net_device for VID: %u on dev:
%s\n",
                         __func__, (unsigned int)vid, dev->name);
                goto err_unlock;
+       } else {
+               /* 2010-06-13: Pedro Garcia
+                  The packet is VLAN tagged, but VID is 0 and the user
has
+                  not defined anything for VLAN 0, so it is a 802.1p
packet.
+                  We will just netif_rx it later to the original
interface,
+                  but with the skb->proto set to the wrapped proto, so we
do
+                  nothing here. */
+