On Tue, 2009-11-24 at 11:49 -0800, Joe Eykholt wrote:
> Joe Eykholt wrote:
> > Vasu Dev wrote:
> >> The user could temporarily disable a fcoe interface
> >> and then enable it again with this added interface,
> >> the fcoemon util can use this to temporarily disable
> >> fcoe when its DCB state is not operational.
> >
> > We could add an 'enable' attribute at the fc_host level, and
> > just write 0 or 1 to that.
> >
Yes can be done but this patch allows fcoemon to disable all VN_PORTs by
simply echoing interface name in just one call. This is specific to fcoe
and DCB case therefore having it per interface will be more simple to
use for fcoemon, otherwise with fc_host level attribute the fcoemon has
to disable each individual affected VN_PORT (including NPIV port) for
DCB link change.
> > Perhaps fcoemon should have its own /sys interface so as not to
If you alluded to added module enable/disable params then I guess
FC-bus WIP would help in moving fcoe specific attributes in common sysfs
container but till then module param is obvious choice.
> > interfere with any user-specified enable/disable. For example,
> > if the user disables it and then the link flaps, fcoemod might
> > enable it after DCB is up, conflicting with the user's wishes.
> >
Added lport->enabled avoids this, I mean added check "if (lport->link_up
&& lport->enabled)" in fc_lport_enter_reset() will ensure that link flap
won't cause lport state transition when fcoemon has set enabled=0 by
calling sysfs disable.
So added lport->enabled is similar to lport->link_up but this time to
track user enable/disable state, perhaps can be worked around by
introducing additional states in lport FSM.
I'll look into eliminating lport->enabled, perhaps by adding next state
to fc_lport_enter_reset and additional check to not transition lport on
link flap from certain states.
<snip>
> >> +
> >> + rtnl_lock();
> >> + fcoe = fcoe_hostlist_lookup_port(netdev);
> >> + if (!fcoe) {
> >> + rtnl_unlock();
> >> + rc = -ENODEV;
> >> + goto out_putdev;
> >> + }
> >> + rtnl_unlock();
>
> You could drop rtnl_lock() before the if-statement.
I'll do that.
> Actually, the list can't change because fcoe_config_mutex is held,
> so rtnl_lock isn't needed.
>
You are suggesting to add a bug here by removing rtnl lock:-) though
with very very unlikly race. In my understanding the fcoe_config_mutex
is not held on NETDEV_UNREGISTER event processing which could remove
fcoe instance under rtnl_lock.
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
