[PATCH 0/6] libfc, libfcoe and fcoe updates for scsi-misc

[PATCH 0/6] libfc, libfcoe and fcoe updates for scsi-misc

[Robert Love]

Here are a few bug fixes across libfc, libfcoe and fcoe.

---

Neil Horman (1):
      fcoe: Cleanup locking on fcoe_percpu_receive_thread

Robert Love (1):
      fcoe: Remove redundant 'less than zero' check

Vasu Dev (2):
      libfc: add exch timer debug info
      libfc: fix retries with FDMI lport states

Yi Zou (2):
      libfc: don't exch_done() on invalid sequence ptr
      libfc: fix sending REC after FCP_RESP is received


 drivers/scsi/fcoe/fcoe.c       |   18 ++++----
 drivers/scsi/fcoe/fcoe_sysfs.c |    2 -
 drivers/scsi/libfc/fc_exch.c   |   96 ++++++++++++++++++++++------------------
 drivers/scsi/libfc/fc_fcp.c    |    6 +--
 drivers/scsi/libfc/fc_lport.c  |    8 ++-
 5 files changed, 72 insertions(+), 58 deletions(-)

-- 
Thanks, //Rob

[PATCH 1/6] fcoe: Cleanup locking on fcoe_percpu_receive_thread

[Robert Love]

From: Neil Horman <nhorman@tuxdriver.com>

Noticed that we can shuffle the code around in fcoe_percpu_receive_thread a bit
and avoid taking the fcoe_rx_list lock twice per iteration.  This should improve
throughput somewhat.  With this change we take the lock, and check for new
frames in a single critical section.  Only if the list is empty do we drop the
lock and re-acquire it after being signaled to wake up.

Change Notes:
v2) did some further cleanup on the patch by replacing the 2nd call of
spin_lock/splice_init with a goto to the top of the outer loop.  This allows me
to change the inner while loop to an if conditional and remove the sencond check
of kthread_should_stop.  Based on suggestion from Vasu Dev.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Acked-by: Vasu Dev <vasu.dev@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/fcoe/fcoe.c |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index fe30b1b..656ff65 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -1851,23 +1851,25 @@ static int fcoe_percpu_receive_thread(void *arg)
 
 	set_user_nice(current, -20);
 
+retry:
 	while (!kthread_should_stop()) {
 
 		spin_lock_bh(&p->fcoe_rx_list.lock);
 		skb_queue_splice_init(&p->fcoe_rx_list, &tmp);
-		spin_unlock_bh(&p->fcoe_rx_list.lock);
-
-		while ((skb = __skb_dequeue(&tmp)) != NULL)
-			fcoe_recv_frame(skb);
 
-		spin_lock_bh(&p->fcoe_rx_list.lock);
-		if (!skb_queue_len(&p->fcoe_rx_list)) {
+		if (!skb_queue_len(&tmp)) {
 			set_current_state(TASK_INTERRUPTIBLE);
 			spin_unlock_bh(&p->fcoe_rx_list.lock);
 			schedule();
 			set_current_state(TASK_RUNNING);
-		} else
-			spin_unlock_bh(&p->fcoe_rx_list.lock);
+			goto retry;
+		}
+
+		spin_unlock_bh(&p->fcoe_rx_list.lock);
+
+		while ((skb = __skb_dequeue(&tmp)) != NULL)
+			fcoe_recv_frame(skb);
+
 	}
 	return 0;
 }

[PATCH 2/6] libfc: add exch timer debug info

[Robert Love]

From: Vasu Dev <vasu.dev@intel.com>

Add exch timeout info to have debug log with exch timeout
value to match with retries, also add debug info
on exch timer cancel.

Added common fc_exch_timer_cancel() func and grouped this
along with fc_exch_timer_set() function, so that
added debug code is not repeated.

Signed-off-by: Vasu Dev <vasu.dev@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/libfc/fc_exch.c |   96 +++++++++++++++++++++++-------------------
 1 file changed, 53 insertions(+), 43 deletions(-)

diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
index aceffad..0c8a61b 100644
--- a/drivers/scsi/libfc/fc_exch.c
+++ b/drivers/scsi/libfc/fc_exch.c
@@ -339,6 +339,52 @@ static void fc_exch_release(struct fc_exch *ep)
 }
 
 /**
+ * fc_exch_timer_cancel() - cancel exch timer
+ * @ep:		The exchange whose timer to be canceled
+ */
+static inline  void fc_exch_timer_cancel(struct fc_exch *ep)
+{
+	if (cancel_delayed_work(&ep->timeout_work)) {
+		FC_EXCH_DBG(ep, "Exchange timer canceled\n");
+		atomic_dec(&ep->ex_refcnt); /* drop hold for timer */
+	}
+}
+
+/**
+ * fc_exch_timer_set_locked() - Start a timer for an exchange w/ the
+ *				the exchange lock held
+ * @ep:		The exchange whose timer will start
+ * @timer_msec: The timeout period
+ *
+ * Used for upper level protocols to time out the exchange.
+ * The timer is cancelled when it fires or when the exchange completes.
+ */
+static inline void fc_exch_timer_set_locked(struct fc_exch *ep,
+					    unsigned int timer_msec)
+{
+	if (ep->state & (FC_EX_RST_CLEANUP | FC_EX_DONE))
+		return;
+
+	FC_EXCH_DBG(ep, "Exchange timer armed : %d msecs\n", timer_msec);
+
+	if (queue_delayed_work(fc_exch_workqueue, &ep->timeout_work,
+			       msecs_to_jiffies(timer_msec)))
+		fc_exch_hold(ep);		/* hold for timer */
+}
+
+/**
+ * fc_exch_timer_set() - Lock the exchange and set the timer
+ * @ep:		The exchange whose timer will start
+ * @timer_msec: The timeout period
+ */
+static void fc_exch_timer_set(struct fc_exch *ep, unsigned int timer_msec)
+{
+	spin_lock_bh(&ep->ex_lock);
+	fc_exch_timer_set_locked(ep, timer_msec);
+	spin_unlock_bh(&ep->ex_lock);
+}
+
+/**
  * fc_exch_done_locked() - Complete an exchange with the exchange lock held
  * @ep: The exchange that is complete
  */
@@ -359,8 +405,7 @@ static int fc_exch_done_locked(struct fc_exch *ep)
 
 	if (!(ep->esb_stat & ESB_ST_REC_QUAL)) {
 		ep->state |= FC_EX_DONE;
-		if (cancel_delayed_work(&ep->timeout_work))
-			atomic_dec(&ep->ex_refcnt); /* drop hold for timer */
+		fc_exch_timer_cancel(ep);
 		rc = 0;
 	}
 	return rc;
@@ -424,40 +469,6 @@ static void fc_exch_delete(struct fc_exch *ep)
 }
 
 /**
- * fc_exch_timer_set_locked() - Start a timer for an exchange w/ the
- *				the exchange lock held
- * @ep:		The exchange whose timer will start
- * @timer_msec: The timeout period
- *
- * Used for upper level protocols to time out the exchange.
- * The timer is cancelled when it fires or when the exchange completes.
- */
-static inline void fc_exch_timer_set_locked(struct fc_exch *ep,
-					    unsigned int timer_msec)
-{
-	if (ep->state & (FC_EX_RST_CLEANUP | FC_EX_DONE))
-		return;
-
-	FC_EXCH_DBG(ep, "Exchange timer armed\n");
-
-	if (queue_delayed_work(fc_exch_workqueue, &ep->timeout_work,
-			       msecs_to_jiffies(timer_msec)))
-		fc_exch_hold(ep);		/* hold for timer */
-}
-
-/**
- * fc_exch_timer_set() - Lock the exchange and set the timer
- * @ep:		The exchange whose timer will start
- * @timer_msec: The timeout period
- */
-static void fc_exch_timer_set(struct fc_exch *ep, unsigned int timer_msec)
-{
-	spin_lock_bh(&ep->ex_lock);
-	fc_exch_timer_set_locked(ep, timer_msec);
-	spin_unlock_bh(&ep->ex_lock);
-}
-
-/**
  * 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
@@ -1549,8 +1560,10 @@ static void fc_exch_abts_resp(struct fc_exch *ep, struct fc_frame *fp)
 	FC_EXCH_DBG(ep, "exch: BLS rctl %x - %s\n", fh->fh_r_ctl,
 		    fc_exch_rctl_name(fh->fh_r_ctl));
 
-	if (cancel_delayed_work_sync(&ep->timeout_work))
+	if (cancel_delayed_work_sync(&ep->timeout_work)) {
+		FC_EXCH_DBG(ep, "Exchange timer canceled\n");
 		fc_exch_release(ep);	/* release from pending timer hold */
