On Mon, Aug 10, 2009 at 05:34:20PM -0700, Joe Eykholt wrote:
> Chris Leech wrote:
> > Add FDISC ELS handling to libfc and libfcoe, treat it the same as FLOGI
>
> In some cases it shouldn't be treated just like FLOGI. See below.
> Maybe I don't understand some stuff. Is there a fcoe_ctlr for each lport?
> If so, that's not quite right since the FCF selection is only done for
> the main N_port.
There is only one fcoe_ctlr, and FCF selection only happens once. There
end up being a few places where libfcoe interacts with individual
VN_Port lports through callbacks.
> Is the data_src MAC address different for each VN_Port?
> That seems undesirable.
It's actually necessary for FPMA addressing. Each N_Port_ID has a mapped
MAC address, and the FDISC LS_ACC is FIP encapsulated with an assigned MAC
address TLV. With SPMA addressing it's possible to use a single source
address for all VN_Ports.
> Overall, the patch set looks pretty reasonable so far.
> More comments below.
>
> > - if (dtype != FIP_DT_FLOGI)
> > + if ((dtype != FIP_DT_FLOGI) && (dtype != FIP_DT_FDISC))
>
> Please don't add the extra parens. Comparisons have precedence over &&.
I'll clean this up.
> > memcpy(mac->fd_mac, fip->data_src_addr, ETH_ALEN);
> > else if (fip->spma)
> > memcpy(mac->fd_mac, fip->ctl_src_addr, ETH_ALEN);
> > @@ -481,7 +481,7 @@ int fcoe_ctlr_els_send(struct fcoe_ctlr *fip, struct
> > sk_buff *skb)
> > fh = (struct fc_frame_header *)skb->data;
> > op = *(u8 *)(fh + 1);
> >
> > - if (op == ELS_FLOGI) {
> > + if (op == ELS_FLOGI || op == ELS_FDISC) {
>
> Maybe we don't want to set flogi_oxid for FDISC?
> Maybe the FLOGI/FDISC accepts should give callbacks from libfc instead
> of having FIP track it here. I also have issues related to
> this for point-to-point mode where the FC-ID assignment is
> supposed to come from PLOGI. That's another problem, but another
> justification for having calls from libfc's lport state machine
> instead of trying to capture the ACC for FLOGIs and FDISCs in
> libfcoe.c. The problem is, those callbacks need MAC address info
> that isn't available to libfc. More thought needed here or maybe
> you have it handled and I haven't seen it yet.
I get to this in one of the later patches. The big issue I ran into is
knowing which VN_Port an FDISC LS_ACC is for. I used the exchange
callback to hook FCoE specific handling into the receive path, allowing
the exchange lookup to find the correct lport. I also stored the
assigned MAC address in the skb->cb, which I admit is a little ugly.
> > old_xid = fip->flogi_oxid;
> > fip->flogi_oxid = ntohs(fh->fh_ox_id);
> > if (fip->state == FIP_ST_AUTO) {
> > @@ -866,8 +866,8 @@ static void fcoe_ctlr_recv_els(struct fcoe_ctlr *fip,
> > struct sk_buff *skb)
> > goto drop;
> > els_op = *(u8 *)(fh + 1);
> >
> > - if (els_dtype == FIP_DT_FLOGI && sub == FIP_SC_REP &&
> > - fip->flogi_oxid == ntohs(fh->fh_ox_id) &&
> > + if ((els_dtype == FIP_DT_FLOGI || els_dtype == FIP_DT_FDISC) &&
> > + sub == FIP_SC_REP && fip->flogi_oxid == ntohs(fh->fh_ox_id) &&
> > els_op == ELS_LS_ACC && is_valid_ether_addr(granted_mac)) {
>
> This block doesn't seem right for FDISC, either. Here we're assigning
> the data_src_addr but that's just for the main lport, right?
>
> > fip->flogi_oxid = FC_XID_UNKNOWN;
> > fip->update_mac(fip, fip->data_src_addr, granted_mac);
> > @@ -1289,7 +1289,8 @@ int fcoe_ctlr_recv_flogi(struct fcoe_ctlr *fip,
> > struct fc_frame *fp, u8 *sa)
> > spin_unlock_bh(&fip->lock);
> >
> > fip->update_mac(fip, mac, fip->data_src_addr);
> > - } else if (op == ELS_FLOGI && fh->fh_r_ctl == FC_RCTL_ELS_REQ && sa) {
> > + } else if ((op == ELS_FLOGI || op == ELS_FDISC) &&
> > + fh->fh_r_ctl == FC_RCTL_ELS_REQ && sa) {
>
> This is for incoming FLOGI requests, which means point-to-point mode.
> We won't be handling incoming FDISCs here. It was right before.
I'll look at this more closely, there are certainly some places that
handle FLOGI that don't need to handle FDISC.
Thanks for the review, and I'll take care of resolving conflicts as
things get merged into fcoe-next.
- Chris
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
