On Wed, 2010-06-16 at 17:19 -0700, Joe Eykholt wrote:
> On 6/16/10 4:37 PM, Vasu Dev wrote:
> > The fcoe_ctlr_link_down() called from fcoe_disable puts
> > lport into reset state and in that state following link up
> > event starts lport again though fcoemon had it disabled
> > until it is re-enabled again from fcoe_enable.
> >
> > This patch disables fip controller along with fcoe_ctlr_link_down
> > and then don't start lport on link up events while controller
> > is disabled. Instead start lport on fcoe_enable followed
> > by fcoe_disable to get controller out of disable and start
> > lport again.
> >
> > The fc_fabric_logoff and fc_fabric_login is redundant
> > after added fcoe_ctlr_link_down/up, therefore removed
> > them and with that lport gets into reset state as soon
> > as fcoe_disable issued to allow re-queuing timed out IOs
> > while lport blocks rports using lport state in
> > fc_io_compl.
> >
> > Signed-off-by: Vasu Dev<[email protected]>
> > ---
> >
> > drivers/scsi/fcoe/fcoe.c | 6 +++---
> > drivers/scsi/fcoe/libfcoe.c | 2 ++
> > 2 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> > index cf66e3c..dfffac2 100644
> > --- a/drivers/scsi/fcoe/fcoe.c
> > +++ b/drivers/scsi/fcoe/fcoe.c
> > @@ -1938,7 +1938,7 @@ static int fcoe_disable(const char *buffer, struct
> > kernel_param *kp)
> > rtnl_unlock();
> >
> > if (fcoe) {
> > - fc_fabric_logoff(fcoe->ctlr.lp);
> > + fcoe->ctlr.mode = FIP_ST_DISABLED;
>
> This conflicts with my VN2VN mode patches. The mode should only be
> set at initialization time and only by fcoe_ctlr_init().
>
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.
> > 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.
> > 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.
> 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.
Thanks
Vasu
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