+	}
 
 	spin_lock_bh(&ep->ex_lock);
 	switch (fh->fh_r_ctl) {
@@ -1737,8 +1750,7 @@ static void fc_exch_reset(struct fc_exch *ep)
 	spin_lock_bh(&ep->ex_lock);
 	fc_exch_abort_locked(ep, 0);
 	ep->state |= FC_EX_RST_CLEANUP;
-	if (cancel_delayed_work(&ep->timeout_work))
-		atomic_dec(&ep->ex_refcnt);	/* drop hold for timer */
+	fc_exch_timer_cancel(ep);
 	resp = ep->resp;
 	ep->resp = NULL;
 	if (ep->esb_stat & ESB_ST_REC_QUAL)
@@ -2133,10 +2145,8 @@ static void fc_exch_els_rrq(struct fc_frame *fp)
 		ep->esb_stat &= ~ESB_ST_REC_QUAL;
 		atomic_dec(&ep->ex_refcnt);	/* drop hold for rec qual */
 	}
-	if (ep->esb_stat & ESB_ST_COMPLETE) {
-		if (cancel_delayed_work(&ep->timeout_work))
-			atomic_dec(&ep->ex_refcnt);	/* drop timer hold */
-	}
+	if (ep->esb_stat & ESB_ST_COMPLETE)
+		fc_exch_timer_cancel(ep);
 
 	spin_unlock_bh(&ep->ex_lock);
 

[PATCH 3/6] libfc: fix retries with FDMI lport states

[Robert Love]

From: Vasu Dev <vasu.dev@intel.com>

The FC-GS-3 sepc requires to wait for least 3 times R_A_TOV per
sec 4.6.1 "If the Requesting_CT does not receive a Response
CT_IU from the Responding_CT within three times R_A_TOV,
it shall consider this to be a protocol error."

This means added four new states with management server
could add significant delay with multiple retries
on default 12 second timeout(3 * R_A_TOV), so instead
just skip these states on very first timeout on any of
these states to not stuck with states for such longer
period.

Signed-off-by: Vasu Dev <vasu.dev@intel.com>
Tested-by: Marcus Dennis <marcusx.e.dennis@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/libfc/fc_lport.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/libfc/fc_lport.c b/drivers/scsi/libfc/fc_lport.c
index c1402fb..45b24a4 100644
--- a/drivers/scsi/libfc/fc_lport.c
+++ b/drivers/scsi/libfc/fc_lport.c
@@ -1590,8 +1590,9 @@ static void fc_lport_timeout(struct work_struct *work)
 	case LPORT_ST_RPA:
 	case LPORT_ST_DHBA:
 	case LPORT_ST_DPRT:
-		fc_lport_enter_ms(lport, lport->state);
-		break;
+		FC_LPORT_DBG(lport, "Skipping lport state %s to SCR\n",
+			     fc_lport_state(lport));
+		/* fall thru */
 	case LPORT_ST_SCR:
 		fc_lport_enter_scr(lport);
 		break;

[PATCH 4/6] fcoe: Remove redundant 'less than zero' check

[Robert Love]

strtoul returns an 'unsigned long' so there is no
reason to check if the value is less than zero.

strtoul already checks for the '-' character deep
in its bowels. It will return an error if the user
has provided a negative value and fcoe_str_to_dev_loss
will return that error to its caller.

This patch fixes the following Coverity reported warning:

CID 703581 -  NO_EFFECT Unsigned compared against 0 - This
less-than-zero comparison of an unsigned value is never true. "*val < 0UL".
drivers/scsi/fcoe/fcoe_sysfs.c:105

Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/fcoe/fcoe_sysfs.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/fcoe/fcoe_sysfs.c b/drivers/scsi/fcoe/fcoe_sysfs.c
index 2bc1631..5e75168 100644
--- a/drivers/scsi/fcoe/fcoe_sysfs.c
+++ b/drivers/scsi/fcoe/fcoe_sysfs.c
@@ -102,7 +102,7 @@ static int fcoe_str_to_dev_loss(const char *buf, unsigned long *val)
 	int ret;
 
 	ret = kstrtoul(buf, 0, val);
-	if (ret || *val < 0)
+	if (ret)
 		return -EINVAL;
 	/*
 	 * Check for overflow; dev_loss_tmo is u32

[PATCH 5/6] libfc: don't exch_done() on invalid sequence ptr

[Robert Love]

From: Yi Zou <yi.zou@intel.com>

The lport_recv(), i.e., fc_lport_recv_req() may get called w/o the sequence ptr
being set in fr_seq(), particularly in the case of vn2vn mode, this may happen
if the passive fcp provider, e.g., tcm_fc, has not been registered yet.

Signed-off-by: Yi Zou <yi.zou@intel.com>
Tested-by: Ross Brattain <ross.b.brattain@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/libfc/fc_lport.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/libfc/fc_lport.c b/drivers/scsi/libfc/fc_lport.c
index 45b24a4..55f58ee 100644
--- a/drivers/scsi/libfc/fc_lport.c
+++ b/drivers/scsi/libfc/fc_lport.c
@@ -973,7 +973,8 @@ drop:
 	rcu_read_unlock();
 	FC_LPORT_DBG(lport, "dropping unexpected frame type %x\n", fh->fh_type);
 	fc_frame_free(fp);
-	lport->tt.exch_done(sp);
+	if (sp)
+		lport->tt.exch_done(sp);
 }
 
 /**

[PATCH 6/6] libfc: fix sending REC after FCP_RESP is received

[Robert Love]

From: Yi Zou <yi.zou@intel.com>

This is exposed in the case the FCP_DATA frames somehow got lost and fc_fcp got
the FCP_RSP, in fc_fcp_recv_resp(), since xfer_len is less than the expected_len
it resets the the timer to wait to 2 more jiffies in case the data frames are
already queued locally. However, for target does not support REC, it would just
send RJT w/ ELS_RJT_UNSUP. The rec response handler thus only clears the rport
flag for not doing REC later, but does not do fcp_io_complete() on the
associated fsp.

The fix is just check status of FCP_RSP being received already, i.e. using the
FC_SRB_RCV_STATUS flag, in fc_fcp_timeout before start sending REC. We should
have waited long enough if there is truely data frames queued locally.

Signed-off-by: Yi Zou <yi.zou@intel.com>
Tested-by: Ross Brattain <ross.b.brattain@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/libfc/fc_fcp.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/libfc/fc_fcp.c b/drivers/scsi/libfc/fc_fcp.c
index f735730..5c8074b 100644
--- a/drivers/scsi/libfc/fc_fcp.c
+++ b/drivers/scsi/libfc/fc_fcp.c
@@ -1372,10 +1372,10 @@ static void fc_fcp_timeout(unsigned long data)
 
 	fsp->state |= FC_SRB_FCP_PROCESSING_TMO;
 
-	if (rpriv->flags & FC_RP_FLAGS_REC_SUPPORTED)
-		fc_fcp_rec(fsp);
-	else if (fsp->state & FC_SRB_RCV_STATUS)
+	if (fsp->state & FC_SRB_RCV_STATUS)
 		fc_fcp_complete_locked(fsp);
+	else if (rpriv->flags & FC_RP_FLAGS_REC_SUPPORTED)
+		fc_fcp_rec(fsp);
 	else
 		fc_fcp_recovery(fsp, FC_TIMED_OUT);
 	fsp->state &= ~FC_SRB_FCP_PROCESSING_TMO;