Re: Repost: Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
From: Patrick McHardy <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> Applied, and I will queue this up for -stable. Thanks everyone. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Repost: Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
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 <[EMAIL PROTECTED]> - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Repost: Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
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 <[EMAIL PROTECTED]> --- linux-2.6.17/net/8021q/vlan.c.orig 2006-07-07 13:00:56.0 +0200 +++ linux-2.6.17/net/8021q/vlan.c 2006-07-11 23:20:32.0 +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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Repost: Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
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 <[EMAIL PROTECTED]> --- linux-2.6.17/net/8021q/vlan.c.orig 2006-07-07 13:00:56.0 +0200 +++ linux-2.6.17/net/8021q/vlan.c 2006-07-11 23:20:32.0 +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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
Stephen Hemminger wrote:
On Sun, 9 Jul 2006 10:49:31 +0200
Stefan Rompf <[EMAIL PROTECTED]> 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=11520078264&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 <[EMAIL PROTECTED]>
Candela Technologies Inc http://www.candelatech.com
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
On Sun, 9 Jul 2006 10:49:31 +0200
Stefan Rompf <[EMAIL PROTECTED]> 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=11520078264&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 <[EMAIL PROTECTED]>
Quis custodiet ipsos custodes?
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
Stefan Rompf <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
David Miller <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
[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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
From: Krzysztof Halasa <[EMAIL PROTECTED]> 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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
David Miller <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
From: Stefan Rompf <[EMAIL PROTECTED]> 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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
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=11520078264&r=1&w=2 Stefan - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
On Fri, 07 Jul 2006 11:56:28 +0200 Patrick McHardy <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> Quis custodiet ipsos custodes? - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
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?
[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 <[EMAIL PROTECTED]>
---
commit 87d93ab26dd0c29d3f6dd3cddfd4eeea21c139f8
tree 9e3777ce697fa4c09f814967f53cb0bd142ff92c
parent 4c2d0d9de3da2b2d420d91dd654ecf1551e24eca
author Patrick McHardy <[EMAIL PROTECTED]> Thu, 06 Jul 2006 09:37:33 +0200
committer Patrick McHardy <[EMAIL PROTECTED]> 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()
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 <[EMAIL PROTECTED]> Candela Technologies Inc http://www.candelatech.com - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
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? - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
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 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [VLAN]: translate IF_OPER_DORMANT to netif_dormant_on()
> commit ddd7bf9fe4e59afc0a041378f82b6e1aa88f714b > tree 98764adba1bae7d128d2e7db7d9fc1e2fe5826d8 > parent b00055aacdb172c05067612278ba27265fcd05ce > author Stefan Rompf <[EMAIL PROTECTED]> Tue, 21 Mar 2006 09:11:41 -0800 > committer David S. Miller <[EMAIL PROTECTED]> 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? - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] vlan: translate IF_OPER_DORMANT to netif_dormant_on()
From: Stefan Rompf <[EMAIL PROTECTED]> Date: Thu, 8 Dec 2005 00:19:56 +0100 > this patch adds support to the VLAN driver to translate IF_OPER_DORMANT of > the > underlying device to netif_dormant_on(). Beside clean state forwarding, this > allows running independant userspace supplicants on both the real device and > the stacked VLAN. It depends on my RFC2863 patch. Also applied to net-2.6.17 Thanks. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] vlan: translate IF_OPER_DORMANT to netif_dormant_on()
Hi David,
this patch adds support to the VLAN driver to translate IF_OPER_DORMANT of the
underlying device to netif_dormant_on(). Beside clean state forwarding, this
allows running independant userspace supplicants on both the real device and
the stacked VLAN. It depends on my RFC2863 patch.
Please apply.
Jamal:
> There is only one gotcha i noticed with vlan - as you keep stacking you
> set the iflink; shouldnt you do the opposite when the last of stacked on
> netdevices disappear, wouldnt the iflink end up pointing to -1 or maybe
> even ifindex of the vlan?
I'm setting iflink on the VLAN device, not on the device the VLAN is stacked
on (as it happens in the two ;-) other places that use iflink)
Signed-Off-By: Stefan Rompf <[EMAIL PROTECTED]>
diff -X dontdiff -uNrp linux-2.6.14/net/8021q/vlan.c linux-2.6.14-rfc2863/net/8021q/vlan.c
--- linux-2.6.14/net/8021q/vlan.c 2005-11-02 11:07:35.0 +0100
+++ linux-2.6.14-rfc2863/net/8021q/vlan.c 2005-11-30 22:48:49.0 +0100
@@ -68,7 +68,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. */
@@ -343,6 +343,26 @@ static void vlan_setup(struct net_device
new_dev->do_ioctl = vlan_dev_ioctl;
}
+static void vlan_transfer_operstate(const struct net_device *dev, struct net_device *vlandev)
+{
+ /* Have to respect userspace enforced dormant state
+ * of real device, also must allow supplicant running
+ * on VLAN device
+ */
+ if (dev->operstate == IF_OPER_DORMANT)
+ netif_dormant_on(vlandev);
+ else
+ netif_dormant_off(vlandev);
+
+ if (netif_carrier_ok(dev)) {
+ if (!netif_carrier_ok(vlandev))
+ netif_carrier_on(vlandev);
+ } else {
+ if (netif_carrier_ok(vlandev))
+ netif_carrier_off(vlandev);
+ }
+}
+
/* Attach a VLAN device to a mac address (ie Ethernet Card).
* Returns the device that was created, or NULL if there was
* an error of some kind.
@@ -449,7 +469,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.
@@ -497,6 +517,10 @@ static struct net_device *register_vlan_
if (register_netdevice(new_dev))
goto out_free_newdev;
+ new_dev->iflink = real_dev->ifindex;
+ vlan_transfer_operstate(real_dev, new_dev);
+ linkwatch_fire_event(new_dev); /* _MUST_ call rfc2863_policy() */
+
/* So, got the sucker initialized, now lets place
* it into our local structure.
*/
@@ -572,25 +596,12 @@ 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 (netif_carrier_ok(dev)) {
-if (!netif_carrier_ok(vlandev))
- netif_carrier_on(vlandev);
- } else {
-if (netif_carrier_ok(vlandev))
- netif_carrier_off(vlandev);
- }
-
- if ((vlandev->state & VLAN_LINK_STATE_MASK) != flgs) {
-vlandev->state = (vlandev->state &~ VLAN_LINK_STATE_MASK)
- | flgs;
-netdev_state_change(vlandev);
- }
+ vlan_transfer_operstate(dev, vlandev);
}
break;
