Stefan Rompf <[EMAIL PROTECTED]> writes:
> +++ linux-2.6.14/net/core/link_watch.c
> @@ -75,7 +75,14 @@ void linkwatch_run_queue(void)
> clear_bit(__LINK_STATE_LINKWATCH_PENDING, &dev->state);
>
> if (dev->flags & IFF_UP) {
> - if (netif_carrier_ok(dev)) {
> + if (netif_get_kernel_operstate(dev) < OPER_UP &&
> + dev->operstate_useroverride ==
> OPERU_UP_UNTIL_DORMANT) {
> + write_lock_bh(&dev_base_lock);
> + dev->operstate_useroverride = OPERU_DORMANT;
> + write_unlock_bh(&dev_base_lock);
Why do you do that instead of atomic write?
The locking works differently - you don't need to protect a single write
(because it can be done atomically), you need to protect fragments of code
that read the value and then change it depending on the value and/or on
other factors (probably not the best description, you may want to seek
a better one).
Such as (example only, probably not exactly correct here):
*lock(xx);
if (netif_get_kernel_operstate(dev) < OPER_UP &&
dev->operstate_useroverride == OPERU_UP_UNTIL_DORMANT) {
dev->operstate_useroverride = OPERU_DORMANT;
}
...
*unlock(xx);
If you don't protect the "critical section" with a lock, the kernel may
change the value in different place (with another CPU, preempt etc.)
before you subsequently alter it again here and the first change (event)
could be lost.
> +void netif_set_kernel_operstate(struct net_device *dev, unsigned char
> newstate) {
> + char oldstate = dev->operstate_kernel;
> + if (oldstate != newstate) {
> + dev->operstate_kernel = newstate;
> + smp_wmb();
> + linkwatch_fire_event(dev);
> + }
> +}
Still not sure any wmb() is needed. Unless I'm mistaken that's for
synchronizing I/O mainly (and to prevent gcc reordering but don't see
any possible here). atomic_set() maybe? But it doesn't make all real
locking issues go away.
> @@ -561,6 +561,9 @@ static int vlan_device_event(struct noti
> struct vlan_group *grp = __vlan_find_group(dev->ifindex);
> int i, flgs;
> struct net_device *vlandev;
> + unsigned char ostate = netif_get_operstate(dev);
> +
> + if (ostate == OPER_DOWN) ostate = OPER_LOWERLAYERDOWN;
>
> if (!grp)
> goto out;
> @@ -578,13 +581,7 @@ static int vlan_device_event(struct noti
> 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);
> - }
> + netif_set_kernel_operstate(vlandev, ostate);
This probably isn't a problem as that's the only writer and calls
are serialized (you don't want admin status - contrary to what SNMP
says - in the operstate, and nothing changes carrier state).
But real world (WLAN, WAN) is different, there are at least 2 writers
even without admin status here.
But if we do that, we should move admin status here as well I think
(and probably get rid of __LIST_STATE_* completely). Nothing more to lose
and at least SNMP consistency to gain (not only that).
--
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
