From e6c10b7c5e0e7c6ecf7d60f502c7eeaf3361b48f Mon Sep 17 00:00:00 2001 From: Krishna Mohan Date: Fri, 1 Mar 2013 11:35:31 +0000 Subject: libfcoe: Fix Conflicting FCFs issue in the fabric When multiple FCFs in use, and first FIP Advertisement received is with "Available for Login" i.e A bit set to 0, FCF selection will fail. The fix is to remove the assumption in the code that first FCF is only allowed selectable FCF. Consider the scenario fip->fcfs contains FCF1(fabricname X, marked A=0) FCF2(fabricname Y, marked A=1). list_first_entry(first) points to FCF1 and 1st iteration we ignore the FCF and on 2nd iteration we compare FCF1 & FCF2 fabric name and we fails to perform FCF selection. Signed-off-by: Krishna Mohan Reviewed-by: Bhanu Prakash Gollapudi Signed-off-by: Robert Love --- drivers/scsi/fcoe/fcoe_ctlr.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) (limited to 'drivers') diff --git a/drivers/scsi/fcoe/fcoe_ctlr.c b/drivers/scsi/fcoe/fcoe_ctlr.c index a76247201be5..4c7764181b79 100644 --- a/drivers/scsi/fcoe/fcoe_ctlr.c +++ b/drivers/scsi/fcoe/fcoe_ctlr.c @@ -1548,9 +1548,6 @@ static struct fcoe_fcf *fcoe_ctlr_select(struct fcoe_ctlr *fip) { struct fcoe_fcf *fcf; struct fcoe_fcf *best = fip->sel_fcf; - struct fcoe_fcf *first; - - first = list_first_entry(&fip->fcfs, struct fcoe_fcf, list); list_for_each_entry(fcf, &fip->fcfs, list) { LIBFCOE_FIP_DBG(fip, "consider FCF fab %16.16llx " @@ -1568,17 +1565,15 @@ static struct fcoe_fcf *fcoe_ctlr_select(struct fcoe_ctlr *fip) "" : "un"); continue; } - if (fcf->fabric_name != first->fabric_name || - fcf->vfid != first->vfid || - fcf->fc_map != first->fc_map) { + if (!best || fcf->pri < best->pri || best->flogi_sent) + best = fcf; + if (fcf->fabric_name != best->fabric_name || + fcf->vfid != best->vfid || + fcf->fc_map != best->fc_map) { LIBFCOE_FIP_DBG(fip, "Conflicting fabric, VFID, " "or FC-MAP\n"); return NULL; } - if (fcf->flogi_sent) - continue; - if (!best || fcf->pri < best->pri || best->flogi_sent) - best = fcf; } fip->sel_fcf = best; if (best) { -- cgit v1.2.3 From 732bdb9d141879b1b5b357f934553fe827c1f46b Mon Sep 17 00:00:00 2001 From: Mark Rustad Date: Wed, 20 Mar 2013 07:17:55 +0000 Subject: libfc: Correct check for initiator role The service_params field is being checked against the symbol FC_RPORT_ROLE_FCP_INITIATOR where it really should be checked against FCP_SPPF_INIT_FCN. Signed-off-by: Mark Rustad Tested-by: Jack Morgan Signed-off-by: Robert Love --- drivers/scsi/libfc/fc_rport.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers') diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c index d518d17e940f..6bbb9447b75d 100644 --- a/drivers/scsi/libfc/fc_rport.c +++ b/drivers/scsi/libfc/fc_rport.c @@ -1962,7 +1962,7 @@ static int fc_rport_fcp_prli(struct fc_rport_priv *rdata, u32 spp_len, rdata->flags |= FC_RP_FLAGS_RETRY; rdata->supported_classes = FC_COS_CLASS3; - if (!(lport->service_params & FC_RPORT_ROLE_FCP_INITIATOR)) + if (!(lport->service_params & FCP_SPPF_INIT_FCN)) return 0; spp->spp_flags |= rspp->spp_flags & FC_SPP_EST_IMG_PAIR; -- cgit v1.2.3 From fb00cc2353ca22b3278f72d73e65a33486d1dbc7 Mon Sep 17 00:00:00 2001 From: Neil Horman Date: Fri, 3 May 2013 19:34:15 +0000 Subject: libfc: extend ex_lock to protect all of fc_seq_send This warning was reported recently: WARNING: at drivers/scsi/libfc/fc_exch.c:478 fc_seq_send+0x14f/0x160 [libfc]() (Not tainted) Hardware name: ProLiant DL120 G7 Modules linked in: tcm_fc target_core_iblock target_core_file target_core_pscsi target_core_mod configfs dm_round_robin dm_multipath 8021q garp stp llc bnx2fc cnic uio fcoe libfcoe libfc scsi_transport_fc scsi_tgt autofs4 sunrpc pcc_cpufreq ipv6 hpilo hpwdt e1000e microcode iTCO_wdt iTCO_vendor_support serio_raw shpchp ixgbe dca mdio sg ext4 mbcache jbd2 sd_mod crc_t10dif pata_acpi ata_generic ata_piix hpsa dm_mirror dm_region_hash dm_log dm_mod [last unloaded: scsi_wait_scan] Pid: 5464, comm: target_completi Not tainted 2.6.32-272.el6.x86_64 #1 Call Trace: [] ? warn_slowpath_common+0x87/0xc0 [] ? warn_slowpath_null+0x1a/0x20 [] ? fc_seq_send+0x14f/0x160 [libfc] [] ? ft_queue_status+0x16e/0x210 [tcm_fc] [] ? target_complete_ok_work+0x0/0x4b0 [target_core_mod] [] ? target_complete_ok_work+0x106/0x4b0 [target_core_mod] [] ? target_complete_ok_work+0x0/0x4b0 [target_core_mod] [] ? worker_thread+0x170/0x2a0 [] ? autoremove_wake_function+0x0/0x40 [] ? worker_thread+0x0/0x2a0 [] ? kthread+0x96/0xa0 [] ? child_rip+0xa/0x20 [] ? kthread+0x0/0xa0 [] ? child_rip+0x0/0x20 It occurs because fc_seq_send can have multiple contexts executing within it at the same time, and fc_seq_send doesn't consistently use the ep->ex_lock that protects this structure. Because of that, its possible for one context to clear the INIT bit in the ep->esb_state field while another checks it, leading to the above stack trace generated by the WARN_ON in the function. We should probably undertake the effort to convert access to the fc_exch structures to use rcu, but that a larger work item. To just fix this specific issue, we can just extend the ex_lock protection through the entire fc_seq_send path Signed-off-by: Neil Horman Reported-by: Gris Ge CC: Robert Love Signed-off-by: Robert Love --- drivers/scsi/libfc/fc_exch.c | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) (limited to 'drivers') diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c index c772d8d27159..8b928c67e4b9 100644 --- a/drivers/scsi/libfc/fc_exch.c +++ b/drivers/scsi/libfc/fc_exch.c @@ -463,13 +463,7 @@ static void fc_exch_delete(struct fc_exch *ep) fc_exch_release(ep); /* drop hold for exch in mp */ } -/** - * fc_seq_send() - Send a frame using existing sequence/exchange pair - * @lport: The local port that the exchange will be sent on - * @sp: The sequence to be sent - * @fp: The frame to be sent on the exchange - */ -static int fc_seq_send(struct fc_lport *lport, struct fc_seq *sp, +static int fc_seq_send_locked(struct fc_lport *lport, struct fc_seq *sp, struct fc_frame *fp) { struct fc_exch *ep; @@ -479,7 +473,7 @@ static int fc_seq_send(struct fc_lport *lport, struct fc_seq *sp, u8 fh_type = fh->fh_type; ep = fc_seq_exch(sp); - WARN_ON((ep->esb_stat & ESB_ST_SEQ_INIT) != ESB_ST_SEQ_INIT); + WARN_ON(!(ep->esb_stat & ESB_ST_SEQ_INIT)); f_ctl = ntoh24(fh->fh_f_ctl); fc_exch_setup_hdr(ep, fp, f_ctl); @@ -502,17 +496,34 @@ static int fc_seq_send(struct fc_lport *lport, struct fc_seq *sp, error = lport->tt.frame_send(lport, fp); if (fh_type == FC_TYPE_BLS) - return error; + goto out; /* * Update the exchange and sequence flags, * assuming all frames for the sequence have been sent. * We can only be called to send once for each sequence. */ - spin_lock_bh(&ep->ex_lock); ep->f_ctl = f_ctl & ~FC_FC_FIRST_SEQ; /* not first seq */ if (f_ctl & FC_FC_SEQ_INIT) ep->esb_stat &= ~ESB_ST_SEQ_INIT; +out: + return error; +} + +/** + * fc_seq_send() - Send a frame using existing sequence/exchange pair + * @lport: The local port that the exchange will be sent on + * @sp: The sequence to be sent + * @fp: The frame to be sent on the exchange + */ +static int fc_seq_send(struct fc_lport *lport, struct fc_seq *sp, + struct fc_frame *fp) +{ + struct fc_exch *ep; + int error; + ep = fc_seq_exch(sp); + spin_lock_bh(&ep->ex_lock); + error = fc_seq_send_locked(lport, sp, fp); spin_unlock_bh(&ep->ex_lock); return error; } @@ -629,7 +640,7 @@ static int fc_exch_abort_locked(struct fc_exch *ep, if (fp) { fc_fill_fc_hdr(fp, FC_RCTL_BA_ABTS, ep->did, ep->sid, FC_TYPE_BLS, FC_FC_END_SEQ | FC_FC_SEQ_INIT, 0); - error = fc_seq_send(ep->lp, sp, fp); + error = fc_seq_send_locked(ep->lp, sp, fp); } else error = -ENOBUFS; return error; @@ -1132,7 +1143,7 @@ static void fc_seq_send_last(struct fc_seq *sp, struct fc_frame *fp, f_ctl = FC_FC_LAST_SEQ | FC_FC_END_SEQ | FC_FC_SEQ_INIT; f_ctl |= ep->f_ctl; fc_fill_fc_hdr(fp, rctl, ep->did, ep->sid, fh_type, f_ctl, 0); - fc_seq_send(ep->lp, sp, fp); + fc_seq_send_locked(ep->lp, sp, fp); } /** @@ -1307,8 +1318,8 @@ static void fc_exch_recv_abts(struct fc_exch *ep, struct fc_frame *rx_fp) ap->ba_low_seq_cnt = htons(sp->cnt); } sp = fc_seq_start_next_locked(sp); - spin_unlock_bh(&ep->ex_lock); fc_seq_send_last(sp, fp, FC_RCTL_BA_ACC, FC_TYPE_BLS); + spin_unlock_bh(&ep->ex_lock); fc_frame_free(rx_fp); return; -- cgit v1.2.3