Vasu Dev wrote:
> 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.
OK, but then every LLD needs to do that if there's a desire to have
user enable/disable (not just fcoemon). fnic doesn't need fcoemon,
but maybe user enable/disable would be nice. I guess that can wait, though.
What you have is OK with me.
>>> 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.
My point was that after the link flap occurs, DCBX will restablish PFC
and then set enabled, which may not be what the user wanted. Basically,
with fcoemon, the user needs to tell fcoemon what's wanted, and not
directly use /sys.
> 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 mentioned this because I added an enabled flag before along with my rport
changes, the goal was to not re-login after fabric_logoff was called
while shutting down. Robert suggested I use states instead, and I
added the disabled state to cover that.
> 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.
If that's the case, then the fcoe can go away as soon as you drop the rtnl_lock,
and you have a bug anyway. NETDEV_UNREGISTER queues a work_struct which
gets the fcoe_config_mutex. I verified that fcoe_config_mutex is always
held when the list is changed. Let me know if you still think I missed
something.
Joe
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
