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. Is the data_src MAC address different for each VN_Port? That seems undesirable. Overall, the patch set looks pretty reasonable so far. More comments below. > > Signed-off-by: Chris Leech <[email protected]> > --- > > drivers/scsi/fcoe/libfcoe.c | 11 ++++++----- > drivers/scsi/libfc/fc_lport.c | 8 ++++++-- > include/scsi/fc/fc_els.h | 3 ++- > include/scsi/fc_encode.h | 30 ++++++++++++++++++++++++++++++ > 4 files changed, 44 insertions(+), 8 deletions(-) > > diff --git a/drivers/scsi/fcoe/libfcoe.c b/drivers/scsi/fcoe/libfcoe.c > index 62a4c20..2bb4193 100644 > --- a/drivers/scsi/fcoe/libfcoe.c > +++ b/drivers/scsi/fcoe/libfcoe.c > @@ -450,7 +450,7 @@ static int fcoe_ctlr_encaps(struct fcoe_ctlr *fip, > memset(mac, 0, sizeof(mac)); > mac->fd_desc.fip_dtype = FIP_DT_MAC; > mac->fd_desc.fip_dlen = sizeof(*mac) / FIP_BPW; > - 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 &&. > 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. > 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. > /* > * Save source MAC for point-to-point responses. > */ > diff --git a/drivers/scsi/libfc/fc_lport.c b/drivers/scsi/libfc/fc_lport.c > index df555a3..a925211 100644 > --- a/drivers/scsi/libfc/fc_lport.c > +++ b/drivers/scsi/libfc/fc_lport.c > @@ -1584,6 +1584,7 @@ err: > void fc_lport_enter_flogi(struct fc_lport *lport) > { > struct fc_frame *fp; > + enum fc_els_cmd cmd; > > FC_LPORT_DBG(lport, "Entered FLOGI state from %s state\n", > fc_lport_state(lport)); > @@ -1594,9 +1595,12 @@ void fc_lport_enter_flogi(struct fc_lport *lport) > if (!fp) > return fc_lport_error(lport, fp); > > - if (!lport->tt.elsct_send(lport, NULL, fp, ELS_FLOGI, > - fc_lport_flogi_resp, lport, lport->e_d_tov)) > + cmd = lport->vn_port ? ELS_FDISC : ELS_FLOGI; > + if (!lport->tt.elsct_send(lport, NULL, fp, cmd, > + fc_lport_flogi_resp, lport, lport->e_d_tov)) { This has an easy-to-resolve conflict with my recent patch sets. The second arg is now the destination ID. > fc_lport_error(lport, fp); > + } > + > } > > /* Configure a fc_lport */ > diff --git a/include/scsi/fc/fc_els.h b/include/scsi/fc/fc_els.h > index 195ca01..b5b5ac2 100644 > --- a/include/scsi/fc/fc_els.h > +++ b/include/scsi/fc/fc_els.h > @@ -248,7 +248,8 @@ struct fc_els_csp { > /* > * sp_features > */ > -#define FC_SP_FT_CIRO 0x8000 /* continuously increasing rel. off. */ > +#define FC_SP_FT_NPIV 0x8000 /* multiple n_port id support (FLOGI) */ > +#define FC_SP_FT_CIRO 0x8000 /* continuously increasing rel off > (PLOGI) */ > #define FC_SP_FT_CLAD 0x8000 /* clean address (in FLOGI LS_ACC) */ > #define FC_SP_FT_RAND 0x4000 /* random relative offset */ > #define FC_SP_FT_VAL 0x2000 /* valid vendor version level */ > diff --git a/include/scsi/fc_encode.h b/include/scsi/fc_encode.h > index a0ff61c..ac39447 100644 > --- a/include/scsi/fc_encode.h > +++ b/include/scsi/fc_encode.h > @@ -169,6 +169,31 @@ static inline void fc_flogi_fill(struct fc_lport *lport, > struct fc_frame *fp) > sp->sp_bb_data = htons((u16) lport->mfs); > cp = &flogi->fl_cssp[3 - 1]; /* class 3 parameters */ > cp->cp_class = htons(FC_CPC_VALID | FC_CPC_SEQ); > + if (lport->does_npiv) > + sp->sp_features = htons(FC_SP_FT_NPIV); > +} > + > +/** > + * fc_fdisc_fill - Fill in a fdisc request frame. > + */ > +static inline void fc_fdisc_fill(struct fc_lport *lport, struct fc_frame *fp) > +{ > + struct fc_els_csp *sp; > + struct fc_els_cssp *cp; > + struct fc_els_flogi *fdisc; > + > + fdisc = fc_frame_payload_get(fp, sizeof(*fdisc)); > + memset(fdisc, 0, sizeof(*fdisc)); > + fdisc->fl_cmd = (u8) ELS_FDISC; > + put_unaligned_be64(lport->wwpn, &fdisc->fl_wwpn); > + put_unaligned_be64(lport->wwnn, &fdisc->fl_wwnn); > + sp = &fdisc->fl_csp; > + sp->sp_hi_ver = 0x20; > + sp->sp_lo_ver = 0x20; > + sp->sp_bb_cred = htons(10); /* this gets set by gateway */ > + sp->sp_bb_data = htons((u16) lport->mfs); > + cp = &fdisc->fl_cssp[3 - 1]; /* class 3 parameters */ > + cp->cp_class = htons(FC_CPC_VALID | FC_CPC_SEQ); > } The above could call fc_flogi_fill, and then make some changes, or perhaps have a common inline function that does the stuff in PLOGI, FLOGI, and FDISC that are the same. But, that said, I can live with it as is. > /** > @@ -264,6 +289,11 @@ static inline int fc_els_fill(struct fc_lport *lport, > struct fc_rport *rport, > *did = FC_FID_FLOGI; > break; > > + case ELS_FDISC: > + fc_fdisc_fill(lport, fp); > + *did = FC_FID_FLOGI; This also conflicts with my patches. > + break; > + > case ELS_LOGO: > fc_logo_fill(lport, fp); > *did = FC_FID_FLOGI; > > _______________________________________________ > devel mailing list > [email protected] > http://www.open-fcoe.org/mailman/listinfo/devel _______________________________________________ devel mailing list [email protected] http://www.open-fcoe.org/mailman/listinfo/devel
