Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[Patrick McHardy]

> commit ddd7bf9fe4e59afc0a041378f82b6e1aa88f714b
> tree 98764adba1bae7d128d2e7db7d9fc1e2fe5826d8
> parent b00055aacdb172c05067612278ba27265fcd05ce
> author Stefan Rompf <stefan@loplof.de> Tue, 21 Mar 2006 09:11:41 -0800
> committer David S. Miller <davem@davemloft.net> Tue, 21 Mar 2006 09:11:41 -0800
> 
> [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
>
> diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
> index fa76220..3948949 100644
> --- a/net/8021q/vlan.c
> +++ b/net/8021q/vlan.c
> @@ -69,7 +69,7 @@ static struct packet_type vlan_packet_ty
>  
>  /* Bits of netdev state that are propagated from real device to virtual */
>  #define VLAN_LINK_STATE_MASK \
> -	((1<<__LINK_STATE_PRESENT)|(1<<__LINK_STATE_NOCARRIER))
> +	((1<<__LINK_STATE_PRESENT)|(1<<__LINK_STATE_NOCARRIER)|(1<<__LINK_STATE_DORMANT))
>  
>  /* End of global variables definitions. */
>  
> @@ -450,7 +470,7 @@ static struct net_device *register_vlan_
>  	new_dev->flags = real_dev->flags;
>  	new_dev->flags &= ~IFF_UP;
>  
> -	new_dev->state = real_dev->state & VLAN_LINK_STATE_MASK;
> +	new_dev->state = real_dev->state & ~(1<<__LINK_STATE_START);
>  
>  	/* need 4 bytes for extra VLAN header info,
>  	 * hope the underlying device can handle it.

This introduced a regression by propagating the __LINK_STATE_XOFF flag,
when the queue of the underlying device is stopped it will be stopped
for the VLAN device too and never be woken up. Since you changed
VLAN_LINK_STATE_MASK, I assume the intention was to just add
__LINK_STATE_DORMANT to the propagated flags and keep using it here?

Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[Stefan Rompf]

Am Dienstag 04 Juli 2006 12:07 schrieb Patrick McHardy:

> > -	new_dev->state = real_dev->state & VLAN_LINK_STATE_MASK;
> > +	new_dev->state = real_dev->state & ~(1<<__LINK_STATE_START);

> This introduced a regression by propagating the __LINK_STATE_XOFF flag,
> when the queue of the underlying device is stopped it will be stopped
> for the VLAN device too and never be woken up. Since you changed
> VLAN_LINK_STATE_MASK, I assume the intention was to just add
> __LINK_STATE_DORMANT to the propagated flags and keep using it here?

Hm, I did not hit that bug during tests, even though starfire calls 
netif_stop_queue() on close. But I don't remember whether I tested added 
VLANs while the main interface was ifconfig'ed down.

Anyway, is it good to propagate __LINK_STATE_PRESENT then? The same situation 
here, add a VLAN while the main interface is "not present", and you are out. 
Can you try to revert the quoted part of my patch, I'll rethink which flags 
should be copied on device creation.

Stefan

Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[Patrick McHardy]

Stefan Rompf wrote:
> Am Dienstag 04 Juli 2006 12:07 schrieb Patrick McHardy:
> 
> 
>>>-	new_dev->state = real_dev->state & VLAN_LINK_STATE_MASK;
>>>+	new_dev->state = real_dev->state & ~(1<<__LINK_STATE_START);
> 
> 
>>This introduced a regression by propagating the __LINK_STATE_XOFF flag,
>>when the queue of the underlying device is stopped it will be stopped
>>for the VLAN device too and never be woken up. Since you changed
>>VLAN_LINK_STATE_MASK, I assume the intention was to just add
>>__LINK_STATE_DORMANT to the propagated flags and keep using it here?
> 
> 
> Hm, I did not hit that bug during tests, even though starfire calls 
> netif_stop_queue() on close. But I don't remember whether I tested added 
> VLANs while the main interface was ifconfig'ed down.

It hits me whenever I boot with sky2, it seems to need a while before
a carrier is detected.

> Anyway, is it good to propagate __LINK_STATE_PRESENT then? The same situation 
> here, add a VLAN while the main interface is "not present", and you are out. 
> Can you try to revert the quoted part of my patch, I'll rethink which flags 
> should be copied on device creation.

I tried both adding LINK_STATE_XOFF to the negated flags and using
VLAN_LINK_STATE_MASK, both as expected solve the problem for me.
I have to admit I was wondering about LINK_STATE_PRESENT as well
(was going to complain about that too until I noticed it is also
set in VLAN_LINK_STATE_MASK). Maybe Ben can tell us the idea behind
this?

Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[Ben Greear]

Patrick McHardy wrote:
> Stefan Rompf wrote:
> 
>>Am Dienstag 04 Juli 2006 12:07 schrieb Patrick McHardy:
>>
>>
>>
>>>>-	new_dev->state = real_dev->state & VLAN_LINK_STATE_MASK;
>>>>+	new_dev->state = real_dev->state & ~(1<<__LINK_STATE_START);

This change looks funky because it ignores the link state mask.

>>Anyway, is it good to propagate __LINK_STATE_PRESENT then? The same situation 
>>here, add a VLAN while the main interface is "not present", and you are out. 
>>Can you try to revert the quoted part of my patch, I'll rethink which flags 
>>should be copied on device creation.
> 
> 
> I tried both adding LINK_STATE_XOFF to the negated flags and using
> VLAN_LINK_STATE_MASK, both as expected solve the problem for me.
> I have to admit I was wondering about LINK_STATE_PRESENT as well
> (was going to complain about that too until I noticed it is also
> set in VLAN_LINK_STATE_MASK). Maybe Ben can tell us the idea behind
> this?

I believe this link-state logic was added by someone else.  I'm not
sure exactly what these flags are supposed to do, so I am not sure if they
should be propagated to the VLAN or not.

Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 1309 bytes --]

Ben Greear wrote:
> Patrick McHardy wrote:
> 
>> Stefan Rompf wrote:
>>
>>> Anyway, is it good to propagate __LINK_STATE_PRESENT then? The same
>>> situation here, add a VLAN while the main interface is "not present",
>>> and you are out. Can you try to revert the quoted part of my patch,
>>> I'll rethink which flags should be copied on device creation.
>>
>>
>> I tried both adding LINK_STATE_XOFF to the negated flags and using
>> VLAN_LINK_STATE_MASK, both as expected solve the problem for me.
>> I have to admit I was wondering about LINK_STATE_PRESENT as well
>> (was going to complain about that too until I noticed it is also
>> set in VLAN_LINK_STATE_MASK). Maybe Ben can tell us the idea behind
>> this?
> 
> 
> I believe this link-state logic was added by someone else.  I'm not
> sure exactly what these flags are supposed to do, so I am not sure if they
> should be propagated to the VLAN or not.

I looked into this. The present flag used to get propagated from the
real device until this patch, presumably to make sure no operations
on the vlan device will be passed through to the underlying device
when it is not present. This patch should take care both of this
problem and the problem of propagating __LINK_STATE_XOFF without
ever clearing it again. Stefan, does this look right to you?


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 2333 bytes --]

[VLAN]: Fix link state propagation

When the queue of the underlying device is stopped at initialization time
or the device is marked "not present", the state will be propagated to the
vlan device and never change. The queue state doesn't need to be propagated
at all and shouldn't be without using netif_wake_queue/netif_stop_queue,
the present state needs to be kept up to date by the NETDEV_CHANGE
notifier.

Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit 87d93ab26dd0c29d3f6dd3cddfd4eeea21c139f8
tree 9e3777ce697fa4c09f814967f53cb0bd142ff92c
parent 4c2d0d9de3da2b2d420d91dd654ecf1551e24eca
author Patrick McHardy <kaber@trash.net> Thu, 06 Jul 2006 09:37:33 +0200
committer Patrick McHardy <kaber@trash.net> Thu, 06 Jul 2006 09:37:33 +0200

 net/8021q/vlan.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 458031b..8b26227 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -68,8 +68,9 @@ static struct packet_type vlan_packet_ty
 };
 
 /* Bits of netdev state that are propagated from real device to virtual */
-#define VLAN_LINK_STATE_MASK \
-	((1<<__LINK_STATE_PRESENT)|(1<<__LINK_STATE_NOCARRIER)|(1<<__LINK_STATE_DORMANT))
+#define VLAN_LINK_STATE_MASK ((1<<__LINK_STATE_PRESENT))
+#define VLAN_LINK_STATE_INITIAL_MASK \
+	(VLAN_LINK_STATE_MASK | (1<<__LINK_STATE_NOCARRIER) | (1<<__LINK_STATE_DORMANT))
 
 /* End of global variables definitions. */
 
@@ -479,7 +480,7 @@ #endif
 	new_dev->flags = real_dev->flags;
 	new_dev->flags &= ~IFF_UP;
 
-	new_dev->state = real_dev->state & ~(1<<__LINK_STATE_START);
+	new_dev->state = real_dev->state & VLAN_LINK_STATE_INITIAL_MASK;
 
 	/* need 4 bytes for extra VLAN header info,
 	 * hope the underlying device can handle it.
@@ -608,11 +609,17 @@ static int vlan_device_event(struct noti
 	switch (event) {
 	case NETDEV_CHANGE:
 		/* Propagate real device state to vlan devices */
+		flgs = dev->state & VLAN_LINK_STATE_MASK;
 		for (i = 0; i < VLAN_GROUP_ARRAY_LEN; i++) {
 			vlandev = grp->vlan_devices[i];
 			if (!vlandev)
 				continue;
 
+			if ((vlandev->state & VLAN_LINK_STATE_MASK) != flgs) {
+				vlandev->state &= ~VLAN_LINK_STATE_MASK;
+				vlandev->state |= flgs;
+				netdev_state_change(vlandev);
+			}
 			vlan_transfer_operstate(dev, vlandev);
 		}
 		break;

Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[Stefan Rompf]

Am Donnerstag 06 Juli 2006 09:42 schrieb Patrick McHardy:

> > I believe this link-state logic was added by someone else.  I'm not
> > sure exactly what these flags are supposed to do, so I am not sure if
> > they should be propagated to the VLAN or not.
>
> I looked into this. The present flag used to get propagated from the
> real device until this patch, presumably to make sure no operations
> on the vlan device will be passed through to the underlying device
> when it is not present.

The present flag is changed by netif_device_attach() and 
netif_device_detach(), and these functions do not emit a 
netdev_state_change() afterwards. So there is a good chance that 
vlan_device_event() won't be called and cannot transfer the flag. 
netif_device_detach() also sets __LINK_STATE_XOFF implicitely. 

Ok, let's see who cares for netif_device_present():

-SIOCSIFMAP, dev->set_config() (change media type)
-dev_open()
-dev_set_mtu()
-dev_set_mac_address()
-dev_watchdog()
 ->not implemented by VLAN / does not call through to underlying device

-multicast ioctls
 ->calls dev_mc_upload() of the underlying device sooner or later, this 
function checks whether the device is present or not. However, if you change 
the multicast list on a VLAN while the real device is not present, 
dev_mc_upload() won't be called on netif_device_attach(). Good thing is that 
most drivers setup multicast list after attach. Fishy.

-several private ioctls
 ->vlan_dev_ioctl() checks whether the real device is present before passing 
an ioctl

So I'd rather drop the __LINK_STATE_PRESENT transfer part, because not 
guaranteed to be called anyway and mostly unneeded. Ok, let me look through 
the history now to find who added transferring it (hope this happened after 
the bitkeeper->git move)

Stefan

Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[Patrick McHardy]

Stefan Rompf wrote:
> Am Donnerstag 06 Juli 2006 09:42 schrieb Patrick McHardy:
> 
> 
>>>I believe this link-state logic was added by someone else.  I'm not
>>>sure exactly what these flags are supposed to do, so I am not sure if
>>>they should be propagated to the VLAN or not.
>>
>>I looked into this. The present flag used to get propagated from the
>>real device until this patch, presumably to make sure no operations
>>on the vlan device will be passed through to the underlying device
>>when it is not present.
> 
> 
> The present flag is changed by netif_device_attach() and 
> netif_device_detach(), and these functions do not emit a 
> netdev_state_change() afterwards. So there is a good chance that 
> vlan_device_event() won't be called and cannot transfer the flag. 
> netif_device_detach() also sets __LINK_STATE_XOFF implicitely. 

True.

> Ok, let's see who cares for netif_device_present():
> 
> -SIOCSIFMAP, dev->set_config() (change media type)
> -dev_open()
> -dev_set_mtu()
> -dev_set_mac_address()
> -dev_watchdog()
>  ->not implemented by VLAN / does not call through to underlying device
>
> -multicast ioctls
>  ->calls dev_mc_upload() of the underlying device sooner or later, this 
> function checks whether the device is present or not. However, if you change 
> the multicast list on a VLAN while the real device is not present, 
> dev_mc_upload() won't be called on netif_device_attach(). Good thing is that 
> most drivers setup multicast list after attach. Fishy.
> 
> -several private ioctls
>  ->vlan_dev_ioctl() checks whether the real device is present before passing 
> an ioctl
> 
> So I'd rather drop the __LINK_STATE_PRESENT transfer part, because not 
> guaranteed to be called anyway and mostly unneeded. Ok, let me look through 
> the history now to find who added transferring it (hope this happened after 
> the bitkeeper->git move)

I tend to agree with you, it doesn't seem to work properly. It was
introduced by Stephen (before the move), lets hope he can tell us more.

Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[Stephen Hemminger]

On Fri, 07 Jul 2006 11:56:28 +0200
Patrick McHardy <kaber@trash.net> wrote:

> Stefan Rompf wrote:
> > Am Donnerstag 06 Juli 2006 09:42 schrieb Patrick McHardy:
> > 
> > 
> >>>I believe this link-state logic was added by someone else.  I'm not
> >>>sure exactly what these flags are supposed to do, so I am not sure if
> >>>they should be propagated to the VLAN or not.
> >>
> >>I looked into this. The present flag used to get propagated from the
> >>real device until this patch, presumably to make sure no operations
> >>on the vlan device will be passed through to the underlying device
> >>when it is not present.
> > 
> > 
> > The present flag is changed by netif_device_attach() and 
> > netif_device_detach(), and these functions do not emit a 
> > netdev_state_change() afterwards. So there is a good chance that 
> > vlan_device_event() won't be called and cannot transfer the flag. 
> > netif_device_detach() also sets __LINK_STATE_XOFF implicitely. 
> 
> True.
> 
> > Ok, let's see who cares for netif_device_present():
> > 
> > -SIOCSIFMAP, dev->set_config() (change media type)
> > -dev_open()
> > -dev_set_mtu()
> > -dev_set_mac_address()
> > -dev_watchdog()
> >  ->not implemented by VLAN / does not call through to underlying device
> >
> > -multicast ioctls
> >  ->calls dev_mc_upload() of the underlying device sooner or later, this 
> > function checks whether the device is present or not. However, if you change 
> > the multicast list on a VLAN while the real device is not present, 
> > dev_mc_upload() won't be called on netif_device_attach(). Good thing is that 
> > most drivers setup multicast list after attach. Fishy.
> > 
> > -several private ioctls
> >  ->vlan_dev_ioctl() checks whether the real device is present before passing 
> > an ioctl
> > 
> > So I'd rather drop the __LINK_STATE_PRESENT transfer part, because not 
> > guaranteed to be called anyway and mostly unneeded. Ok, let me look through 
> > the history now to find who added transferring it (hope this happened after 
> > the bitkeeper->git move)
> 
> I tend to agree with you, it doesn't seem to work properly. It was
> introduced by Stephen (before the move), lets hope he can tell us more.
> 

Not really. The flag code last major change was to do the dormant
stuff that HDLC wanted.

IMHO VLAN device's should mirror the state of the underlying physical
device.

I can't really follow the thread well. What is broken?



-- 
Stephen Hemminger <shemminger@osdl.org>
Quis custodiet ipsos custodes?

Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[Stefan Rompf]

Am Freitag, 7. Juli 2006 23:33 schrieb Stephen Hemminger:

> Not really. The flag code last major change was to do the dormant
> stuff that HDLC wanted.

... and where the maintainer doesn't seem to care to use it now that the 
infrastructure is there. Sigh.

> IMHO VLAN device's should mirror the state of the underlying physical
> device.
>
> I can't really follow the thread well. What is broken?

The thread is still quite short and describes the problem, look at 
http://marc.theaimsgroup.com/?t=115200782600004&r=1&w=2

Stefan

Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[David Miller]

From: Stefan Rompf <stefan@loplof.de>
Date: Sun, 9 Jul 2006 10:49:31 +0200

> Am Freitag, 7. Juli 2006 23:33 schrieb Stephen Hemminger:
> 
> > Not really. The flag code last major change was to do the dormant
> > stuff that HDLC wanted.
> 
> ... and where the maintainer doesn't seem to care to use it now that the 
> infrastructure is there. Sigh.

Yes, this is very unfortunate.  They made the loudest noise about
wanting the change, yet they aren't even responsible enough to submit
the HDLC patches necessary to make use of it.  This was apparently
needed to fix HDLC, so where's the followup?

This is why the HDLC "maintainers" shouldn't be surprised if we flat
out ignore them the next time they complain about anything.  They
have proven that they are pure whiners.

Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[Krzysztof Halasa]

David Miller <davem@davemloft.net> writes:

>> > Not really. The flag code last major change was to do the dormant
>> > stuff that HDLC wanted.
>> 
>> ... and where the maintainer doesn't seem to care to use it now that the 
>> infrastructure is there. Sigh.

This is very different from what I proposed and doesn't fit very well.
We've been discussing this to death.

> Yes, this is very unfortunate.  They made the loudest noise about
> wanting the change, yet they aren't even responsible enough to submit
> the HDLC patches necessary to make use of it.  This was apparently
> needed to fix HDLC, so where's the followup?

I'm still thinking how to use it safely. Should it be implemented
sanely, things would be different.

> This is why the HDLC "maintainers" shouldn't be surprised if we flat
> out ignore them the next time they complain about anything.

You are correct - some time ago, I was really surprised. Now, especially
having received that private mail from you - the situation is obvious.

I'm a single developer BTW.
-- 
Krzysztof Halasa

Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[David Miller]

From: Krzysztof Halasa <khc@pm.waw.pl>
Date: Sun, 09 Jul 2006 22:05:43 +0200

> I'm a single developer BTW.

So am I, and I've been keeping the core networking and the sparc64
port afloat for more than 10 years.

In comparison, very little is being asked of you.

Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[Stefan Rompf]

[Trimmed CC list as we're off topic]

Am Sonntag, 9. Juli 2006 22:05 schrieb Krzysztof Halasa:

> >> ... and where the maintainer doesn't seem to care to use it now that the
> >> infrastructure is there. Sigh.
>
> This is very different from what I proposed and doesn't fit very well.
> We've been discussing this to death.

You've been asking for two independant flags of which one does not stop the 
queue. You've got two independant flags of which one does not stop the queue. 
If you're claiming now that this doesn't fit well you either did not bother 
to look at the result or you are outrigthly lying. End of story.

Stefan

Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[Krzysztof Halasa]

David Miller <davem@davemloft.net> writes:

>> I'm a single developer BTW.
>
> So am I, and I've been keeping the core networking and the sparc64
> port afloat for more than 10 years.

I was just referring to your use of plural form.

I don't know about you, but I'm doing Linux (and other related) work
in my free time, and things like feeding my family have priority.
I was under impression this is common and acceptable.
-- 
Krzysztof Halasa

Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[Krzysztof Halasa]

Stefan Rompf <stefan@loplof.de> writes:

> You've been asking for two independant flags of which one does not stop the 
> queue.

Actually I asked for only one flag which can be set independently of
others, and which would be visible to userspace. I provided a patch
as well. It didn't break anything. I provided a sample of code
showing usage of the flag. I still have Message-Ids and the actual
messages so don't hesitate to ask if you want to see that again.

Then we had that long discussion with you and Jamal and, I admit,
I said "pass".

> You've got two independant flags of which one does not stop the queue.

Is it ok to set that flag without synchronization with other flags?
I.e, from within another module and without using cross-module locks,
as I've shown at the time? Just asking, I don't know what the final
conclusion was.

I.e., is it ok if the hardware module does netif_carrier_on/off()
(for example, from its IRQ handler) and if the protocol module does
netif_dormant_on/off() independently (for example, from its timer
or linkwatch)?

If it's ok then I'll be happy to implement the support in my drivers
ASAP (this uncertainty was, in fact, the main problem). That should
also mean others things I have on queue (blocked by this issue) would
go upstream.
-- 
Krzysztof Halasa

Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[Stephen Hemminger]

On Sun, 9 Jul 2006 10:49:31 +0200
Stefan Rompf <stefan@loplof.de> wrote:

> Am Freitag, 7. Juli 2006 23:33 schrieb Stephen Hemminger:
> 
> > Not really. The flag code last major change was to do the dormant
> > stuff that HDLC wanted.
> 
> ... and where the maintainer doesn't seem to care to use it now that the 
> infrastructure is there. Sigh.
> 
> > IMHO VLAN device's should mirror the state of the underlying physical
> > device.
> >
> > I can't really follow the thread well. What is broken?
> 
> The thread is still quite short and describes the problem, look at 
> http://marc.theaimsgroup.com/?t=115200782600004&r=1&w=2
> 
> Stefan


Okay, going back to the original problem, before the current round
of name calling. This bug shows a lot of problems with the existing
VLAN device state management.

1. I think vlan code should never be using the state bits directly at all.
It makes the code error prone if the bits ever change, and it means
that the proper callbacks are not being done. The existing vlan_transfer_operstate
does what you want. VLAN_LINK_STATE_MASK etc, should go.

2. The vlan device should not be marked as up when it
is registered.  Couldn't it wait for the user to bring it up?
If you want to automagically bring the device up, you need to call
dev_open() so that all the proper notifier's get called.

3. All checks for IFF_UP should be using netif_running instead.
IFF_UP is a leftover BSDism.

Instead of:

int vlan_dev_open(struct net_device *dev)
{
	if (!(VLAN_DEV_INFO(dev)->real_dev->flags & IFF_UP))
		return -ENETDOWN;

	return 0;
}

Use:
int vlan_dev_open(struct net_device *dev)
{
	return netif_running(VLAN_DEV_INFO(dev)->real_dev) ? 0 : -ENETDOWN;
}

-- 
Stephen Hemminger <shemminger@osdl.org>
Quis custodiet ipsos custodes?

Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[Ben Greear]

Stephen Hemminger wrote:
> On Sun, 9 Jul 2006 10:49:31 +0200
> Stefan Rompf <stefan@loplof.de> wrote:
> 
> 
>>Am Freitag, 7. Juli 2006 23:33 schrieb Stephen Hemminger:
>>
>>
>>>Not really. The flag code last major change was to do the dormant
>>>stuff that HDLC wanted.
>>
>>... and where the maintainer doesn't seem to care to use it now that the 
>>infrastructure is there. Sigh.
>>
>>
>>>IMHO VLAN device's should mirror the state of the underlying physical
>>>device.
>>>
>>>I can't really follow the thread well. What is broken?
>>
>>The thread is still quite short and describes the problem, look at 
>>http://marc.theaimsgroup.com/?t=115200782600004&r=1&w=2
>>
>>Stefan
> 
> 
> 
> Okay, going back to the original problem, before the current round
> of name calling. This bug shows a lot of problems with the existing
> VLAN device state management.
> 
> 1. I think vlan code should never be using the state bits directly at all.
> It makes the code error prone if the bits ever change, and it means
> that the proper callbacks are not being done. The existing vlan_transfer_operstate
> does what you want. VLAN_LINK_STATE_MASK etc, should go.
> 
> 2. The vlan device should not be marked as up when it
> is registered.  Couldn't it wait for the user to bring it up?
> If you want to automagically bring the device up, you need to call
> dev_open() so that all the proper notifier's get called.

This sounds good to me.

> 3. All checks for IFF_UP should be using netif_running instead.
> IFF_UP is a leftover BSDism.

That also sounds good.

> 
> Instead of:
> 
> int vlan_dev_open(struct net_device *dev)
> {
> 	if (!(VLAN_DEV_INFO(dev)->real_dev->flags & IFF_UP))
> 		return -ENETDOWN;
> 
> 	return 0;
> }
> 
> Use:
> int vlan_dev_open(struct net_device *dev)
> {
> 	return netif_running(VLAN_DEV_INFO(dev)->real_dev) ? 0 : -ENETDOWN;
> }
> 


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[Stefan Rompf]

Am Montag, 10. Juli 2006 14:01 schrieb Krzysztof Halasa:

> > You've got two independant flags of which one does not stop the queue.
>
> Is it ok to set that flag without synchronization with other flags?
> I.e, from within another module and without using cross-module locks,
> as I've shown at the time? Just asking, I don't know what the final
> conclusion was.
>
> I.e., is it ok if the hardware module does netif_carrier_on/off()
> (for example, from its IRQ handler) and if the protocol module does
> netif_dormant_on/off() independently (for example, from its timer
> or linkwatch)?

Yes, you can read and write these flags independantly. For the details, look 
at Documentation/networking/operstates.txt in the 2.6.17 tree.

Stefan

Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[Stefan Rompf]

Am Montag, 10. Juli 2006 18:56 schrieb Stephen Hemminger:

> 1. I think vlan code should never be using the state bits directly at all.
> It makes the code error prone if the bits ever change, and it means
> that the proper callbacks are not being done. The existing
> vlan_transfer_operstate does what you want. VLAN_LINK_STATE_MASK etc,
> should go.

I just realized why 2.6.16 explicitely transfers LINK_STATE_PRESENT. This flag 
is positive true, and the code just assumes that it is always set in 
real_dev:

 new_dev->state = real_dev->state & VLAN_LINK_STATE_INITIAL_MASK;

So I think the fix for 2.6.17-stable should be:

-       new_dev->state = real_dev->state & ~(1<<__LINK_STATE_START);
+       new_dev->state = real_dev->state & (1<<__LINK_STATE_NOCARRIER) | 
(1<<__LINK_STATE_DORMANT)) | (1<<__LINK_STATE_PRESENT); , dropping 
VLAN_LINK_STATE_INITIAL_MASK.

I can produce and test such a patch tomorrow evening, if someone needs it 
faster, feel free to go ahead ;-)

