Am Montag 28 November 2005 23:13 schrieb Thomas Graf:
> Looks good, it would be nice if you could separate the vlan part as a
> separate patch for later on though.
This will happen when I submit the final patch for kernel inclusion.
> The effort is nice but why do we need sysfs? Isn't netlink enough for
> you?
Well, there are already a *lot* of device attributes in sysfs (just ls
-l /sys/class/net/eth0 f.e.), so it seems politically correct to add mine,
too ;-)
> > +#define IFF_CARRIER 0x10000 /* driver signals carrier
> > */
> > +#define IFF_DORMANT 0x20000 /* driver signals dormant
> > */
>
> I would favour another name for IFF_CARRIER such as IFF_LOWERLAYER or
> something like that so we can use it to define it to be the status of
> the lower layer in general not just of underlying hardware media.
OTOH this naming is consistent to the names of the
netif_carrier_*()-functions.
> > + IF_OPER_NOTPRESENT,
>
> Why don't we use this to represent the hardware not being present?
Currently, a netdevice that is not present is removed due to the way the
device model works, so I just omitted the code for it. I can add it though.
> I did like your way of leaving gaps to ease inserting new states.
But not needed anymore since I dropped the higher/lower comparisons against
operstate.
> Nice idea although I think it's not needed. I disagree with adding a new
> feature flag just to please an RFC which is obviously incomplete in this
> regard. The purpose of LOWERLAYERDOWN proposed by the RFC is correct but
> it can easly be extended to include the hardware carrier without violating
> any rules. LOWERLAYERDOWN is a refinement of DOWN so all stacking rules
> will continue to work.
I must admit that I interpret the RFC differently here. But the question
remains, do we want to differ between LOWERLAYERDOWN and DOWN. Jamal, your
thoughts?
Too bad that I had to drop the device specific oper_state() method, it would
have avoided that flag ;-)
> > + unsigned char operstate; /* RFC2863 operstate */
>
> Why do we have to store the operstate?
userspace interaction below.
> > + unsigned char link_mode; /* mapping policy to operstate */
>
> We can't use flags for this? Do we really need a new field just to
> mark an interface to go dormant?
I'd ask the other way: Will we need further link modes in the future? Not
sure, but willing to sacrifice a byte for it that would be alignment
otherwise.
> > +void vlan_transfer_operstate(const struct net_device *dev, struct
> [...]
> Dormant state should be handled before announcing the carrier or
> UP state is leaked for a moment.
Will fix, even though it runs in process context under RTNL, locking out
linkwatch_event().
> > + vlan_transfer_operstate(VLAN_DEV_INFO(dev)->real_dev, dev);
> > + linkwatch_fire_event(dev); /* force event to setup operstate */
>
> Shouldn't this be handled by vlan_transfer_operstate() already?
Not if carrier flag or operstate of the underlying device changes while the
VLAN device is admin down. I'll check of I can solve it by calling
rfc2863_policy() outside of if (dev->flags & IFF_UP).
> > +static void set_operstate(struct net_device *dev, unsigned char
> > transition) { + unsigned char operstate = dev->operstate;
> [...]
> Now this is really racy.
Why? We have RTNL, so no other writer preparing for an update will interfere.
Before doing the actual write, we get dev_base_lock, locking out pure
readers, too. I can't see how this is racy.
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
