On 6/17/10 6:05 PM, Vasu Dev wrote:
> On Thu, 2010-06-17 at 16:21 -0700, Joe Eykholt wrote:
>>
>> Yes. I didn't mean to imply that what you did wouldn't work. It
>> will,
>> it's just that it doesn't match my plan with VN2VN. With VN2VN,
>> the mode can't change after init.
>>
>
> Thanks for clarification and that is issue anymore as I'm removing mode
> from this patch.
>
>>> OK, I'll take this out, I haven't looked at VN2VN patches in detail
>> but
>>> I guess its similar current use in FNIC to set non FIP mode will get
>>> fixed with VN2VN patches.
>>
>> I'd really appreciate your review after I send out the next batch.
>
> I'll be glad to take a look beside Rob's review.
>
>> I hope it cleans up a few things as well as adds that new feature.
>>
>
> Thats good.
>
>>>>> fcoe_ctlr_link_down(&fcoe->ctlr);
>>>>> } else
>>>>> rc = -ENODEV;
>>>>> @@ -1993,9 +1993,9 @@ static int fcoe_enable(const char *buffer,
>> struct kernel_param *kp)
>>>>> rtnl_unlock();
>>>>>
>>>>> if (fcoe) {
>>>>> - if (!fcoe_link_ok(fcoe->ctlr.lp))
>>>>> + fcoe->ctlr.mode = FIP_ST_AUTO;
>>>>> + if (!fcoe_link_ok(fcoe->ctlr.lp))
>>>>> fcoe_ctlr_link_up(&fcoe->ctlr);
>>>>> - rc = fc_fabric_login(fcoe->ctlr.lp);
>>>>> } else
>>>>> rc = -ENODEV;
>>>>>
>>>>> diff --git a/drivers/scsi/fcoe/libfcoe.c
>> b/drivers/scsi/fcoe/libfcoe.c
>>>>> index f009191..6fd9dbb 100644
>>>>> --- a/drivers/scsi/fcoe/libfcoe.c
>>>>> +++ b/drivers/scsi/fcoe/libfcoe.c
>>>>> @@ -255,6 +255,8 @@ static void fcoe_ctlr_solicit(struct fcoe_ctlr
>> *fip, struct fcoe_fcf *fcf)
>>>>> */
>>>>> void fcoe_ctlr_link_up(struct fcoe_ctlr *fip)
>>>>> {
>>>>> + if (fip->mode == FIP_ST_DISABLED)
>>>>> + return;
>>>>
>>>> No, it should reflect the link change up to libfc.
>>>>
>>>
>>> Currently we proceed with link-up change only under certain
>> fip->state
>>> conditions and use of mode overlaps with fip->state as FIP FSM moves
>> to
>>> fip->mode state from FIP_ST_LINK_WAIT (BTW that is always AUTO in
>> case
>>> of fcoe.ko). So adding one more condition based on mode should be
>> okay
>>> just as state is checked to proceed with link-up.
>>>
>>> I added this is to allow disabled link to come-up only after enable
>> to
>>> be more symmetric with disable but otherwise I don't see any issue
>> on
>>> lport going online here from fcoe_disable, therefore I'll take all
>> mode
>>> changes out.
>>
>> It makes sense as the code is now, and would work, but I prefer to
>> have libfcoe reflect the link up for HBA-API but offline when the
>> login
>> can't occur.
>>
>>>>> spin_lock_bh(&fip->lock);
>>>>> if (fip->state == FIP_ST_NON_FIP || fip->state == FIP_ST_AUTO)
>> {
>>>>> spin_unlock_bh(&fip->lock);
>>>>>
>>>>> _______________________________________________
>>>>
>>>>
>>>> The purpose of fc_fabric_logoff/login() are to reflect the
>> enable/disable
>>>> mode up to libfc, so if there's something wrong where a linkup
>> while disabled
>>>> via fc_fabric_logoff() causes us to login, it should be fixed in
>> libfc.
>>>>
>>>
>>> It had only fc_fabric_logoff/login() but later changed to also have
>> link
>>> down/up for all good reasons as described in that commit:-
>>>
>>> commit 9ee50e48d8370dbcb42fa5b62b5bb3a9877e1f47
>>> Author: Chris Leech<[email protected]>
>>> Date: Fri Apr 9 14:22:23 2010 -0700
>>>
>>> [SCSI] fcoe: reset FIP ctlr link state on disable/enable
>>>
>>> The FIP controler state wasn't being reset on a disable.
>>> A disable/enable sequence should be treated as a link event.
>>> Otherwise, when using disable to mask a time when the link
>>> is up but unusable, FCF discovery would attempt to continue
>>> and login would jump directly to the non-FIP fallback on
>>> enable.
>>>
>>> I don't think we need both and having only link down/up is
>> sufficient
>>> for current enable/disable use with DCB operational state of the
>> link,
>>> so if DCB is not operational then it would be as if link is down and
>>> probably some switch won't respond to logoff in that state anyway.
>>
>> It seems like we want to distinguish so we know what the problem
>> is ...
>> that the interface is disabled vs. not having a link.
>>
>
> The HBA-API status as "Linkdown" on disable will be more helpful to
> easily figure out that DCB link is not up for only fcoe traffic as eth
> interface would be still UP in that case to easily suggest that only
> fcoe link is down, other choice could be "Offline" from limited
> available HBA-API status but it could be offlined due to other fabric
> side issues also. i.e. no flogi respone or FCF not available etc.
> Perhaps both are not good match for disable or I'm missing some other
> better match. So leaving it as it is linkdown looks ok to me and anyway
> disable is internally used by fcoemon not exposed to any user tools
> (fcc/fcoeadm).
>
>>>> I think we need a enable/disable interface to libfcoe so that it
>>>> can take care of putting itself in disabled state. So maybe there
>> should
>>>> be a fcoe_ctlr_login/logoff interface that takes care of
>> starting/stopping FIP
>>>> and also calls fc_fabric_login/logoff.
>>>>
>>>
>>> Make sense to move to libfcoe so that all fcoe.ko or its equivalent
>> can
>>> use it.
>>
>> That would be nice.
>>
>
> This move would require fcoemon changes and I like to hold off this
> till /fcoe/sysfs work which seems to getting ready and that may also
> help in eliminating module param moving to libfcoe module.
I see. I was thinking of leaving the parameters as the were (in fcoe)
so fcoemon wouldn't need to change, but moving the rest of the
enable/disable code into libfcoe. We could do both, I suppose,
where the enable/disable parameters could be in both places for a while.
But anyway, I think your patch below accomplishes what you want and is
fine with me.
> For now, I like to clean up redundant logoff/login in this patch and
> that also helps in getting lport out of ready state quickly in DCB link
> down/fcoe_disable handling to re-queue timed out IOs while rport getting
> blocked on disable.
>
> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> index cf66e3c..ba486bb 100644
> --- a/drivers/scsi/fcoe/fcoe.c
> +++ b/drivers/scsi/fcoe/fcoe.c
> @@ -1938,8 +1938,8 @@ static int fcoe_disable(const char *buffer, struct
> kernel_param *kp)
> rtnl_unlock();
>
> if (fcoe) {
> - fc_fabric_logoff(fcoe->ctlr.lp);
> fcoe_ctlr_link_down(&fcoe->ctlr);
> + fcoe_clean_pending_queue(fcoe->ctlr.lp);
> } else
> rc = -ENODEV;
>
> @@ -1992,11 +1992,9 @@ static int fcoe_enable(const char *buffer, struct
> kernel_param *kp)
> fcoe = fcoe_hostlist_lookup_port(netdev);
> rtnl_unlock();
>
> - if (fcoe) {
> - if (!fcoe_link_ok(fcoe->ctlr.lp))
> - fcoe_ctlr_link_up(&fcoe->ctlr);
> - rc = fc_fabric_login(fcoe->ctlr.lp);
> - } else
> + if (fcoe && !fcoe_link_ok(fcoe->ctlr.lp))
> + fcoe_ctlr_link_up(&fcoe->ctlr);
> + else
> rc = -ENODEV;
>
>
> Thanks as always for good review.
> Vasu
You're welcome.
Cheers,
Joe
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