For the rest of your comment and 

> 3. All checks for IFF_UP should be using netif_running instead.
> IFF_UP is a leftover BSDism.

ACK. However,

> 2. The vlan device should not be marked as up when it
> is registered.

this is a userspace visible API change I don't like, but you are right it 
should use dev_open(). 

I would take responsibility to implement this on one of the next two weekends. 
Should be 2.6.19 stuff IMHO.

Stefan

[RFC] vlan handling of up/down

[Stephen Hemminger]

Untested, but here is the basic idea of how I think up/down should be
handled.  Basically, rather than changing the flags directly the VLAN code
should call dev_open/dev_close.  The notifier end's up recursively calling
but this is okay.


--- vlan.orig/net/8021q/vlan.c
+++ vlan/net/8021q/vlan.c
@@ -427,7 +427,7 @@ static struct net_device *register_vlan_
 	/* The real device must be up and operating in order to
 	 * assosciate a VLAN device with it.
 	 */
-	if (!(real_dev->flags & IFF_UP))
+	if (!netif_running(real_dev))
 		goto out_unlock;
 
 	if (__find_vlan_dev(real_dev, VLAN_ID) != NULL) {
@@ -476,10 +476,7 @@ static struct net_device *register_vlan_
 	printk(VLAN_DBG "Allocated new name -:%s:-\n", new_dev->name);
 #endif
 	/* IFF_BROADCAST|IFF_MULTICAST; ??? */
-	new_dev->flags = real_dev->flags;
-	new_dev->flags &= ~IFF_UP;
-
-	new_dev->state = real_dev->state & ~(1<<__LINK_STATE_START);
+	new_dev->flags = real_dev->flags & ~IFF_UP;
 
 	/* need 4 bytes for extra VLAN header info,
 	 * hope the underlying device can handle it.
@@ -566,6 +563,9 @@ static struct net_device *register_vlan_
 	if (real_dev->features & NETIF_F_HW_VLAN_FILTER)
 		real_dev->vlan_rx_add_vid(real_dev, VLAN_ID);
 
+	/* Real device is up so bring up the vlan */
+	dev_open(new_dev);
+
 	rtnl_unlock();
 
 
@@ -624,11 +624,7 @@ static int vlan_device_event(struct noti
 			if (!vlandev)
 				continue;
 
-			flgs = vlandev->flags;
-			if (!(flgs & IFF_UP))
-				continue;
-
-			dev_change_flags(vlandev, flgs & ~IFF_UP);
+			dev_close(vlandev);
 		}
 		break;
 
@@ -638,12 +634,8 @@ static int vlan_device_event(struct noti
 			vlandev = grp->vlan_devices[i];
 			if (!vlandev)
 				continue;
-				
-			flgs = vlandev->flags;
-			if (flgs & IFF_UP)
-				continue;
 
-			dev_change_flags(vlandev, flgs | IFF_UP);
+			dev_open(vlandev);
 		}
 		break;
 		


-- 
Stephen Hemminger <shemminger@osdl.org>
Quis custodiet ipsos custodes?

Re: [RFC] vlan handling of up/down

[Ben Greear]

Stephen Hemminger wrote:
> Untested, but here is the basic idea of how I think up/down should be
> handled.  Basically, rather than changing the flags directly the VLAN code
> should call dev_open/dev_close.  The notifier end's up recursively calling
> but this is okay.
> 
> 
> --- vlan.orig/net/8021q/vlan.c
> +++ vlan/net/8021q/vlan.c
> @@ -427,7 +427,7 @@ static struct net_device *register_vlan_
>  	/* The real device must be up and operating in order to
>  	 * assosciate a VLAN device with it.
>  	 */
> -	if (!(real_dev->flags & IFF_UP))
> +	if (!netif_running(real_dev))
>  		goto out_unlock;

I can't remember why I thought this check was a good idea..but is there
any reason to keep it in?  I mean, why not allow attaching VLANs to a
'down' device?

>  	if (__find_vlan_dev(real_dev, VLAN_ID) != NULL) {
> @@ -476,10 +476,7 @@ static struct net_device *register_vlan_
>  	printk(VLAN_DBG "Allocated new name -:%s:-\n", new_dev->name);
>  #endif
>  	/* IFF_BROADCAST|IFF_MULTICAST; ??? */
> -	new_dev->flags = real_dev->flags;
> -	new_dev->flags &= ~IFF_UP;
> -
> -	new_dev->state = real_dev->state & ~(1<<__LINK_STATE_START);
> +	new_dev->flags = real_dev->flags & ~IFF_UP;
>  
>  	/* need 4 bytes for extra VLAN header info,
>  	 * hope the underlying device can handle it.
> @@ -566,6 +563,9 @@ static struct net_device *register_vlan_
>  	if (real_dev->features & NETIF_F_HW_VLAN_FILTER)
>  		real_dev->vlan_rx_add_vid(real_dev, VLAN_ID);
>  
> +	/* Real device is up so bring up the vlan */
> +	dev_open(new_dev);
> +
>  	rtnl_unlock();

Assuming we take out the 'is-up' check above, this would need
a check added here (if real-dev is up, then bring up vlan, else leave it down.)

> @@ -624,11 +624,7 @@ static int vlan_device_event(struct noti
>  			if (!vlandev)
>  				continue;
>  
> -			flgs = vlandev->flags;
> -			if (!(flgs & IFF_UP))
> -				continue;
> -
> -			dev_change_flags(vlandev, flgs & ~IFF_UP);
> +			dev_close(vlandev);
>  		}
>  		break;
>  
> @@ -638,12 +634,8 @@ static int vlan_device_event(struct noti
>  			vlandev = grp->vlan_devices[i];
>  			if (!vlandev)
>  				continue;
> -				
> -			flgs = vlandev->flags;
> -			if (flgs & IFF_UP)
> -				continue;
>  
> -			dev_change_flags(vlandev, flgs | IFF_UP);
> +			dev_open(vlandev);
>  		}
>  		break;

Although it may be too late to change this (don't want to change
user-visible behaviour), I don't really like that bouncing the
real-dev could cause the VLANs to come up, even if previously
user-space had forced them down.

Perhaps a piece of state in the vlan dev could be added to remember
the 'desired state', and then only bring back up interfaces that are
desired-up when the real-dev comes back online....

Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[Stefan Rompf]

Ok,

the following patch should fix the problem. Patrick, can you give it a
try? As the bug did not affect me through my testing, I want to be sure it
works now. This is stuff for 2.6.18 and 2.6.17-stable.

Stefan


[VLAN]: Fix link state propagation

When the queue of the underlying device is stopped at initialization time
or the device is marked "not present", the state will be propagated to the
vlan device and never change. This also fixes VLAN devices being wrongly
registered as admin up since 2.6.17. Based on an analysis by Patrick
McHardy.

Signed-off-by: Stefan Rompf <stefan@loplof.de>

--- linux-2.6.17/net/8021q/vlan.c.orig	2006-07-07 13:00:56.000000000 +0200
+++ linux-2.6.17/net/8021q/vlan.c	2006-07-11 23:20:32.000000000 +0200
@@ -67,10 +67,6 @@ static struct packet_type vlan_packet_ty
 	.func = vlan_skb_recv, /* VLAN receive method */
 };
 
-/* Bits of netdev state that are propagated from real device to virtual */
-#define VLAN_LINK_STATE_MASK \
-	((1<<__LINK_STATE_PRESENT)|(1<<__LINK_STATE_NOCARRIER)|(1<<__LINK_STATE_DORMANT))
-
 /* End of global variables definitions. */
 
 /*
@@ -470,7 +466,9 @@ static struct net_device *register_vlan_
 	new_dev->flags = real_dev->flags;
 	new_dev->flags &= ~IFF_UP;
 
-	new_dev->state = real_dev->state & ~(1<<__LINK_STATE_START);
+	new_dev->state = (real_dev->state & ((1<<__LINK_STATE_NOCARRIER) |
+					     (1<<__LINK_STATE_DORMANT))) |
+			 (1<<__LINK_STATE_PRESENT); 
 
 	/* need 4 bytes for extra VLAN header info,
 	 * hope the underlying device can handle it.

Repost: Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[Stefan Rompf]

VLAN devices did not get registered as admin up in 2.6.16 and IMHO also
not in 2.6.17. So update patch description.

Ok,

the following patch should fix the problem. Patrick, can you give it a
try? As the bug did not affect me through my testing, I want to be sure it
works now. This is stuff for 2.6.18 and 2.6.17-stable.

Stefan


[VLAN]: Fix link state propagation

When the queue of the underlying device is stopped at initialization time
or the device is marked "not present", the state will be propagated to the
vlan device and never change. Based on an analysis by Patrick McHardy.

Signed-off-by: Stefan Rompf <stefan@loplof.de>

--- linux-2.6.17/net/8021q/vlan.c.orig	2006-07-07 13:00:56.000000000 +0200
+++ linux-2.6.17/net/8021q/vlan.c	2006-07-11 23:20:32.000000000 +0200
@@ -67,10 +67,6 @@ static struct packet_type vlan_packet_ty
 	.func = vlan_skb_recv, /* VLAN receive method */
 };
 
