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.
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
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