-/* Bits of netdev state that are propagated from real device to virtual */
-#define VLAN_LINK_STATE_MASK \
-	((1<<__LINK_STATE_PRESENT)|(1<<__LINK_STATE_NOCARRIER)|(1<<__LINK_STATE_DORMANT))
-
 /* End of global variables definitions. */
 
 /*
@@ -470,7 +466,9 @@ static struct net_device *register_vlan_
 	new_dev->flags = real_dev->flags;
 	new_dev->flags &= ~IFF_UP;
 
-	new_dev->state = real_dev->state & ~(1<<__LINK_STATE_START);
+	new_dev->state = (real_dev->state & ((1<<__LINK_STATE_NOCARRIER) |
+					     (1<<__LINK_STATE_DORMANT))) |
+			 (1<<__LINK_STATE_PRESENT); 
 
 	/* need 4 bytes for extra VLAN header info,
 	 * hope the underlying device can handle it.

Re: [RFC] vlan handling of up/down

[Stefan Rompf]

Am Dienstag, 11. Juli 2006 23:28 schrieb Stephen Hemminger:

> Untested, but here is the basic idea of how I think up/down should be
> handled.  Basically, rather than changing the flags directly the VLAN code
> should call dev_open/dev_close.  The notifier end's up recursively calling
> but this is okay.

Seems false alarm. I've just tested 2.6.16(-Suse), and new VLAN devices have 
been registered as admin down. Same for 2.6.17 as even with my change 
__LINK_STATE_START was not transferred.

> -	new_dev->state = real_dev->state & ~(1<<__LINK_STATE_START);

No reason to change this behaviour, we just need to stop messing with 
new_dev->state (IMHO too late for 2.6.18, but I've already been wrong ;-)

Stefan

Re: Repost: Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[Patrick McHardy]

Stefan Rompf wrote:
> the following patch should fix the problem. Patrick, can you give it a
> try? As the bug did not affect me through my testing, I want to be sure it
> works now. This is stuff for 2.6.18 and 2.6.17-stable.

I'm on my way out the door and will be gone for a couple of days,
so its going to take me a while. But it looks fine, if you want
to test it yourself, just pull the ethernet cable before adding
a VLAN and plug it in again afterwards.

Re: Repost: Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[Patrick McHardy]

Stefan Rompf wrote:
> VLAN devices did not get registered as admin up in 2.6.16 and IMHO also
> not in 2.6.17. So update patch description.
> 
> Ok,
> 
> the following patch should fix the problem. Patrick, can you give it a
> try? As the bug did not affect me through my testing, I want to be sure it
> works now. This is stuff for 2.6.18 and 2.6.17-stable.

Sorry for the delay. Just tested by unplugging the cable from eth0,
adding a bunch of VLANs and plugging the cable again, everything
works fine.

> [VLAN]: Fix link state propagation
> 
> When the queue of the underlying device is stopped at initialization time
> or the device is marked "not present", the state will be propagated to the
> vlan device and never change. Based on an analysis by Patrick McHardy.

ACKed-by: Patrick McHardy <kaber@trash.net>

Re: Repost: Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Wed, 19 Jul 2006 14:42:35 +0200

> Stefan Rompf wrote:
> > [VLAN]: Fix link state propagation
> > 
> > When the queue of the underlying device is stopped at initialization time
> > or the device is marked "not present", the state will be propagated to the
> > vlan device and never change. Based on an analysis by Patrick McHardy.
> 
> ACKed-by: Patrick McHardy <kaber@trash.net>

Applied, and I will queue this up for -stable.
Thanks everyone.