[PATCH 00/22] FCoE VN2VN fixes

[PATCH 00/22] FCoE VN2VN fixes

[Hannes Reinecke]

Hi all,

after long and painful debugging session I finally managed to get
FCoE VN2VN running on a point-to-point connection.
There wasn't so much a single issue, but rather an entire slew
of rather annoying problems:
- rport kref handling is bonkers. Taking a kref once and releasing
  it when attempting to free the structure is just not what krefs
  are designed for.
- The FCoE stack reacts allergic to out-of-order frames. While in
  theory this shouldn't happen, it's hard to avoid with modern HW.
  Plus I even had been seeing this when using virtio. And to make
  matters worse, it would occasionally just drop any out of order
  frames, while handling them correctly in other cases.
- Failed FC REC/ABTS will cause an I/O error. There are perfectly
  legit cases where a REC might fail (eg if the exchange was already
  sent and it just got stuck in the RX queue), so it should not
  considered an error. A simple retry will be sufficient.
- FLOGI are handled ... interestingly. If both ends attempt an
  FLOGI the state machine is advanved on the incoming FLOGI
  request, so the stack gets confused when the (original) FLOGI
  ACC is being returned.

Additionally I've added lots of debugging statements and some
general cleanup.

As usual, comments and reviews are welcome.

The entire patchset can be found at
git://git.kernel.org/hare/scsi-devel/h/vn2vn

Hannes Reinecke (22):
  libfc: Revisit kref handling
  libfc: additional debugging messages
  fcoe: FIP debugging
  libfc: spurious I/O error under high load
  libfc: Do not attempt to login if the port is already started
  libfc: Debug PRLI failures
  fcoe: filter out frames from invalid vlans
  fcoe: make R_A_TOV and E_D_TOV configurable
  libfc: use configured lport R_A_TOV when sending exchange
  libfc: use configured e_d_tov for remote port state retries
  fcoe: inhibit writing invalid values into the 'enabled' attribute
  libfc: don't fail sequence abort for completed exchanges
  libfc: do not overwrite DID_TIME_OUT status
  libfc: use error code for fc_rport_error()
  libfc: frame alloc failure messages
  fc: add missing ELS explanation values
  libfc: Send LS_RJT responses on frame allocation failure
  libfc: don't advance state machine for incoming FLOGI
  libfc: Implement RTV responder
  libfc: Do not drop out-of-order frames
  libfc: reset timeout on queue full
  fcoe: set default TC priority

 drivers/scsi/fcoe/fcoe.c       |  21 ++-
 drivers/scsi/fcoe/fcoe_ctlr.c  |  63 +++++++-
 drivers/scsi/fcoe/fcoe_sysfs.c |  83 +++++++++-
 drivers/scsi/libfc/fc_exch.c   |  64 ++++++--
 drivers/scsi/libfc/fc_fcp.c    | 131 ++++++++++++----
 drivers/scsi/libfc/fc_rport.c  | 349 ++++++++++++++++++++++++++++++-----------
 include/scsi/libfc.h           |   8 +-
 include/uapi/scsi/fc/fc_els.h  |  17 +-
 8 files changed, 584 insertions(+), 152 deletions(-)

-- 
1.8.5.6

[PATCH 01/22] libfc: Revisit kref handling

[Hannes Reinecke]

The kref handling in fc_rport is a mess. This patch updates
the kref handling according to the following rules:

- Take a reference whenever scheduling a workqueue
- Take a reference whenever an ELS command is send
- Drop the reference at the end of the workqueue function
- Drop the reference at the end of handling ELS replies
- Take a reference when allocating an rport
- Drop the reference when removing an rport

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libfc/fc_rport.c | 134 ++++++++++++++++++++++++++++++++----------
 1 file changed, 103 insertions(+), 31 deletions(-)

diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
index 93f5961..6a98bb8 100644
--- a/drivers/scsi/libfc/fc_rport.c
+++ b/drivers/scsi/libfc/fc_rport.c
@@ -44,6 +44,17 @@
  * path this potential over-use of the mutex is acceptable.
  */
 
+/*
+ * RPORT REFERENCE COUNTING
+ *
+ * A rport reference should be taken when:
+ * - a workqueue item is scheduled
+ * - an ELS request is send
+ * The reference should be dropped when:
+ * - the workqueue function has finished
+ * - the ELS response is handled
+ */
+
 #include <linux/kernel.h>
 #include <linux/spinlock.h>
 #include <linux/interrupt.h>
@@ -242,6 +253,8 @@ static void fc_rport_state_enter(struct fc_rport_priv *rdata,
 /**
  * fc_rport_work() - Handler for remote port events in the rport_event_queue
  * @work: Handle to the remote port being dequeued
+ *
+ * Reference counting: drops kref on return
  */
 static void fc_rport_work(struct work_struct *work)
 {
@@ -272,8 +285,10 @@ static void fc_rport_work(struct work_struct *work)
 		kref_get(&rdata->kref);
 		mutex_unlock(&rdata->rp_mutex);
 
-		if (!rport)
+		if (!rport) {
+			FC_RPORT_DBG(rdata, "No rport!\n");
 			rport = fc_remote_port_add(lport->host, 0, &ids);
+		}
 		if (!rport) {
 			FC_RPORT_DBG(rdata, "Failed to add the rport\n");
 			lport->tt.rport_logoff(rdata);
@@ -329,7 +344,8 @@ static void fc_rport_work(struct work_struct *work)
 			FC_RPORT_DBG(rdata, "lld callback ev %d\n", event);
 			rdata->lld_event_callback(lport, rdata, event);
 		}
-		cancel_delayed_work_sync(&rdata->retry_work);
+		if (cancel_delayed_work_sync(&rdata->retry_work))
+			kref_put(&rdata->kref, lport->tt.rport_destroy);
 
 		/*
 		 * Reset any outstanding exchanges before freeing rport.
@@ -381,6 +397,7 @@ static void fc_rport_work(struct work_struct *work)
 		mutex_unlock(&rdata->rp_mutex);
 		break;
 	}
+	kref_put(&rdata->kref, lport->tt.rport_destroy);
 }
 
 /**
@@ -431,10 +448,14 @@ static int fc_rport_login(struct fc_rport_priv *rdata)
  * Set the new event so that the old pending event will not occur.
  * Since we have the mutex, even if fc_rport_work() is already started,
  * it'll see the new event.
+ *
+ * Reference counting: does not modify kref
  */
 static void fc_rport_enter_delete(struct fc_rport_priv *rdata,
 				  enum fc_rport_event event)
 {
+	struct fc_lport *lport = rdata->local_port;
+
 	if (rdata->rp_state == RPORT_ST_DELETE)
 		return;
 
@@ -442,8 +463,11 @@ static void fc_rport_enter_delete(struct fc_rport_priv *rdata,
 
 	fc_rport_state_enter(rdata, RPORT_ST_DELETE);
 
-	if (rdata->event == RPORT_EV_NONE)
-		queue_work(rport_event_queue, &rdata->event_work);
+	kref_get(&rdata->kref);
+	if (rdata->event == RPORT_EV_NONE &&
+	    !queue_work(rport_event_queue, &rdata->event_work))
+		kref_put(&rdata->kref, lport->tt.rport_destroy);
+
 	rdata->event = event;
 }
 
@@ -484,15 +508,22 @@ out:
  *
  * Locking Note: The rport lock is expected to be held before calling
  * this routine.
+ *
+ * Reference counting: schedules workqueue, does not modify kref
  */
 static void fc_rport_enter_ready(struct fc_rport_priv *rdata)
 {
+	struct fc_lport *lport = rdata->local_port;
+
 	fc_rport_state_enter(rdata, RPORT_ST_READY);
 
 	FC_RPORT_DBG(rdata, "Port is Ready\n");
 
-	if (rdata->event == RPORT_EV_NONE)
-		queue_work(rport_event_queue, &rdata->event_work);
+	kref_get(&rdata->kref);
+	if (rdata->event == RPORT_EV_NONE &&
+	    !queue_work(rport_event_queue, &rdata->event_work))
+		kref_put(&rdata->kref, lport->tt.rport_destroy);
+
 	rdata->event = RPORT_EV_READY;
 }
 
@@ -503,13 +534,17 @@ static void fc_rport_enter_ready(struct fc_rport_priv *rdata)
  * Locking Note: Called without the rport lock held. This
  * function will hold the rport lock, call an _enter_*
  * function and then unlock the rport.
+ *
+ * Reference counting: Drops kref on return.
  */
 static void fc_rport_timeout(struct work_struct *work)
 {
 	struct fc_rport_priv *rdata =
 		container_of(work, struct fc_rport_priv, retry_work.work);
+	struct fc_lport *lport = rdata->local_port;
 
 	mutex_lock(&rdata->rp_mutex);
+	FC_RPORT_DBG(rdata, "Port timeout, state %s\n", fc_rport_state(rdata));
 
 	switch (rdata->rp_state) {
 	case RPORT_ST_FLOGI:
@@ -535,6 +570,7 @@ static void fc_rport_timeout(struct work_struct *work)
 	}
 
 	mutex_unlock(&rdata->rp_mutex);
+	kref_put(&rdata->kref, lport->tt.rport_destroy);
 }
 
 /**
@@ -544,6 +580,8 @@ static void fc_rport_timeout(struct work_struct *work)
  *
  * Locking Note: The rport lock is expected to be held before
  * calling this routine
+ *
+ * Reference counting: does not modify kref
  */
 static void fc_rport_error(struct fc_rport_priv *rdata, struct fc_frame *fp)
 {
@@ -582,11 +620,14 @@ static void fc_rport_error(struct fc_rport_priv *rdata, struct fc_frame *fp)
  *
  * Locking Note: The rport lock is expected to be held before
  * calling this routine
+ *
+ * Reference counting: increments kref when scheduling retry_work
  */
 static void fc_rport_error_retry(struct fc_rport_priv *rdata,
 				 struct fc_frame *fp)
 {
 	unsigned long delay = msecs_to_jiffies(FC_DEF_E_D_TOV);
+	struct fc_lport *lport = rdata->local_port;
 
 	/* make sure this isn't an FC_EX_CLOSED error, never retry those */
 	if (PTR_ERR(fp) == -FC_EX_CLOSED)
@@ -599,7 +640,9 @@ static void fc_rport_error_retry(struct fc_rport_priv *rdata,
 		/* no additional delay on exchange timeouts */
 		if (PTR_ERR(fp) == -FC_EX_TIMEOUT)
 			delay = 0;
-		schedule_delayed_work(&rdata->retry_work, delay);
+		kref_get(&rdata->kref);
+		if (!schedule_delayed_work(&rdata->retry_work, delay))
+			kref_put(&rdata->kref, lport->tt.rport_destroy);
 		return;
 	}
 
@@ -720,6 +763,8 @@ bad:
  *
  * Locking Note: The rport lock is expected to be held before calling
  * this routine.
+ *
+ * Reference counting: increments kref when sending ELS
  */
 static void fc_rport_enter_flogi(struct fc_rport_priv *rdata)
 {
@@ -738,18 +783,21 @@ static void fc_rport_enter_flogi(struct fc_rport_priv *rdata)
 	if (!fp)
 		return fc_rport_error_retry(rdata, fp);
 
+	kref_get(&rdata->kref);
 	if (!lport->tt.elsct_send(lport, rdata->ids.port_id, fp, ELS_FLOGI,
 				  fc_rport_flogi_resp, rdata,
-				  2 * lport->r_a_tov))
+				  2 * lport->r_a_tov)) {
 		fc_rport_error_retry(rdata, NULL);
-	else
-		kref_get(&rdata->kref);
+		kref_put(&rdata->kref, lport->tt.rport_destroy);
+	}
 }
 
 /**
  * fc_rport_recv_flogi_req() - Handle Fabric Login (FLOGI) request in p-mp mode
  * @lport: The local port that received the PLOGI request
  * @rx_fp: The PLOGI request frame
+ *
+ * Reference counting: drops kref on return
  */
 static void fc_rport_recv_flogi_req(struct fc_lport *lport,
 				    struct fc_frame *rx_fp)
@@ -804,8 +852,7 @@ static void fc_rport_recv_flogi_req(struct fc_lport *lport,
 		 * RPORT wouldn;t have created and 'rport_lookup' would have
 		 * failed anyway in that case.
 		 */
-		if (lport->point_to_multipoint)
-			break;
+		break;
 	case RPORT_ST_DELETE:
 		mutex_unlock(&rdata->rp_mutex);
 		rjt_data.reason = ELS_RJT_FIP;
@@ -949,6 +996,8 @@ fc_rport_compatible_roles(struct fc_lport *lport, struct fc_rport_priv *rdata)
  *
  * Locking Note: The rport lock is expected to be held before calling
  * this routine.
+ *
+ * Reference counting: increments kref when sending ELS
  */
 static void fc_rport_enter_plogi(struct fc_rport_priv *rdata)
 {
@@ -975,12 +1024,13 @@ static void fc_rport_enter_plogi(struct fc_rport_priv *rdata)
 	}
 	rdata->e_d_tov = lport->e_d_tov;
 
+	kref_get(&rdata->kref);
 	if (!lport->tt.elsct_send(lport, rdata->ids.port_id, fp, ELS_PLOGI,
 				  fc_rport_plogi_resp, rdata,
-				  2 * lport->r_a_tov))
+				  2 * lport->r_a_tov)) {
 		fc_rport_error_retry(rdata, NULL);
-	else
-		kref_get(&rdata->kref);
+		kref_put(&rdata->kref, lport->tt.rport_destroy);
+	}
 }
 
 /**
@@ -1088,6 +1138,8 @@ err:
  *
  * Locking Note: The rport lock is expected to be held before calling
  * this routine.
+ *
+ * Reference counting: increments kref when sending ELS
  */
 static void fc_rport_enter_prli(struct fc_rport_priv *rdata)
 {
@@ -1131,11 +1183,12 @@ static void fc_rport_enter_prli(struct fc_rport_priv *rdata)
 		       fc_host_port_id(lport->host), FC_TYPE_ELS,
 		       FC_FC_FIRST_SEQ | FC_FC_END_SEQ | FC_FC_SEQ_INIT, 0);
 
+	kref_get(&rdata->kref);
 	if (!lport->tt.exch_seq_send(lport, fp, fc_rport_prli_resp,
-				    NULL, rdata, 2 * lport->r_a_tov))
+				     NULL, rdata, 2 * lport->r_a_tov)) {
 		fc_rport_error_retry(rdata, NULL);
-	else
-		kref_get(&rdata->kref);
+		kref_put(&rdata->kref, lport->tt.rport_destroy);
+	}
 }
 
 /**
@@ -1210,6 +1263,8 @@ err:
  *
  * Locking Note: The rport lock is expected to be held before calling
  * this routine.
+ *
+ * Reference counting: increments kref when sending ELS
  */
 static void fc_rport_enter_rtv(struct fc_rport_priv *rdata)
 {
@@ -1227,12 +1282,13 @@ static void fc_rport_enter_rtv(struct fc_rport_priv *rdata)
 		return;
 	}
 
+	kref_get(&rdata->kref);
 	if (!lport->tt.elsct_send(lport, rdata->ids.port_id, fp, ELS_RTV,
 				  fc_rport_rtv_resp, rdata,
-				  2 * lport->r_a_tov))
+				  2 * lport->r_a_tov)) {
 		fc_rport_error_retry(rdata, NULL);
-	else
-		kref_get(&rdata->kref);
+		kref_put(&rdata->kref, lport->tt.rport_destroy);
+	}
 }
 
 /**
@@ -1242,15 +1298,16 @@ static void fc_rport_enter_rtv(struct fc_rport_priv *rdata)
  * @lport_arg: The local port
  */
 static void fc_rport_logo_resp(struct fc_seq *sp, struct fc_frame *fp,
-			       void *lport_arg)
+			       void *rdata_arg)
 {
-	struct fc_lport *lport = lport_arg;
+	struct fc_rport_priv *rdata = rdata_arg;
+	struct fc_lport *lport = rdata->local_port;
 
 	FC_RPORT_ID_DBG(lport, fc_seq_exch(sp)->did,
 			"Received a LOGO %s\n", fc_els_resp_type(fp));
-	if (IS_ERR(fp))
-		return;
-	fc_frame_free(fp);
+	if (!IS_ERR(fp))
+		fc_frame_free(fp);
+	kref_put(&rdata->kref, lport->tt.rport_destroy);
 }
 
 /**
@@ -1259,6 +1316,8 @@ static void fc_rport_logo_resp(struct fc_seq *sp, struct fc_frame *fp,
  *
  * Locking Note: The rport lock is expected to be held before calling
  * this routine.
+ *
+ * Reference counting: increments kref when sending ELS
  */
 static void fc_rport_enter_logo(struct fc_rport_priv *rdata)
 {
@@ -1271,8 +1330,10 @@ static void fc_rport_enter_logo(struct fc_rport_priv *rdata)
 	fp = fc_frame_alloc(lport, sizeof(struct fc_els_logo));
 	if (!fp)
 		return;
-	(void)lport->tt.elsct_send(lport, rdata->ids.port_id, fp, ELS_LOGO,
-				   fc_rport_logo_resp, lport, 0);
+	kref_get(&rdata->kref);
+	if (!lport->tt.elsct_send(lport, rdata->ids.port_id, fp, ELS_LOGO,
+				  fc_rport_logo_resp, rdata, 0))
+		kref_put(&rdata->kref, lport->tt.rport_destroy);
 }
 
 /**
@@ -1339,6 +1400,8 @@ err:
  *
  * Locking Note: The rport lock is expected to be held before calling
  * this routine.
+ *
+ * Reference counting: increments kref when sending ELS
  */
 static void fc_rport_enter_adisc(struct fc_rport_priv *rdata)
 {
@@ -1355,12 +1418,13 @@ static void fc_rport_enter_adisc(struct fc_rport_priv *rdata)
 		fc_rport_error_retry(rdata, fp);
 		return;
 	}
+	kref_get(&rdata->kref);
 	if (!lport->tt.elsct_send(lport, rdata->ids.port_id, fp, ELS_ADISC,
 				  fc_rport_adisc_resp, rdata,
-				  2 * lport->r_a_tov))
+				  2 * lport->r_a_tov)) {
 		fc_rport_error_retry(rdata, NULL);
-	else
-		kref_get(&rdata->kref);
+		kref_put(&rdata->kref, lport->tt.rport_destroy);
+	}
 }
 
 /**
@@ -1474,6 +1538,8 @@ out:
  * The ELS opcode has already been validated by the caller.
  *
  * Locking Note: Called with the lport lock held.
+ *
+ * Reference counting: does not modify kref
  */
 static void fc_rport_recv_els_req(struct fc_lport *lport, struct fc_frame *fp)
 {
@@ -1541,6 +1607,8 @@ reject:
  * @fp:	   The request frame
  *
  * Locking Note: Called with the lport lock held.
+ *
+ * Reference counting: does not modify kref
  */
 static void fc_rport_recv_req(struct fc_lport *lport, struct fc_frame *fp)
 {
@@ -1585,6 +1653,8 @@ static void fc_rport_recv_req(struct fc_lport *lport, struct fc_frame *fp)
  * @rx_fp: The PLOGI request frame
  *
  * Locking Note: The rport lock is held before calling this function.
+ *
+ * Reference counting: increments kref on return
  */
 static void fc_rport_recv_plogi_req(struct fc_lport *lport,
 				    struct fc_frame *rx_fp)
@@ -1899,6 +1969,8 @@ drop:
  *
  * Locking Note: The rport lock is expected to be held before calling
  * this function.
+ *
+ * Reference counting: drops kref on return
  */
 static void fc_rport_recv_logo_req(struct fc_lport *lport, struct fc_frame *fp)
 {
-- 
1.8.5.6

Re: [PATCH 01/22] libfc: Revisit kref handling

[Johannes Thumshirn]

On Wed, Aug 03, 2016 at 03:13:01PM +0200, Hannes Reinecke wrote:
> The kref handling in fc_rport is a mess. This patch updates
> the kref handling according to the following rules:
> 
> - Take a reference whenever scheduling a workqueue
> - Take a reference whenever an ELS command is send
> - Drop the reference at the end of the workqueue function
> - Drop the reference at the end of handling ELS replies
> - Take a reference when allocating an rport
> - Drop the reference when removing an rport
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/libfc/fc_rport.c | 134 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 103 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
> index 93f5961..6a98bb8 100644
> --- a/drivers/scsi/libfc/fc_rport.c
> +++ b/drivers/scsi/libfc/fc_rport.c
> @@ -44,6 +44,17 @@
>   * path this potential over-use of the mutex is acceptable.
>   */
>  
> +/*
> + * RPORT REFERENCE COUNTING
> + *
> + * A rport reference should be taken when:
> + * - a workqueue item is scheduled
> + * - an ELS request is send
> + * The reference should be dropped when:
> + * - the workqueue function has finished
> + * - the ELS response is handled
> + */

Please sync with the rules in the commit message.

> +
>  #include <linux/kernel.h>
>  #include <linux/spinlock.h>
>  #include <linux/interrupt.h>
> @@ -242,6 +253,8 @@ static void fc_rport_state_enter(struct fc_rport_priv *rdata,
>  /**
>   * fc_rport_work() - Handler for remote port events in the rport_event_queue
>   * @work: Handle to the remote port being dequeued
> + *
> + * Reference counting: drops kref on return
>   */
>  static void fc_rport_work(struct work_struct *work)
>  {
> @@ -272,8 +285,10 @@ static void fc_rport_work(struct work_struct *work)
>  		kref_get(&rdata->kref);
>  		mutex_unlock(&rdata->rp_mutex);
>  
> -		if (!rport)
> +		if (!rport) {
> +			FC_RPORT_DBG(rdata, "No rport!\n");

If you're re-sending the series, this and the other added debug statements
might be better suited in '[PATCH 02/22] libfc: additional debugging messages'.

Otherwise

Acked-by: Johannes Thumshirn <jth@kernel.org>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 01/22] libfc: Revisit kref handling

[Chad Dupuis]

On Wed, 3 Aug 2016, 1:13pm -0000, Hannes Reinecke wrote:

> The kref handling in fc_rport is a mess. This patch updates
> the kref handling according to the following rules:
> 
> - Take a reference whenever scheduling a workqueue
> - Take a reference whenever an ELS command is send
> - Drop the reference at the end of the workqueue function
> - Drop the reference at the end of handling ELS replies
> - Take a reference when allocating an rport
> - Drop the reference when removing an rport
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/libfc/fc_rport.c | 134 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 103 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
> index 93f5961..6a98bb8 100644
> --- a/drivers/scsi/libfc/fc_rport.c
> +++ b/drivers/scsi/libfc/fc_rport.c
> @@ -44,6 +44,17 @@
>   * path this potential over-use of the mutex is acceptable.
>   */
>  

<-- snip -->

Yes, this took quite a few iterations to get right but your rules make 
sense.  I've tested with this patch and it successfully passed.

Tested-by: Chad Dupuis <chad.dupuis@qlogic.com>

[PATCH 02/22] libfc: additional debugging messages

[Hannes Reinecke]

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libfc/fc_exch.c  | 18 +++++++++++++++---
 drivers/scsi/libfc/fc_fcp.c   | 39 +++++++++++++++++++++++++++++++++------
 drivers/scsi/libfc/fc_rport.c | 11 ++++++++---
 3 files changed, 56 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
index e72673b..dc078d3 100644
--- a/drivers/scsi/libfc/fc_exch.c
+++ b/drivers/scsi/libfc/fc_exch.c
@@ -362,8 +362,10 @@ static inline void fc_exch_timer_set_locked(struct fc_exch *ep,
 
 	fc_exch_hold(ep);		/* hold for timer */
 	if (!queue_delayed_work(fc_exch_workqueue, &ep->timeout_work,
-				msecs_to_jiffies(timer_msec)))
+				msecs_to_jiffies(timer_msec))) {
+		FC_EXCH_DBG(ep, "Exchange already queued\n");
 		fc_exch_release(ep);
+	}
 }
 
 /**
@@ -632,9 +634,13 @@ static int fc_exch_abort_locked(struct fc_exch *ep,
 	struct fc_frame *fp;
 	int error;
 
+	FC_EXCH_DBG(ep, "exch: abort, time %d msecs\n", timer_msec);
 	if (ep->esb_stat & (ESB_ST_COMPLETE | ESB_ST_ABNORMAL) ||
-	    ep->state & (FC_EX_DONE | FC_EX_RST_CLEANUP))
+	    ep->state & (FC_EX_DONE | FC_EX_RST_CLEANUP)) {
+		FC_EXCH_DBG(ep, "exch: already completed esb %x state %x\n",
+			    ep->esb_stat, ep->state);
 		return -ENXIO;
+	}
 
 	/*
 	 * Send the abort on a new sequence if possible.
@@ -758,7 +764,7 @@ static void fc_exch_timeout(struct work_struct *work)
 	u32 e_stat;
 	int rc = 1;
 
-	FC_EXCH_DBG(ep, "Exchange timed out\n");
+	FC_EXCH_DBG(ep, "Exchange timed out state %x\n", ep->state);
 
 	spin_lock_bh(&ep->ex_lock);
 	if (ep->state & (FC_EX_RST_CLEANUP | FC_EX_DONE))
@@ -1383,6 +1389,7 @@ static void fc_exch_recv_abts(struct fc_exch *ep, struct fc_frame *rx_fp)
 	if (!ep)
 		goto reject;
 
+	FC_EXCH_DBG(ep, "exch: ABTS received\n");
 	fp = fc_frame_alloc(ep->lp, sizeof(*ap));
 	if (!fp)
 		goto free;
@@ -1978,6 +1985,8 @@ static void fc_exch_els_rec(struct fc_frame *rfp)
 	explan = ELS_EXPL_OXID_RXID;
 	if (!ep)
 		goto reject;
+	FC_EXCH_DBG(ep, "REC request from %x: rxid %x oxid %x\n",
+		    sid, rxid, oxid);
 	if (ep->oid != sid || oxid != ep->oxid)
 		goto rel;
 	if (rxid != FC_XID_UNKNOWN && rxid != ep->rxid)
@@ -2177,6 +2186,7 @@ static void fc_exch_rrq(struct fc_exch *ep)
 		return;
 
 retry:
+	FC_EXCH_DBG(ep, "exch: RRQ send failed\n");
 	spin_lock_bh(&ep->ex_lock);
 	if (ep->state & (FC_EX_RST_CLEANUP | FC_EX_DONE)) {
 		spin_unlock_bh(&ep->ex_lock);
@@ -2219,6 +2229,8 @@ static void fc_exch_els_rrq(struct fc_frame *fp)
 	if (!ep)
 		goto reject;
 	spin_lock_bh(&ep->ex_lock);
+	FC_EXCH_DBG(ep, "RRQ request from %x: xid %x rxid %x oxid %x\n",
+		    sid, xid, ntohs(rp->rrq_rx_id), ntohs(rp->rrq_ox_id));
 	if (ep->oxid != ntohs(rp->rrq_ox_id))
 		goto unlock_reject;
 	if (ep->rxid != ntohs(rp->rrq_rx_id) &&
diff --git a/drivers/scsi/libfc/fc_fcp.c b/drivers/scsi/libfc/fc_fcp.c
index 5121272..bd4bdbf 100644
--- a/drivers/scsi/libfc/fc_fcp.c
+++ b/drivers/scsi/libfc/fc_fcp.c
@@ -764,8 +764,11 @@ static void fc_fcp_recv(struct fc_seq *seq, struct fc_frame *fp, void *arg)
 	fh = fc_frame_header_get(fp);
 	r_ctl = fh->fh_r_ctl;
 
-	if (lport->state != LPORT_ST_READY)
+	if (lport->state != LPORT_ST_READY) {
+		FC_FCP_DBG(fsp, "lport state %d, ignoring r_ctl %x\n",
+			   lport->state, r_ctl);
 		goto out;
+	}
 	if (fc_fcp_lock_pkt(fsp))
 		goto out;
 
@@ -774,8 +777,10 @@ static void fc_fcp_recv(struct fc_seq *seq, struct fc_frame *fp, void *arg)
 		goto unlock;
 	}
 
-	if (fsp->state & (FC_SRB_ABORTED | FC_SRB_ABORT_PENDING))
+	if (fsp->state & (FC_SRB_ABORTED | FC_SRB_ABORT_PENDING)) {
+		FC_FCP_DBG(fsp, "command aborted, ignoring r_ctl %x\n", r_ctl);
 		goto unlock;
+	}
 
 	if (r_ctl == FC_RCTL_DD_DATA_DESC) {
 		/*
@@ -910,6 +915,10 @@ static void fc_fcp_resp(struct fc_fcp_pkt *fsp, struct fc_frame *fp)
 			 * Wait a at least one jiffy to see if it is delivered.
 			 * If this expires without data, we may do SRR.
 			 */
+			FC_FCP_DBG(fsp, "tgt %6.6x xfer len %zx data underrun "
+				   "len %x, data len %x\n",
+				   fsp->rport->port_id,
+				   fsp->xfer_len, expected_len, fsp->data_len);
 			fc_fcp_timer_set(fsp, 2);
 			return;
 		}
@@ -959,8 +968,12 @@ static void fc_fcp_complete_locked(struct fc_fcp_pkt *fsp)
 		if (fsp->cdb_status == SAM_STAT_GOOD &&
 		    fsp->xfer_len < fsp->data_len && !fsp->io_status &&
 		    (!(fsp->scsi_comp_flags & FCP_RESID_UNDER) ||
-		     fsp->xfer_len < fsp->data_len - fsp->scsi_resid))
+		     fsp->xfer_len < fsp->data_len - fsp->scsi_resid)) {
+			FC_FCP_DBG( fsp, "data underrun, "
+				    "xfer_len %zx data_len %x\n",
+				    fsp->xfer_len, fsp->data_len );
 			fsp->status_code = FC_DATA_UNDRUN;
+		}
 	}
 
 	seq = fsp->seq_ptr;
@@ -1222,8 +1235,11 @@ static int fc_fcp_pkt_abort(struct fc_fcp_pkt *fsp)
 	int rc = FAILED;
 	unsigned long ticks_left;
 
-	if (fc_fcp_send_abort(fsp))
+	FC_FCP_DBG(fsp, "pkt abort state %x\n", fsp->state);
+	if (fc_fcp_send_abort(fsp)) {
+		FC_FCP_DBG(fsp, "failed to send abort\n");
 		return FAILED;
+	}
 
 	init_completion(&fsp->tm_done);
 	fsp->wait_for_comp = 1;
@@ -1394,6 +1410,8 @@ static void fc_fcp_timeout(unsigned long data)
 	if (fsp->cdb_cmd.fc_tm_flags)
 		goto unlock;
 
+	FC_FCP_DBG(fsp, "fcp timeout, flags %x state %x\n",
+		   rpriv->flags, fsp->state);
 	fsp->state |= FC_SRB_FCP_PROCESSING_TMO;
 
 	if (rpriv->flags & FC_RP_FLAGS_REC_SUPPORTED)
@@ -1503,6 +1521,10 @@ static void fc_fcp_rec_resp(struct fc_seq *seq, struct fc_frame *fp, void *arg)
 			break;
 		case ELS_RJT_LOGIC:
 		case ELS_RJT_UNAB:
+			FC_FCP_DBG(fsp, "device %x REC reject "
+				   "reason %d expl %d\n",
+				   fsp->rport->port_id, rjt->er_reason,
+				   rjt->er_explan);
 			/*
 			 * If no data transfer, the command frame got dropped
 			 * so we just retry.  If data was transferred, we
@@ -1608,6 +1630,8 @@ static void fc_fcp_rec_error(struct fc_fcp_pkt *fsp, struct fc_frame *fp)
 
 	switch (error) {
 	case -FC_EX_CLOSED:
+		FC_FCP_DBG(fsp, "REC %p fid %6.6x exchange closed\n",
+			   fsp, fsp->rport->port_id);
 		fc_fcp_retry_cmd(fsp);
 		break;
 
@@ -1622,8 +1646,8 @@ static void fc_fcp_rec_error(struct fc_fcp_pkt *fsp, struct fc_frame *fp)
 		 * Assume REC or LS_ACC was lost.
 		 * The exchange manager will have aborted REC, so retry.
 		 */
-		FC_FCP_DBG(fsp, "REC fid %6.6x error error %d retry %d/%d\n",
-			   fsp->rport->port_id, error, fsp->recov_retry,
+		FC_FCP_DBG(fsp, "REC %p fid %6.6x exchange timeout retry %d/%d\n",
+			   fsp, fsp->rport->port_id, fsp->recov_retry,
 			   FC_MAX_RECOV_RETRY);
 		if (fsp->recov_retry++ < FC_MAX_RECOV_RETRY)
 			fc_fcp_rec(fsp);
@@ -1642,6 +1666,7 @@ out:
  */
 static void fc_fcp_recovery(struct fc_fcp_pkt *fsp, u8 code)
 {
+	FC_FCP_DBG(fsp, "start recovery code %x\n", code);
 	fsp->status_code = code;
 	fsp->cdb_status = 0;
 	fsp->io_status = 0;
@@ -1768,12 +1793,14 @@ static void fc_fcp_srr_error(struct fc_fcp_pkt *fsp, struct fc_frame *fp)
 		goto out;
 	switch (PTR_ERR(fp)) {
 	case -FC_EX_TIMEOUT:
+		FC_FCP_DBG(fsp, "SRR timeout, retries %d\n", fsp->recov_retry);
 		if (fsp->recov_retry++ < FC_MAX_RECOV_RETRY)
 			fc_fcp_rec(fsp);
 		else
 			fc_fcp_recovery(fsp, FC_TIMED_OUT);
 		break;
 	case -FC_EX_CLOSED:			/* e.g., link failure */
+		FC_FCP_DBG(fsp, "SRR error, exchange closed\n");
 		/* fall through */
 	default:
 		fc_fcp_retry_cmd(fsp);
diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
index 6a98bb8..afc1f9b 100644
--- a/drivers/scsi/libfc/fc_rport.c
+++ b/drivers/scsi/libfc/fc_rport.c
@@ -387,8 +387,10 @@ static void fc_rport_work(struct work_struct *work)
 			 * Re-open for events.  Reissue READY event if ready.
 			 */
 			rdata->event = RPORT_EV_NONE;
-			if (rdata->rp_state == RPORT_ST_READY)
+			if (rdata->rp_state == RPORT_ST_READY) {
+				FC_RPORT_DBG(rdata, "work reopen\n");
 				fc_rport_enter_ready(rdata);
+			}
 			mutex_unlock(&rdata->rp_mutex);
 		}
 		break;
@@ -1052,6 +1054,7 @@ static void fc_rport_prli_resp(struct fc_seq *sp, struct fc_frame *fp,
 		struct fc_els_spp spp;
 	} *pp;
 	struct fc_els_spp temp_spp;
+	struct fc_els_ls_rjt *rjt;
 	struct fc4_prov *prov;
 	u32 roles = FC_RPORT_ROLE_UNKNOWN;
 	u32 fcp_parm = 0;
@@ -1121,8 +1124,10 @@ static void fc_rport_prli_resp(struct fc_seq *sp, struct fc_frame *fp,
 		fc_rport_enter_rtv(rdata);
 
 	} else {
-		FC_RPORT_DBG(rdata, "Bad ELS response for PRLI command\n");
-		fc_rport_error_retry(rdata, fp);
+		rjt = fc_frame_payload_get(fp, sizeof (*rjt));
+		FC_RPORT_DBG(rdata, "PRLI ELS rejected, reason %x expl %x\n",
+			     rjt->er_reason, rjt->er_explan);
+		fc_rport_error_retry(rdata, NULL);
 	}
 
 out:
-- 
1.8.5.6

Re: [PATCH 02/22] libfc: additional debugging messages

[Bart Van Assche]

On 08/03/2016 06:13 AM, Hannes Reinecke wrote:
> @@ -1121,8 +1124,10 @@ static void fc_rport_prli_resp(struct fc_seq *sp, struct fc_frame *fp,
>  		fc_rport_enter_rtv(rdata);
>
>  	} else {
> -		FC_RPORT_DBG(rdata, "Bad ELS response for PRLI command\n");
> -		fc_rport_error_retry(rdata, fp);
> +		rjt = fc_frame_payload_get(fp, sizeof (*rjt));
> +		FC_RPORT_DBG(rdata, "PRLI ELS rejected, reason %x expl %x\n",
> +			     rjt->er_reason, rjt->er_explan);
> +		fc_rport_error_retry(rdata, NULL);
>  	}

Shouldn't the type of the frame be checked in this code instead of 
assuming that it's a reject?

Bart.

Re: [PATCH 02/22] libfc: additional debugging messages

[Hannes Reinecke]

On 08/03/2016 07:33 PM, Bart Van Assche wrote:
> On 08/03/2016 06:13 AM, Hannes Reinecke wrote:
>> @@ -1121,8 +1124,10 @@ static void fc_rport_prli_resp(struct fc_seq
>> *sp, struct fc_frame *fp,
>>          fc_rport_enter_rtv(rdata);
>>
>>      } else {
>> -        FC_RPORT_DBG(rdata, "Bad ELS response for PRLI command\n");
>> -        fc_rport_error_retry(rdata, fp);
>> +        rjt = fc_frame_payload_get(fp, sizeof (*rjt));
>> +        FC_RPORT_DBG(rdata, "PRLI ELS rejected, reason %x expl %x\n",
>> +                 rjt->er_reason, rjt->er_explan);
>> +        fc_rport_error_retry(rdata, NULL);
>>      }
> 
> Shouldn't the type of the frame be checked in this code instead of
> assuming that it's a reject?
> 
I've asked me the same question, but as it stands FC-LS only allows for
two frame types (LS_ACC or LS_RJT).
And it's (more-or-less) consistent usage throughout libfc.
But yeah, for safety one really should check for LS_ACC and LS_RJT and
throw an error for any other type.
However, that should be done throughout libfc, and would warrant a
different patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 02/22] libfc: additional debugging messages

[Johannes Thumshirn]

On Wed, Aug 03, 2016 at 03:13:02PM +0200, Hannes Reinecke wrote:
> Signed-off-by: Hannes Reinecke <hare@suse.com>

Acked-by: Johannes Thumshirn <jth@kernel.org>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 03/22] fcoe: FIP debugging

[Hannes Reinecke]

Add additional statements for debugging FIP frames.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/fcoe/fcoe_ctlr.c | 51 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 46 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe_ctlr.c b/drivers/scsi/fcoe/fcoe_ctlr.c
index a569c65..1d0bec6 100644
--- a/drivers/scsi/fcoe/fcoe_ctlr.c
+++ b/drivers/scsi/fcoe/fcoe_ctlr.c
@@ -801,6 +801,8 @@ int fcoe_ctlr_els_send(struct fcoe_ctlr *fip, struct fc_lport *lport,
 	return -EINPROGRESS;
 drop:
 	kfree_skb(skb);
+	LIBFCOE_FIP_DBG(fip, "drop els_send op %u d_id %x\n",
+			op, ntoh24(fh->fh_d_id));
 	return -EINVAL;
 }
 EXPORT_SYMBOL(fcoe_ctlr_els_send);
@@ -2393,6 +2395,8 @@ static void fcoe_ctlr_vn_probe_req(struct fcoe_ctlr *fip,
 	switch (fip->state) {
 	case FIP_ST_VNMP_CLAIM:
 	case FIP_ST_VNMP_UP:
+		LIBFCOE_FIP_DBG(fip, "vn_probe_req: send reply, state %x\n",
+				fip->state);
 		fcoe_ctlr_vn_send(fip, FIP_SC_VN_PROBE_REP,
 				  frport->enode_mac, 0);
 		break;
@@ -2407,15 +2411,21 @@ static void fcoe_ctlr_vn_probe_req(struct fcoe_ctlr *fip,
 		 */
 		if (fip->lp->wwpn > rdata->ids.port_name &&
 		    !(frport->flags & FIP_FL_REC_OR_P2P)) {
+			LIBFCOE_FIP_DBG(fip, "vn_probe_req: "
+					"port_id collision\n");
 			fcoe_ctlr_vn_send(fip, FIP_SC_VN_PROBE_REP,
 					  frport->enode_mac, 0);
 			break;
 		}
 		/* fall through */
 	case FIP_ST_VNMP_START:
+		LIBFCOE_FIP_DBG(fip, "vn_probe_req: "
+				"restart VN2VN negotiation\n");
 		fcoe_ctlr_vn_restart(fip);
 		break;
 	default:
+		LIBFCOE_FIP_DBG(fip, "vn_probe_req: ignore state %x\n",
+				fip->state);
 		break;
 	}
 }
@@ -2437,9 +2447,12 @@ static void fcoe_ctlr_vn_probe_reply(struct fcoe_ctlr *fip,
 	case FIP_ST_VNMP_PROBE1:
 	case FIP_ST_VNMP_PROBE2:
 	case FIP_ST_VNMP_CLAIM:
+		LIBFCOE_FIP_DBG(fip, "vn_probe_reply: restart state %x\n",
+				fip->state);
 		fcoe_ctlr_vn_restart(fip);
 		break;
 	case FIP_ST_VNMP_UP:
+		LIBFCOE_FIP_DBG(fip, "vn_probe_reply: send claim notify\n");
 		fcoe_ctlr_vn_send_claim(fip);
 		break;
 	default:
@@ -2478,15 +2491,18 @@ static void fcoe_ctlr_vn_add(struct fcoe_ctlr *fip, struct fc_rport_priv *new)
 
 	ids = &rdata->ids;
 	if ((ids->port_name != -1 && ids->port_name != new->ids.port_name) ||
-	    (ids->node_name != -1 && ids->node_name != new->ids.node_name))
+	    (ids->node_name != -1 && ids->node_name != new->ids.node_name)) {
+		LIBFCOE_FIP_DBG(fip, "vn_add rport logoff %6.6x\n", port_id);
 		lport->tt.rport_logoff(rdata);
+	}
 	ids->port_name = new->ids.port_name;
 	ids->node_name = new->ids.node_name;
 	mutex_unlock(&lport->disc.disc_mutex);
 
 	frport = fcoe_ctlr_rport(rdata);
-	LIBFCOE_FIP_DBG(fip, "vn_add rport %6.6x %s\n",
-			port_id, frport->fcoe_len ? "old" : "new");
+	LIBFCOE_FIP_DBG(fip, "vn_add rport %6.6x %s state %d\n",
+			port_id, frport->fcoe_len ? "old" : "new",
+			rdata->rp_state);
 	*frport = *fcoe_ctlr_rport(new);
 	frport->time = 0;
 }
@@ -2529,6 +2545,7 @@ static void fcoe_ctlr_vn_claim_notify(struct fcoe_ctlr *fip,
 	struct fcoe_rport *frport = fcoe_ctlr_rport(new);
 
 	if (frport->flags & FIP_FL_REC_OR_P2P) {
+		LIBFCOE_FIP_DBG(fip, "send probe req for P2P/REC\n");
 		fcoe_ctlr_vn_send(fip, FIP_SC_VN_PROBE_REQ, fcoe_all_vn2vn, 0);
 		return;
 	}
@@ -2536,25 +2553,37 @@ static void fcoe_ctlr_vn_claim_notify(struct fcoe_ctlr *fip,
 	case FIP_ST_VNMP_START:
 	case FIP_ST_VNMP_PROBE1:
 	case FIP_ST_VNMP_PROBE2:
-		if (new->ids.port_id == fip->port_id)
+		if (new->ids.port_id == fip->port_id) {
+			LIBFCOE_FIP_DBG(fip, "vn_claim_notify: "
+					"restart, state %d\n",
+					fip->state);
 			fcoe_ctlr_vn_restart(fip);
+		}
 		break;
 	case FIP_ST_VNMP_CLAIM:
 	case FIP_ST_VNMP_UP:
 		if (new->ids.port_id == fip->port_id) {
 			if (new->ids.port_name > fip->lp->wwpn) {
+				LIBFCOE_FIP_DBG(fip, "vn_claim_notify: "
+						"restart, port_id collision\n");
 				fcoe_ctlr_vn_restart(fip);
 				break;
 			}
+			LIBFCOE_FIP_DBG(fip, "vn_claim_notify: "
+					"send claim notify\n");
 			fcoe_ctlr_vn_send_claim(fip);
 			break;
 		}
+		LIBFCOE_FIP_DBG(fip, "vn_claim_notify: send reply to %x\n",
+				new->ids.port_id);
 		fcoe_ctlr_vn_send(fip, FIP_SC_VN_CLAIM_REP, frport->enode_mac,
 				  min((u32)frport->fcoe_len,
 				      fcoe_ctlr_fcoe_size(fip)));
 		fcoe_ctlr_vn_add(fip, new);
 		break;
 	default:
+		LIBFCOE_FIP_DBG(fip, "vn_claim_notify: "
+				"ignoring claim from %x\n", new->ids.port_id);
 		break;
 	}
 }
@@ -2591,6 +2620,7 @@ static void fcoe_ctlr_vn_beacon(struct fcoe_ctlr *fip,
 
 	frport = fcoe_ctlr_rport(new);
 	if (frport->flags & FIP_FL_REC_OR_P2P) {
+		LIBFCOE_FIP_DBG(fip, "p2p beacon while in vn2vn mode\n");
 		fcoe_ctlr_vn_send(fip, FIP_SC_VN_PROBE_REQ, fcoe_all_vn2vn, 0);
 		return;
 	}
@@ -2599,8 +2629,14 @@ static void fcoe_ctlr_vn_beacon(struct fcoe_ctlr *fip,
 		if (rdata->ids.node_name == new->ids.node_name &&
 		    rdata->ids.port_name == new->ids.port_name) {
 			frport = fcoe_ctlr_rport(rdata);
-			if (!frport->time && fip->state == FIP_ST_VNMP_UP)
+			LIBFCOE_FIP_DBG(fip, "beacon from rport %x\n",
+					rdata->ids.port_id);
+			if (!frport->time && fip->state == FIP_ST_VNMP_UP) {
+				LIBFCOE_FIP_DBG(fip, "beacon expired "
+						"for rport %x\n",
+						rdata->ids.port_id);
 				lport->tt.rport_login(rdata);
+			}
 			frport->time = jiffies;
 		}
 		kref_put(&rdata->kref, lport->tt.rport_destroy);
@@ -3015,11 +3051,13 @@ static void fcoe_ctlr_vn_timeout(struct fcoe_ctlr *fip)
 	switch (fip->state) {
 	case FIP_ST_VNMP_START:
 		fcoe_ctlr_set_state(fip, FIP_ST_VNMP_PROBE1);
+		LIBFCOE_FIP_DBG(fip, "vn_timeout: send 1st probe request\n");
 		fcoe_ctlr_vn_send(fip, FIP_SC_VN_PROBE_REQ, fcoe_all_vn2vn, 0);
 		next_time = jiffies + msecs_to_jiffies(FIP_VN_PROBE_WAIT);
 		break;
 	case FIP_ST_VNMP_PROBE1:
 		fcoe_ctlr_set_state(fip, FIP_ST_VNMP_PROBE2);
+		LIBFCOE_FIP_DBG(fip, "vn_timeout: send 2nd probe request\n");
 		fcoe_ctlr_vn_send(fip, FIP_SC_VN_PROBE_REQ, fcoe_all_vn2vn, 0);
 		next_time = jiffies + msecs_to_jiffies(FIP_VN_ANN_WAIT);
 		break;
@@ -3030,6 +3068,7 @@ static void fcoe_ctlr_vn_timeout(struct fcoe_ctlr *fip)
 		hton24(mac + 3, new_port_id);
 		fcoe_ctlr_map_dest(fip);
 		fip->update_mac(fip->lp, mac);
+		LIBFCOE_FIP_DBG(fip, "vn_timeout: send claim notify\n");
 		fcoe_ctlr_vn_send_claim(fip);
 		next_time = jiffies + msecs_to_jiffies(FIP_VN_ANN_WAIT);
 		break;
@@ -3041,6 +3080,7 @@ static void fcoe_ctlr_vn_timeout(struct fcoe_ctlr *fip)
 		next_time = fip->sol_time + msecs_to_jiffies(FIP_VN_ANN_WAIT);
 		if (time_after_eq(jiffies, next_time)) {
 			fcoe_ctlr_set_state(fip, FIP_ST_VNMP_UP);
+			LIBFCOE_FIP_DBG(fip, "vn_timeout: send vn2vn beacon\n");
 			fcoe_ctlr_vn_send(fip, FIP_SC_VN_BEACON,
 					  fcoe_all_vn2vn, 0);
 			next_time = jiffies + msecs_to_jiffies(FIP_VN_ANN_WAIT);
@@ -3051,6 +3091,7 @@ static void fcoe_ctlr_vn_timeout(struct fcoe_ctlr *fip)
 	case FIP_ST_VNMP_UP:
 		next_time = fcoe_ctlr_vn_age(fip);
 		if (time_after_eq(jiffies, fip->port_ka_time)) {
+			LIBFCOE_FIP_DBG(fip, "vn_timeout: send vn2vn beacon\n");
 			fcoe_ctlr_vn_send(fip, FIP_SC_VN_BEACON,
 					  fcoe_all_vn2vn, 0);
 			fip->port_ka_time = jiffies +
-- 
1.8.5.6

Re: [PATCH 03/22] fcoe: FIP debugging

[Bart Van Assche]

On 08/03/2016 06:13 AM, Hannes Reinecke wrote:
> @@ -2407,15 +2411,21 @@ static void fcoe_ctlr_vn_probe_req(struct fcoe_ctlr *fip,
>  		 */
>  		if (fip->lp->wwpn > rdata->ids.port_name &&
>  		    !(frport->flags & FIP_FL_REC_OR_P2P)) {
> +			LIBFCOE_FIP_DBG(fip, "vn_probe_req: "
> +					"port_id collision\n");
>  			fcoe_ctlr_vn_send(fip, FIP_SC_VN_PROBE_REP,
>  					  frport->enode_mac, 0);

Some users search through the kernel source tree for error and debug 
messages. For these users it is easier if the entire message occurs on a 
single line. Otherwise this patch looks fine to me.

Bart.

Re: [PATCH 03/22] fcoe: FIP debugging

[Hannes Reinecke]

On 08/03/2016 07:36 PM, Bart Van Assche wrote:
> On 08/03/2016 06:13 AM, Hannes Reinecke wrote:
>> @@ -2407,15 +2411,21 @@ static void fcoe_ctlr_vn_probe_req(struct
>> fcoe_ctlr *fip,
>>           */
>>          if (fip->lp->wwpn > rdata->ids.port_name &&
>>              !(frport->flags & FIP_FL_REC_OR_P2P)) {
>> +            LIBFCOE_FIP_DBG(fip, "vn_probe_req: "
>> +                    "port_id collision\n");
>>              fcoe_ctlr_vn_send(fip, FIP_SC_VN_PROBE_REP,
>>                        frport->enode_mac, 0);
> 
> Some users search through the kernel source tree for error and debug
> messages. For these users it is easier if the entire message occurs on a
> single line. Otherwise this patch looks fine to me.
> 
Multiline logging messages; I know.
I have the constant urge to fold everything on 80 chars.
And even checkpatch seems to be unsure whether logging messages
extending beyond 80 chars should be allowed or not.
But okay, one line it is.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 03/22] fcoe: FIP debugging

[Johannes Thumshirn]

On Wed, Aug 03, 2016 at 03:13:03PM +0200, Hannes Reinecke wrote:
> Add additional statements for debugging FIP frames.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>

With Bart's comment addressed:
Acked-by: Johannes Thumshirn <jth@kernel.org>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 04/22] libfc: spurious I/O error under high load

[Hannes Reinecke]

If a command times out libfc is sending an REC, which also
might fail (due to frames being lost or something).
If no data has been transferred we can simply retry
the command, but the current code sets a state of FC_ERROR,
which then is being translated into DID_ERROR, resulting
in an I/O error.
So to handle this properly we need to set a separate
state FC_TRANS_RESET and mapping it onto DID_SOFT_RETRY.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/libfc/fc_fcp.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/libfc/fc_fcp.c b/drivers/scsi/libfc/fc_fcp.c
index bd4bdbf..289c481 100644
--- a/drivers/scsi/libfc/fc_fcp.c
+++ b/drivers/scsi/libfc/fc_fcp.c
@@ -122,6 +122,7 @@ static void fc_fcp_srr_error(struct fc_fcp_pkt *, struct fc_frame *);
 #define FC_HRD_ERROR		9
 #define FC_CRC_ERROR		10
 #define FC_TIMED_OUT		11
+#define FC_TRANS_RESET		12
 
 /*
  * Error recovery timeout values.
@@ -283,7 +284,7 @@ static int fc_fcp_send_abort(struct fc_fcp_pkt *fsp)
  * fc_io_compl() will notify the SCSI-ml that the I/O is done.
  * The SCSI-ml will retry the command.
  */
-static void fc_fcp_retry_cmd(struct fc_fcp_pkt *fsp)
+static void fc_fcp_retry_cmd(struct fc_fcp_pkt *fsp, int status_code)
 {
 	if (fsp->seq_ptr) {
 		fsp->lp->tt.exch_done(fsp->seq_ptr);
@@ -292,7 +293,7 @@ static void fc_fcp_retry_cmd(struct fc_fcp_pkt *fsp)
 
 	fsp->state &= ~FC_SRB_ABORT_PENDING;
 	fsp->io_status = 0;
-	fsp->status_code = FC_ERROR;
+	fsp->status_code = status_code;
 	fc_fcp_complete_locked(fsp);
 }
 
@@ -1209,7 +1210,7 @@ static void fc_fcp_error(struct fc_fcp_pkt *fsp, struct fc_frame *fp)
 		return;
 
 	if (error == -FC_EX_CLOSED) {
-		fc_fcp_retry_cmd(fsp);
+		fc_fcp_retry_cmd(fsp, FC_ERROR);
 		goto unlock;
 	}
 
@@ -1522,9 +1523,9 @@ static void fc_fcp_rec_resp(struct fc_seq *seq, struct fc_frame *fp, void *arg)
 		case ELS_RJT_LOGIC:
 		case ELS_RJT_UNAB:
 			FC_FCP_DBG(fsp, "device %x REC reject "
-				   "reason %d expl %d\n",
+				   "reason %x expl %x xfer_len %zx\n",
 				   fsp->rport->port_id, rjt->er_reason,
-				   rjt->er_explan);
+				   rjt->er_explan, fsp->xfer_len);
 			/*
 			 * If no data transfer, the command frame got dropped
 			 * so we just retry.  If data was transferred, we
@@ -1533,10 +1534,11 @@ static void fc_fcp_rec_resp(struct fc_seq *seq, struct fc_frame *fp, void *arg)
 			 */
 			if (rjt->er_explan == ELS_EXPL_OXID_RXID &&
 			    fsp->xfer_len == 0) {
-				fc_fcp_retry_cmd(fsp);
+				fsp->state |= FC_SRB_ABORTED;
+				fc_fcp_retry_cmd(fsp, FC_TRANS_RESET);
 				break;
 			}
-			fc_fcp_recovery(fsp, FC_ERROR);
+			fc_fcp_recovery(fsp, FC_TRANS_RESET);
 			break;
 		}
 	} else if (opcode == ELS_LS_ACC) {
@@ -1632,7 +1634,7 @@ static void fc_fcp_rec_error(struct fc_fcp_pkt *fsp, struct fc_frame *fp)
 	case -FC_EX_CLOSED:
 		FC_FCP_DBG(fsp, "REC %p fid %6.6x exchange closed\n",
 			   fsp, fsp->rport->port_id);
-		fc_fcp_retry_cmd(fsp);
+		fc_fcp_retry_cmd(fsp, FC_ERROR);
 		break;
 
 	default:
@@ -1731,7 +1733,7 @@ static void fc_fcp_srr(struct fc_fcp_pkt *fsp, enum fc_rctl r_ctl, u32 offset)
 	fc_fcp_pkt_hold(fsp);		/* hold for outstanding SRR */
 	return;
 retry:
-	fc_fcp_retry_cmd(fsp);
+	fc_fcp_retry_cmd(fsp, FC_TRANS_RESET);
 }
 
 /**
@@ -1803,7 +1805,7 @@ static void fc_fcp_srr_error(struct fc_fcp_pkt *fsp, struct fc_frame *fp)
 		FC_FCP_DBG(fsp, "SRR error, exchange closed\n");
 		/* fall through */
 	default:
-		fc_fcp_retry_cmd(fsp);
+		fc_fcp_retry_cmd(fsp, FC_ERROR);
 		break;
 	}
 	fc_fcp_unlock_pkt(fsp);
@@ -2016,6 +2018,11 @@ static void fc_io_compl(struct fc_fcp_pkt *fsp)
 			   "due to FC_CMD_RESET\n");
 		sc_cmd->result = (DID_RESET << 16);
 		break;
+	case FC_TRANS_RESET:
+		FC_FCP_DBG(fsp, "Returning DID_SOFT_ERROR to scsi-ml "
+			   "due to FC_TRANS_RESET\n");
+		sc_cmd->result = (DID_SOFT_ERROR << 16);
+		break;
 	case FC_HRD_ERROR:
 		FC_FCP_DBG(fsp, "Returning DID_NO_CONNECT to scsi-ml "
 			   "due to FC_HRD_ERROR\n");
-- 
1.8.5.6

Re: [PATCH 04/22] libfc: spurious I/O error under high load

[Johannes Thumshirn]

On Wed, Aug 03, 2016 at 03:13:04PM +0200, Hannes Reinecke wrote:
> If a command times out libfc is sending an REC, which also
> might fail (due to frames being lost or something).
> If no data has been transferred we can simply retry
> the command, but the current code sets a state of FC_ERROR,
> which then is being translated into DID_ERROR, resulting
> in an I/O error.
> So to handle this properly we need to set a separate
> state FC_TRANS_RESET and mapping it onto DID_SOFT_RETRY.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---

Can you try to get that message into one line?

>  	case FC_HRD_ERROR:
>  		FC_FCP_DBG(fsp, "Returning DID_NO_CONNECT to scsi-ml "
>  			   "due to FC_HRD_ERROR\n");

Otherwise:

Acked-by: Johannes Thumshirn <jth@kernel.org>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 05/22] libfc: Do not attempt to login if the port is already started

[Hannes Reinecke]

When the port is already started we don't need to login; that
will only confuse the state machine.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libfc/fc_rport.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
index afc1f9b..8dd656e 100644
--- a/drivers/scsi/libfc/fc_rport.c
+++ b/drivers/scsi/libfc/fc_rport.c
@@ -418,6 +418,12 @@ static int fc_rport_login(struct fc_rport_priv *rdata)
 {
 	mutex_lock(&rdata->rp_mutex);
 
+	if (rdata->flags & FC_RP_STARTED) {
+		FC_RPORT_DBG(rdata, "port already started\n");
+		mutex_unlock(&rdata->rp_mutex);
+		return 0;
+	}
+
 	rdata->flags |= FC_RP_STARTED;
 	switch (rdata->rp_state) {
 	case RPORT_ST_READY:
-- 
1.8.5.6

Re: [PATCH 05/22] libfc: Do not attempt to login if the port is already started

[Johannes Thumshirn]

On Wed, Aug 03, 2016 at 03:13:05PM +0200, Hannes Reinecke wrote:
> When the port is already started we don't need to login; that
> will only confuse the state machine.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>

Acked-by: Johannes Thumshirn <jth@kernel.org>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 06/22] libfc: Debug PRLI failures

[Hannes Reinecke]

The initial PRLI is errored out with these messages:
[ 5424.530686] host8: rport 001a1e: Received a PRLI accept
[ 5424.530687] host8: rport 001a1e: PRLI spp_flags = 0x0
[ 5424.530688] host8: rport 001a1e: Error -131938289606656 in state PRLI, retrying

'spp_flags=0' is decidedly dodgy, as we should always return a valid PRLI
state here.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libfc/fc_rport.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
index 8dd656e..1f22c2e 100644
--- a/drivers/scsi/libfc/fc_rport.c
+++ b/drivers/scsi/libfc/fc_rport.c
@@ -1094,8 +1094,8 @@ static void fc_rport_prli_resp(struct fc_seq *sp, struct fc_frame *fp,
 			goto out;
 
 		resp_code = (pp->spp.spp_flags & FC_SPP_RESP_MASK);
-		FC_RPORT_DBG(rdata, "PRLI spp_flags = 0x%x\n",
-			     pp->spp.spp_flags);
+		FC_RPORT_DBG(rdata, "PRLI spp_flags = 0x%x spp_type 0x%x\n",
+			     pp->spp.spp_flags, pp->spp.spp_type);
 		rdata->spp_type = pp->spp.spp_type;
 		if (resp_code != FC_SPP_RESP_ACK) {
 			if (resp_code == FC_SPP_RESP_CONF)
@@ -1805,7 +1805,6 @@ static void fc_rport_recv_prli_req(struct fc_rport_priv *rdata,
 	unsigned int len;
 	unsigned int plen;
 	enum fc_els_spp_resp resp;
-	enum fc_els_spp_resp passive;
 	struct fc_seq_els_data rjt_data;
 	struct fc4_prov *prov;
 
@@ -1855,15 +1854,21 @@ static void fc_rport_recv_prli_req(struct fc_rport_priv *rdata,
 		resp = 0;
 
 		if (rspp->spp_type < FC_FC4_PROV_SIZE) {
+			enum fc_els_spp_resp active = 0, passive = 0;
+
 			prov = fc_active_prov[rspp->spp_type];
 			if (prov)
-				resp = prov->prli(rdata, plen, rspp, spp);
+				active = prov->prli(rdata, plen, rspp, spp);
 			prov = fc_passive_prov[rspp->spp_type];
-			if (prov) {
+			if (prov)
 				passive = prov->prli(rdata, plen, rspp, spp);
-				if (!resp || passive == FC_SPP_RESP_ACK)
-					resp = passive;
-			}
+			if (!active || passive == FC_SPP_RESP_ACK)
+				resp = passive;
+			else
+				resp = active;
+			FC_RPORT_DBG(rdata, "PRLI rspp type %x "
+				     "active %x passive %x\n",
+				     rspp->spp_type, active, passive);
 		}
 		if (!resp) {
 			if (spp->spp_flags & FC_SPP_EST_IMG_PAIR)
-- 
1.8.5.6

Re: [PATCH 06/22] libfc: Debug PRLI failures

[Johannes Thumshirn]

On Wed, Aug 03, 2016 at 03:13:06PM +0200, Hannes Reinecke wrote:
> The initial PRLI is errored out with these messages:
> [ 5424.530686] host8: rport 001a1e: Received a PRLI accept
> [ 5424.530687] host8: rport 001a1e: PRLI spp_flags = 0x0
> [ 5424.530688] host8: rport 001a1e: Error -131938289606656 in state PRLI, retrying
> 
> 'spp_flags=0' is decidedly dodgy, as we should always return a valid PRLI
> state here.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>

Acked-by: Johannes Thumshirn <jth@kernel.org>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 07/22] fcoe: filter out frames from invalid vlans

[Hannes Reinecke]

---
 drivers/scsi/fcoe/fcoe_ctlr.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/fcoe/fcoe_ctlr.c b/drivers/scsi/fcoe/fcoe_ctlr.c
index 1d0bec6..e7a14aa 100644
--- a/drivers/scsi/fcoe/fcoe_ctlr.c
+++ b/drivers/scsi/fcoe/fcoe_ctlr.c
@@ -2710,11 +2710,21 @@ static int fcoe_ctlr_vn_recv(struct fcoe_ctlr *fip, struct sk_buff *skb)
 		struct fc_rport_priv rdata;
 		struct fcoe_rport frport;
 	} buf;
-	int rc;
+	int rc, vlan_id = 0;
 
 	fiph = (struct fip_header *)skb->data;
 	sub = fiph->fip_subcode;
 
+	if (fip->lp->vlan)
+		vlan_id = skb_vlan_tag_get_id(skb);
+
+	if (vlan_id && vlan_id != fip->lp->vlan) {
+		LIBFCOE_FIP_DBG(fip, "vn_recv drop frame sub %x vlan %d\n",
+				sub, vlan_id);
+		rc = -EAGAIN;
+		goto drop;
+	}
+
 	rc = fcoe_ctlr_vn_parse(fip, skb, &buf.rdata);
 	if (rc) {
 		LIBFCOE_FIP_DBG(fip, "vn_recv vn_parse error %d\n", rc);
-- 
1.8.5.6

Re: [PATCH 07/22] fcoe: filter out frames from invalid vlans

[Johannes Thumshirn]

On Wed, Aug 03, 2016 at 03:13:07PM +0200, Hannes Reinecke wrote:
> ---
>  drivers/scsi/fcoe/fcoe_ctlr.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/fcoe/fcoe_ctlr.c b/drivers/scsi/fcoe/fcoe_ctlr.c
> index 1d0bec6..e7a14aa 100644
> --- a/drivers/scsi/fcoe/fcoe_ctlr.c
> +++ b/drivers/scsi/fcoe/fcoe_ctlr.c
> @@ -2710,11 +2710,21 @@ static int fcoe_ctlr_vn_recv(struct fcoe_ctlr *fip, struct sk_buff *skb)
>  		struct fc_rport_priv rdata;
>  		struct fcoe_rport frport;
>  	} buf;
> -	int rc;
> +	int rc, vlan_id = 0;
>  
>  	fiph = (struct fip_header *)skb->data;
>  	sub = fiph->fip_subcode;
>  
> +	if (fip->lp->vlan)
> +		vlan_id = skb_vlan_tag_get_id(skb);
> +
> +	if (vlan_id && vlan_id != fip->lp->vlan) {
> +		LIBFCOE_FIP_DBG(fip, "vn_recv drop frame sub %x vlan %d\n",
> +				sub, vlan_id);
> +		rc = -EAGAIN;
> +		goto drop;
> +	}
> +
>  	rc = fcoe_ctlr_vn_parse(fip, skb, &buf.rdata);
>  	if (rc) {
>  		LIBFCOE_FIP_DBG(fip, "vn_recv vn_parse error %d\n", rc);
> -- 
> 1.8.5.6

-ENOSIGNEDOFF and while you're at it, can you give 1 or 2 lines why we need to
filter it out in the commit message.

Thanks,
	Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 08/22] fcoe: make R_A_TOV and E_D_TOV configurable

[Hannes Reinecke]

The user might want to modify the values for R_A_TOV and E_D_TOV,
so add new module parameters 'e_d_tov' and 'r_a_tov' for the
'fcoe' modules and allow to modify them via sysfs attributes.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/fcoe/fcoe.c       | 12 +++++--
 drivers/scsi/fcoe/fcoe_sysfs.c | 71 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 9bd41a3..c907661 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -63,6 +63,14 @@ unsigned int fcoe_debug_logging;
 module_param_named(debug_logging, fcoe_debug_logging, int, S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(debug_logging, "a bit mask of logging levels");
 
+unsigned int fcoe_e_d_tov = 2 * 1000;
+module_param_named(e_d_tov, fcoe_e_d_tov, int, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(e_d_tov, "E_D_TOV in ms, default 2000");
+
+unsigned int fcoe_r_a_tov = 2 * 2 * 1000;
+module_param_named(r_a_tov, fcoe_r_a_tov, int, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(r_a_tov, "R_A_TOV in ms, default 4000");
+
 static DEFINE_MUTEX(fcoe_config_mutex);
 
 static struct workqueue_struct *fcoe_wq;
@@ -633,8 +641,8 @@ static int fcoe_lport_config(struct fc_lport *lport)
 	lport->qfull = 0;
 	lport->max_retry_count = 3;
 	lport->max_rport_retry_count = 3;
-	lport->e_d_tov = 2 * 1000;	/* FC-FS default */
-	lport->r_a_tov = 2 * 2 * 1000;
+	lport->e_d_tov = fcoe_e_d_tov;
+	lport->r_a_tov = fcoe_r_a_tov;
 	lport->service_params = (FCP_SPPF_INIT_FCN | FCP_SPPF_RD_XRDY_DIS |
 				 FCP_SPPF_RETRY | FCP_SPPF_CONF_COMPL);
 	lport->does_npiv = 1;
diff --git a/drivers/scsi/fcoe/fcoe_sysfs.c b/drivers/scsi/fcoe/fcoe_sysfs.c
index 0675fd1..9e6baac 100644
--- a/drivers/scsi/fcoe/fcoe_sysfs.c
+++ b/drivers/scsi/fcoe/fcoe_sysfs.c
@@ -424,6 +424,75 @@ static FCOE_DEVICE_ATTR(ctlr, fip_vlan_responder, S_IRUGO | S_IWUSR,
 			store_ctlr_fip_resp);
 
 static ssize_t
+fcoe_ctlr_var_store(u32 *var, const char *buf, size_t count)
+{
+	int err;
+	unsigned long v;
+
+	err = kstrtoul(buf, 10, &v);
+	if (err || v > UINT_MAX)
+		return -EINVAL;
+
+	*var = v;
+
+	return count;
+}
+
+static ssize_t store_ctlr_r_a_tov(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct fcoe_ctlr_device *ctlr_dev = dev_to_ctlr(dev);
+	struct fcoe_ctlr *ctlr = fcoe_ctlr_device_priv(ctlr_dev);
+
+	if (ctlr_dev->enabled == FCOE_CTLR_ENABLED)
+		return -EBUSY;
+	if (ctlr_dev->enabled == FCOE_CTLR_DISABLED)
+		return fcoe_ctlr_var_store(&ctlr->lp->r_a_tov, buf, count);
+	return -ENOTSUPP;
+}
+
+static ssize_t show_ctlr_r_a_tov(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct fcoe_ctlr_device *ctlr_dev = dev_to_ctlr(dev);
+	struct fcoe_ctlr *ctlr = fcoe_ctlr_device_priv(ctlr_dev);
+
+	return sprintf(buf, "%d\n", ctlr->lp->r_a_tov);
+}
+
+static FCOE_DEVICE_ATTR(ctlr, r_a_tov, S_IRUGO | S_IWUSR,
+			show_ctlr_r_a_tov, store_ctlr_r_a_tov);
+
+static ssize_t store_ctlr_e_d_tov(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct fcoe_ctlr_device *ctlr_dev = dev_to_ctlr(dev);
+	struct fcoe_ctlr *ctlr = fcoe_ctlr_device_priv(ctlr_dev);
+
+	if (ctlr_dev->enabled == FCOE_CTLR_ENABLED)
+		return -EBUSY;
+	if (ctlr_dev->enabled == FCOE_CTLR_DISABLED)
+		return fcoe_ctlr_var_store(&ctlr->lp->e_d_tov, buf, count);
+	return -ENOTSUPP;
+}
+
+static ssize_t show_ctlr_e_d_tov(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct fcoe_ctlr_device *ctlr_dev = dev_to_ctlr(dev);
+	struct fcoe_ctlr *ctlr = fcoe_ctlr_device_priv(ctlr_dev);
+
+	return sprintf(buf, "%d\n", ctlr->lp->e_d_tov);
+}
+
+static FCOE_DEVICE_ATTR(ctlr, e_d_tov, S_IRUGO | S_IWUSR,
+			show_ctlr_e_d_tov, store_ctlr_e_d_tov);
+
+static ssize_t
 store_private_fcoe_ctlr_fcf_dev_loss_tmo(struct device *dev,
 					 struct device_attribute *attr,
 					 const char *buf, size_t count)
@@ -507,6 +576,8 @@ static struct attribute_group fcoe_ctlr_lesb_attr_group = {
 static struct attribute *fcoe_ctlr_attrs[] = {
 	&device_attr_fcoe_ctlr_fip_vlan_responder.attr,
 	&device_attr_fcoe_ctlr_fcf_dev_loss_tmo.attr,
+	&device_attr_fcoe_ctlr_r_a_tov.attr,
+	&device_attr_fcoe_ctlr_e_d_tov.attr,
 	&device_attr_fcoe_ctlr_enabled.attr,
 	&device_attr_fcoe_ctlr_mode.attr,
 	NULL,
-- 
1.8.5.6

Re: [PATCH 08/22] fcoe: make R_A_TOV and E_D_TOV configurable

[Johannes Thumshirn]

On Wed, Aug 03, 2016 at 03:13:08PM +0200, Hannes Reinecke wrote:
> The user might want to modify the values for R_A_TOV and E_D_TOV,
> so add new module parameters 'e_d_tov' and 'r_a_tov' for the
> 'fcoe' modules and allow to modify them via sysfs attributes.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>

Acked-by: Johannes Thumshirn <jth@kernel.org>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 09/22] libfc: use configured lport R_A_TOV when sending exchange

[Hannes Reinecke]

We should be using the configured R_A_TOV value when sending the
exchange.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libfc/fc_exch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
index dc078d3..51e7fb1 100644
--- a/drivers/scsi/libfc/fc_exch.c
+++ b/drivers/scsi/libfc/fc_exch.c
@@ -2111,7 +2111,7 @@ static struct fc_seq *fc_exch_seq_send(struct fc_lport *lport,
 	ep->resp = resp;
 	ep->destructor = destructor;
 	ep->arg = arg;
-	ep->r_a_tov = FC_DEF_R_A_TOV;
+	ep->r_a_tov = lport->r_a_tov;
 	ep->lp = lport;
 	sp = &ep->seq;
 
-- 
1.8.5.6

Re: [PATCH 09/22] libfc: use configured lport R_A_TOV when sending exchange

[Johannes Thumshirn]

On Wed, Aug 03, 2016 at 03:13:09PM +0200, Hannes Reinecke wrote:
> We should be using the configured R_A_TOV value when sending the
> exchange.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>

Acked-by: Johannes Thumshirn <jth@kernel.org>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 10/22] libfc: use configured e_d_tov for remote port state retries

[Hannes Reinecke]

If fc_rport_error_retry() is attempting to retry the remote
port state we should be waiting for the configured e_d_tov
value rather than the default.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libfc/fc_rport.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
index 1f22c2e..8fad3e9 100644
--- a/drivers/scsi/libfc/fc_rport.c
+++ b/drivers/scsi/libfc/fc_rport.c
@@ -634,7 +634,7 @@ static void fc_rport_error(struct fc_rport_priv *rdata, struct fc_frame *fp)
 static void fc_rport_error_retry(struct fc_rport_priv *rdata,
 				 struct fc_frame *fp)
 {
-	unsigned long delay = msecs_to_jiffies(FC_DEF_E_D_TOV);
+	unsigned long delay = msecs_to_jiffies(rdata->e_d_tov);
 	struct fc_lport *lport = rdata->local_port;
 
 	/* make sure this isn't an FC_EX_CLOSED error, never retry those */
-- 
1.8.5.6

Re: [PATCH 10/22] libfc: use configured e_d_tov for remote port state retries

[Johannes Thumshirn]

On Wed, Aug 03, 2016 at 03:13:10PM +0200, Hannes Reinecke wrote:
> If fc_rport_error_retry() is attempting to retry the remote
> port state we should be waiting for the configured e_d_tov
> value rather than the default.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>

Acked-by: Johannes Thumshirn <jth@kernel.org>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 11/22] fcoe: inhibit writing invalid values into the 'enabled' attribute

[Hannes Reinecke]

The 'enabled' sysfs attribute only accepts the values '0' and '1',
so we should error out any other values.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/fcoe/fcoe_sysfs.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe_sysfs.c b/drivers/scsi/fcoe/fcoe_sysfs.c
index 9e6baac..9cf3d56 100644
--- a/drivers/scsi/fcoe/fcoe_sysfs.c
+++ b/drivers/scsi/fcoe/fcoe_sysfs.c
@@ -335,16 +335,24 @@ static ssize_t store_ctlr_enabled(struct device *dev,
 				  const char *buf, size_t count)
 {
 	struct fcoe_ctlr_device *ctlr = dev_to_ctlr(dev);
+	bool enabled;
 	int rc;
 
+	if (*buf == '1')
+		enabled = true;
+	else if (*buf == '0')
+		enabled = false;
+	else
+		return -EINVAL;
+
 	switch (ctlr->enabled) {
 	case FCOE_CTLR_ENABLED:
-		if (*buf == '1')
+		if (enabled)
 			return count;
 		ctlr->enabled = FCOE_CTLR_DISABLED;
 		break;
 	case FCOE_CTLR_DISABLED:
-		if (*buf == '0')
+		if (!enabled)
 			return count;
 		ctlr->enabled = FCOE_CTLR_ENABLED;
 		break;
-- 
1.8.5.6

Re: [PATCH 11/22] fcoe: inhibit writing invalid values into the 'enabled' attribute

[Bart Van Assche]

On 08/03/2016 06:13 AM, Hannes Reinecke wrote:
> The 'enabled' sysfs attribute only accepts the values '0' and '1',
> so we should error out any other values.
>
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/fcoe/fcoe_sysfs.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/fcoe/fcoe_sysfs.c b/drivers/scsi/fcoe/fcoe_sysfs.c
> index 9e6baac..9cf3d56 100644
> --- a/drivers/scsi/fcoe/fcoe_sysfs.c
> +++ b/drivers/scsi/fcoe/fcoe_sysfs.c
> @@ -335,16 +335,24 @@ static ssize_t store_ctlr_enabled(struct device *dev,
>  				  const char *buf, size_t count)
>  {
>  	struct fcoe_ctlr_device *ctlr = dev_to_ctlr(dev);
> +	bool enabled;
>  	int rc;
>
> +	if (*buf == '1')
> +		enabled = true;
> +	else if (*buf == '0')
> +		enabled = false;
> +	else
> +		return -EINVAL;
> +
>  	switch (ctlr->enabled) {
>  	case FCOE_CTLR_ENABLED:
> -		if (*buf == '1')
> +		if (enabled)
>  			return count;
>  		ctlr->enabled = FCOE_CTLR_DISABLED;
>  		break;
>  	case FCOE_CTLR_DISABLED:
> -		if (*buf == '0')
> +		if (!enabled)
>  			return count;
>  		ctlr->enabled = FCOE_CTLR_ENABLED;
>  		break;

Hello Hannes,

Please use kstrtoint() or similar such that strings like "01" are parsed 
correctly.

Thanks,

Bart.

Re: [PATCH 11/22] fcoe: inhibit writing invalid values into the 'enabled' attribute

[Hannes Reinecke]

On 08/03/2016 07:25 PM, Bart Van Assche wrote:
> On 08/03/2016 06:13 AM, Hannes Reinecke wrote:
>> The 'enabled' sysfs attribute only accepts the values '0' and '1',
>> so we should error out any other values.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>> ---
>>  drivers/scsi/fcoe/fcoe_sysfs.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/fcoe/fcoe_sysfs.c
>> b/drivers/scsi/fcoe/fcoe_sysfs.c
>> index 9e6baac..9cf3d56 100644
>> --- a/drivers/scsi/fcoe/fcoe_sysfs.c
>> +++ b/drivers/scsi/fcoe/fcoe_sysfs.c
>> @@ -335,16 +335,24 @@ static ssize_t store_ctlr_enabled(struct device
>> *dev,
>>                    const char *buf, size_t count)
>>  {
>>      struct fcoe_ctlr_device *ctlr = dev_to_ctlr(dev);
>> +    bool enabled;
>>      int rc;
>>
>> +    if (*buf == '1')
>> +        enabled = true;
>> +    else if (*buf == '0')
>> +        enabled = false;
>> +    else
>> +        return -EINVAL;
>> +
>>      switch (ctlr->enabled) {
>>      case FCOE_CTLR_ENABLED:
>> -        if (*buf == '1')
>> +        if (enabled)
>>              return count;
>>          ctlr->enabled = FCOE_CTLR_DISABLED;
>>          break;
>>      case FCOE_CTLR_DISABLED:
>> -        if (*buf == '0')
>> +        if (!enabled)
>>              return count;
>>          ctlr->enabled = FCOE_CTLR_ENABLED;
>>          break;
> 
> Hello Hannes,
> 
> Please use kstrtoint() or similar such that strings like "01" are parsed
> correctly.
> 
Yeah, I'll be redoing them; the store and show methods in that entire
file could do with an overall simplification.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[PATCH 12/22] libfc: don't fail sequence abort for completed exchanges

[Hannes Reinecke]

If a sequence should be aborted the exchange might already
be completed (eg if the response is still queued in the rx
queue), so this shouldn't considered as an error.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libfc/fc_fcp.c | 40 ++++++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/libfc/fc_fcp.c b/drivers/scsi/libfc/fc_fcp.c
index 289c481..ceb4a65 100644
--- a/drivers/scsi/libfc/fc_fcp.c
+++ b/drivers/scsi/libfc/fc_fcp.c
@@ -258,6 +258,17 @@ static void fc_fcp_timer_set(struct fc_fcp_pkt *fsp, unsigned long delay)
 		mod_timer(&fsp->timer, jiffies + delay);
 }
 
+static void fc_fcp_abort_done(struct fc_fcp_pkt *fsp)
+{
+	fsp->state |= FC_SRB_ABORTED;
+	fsp->state &= ~FC_SRB_ABORT_PENDING;
+
+	if (fsp->wait_for_comp)
+		complete(&fsp->tm_done);
+	else
+		fc_fcp_complete_locked(fsp);
+}
+
 /**
  * fc_fcp_send_abort() - Send an abort for exchanges associated with a
  *			 fcp_pkt
@@ -265,6 +276,8 @@ static void fc_fcp_timer_set(struct fc_fcp_pkt *fsp, unsigned long delay)
  */
 static int fc_fcp_send_abort(struct fc_fcp_pkt *fsp)
 {
+	int rc;
+
 	if (!fsp->seq_ptr)
 		return -EINVAL;
 
@@ -272,7 +285,16 @@ static int fc_fcp_send_abort(struct fc_fcp_pkt *fsp)
 	put_cpu();
 
 	fsp->state |= FC_SRB_ABORT_PENDING;
-	return fsp->lp->tt.seq_exch_abort(fsp->seq_ptr, 0);
+	rc = fsp->lp->tt.seq_exch_abort(fsp->seq_ptr, 0);
+	/*
+	 * ->seq_exch_abort() might return -ENXIO if
+	 * the sequence is already completed
+	 */
+	if (rc == -ENXIO) {
+		fc_fcp_abort_done(fsp);
+		rc = 0;
+	}
+	return rc;
 }
 
 /**
@@ -728,15 +750,8 @@ static void fc_fcp_abts_resp(struct fc_fcp_pkt *fsp, struct fc_frame *fp)
 		ba_done = 0;
 	}
 
-	if (ba_done) {
-		fsp->state |= FC_SRB_ABORTED;
-		fsp->state &= ~FC_SRB_ABORT_PENDING;
-
-		if (fsp->wait_for_comp)
-			complete(&fsp->tm_done);
-		else
-			fc_fcp_complete_locked(fsp);
-	}
+	if (ba_done)
+		fc_fcp_abort_done(fsp);
 }
 
 /**
@@ -1242,6 +1257,11 @@ static int fc_fcp_pkt_abort(struct fc_fcp_pkt *fsp)
 		return FAILED;
 	}
 
+	if (fsp->state & FC_SRB_ABORTED) {
+		FC_FCP_DBG(fsp, "target abort cmd  completed\n");
+		return SUCCESS;
+	}
+
 	init_completion(&fsp->tm_done);
 	fsp->wait_for_comp = 1;
 
-- 
1.8.5.6

Re: [PATCH 12/22] libfc: don't fail sequence abort for completed exchanges

[Johannes Thumshirn]

On Wed, Aug 03, 2016 at 03:13:12PM +0200, Hannes Reinecke wrote:
> If a sequence should be aborted the exchange might already
> be completed (eg if the response is still queued in the rx
> queue), so this shouldn't considered as an error.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>

Acked-by: Johannes Thumshirn <jth@kernel.org>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 13/22] libfc: do not overwrite DID_TIME_OUT status

[Hannes Reinecke]

When a command is aborted it might already have the DID_TIME_OUT
status set, so we shouldn't be overwriting that.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libfc/fc_fcp.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/libfc/fc_fcp.c b/drivers/scsi/libfc/fc_fcp.c
index ceb4a65..7aa28a6 100644
--- a/drivers/scsi/libfc/fc_fcp.c
+++ b/drivers/scsi/libfc/fc_fcp.c
@@ -2029,9 +2029,15 @@ static void fc_io_compl(struct fc_fcp_pkt *fsp)
 		sc_cmd->result = (DID_ERROR << 16) | fsp->cdb_status;
 		break;
 	case FC_CMD_ABORTED:
-		FC_FCP_DBG(fsp, "Returning DID_ERROR to scsi-ml "
-			  "due to FC_CMD_ABORTED\n");
-		sc_cmd->result = (DID_ERROR << 16) | fsp->io_status;
+		if (host_byte(sc_cmd->result) == DID_TIME_OUT)
+			FC_FCP_DBG(fsp, "Returning DID_TIME_OUT to scsi-ml "
+				   "due to FC_CMD_ABORTED\n");
+		else {
+			FC_FCP_DBG(fsp, "Returning DID_ERROR to scsi-ml "
+				   "due to FC_CMD_ABORTED\n");
+			set_host_byte(sc_cmd, DID_ERROR);
+		}
+		sc_cmd->result |= fsp->io_status;
 		break;
 	case FC_CMD_RESET:
 		FC_FCP_DBG(fsp, "Returning DID_RESET to scsi-ml "
-- 
1.8.5.6

Re: [PATCH 13/22] libfc: do not overwrite DID_TIME_OUT status

[Johannes Thumshirn]

On Wed, Aug 03, 2016 at 03:13:13PM +0200, Hannes Reinecke wrote:
> When a command is aborted it might already have the DID_TIME_OUT
> status set, so we shouldn't be overwriting that.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>

Acked-by: Johannes Thumshirn <jth@kernel.org>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 14/22] libfc: use error code for fc_rport_error()

[Hannes Reinecke]

We only ever use the 'fp' argument for fc_rport_error() to
encapsulate the error code, so we can as well do away with that
and pass the error directly.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libfc/fc_rport.c | 97 +++++++++++++++++++++++++------------------
 include/scsi/libfc.h          |  5 +++
 2 files changed, 62 insertions(+), 40 deletions(-)

diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
index 8fad3e9..ec608ef 100644
--- a/drivers/scsi/libfc/fc_rport.c
+++ b/drivers/scsi/libfc/fc_rport.c
@@ -85,8 +85,8 @@ static void fc_rport_recv_prli_req(struct fc_rport_priv *, struct fc_frame *);
 static void fc_rport_recv_prlo_req(struct fc_rport_priv *, struct fc_frame *);
 static void fc_rport_recv_logo_req(struct fc_lport *, struct fc_frame *);
 static void fc_rport_timeout(struct work_struct *);
-static void fc_rport_error(struct fc_rport_priv *, struct fc_frame *);
-static void fc_rport_error_retry(struct fc_rport_priv *, struct fc_frame *);
+static void fc_rport_error(struct fc_rport_priv *, int);
+static void fc_rport_error_retry(struct fc_rport_priv *, int);
 static void fc_rport_work(struct work_struct *);
 
 static const char *fc_rport_state_names[] = {
@@ -584,18 +584,17 @@ static void fc_rport_timeout(struct work_struct *work)
 /**
  * fc_rport_error() - Error handler, called once retries have been exhausted
  * @rdata: The remote port the error is happened on
- * @fp:	   The error code encapsulated in a frame pointer
+ * @err:   The error code
  *
  * Locking Note: The rport lock is expected to be held before
  * calling this routine
  *
  * Reference counting: does not modify kref
  */
-static void fc_rport_error(struct fc_rport_priv *rdata, struct fc_frame *fp)
+static void fc_rport_error(struct fc_rport_priv *rdata, int err)
 {
-	FC_RPORT_DBG(rdata, "Error %ld in state %s, retries %d\n",
-		     IS_ERR(fp) ? -PTR_ERR(fp) : 0,
-		     fc_rport_state(rdata), rdata->retries);
+	FC_RPORT_DBG(rdata, "Error %d in state %s, retries %d\n",
+		     -err, fc_rport_state(rdata), rdata->retries);
 
 	switch (rdata->rp_state) {
 	case RPORT_ST_FLOGI:
@@ -621,7 +620,7 @@ static void fc_rport_error(struct fc_rport_priv *rdata, struct fc_frame *fp)
 /**
  * fc_rport_error_retry() - Handler for remote port state retries
  * @rdata: The remote port whose state is to be retried
- * @fp:	   The error code encapsulated in a frame pointer
+ * @err:   The error code
  *
  * If the error was an exchange timeout retry immediately,
  * otherwise wait for E_D_TOV.
@@ -631,22 +630,21 @@ static void fc_rport_error(struct fc_rport_priv *rdata, struct fc_frame *fp)
  *
  * Reference counting: increments kref when scheduling retry_work
  */
-static void fc_rport_error_retry(struct fc_rport_priv *rdata,
-				 struct fc_frame *fp)
+static void fc_rport_error_retry(struct fc_rport_priv *rdata, int err)
 {
 	unsigned long delay = msecs_to_jiffies(rdata->e_d_tov);
 	struct fc_lport *lport = rdata->local_port;
 
 	/* make sure this isn't an FC_EX_CLOSED error, never retry those */
-	if (PTR_ERR(fp) == -FC_EX_CLOSED)
+	if (err == -FC_EX_CLOSED)
 		goto out;
 
 	if (rdata->retries < rdata->local_port->max_rport_retry_count) {
-		FC_RPORT_DBG(rdata, "Error %ld in state %s, retrying\n",
-			     PTR_ERR(fp), fc_rport_state(rdata));
+		FC_RPORT_DBG(rdata, "Error %d in state %s, retrying\n",
+			     err, fc_rport_state(rdata));
 		rdata->retries++;
 		/* no additional delay on exchange timeouts */
-		if (PTR_ERR(fp) == -FC_EX_TIMEOUT)
+		if (err == -FC_EX_TIMEOUT)
 			delay = 0;
 		kref_get(&rdata->kref);
 		if (!schedule_delayed_work(&rdata->retry_work, delay))
@@ -655,7 +653,7 @@ static void fc_rport_error_retry(struct fc_rport_priv *rdata,
 	}
 
 out:
-	fc_rport_error(rdata, fp);
+	fc_rport_error(rdata, err);
 }
 
 /**
@@ -715,8 +713,10 @@ static void fc_rport_flogi_resp(struct fc_seq *sp, struct fc_frame *fp,
 	struct fc_lport *lport = rdata->local_port;
 	struct fc_els_flogi *flogi;
 	unsigned int r_a_tov;
+	int err = 0;
 
-	FC_RPORT_DBG(rdata, "Received a FLOGI %s\n", fc_els_resp_type(fp));
+	FC_RPORT_DBG(rdata, "Received a FLOGI %s\n",
+		     IS_ERR(fp)? "error" : fc_els_resp_type(fp));
 
 	if (fp == ERR_PTR(-FC_EX_CLOSED))
 		goto put;
@@ -732,18 +732,30 @@ static void fc_rport_flogi_resp(struct fc_seq *sp, struct fc_frame *fp,
 	}
 
 	if (IS_ERR(fp)) {
-		fc_rport_error(rdata, fp);
+		fc_rport_error(rdata, PTR_ERR(fp));
 		goto err;
 	}
 
-	if (fc_frame_payload_op(fp) != ELS_LS_ACC)
+	if (fc_frame_payload_op(fp) != ELS_LS_ACC) {
+		struct fc_els_ls_rjt *rjt;
+
+		rjt = fc_frame_payload_get(fp, sizeof (*rjt));
+		FC_RPORT_DBG(rdata, "FLOGI ELS rejected, reason %x expl %x\n",
+			     rjt->er_reason, rjt->er_explan);
+		err = -FC_EX_ELS_RJT;
 		goto bad;
-	if (fc_rport_login_complete(rdata, fp))
+	}
+	if (fc_rport_login_complete(rdata, fp)) {
+		FC_RPORT_DBG(rdata, "FLOGI failed, no login\n");
+		err = -FC_EX_INV_LOGIN;
 		goto bad;
+	}
 
 	flogi = fc_frame_payload_get(fp, sizeof(*flogi));
-	if (!flogi)
+	if (!flogi) {
+		err = -FC_EX_ALLOC_ERR;
 		goto bad;
+	}
 	r_a_tov = ntohl(flogi->fl_csp.sp_r_a_tov);
 	if (r_a_tov > rdata->r_a_tov)
 		rdata->r_a_tov = r_a_tov;
@@ -761,7 +773,7 @@ put:
 	return;
 bad:
 	FC_RPORT_DBG(rdata, "Bad FLOGI response\n");
-	fc_rport_error_retry(rdata, fp);
+	fc_rport_error_retry(rdata, err);
 	goto out;
 }
 
@@ -789,13 +801,13 @@ static void fc_rport_enter_flogi(struct fc_rport_priv *rdata)
 
 	fp = fc_frame_alloc(lport, sizeof(struct fc_els_flogi));
 	if (!fp)
-		return fc_rport_error_retry(rdata, fp);
+		return fc_rport_error_retry(rdata, -FC_EX_ALLOC_ERR);
 
 	kref_get(&rdata->kref);
 	if (!lport->tt.elsct_send(lport, rdata->ids.port_id, fp, ELS_FLOGI,
 				  fc_rport_flogi_resp, rdata,
 				  2 * lport->r_a_tov)) {
-		fc_rport_error_retry(rdata, NULL);
+		fc_rport_error_retry(rdata, -FC_EX_XMIT_ERR);
 		kref_put(&rdata->kref, lport->tt.rport_destroy);
 	}
 }
@@ -952,7 +964,7 @@ static void fc_rport_plogi_resp(struct fc_seq *sp, struct fc_frame *fp,
 	}
 
 	if (IS_ERR(fp)) {
-		fc_rport_error_retry(rdata, fp);
+		fc_rport_error_retry(rdata, PTR_ERR(fp));
 		goto err;
 	}
 
@@ -974,9 +986,14 @@ static void fc_rport_plogi_resp(struct fc_seq *sp, struct fc_frame *fp,
 		rdata->max_seq = csp_seq;
 		rdata->maxframe_size = fc_plogi_get_maxframe(plp, lport->mfs);
 		fc_rport_enter_prli(rdata);
-	} else
-		fc_rport_error_retry(rdata, fp);
+	} else {
+		struct fc_els_ls_rjt *rjt;
 
+		rjt = fc_frame_payload_get(fp, sizeof (*rjt));
+		FC_RPORT_DBG(rdata, "PLOGI ELS rejected, reason %x expl %x\n",
+			     rjt->er_reason, rjt->er_explan);
+		fc_rport_error_retry(rdata, -FC_EX_ELS_RJT);
+	}
 out:
 	fc_frame_free(fp);
 err:
@@ -1027,7 +1044,7 @@ static void fc_rport_enter_plogi(struct fc_rport_priv *rdata)
 	fp = fc_frame_alloc(lport, sizeof(struct fc_els_flogi));
 	if (!fp) {
 		FC_RPORT_DBG(rdata, "%s frame alloc failed\n", __func__);
-		fc_rport_error_retry(rdata, fp);
+		fc_rport_error_retry(rdata, -FC_EX_ALLOC_ERR);
 		return;
 	}
 	rdata->e_d_tov = lport->e_d_tov;
@@ -1036,7 +1053,7 @@ static void fc_rport_enter_plogi(struct fc_rport_priv *rdata)
 	if (!lport->tt.elsct_send(lport, rdata->ids.port_id, fp, ELS_PLOGI,
 				  fc_rport_plogi_resp, rdata,
 				  2 * lport->r_a_tov)) {
-		fc_rport_error_retry(rdata, NULL);
+		fc_rport_error_retry(rdata, -FC_EX_XMIT_ERR);
 		kref_put(&rdata->kref, lport->tt.rport_destroy);
 	}
 }
@@ -1080,7 +1097,7 @@ static void fc_rport_prli_resp(struct fc_seq *sp, struct fc_frame *fp,
 	}
 
 	if (IS_ERR(fp)) {
-		fc_rport_error_retry(rdata, fp);
+		fc_rport_error_retry(rdata, PTR_ERR(fp));
 		goto err;
 	}
 
@@ -1099,9 +1116,9 @@ static void fc_rport_prli_resp(struct fc_seq *sp, struct fc_frame *fp,
 		rdata->spp_type = pp->spp.spp_type;
 		if (resp_code != FC_SPP_RESP_ACK) {
 			if (resp_code == FC_SPP_RESP_CONF)
-				fc_rport_error(rdata, fp);
+				fc_rport_error(rdata, -FC_EX_SEQ_ERR);
 			else
-				fc_rport_error_retry(rdata, fp);
+				fc_rport_error_retry(rdata, -FC_EX_SEQ_ERR);
 			goto out;
 		}
 		if (pp->prli.prli_spp_len < sizeof(pp->spp))
@@ -1133,7 +1150,7 @@ static void fc_rport_prli_resp(struct fc_seq *sp, struct fc_frame *fp,
 		rjt = fc_frame_payload_get(fp, sizeof (*rjt));
 		FC_RPORT_DBG(rdata, "PRLI ELS rejected, reason %x expl %x\n",
 			     rjt->er_reason, rjt->er_explan);
-		fc_rport_error_retry(rdata, NULL);
+		fc_rport_error_retry(rdata, FC_EX_ELS_RJT);
 	}
 
 out:
@@ -1178,7 +1195,7 @@ static void fc_rport_enter_prli(struct fc_rport_priv *rdata)
 
 	fp = fc_frame_alloc(lport, sizeof(*pp));
 	if (!fp) {
-		fc_rport_error_retry(rdata, fp);
+		fc_rport_error_retry(rdata, -FC_EX_ALLOC_ERR);
 		return;
 	}
 
@@ -1197,7 +1214,7 @@ static void fc_rport_enter_prli(struct fc_rport_priv *rdata)
 	kref_get(&rdata->kref);
 	if (!lport->tt.exch_seq_send(lport, fp, fc_rport_prli_resp,
 				     NULL, rdata, 2 * lport->r_a_tov)) {
-		fc_rport_error_retry(rdata, NULL);
+		fc_rport_error_retry(rdata, -FC_EX_XMIT_ERR);
 		kref_put(&rdata->kref, lport->tt.rport_destroy);
 	}
 }
@@ -1233,7 +1250,7 @@ static void fc_rport_rtv_resp(struct fc_seq *sp, struct fc_frame *fp,
 	}
 
 	if (IS_ERR(fp)) {
-		fc_rport_error(rdata, fp);
+		fc_rport_error(rdata, PTR_ERR(fp));
 		goto err;
 	}
 
@@ -1289,7 +1306,7 @@ static void fc_rport_enter_rtv(struct fc_rport_priv *rdata)
 
 	fp = fc_frame_alloc(lport, sizeof(struct fc_els_rtv));
 	if (!fp) {
-		fc_rport_error_retry(rdata, fp);
+		fc_rport_error_retry(rdata, -FC_EX_ALLOC_ERR);
 		return;
 	}
 
@@ -1297,7 +1314,7 @@ static void fc_rport_enter_rtv(struct fc_rport_priv *rdata)
 	if (!lport->tt.elsct_send(lport, rdata->ids.port_id, fp, ELS_RTV,
 				  fc_rport_rtv_resp, rdata,
 				  2 * lport->r_a_tov)) {
-		fc_rport_error_retry(rdata, NULL);
+		fc_rport_error_retry(rdata, -FC_EX_XMIT_ERR);
 		kref_put(&rdata->kref, lport->tt.rport_destroy);
 	}
 }
@@ -1377,7 +1394,7 @@ static void fc_rport_adisc_resp(struct fc_seq *sp, struct fc_frame *fp,
 	}
 
 	if (IS_ERR(fp)) {
-		fc_rport_error(rdata, fp);
+		fc_rport_error(rdata, PTR_ERR(fp));
 		goto err;
 	}
 
@@ -1426,14 +1443,14 @@ static void fc_rport_enter_adisc(struct fc_rport_priv *rdata)
 
 	fp = fc_frame_alloc(lport, sizeof(struct fc_els_adisc));
 	if (!fp) {
-		fc_rport_error_retry(rdata, fp);
+		fc_rport_error_retry(rdata, -FC_EX_ALLOC_ERR);
 		return;
 	}
 	kref_get(&rdata->kref);
 	if (!lport->tt.elsct_send(lport, rdata->ids.port_id, fp, ELS_ADISC,
 				  fc_rport_adisc_resp, rdata,
 				  2 * lport->r_a_tov)) {
-		fc_rport_error_retry(rdata, NULL);
+		fc_rport_error_retry(rdata, -FC_EX_XMIT_ERR);
 		kref_put(&rdata->kref, lport->tt.rport_destroy);
 	}
 }
diff --git a/include/scsi/libfc.h b/include/scsi/libfc.h
index 7428a53..dc42d80 100644
--- a/include/scsi/libfc.h
+++ b/include/scsi/libfc.h
@@ -44,6 +44,11 @@
 #define	FC_NO_ERR	0	/* no error */
 #define	FC_EX_TIMEOUT	1	/* Exchange timeout */
 #define	FC_EX_CLOSED	2	/* Exchange closed */
+#define FC_EX_ALLOC_ERR	3	/* Exchange allocation failed */
+#define FC_EX_XMIT_ERR	4	/* Exchange transmit failed */
+#define FC_EX_ELS_RJT	5	/* ELS rejected */
+#define FC_EX_INV_LOGIN	6	/* Login not completed */
+#define FC_EX_SEQ_ERR	6	/* Exchange sequence error */
 
 /**
  * enum fc_lport_state - Local port states
-- 
1.8.5.6

Re: [PATCH 14/22] libfc: use error code for fc_rport_error()

[Johannes Thumshirn]

On Wed, Aug 03, 2016 at 03:13:14PM +0200, Hannes Reinecke wrote:
> We only ever use the 'fp' argument for fc_rport_error() to
> encapsulate the error code, so we can as well do away with that
> and pass the error directly.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>

Acked-by: Johannes Thumshirn <jth@kernel.org>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 15/22] libfc: frame alloc failure messages

[Hannes Reinecke]

---
 drivers/scsi/libfc/fc_exch.c | 41 +++++++++++++++++++++++++++++++++--------
 1 file changed, 33 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
index 51e7fb1..26a43b3 100644
--- a/drivers/scsi/libfc/fc_exch.c
+++ b/drivers/scsi/libfc/fc_exch.c
@@ -1264,8 +1264,10 @@ static void fc_seq_send_ack(struct fc_seq *sp, const struct fc_frame *rx_fp)
 	 */
 	if (fc_sof_needs_ack(fr_sof(rx_fp))) {
 		fp = fc_frame_alloc(lport, 0);
-		if (!fp)
+		if (!fp) {
+			FC_EXCH_DBG(ep, "Drop ACK request, out of memory\n");
 			return;
+		}
 
 		fh = fc_frame_header_get(fp);
 		fh->fh_r_ctl = FC_RCTL_ACK_1;
@@ -1318,13 +1320,18 @@ static void fc_exch_send_ba_rjt(struct fc_frame *rx_fp,
 	struct fc_frame_header *rx_fh;
 	struct fc_frame_header *fh;
 	struct fc_ba_rjt *rp;
+	struct fc_seq *sp;
 	struct fc_lport *lport;
 	unsigned int f_ctl;
 
 	lport = fr_dev(rx_fp);
+	sp = fr_seq(rx_fp);
 	fp = fc_frame_alloc(lport, sizeof(*rp));
-	if (!fp)
+	if (!fp) {
+		FC_EXCH_DBG(fc_seq_exch(sp),
+			     "Drop BA_RJT request, out of memory\n");
 		return;
+	}
 	fh = fc_frame_header_get(fp);
 	rx_fh = fc_frame_header_get(rx_fp);
 
@@ -1391,13 +1398,15 @@ static void fc_exch_recv_abts(struct fc_exch *ep, struct fc_frame *rx_fp)
 
 	FC_EXCH_DBG(ep, "exch: ABTS received\n");
 	fp = fc_frame_alloc(ep->lp, sizeof(*ap));
-	if (!fp)
+	if (!fp) {
+		FC_EXCH_DBG(ep, "Drop ABTS request, out of memory\n");
 		goto free;
+	}
 
 	spin_lock_bh(&ep->ex_lock);
 	if (ep->esb_stat & ESB_ST_COMPLETE) {
 		spin_unlock_bh(&ep->ex_lock);
-
+		FC_EXCH_DBG(ep, "exch: ABTS rejected, exchange complete\n");
 		fc_frame_free(fp);
 		goto reject;
 	}
@@ -1791,11 +1800,16 @@ static void fc_seq_ls_acc(struct fc_frame *rx_fp)
 	struct fc_lport *lport;
 	struct fc_els_ls_acc *acc;
 	struct fc_frame *fp;
+	struct fc_seq *sp;
 
 	lport = fr_dev(rx_fp);
+	sp = fr_seq(rx_fp);
 	fp = fc_frame_alloc(lport, sizeof(*acc));
-	if (!fp)
+	if (!fp) {
+		FC_EXCH_DBG(fc_seq_exch(sp),
+			    "exch: drop LS_ACC, out of memory\n");
 		return;
+	}
 	acc = fc_frame_payload_get(fp, sizeof(*acc));
 	memset(acc, 0, sizeof(*acc));
 	acc->la_cmd = ELS_LS_ACC;
@@ -1818,11 +1832,16 @@ static void fc_seq_ls_rjt(struct fc_frame *rx_fp, enum fc_els_rjt_reason reason,
 	struct fc_lport *lport;
 	struct fc_els_ls_rjt *rjt;
 	struct fc_frame *fp;
+	struct fc_seq *sp;
 
 	lport = fr_dev(rx_fp);
+	sp = fr_seq(rx_fp);
 	fp = fc_frame_alloc(lport, sizeof(*rjt));
-	if (!fp)
+	if (!fp) {
+		FC_EXCH_DBG(fc_seq_exch(sp),
+			    "exch: drop LS_ACC, out of memory\n");
 		return;
+	}
 	rjt = fc_frame_payload_get(fp, sizeof(*rjt));
 	memset(rjt, 0, sizeof(*rjt));
 	rjt->er_cmd = ELS_LS_RJT;
@@ -1983,8 +2002,12 @@ static void fc_exch_els_rec(struct fc_frame *rfp)
 	ep = fc_exch_lookup(lport,
 			    sid == fc_host_port_id(lport->host) ? oxid : rxid);
 	explan = ELS_EXPL_OXID_RXID;
-	if (!ep)
+	if (!ep) {
+		FC_LPORT_DBG(lport, "REC request from %x: "
+			     "xid %4.4x-%4.4x not found\n",
+			     sid, rxid, oxid);
 		goto reject;
+	}
 	FC_EXCH_DBG(ep, "REC request from %x: rxid %x oxid %x\n",
 		    sid, rxid, oxid);
 	if (ep->oid != sid || oxid != ep->oxid)
@@ -1992,8 +2015,10 @@ static void fc_exch_els_rec(struct fc_frame *rfp)
 	if (rxid != FC_XID_UNKNOWN && rxid != ep->rxid)
 		goto rel;
 	fp = fc_frame_alloc(lport, sizeof(*acc));
-	if (!fp)
+	if (!fp) {
+		FC_EXCH_DBG(ep, "Drop REC request, out of memory\n");
 		goto out;
+	}
 
 	acc = fc_frame_payload_get(fp, sizeof(*acc));
 	memset(acc, 0, sizeof(*acc));
-- 
1.8.5.6

Re: [PATCH 15/22] libfc: frame alloc failure messages

[Johannes Thumshirn]

On Wed, Aug 03, 2016 at 03:13:15PM +0200, Hannes Reinecke wrote:
> ---
>  drivers/scsi/libfc/fc_exch.c | 41 +++++++++++++++++++++++++++++++++--------
>  1 file changed, 33 insertions(+), 8 deletions(-)
> 

Missing Signed-off-by...

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 16/22] fc: add missing ELS explanation values

[Hannes Reinecke]

Add missing ELS RJT explanation values from FC-LS3.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 include/uapi/scsi/fc/fc_els.h | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/include/uapi/scsi/fc/fc_els.h b/include/uapi/scsi/fc/fc_els.h
index 481abbd..989965f 100644
--- a/include/uapi/scsi/fc/fc_els.h
+++ b/include/uapi/scsi/fc/fc_els.h
@@ -203,18 +203,33 @@ enum fc_els_rjt_explan {
 	ELS_EXPL_NONE =		0x00,	/* No additional explanation */
 	ELS_EXPL_SPP_OPT_ERR =	0x01,	/* service parameter error - options */
 	ELS_EXPL_SPP_ICTL_ERR =	0x03,	/* service parm error - initiator ctl */
+	ELS_EXPL_SPP_RCTL_ERR = 0x05,	/* service parm error - recipient ctl */
+	ELS_EXPL_SPP_RDF_ERR =	0x07,	/* service parm error - receive data field size */
+	ELS_EXPL_SPP_SEQ_ERR =	0x09,	/* service parm error - concurrrent seq */
+	ELS_EXPL_SPP_CRED_ERR =	0x0b,	/* service parm error - credit */
+	ELS_EXPL_PORT_NAME =	0x0d,	/* invalid n_port/f_port name */
+	ELS_EXPL_NODE_NAME =	0x0e,	/* invalid node/fabric name */
+	ELS_EXPL_CSP =		0x0f,	/* invalid common service parameters */
 	ELS_EXPL_AH =		0x11,	/* invalid association header */
 	ELS_EXPL_AH_REQ =	0x13,	/* association_header required */
 	ELS_EXPL_SID =		0x15,	/* invalid originator S_ID */
 	ELS_EXPL_OXID_RXID =	0x17,	/* invalid OX_ID-RX_ID combination */
 	ELS_EXPL_INPROG =	0x19,	/* Request already in progress */
 	ELS_EXPL_PLOGI_REQD =	0x1e,	/* N_Port login required */
+	ELS_EXPL_PORT_ID =	0x1f,	/* invalid N_Port ID */
 	ELS_EXPL_INSUF_RES =	0x29,	/* insufficient resources */
 	ELS_EXPL_UNAB_DATA =	0x2a,	/* unable to supply requested data */
 	ELS_EXPL_UNSUPR =	0x2c,	/* Request not supported */
 	ELS_EXPL_INV_LEN =	0x2d,	/* Invalid payload length */
+	ELS_EXPL_INV_PORT =	0x44,	/* Invalid Port/Node name */
+	ELS_EXPL_LOGIN_EXT =	0x46,	/* Login extension not supported */
+	ELS_EXPL_AUTH =		0x48,	/* Authentification required */
+	ELS_EXPL_VAL_SCAN =	0x50,	/* Periodic scan value not allowed */
+	ELS_EXPL_INV_SCAN =	0x51,	/* Periodic scanning not supported */
+	ELS_EXPL_RES =		0x52,	/* No resources assigned */
+	ELS_EXPL_INV_MAC =	0x60,	/* MAC addressing not supported */
+	ELS_EXPL_VAL_MAC =	0x61,	/* Proposed MAC address incorrectly formed */
 	ELS_EXPL_NOT_NEIGHBOR = 0x62,	/* VN2VN_Port not in neighbor set */
-	/* TBD - above definitions incomplete */
 };
 
 /*
-- 
1.8.5.6

Re: [PATCH 16/22] fc: add missing ELS explanation values

[Johannes Thumshirn]

On Wed, Aug 03, 2016 at 03:13:16PM +0200, Hannes Reinecke wrote:
> Add missing ELS RJT explanation values from FC-LS3.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>

Acked-by: Johannes Thumshirn <jth@kernel.org>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 17/22] libfc: Send LS_RJT responses on frame allocation failure

[Hannes Reinecke]

When we fail to allocate a frame we should be sending an LS_RJT
response and not just silently drop the frame altogether.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libfc/fc_rport.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
index ec608ef..3edcc27 100644
--- a/drivers/scsi/libfc/fc_rport.c
+++ b/drivers/scsi/libfc/fc_rport.c
@@ -904,8 +904,12 @@ static void fc_rport_recv_flogi_req(struct fc_lport *lport,
 	}
 
 	fp = fc_frame_alloc(lport, sizeof(*flp));
-	if (!fp)
-		goto out;
+	if (!fp) {
+		mutex_unlock(&rdata->rp_mutex);
+		rjt_data.reason = ELS_RJT_UNAB;
+		rjt_data.explan = ELS_EXPL_INSUF_RES;
+		goto reject_put;
+	}
 
 	fc_flogi_fill(lport, fp);
 	flp = fc_frame_payload_get(fp, sizeof(*flp));
@@ -918,7 +922,7 @@ static void fc_rport_recv_flogi_req(struct fc_lport *lport,
 		fc_rport_enter_plogi(rdata);
 	else
 		fc_rport_state_enter(rdata, RPORT_ST_PLOGI_WAIT);
-out:
+
 	mutex_unlock(&rdata->rp_mutex);
 	kref_put(&rdata->kref, lport->tt.rport_destroy);
 	fc_frame_free(rx_fp);
@@ -1481,8 +1485,12 @@ static void fc_rport_recv_adisc_req(struct fc_rport_priv *rdata,
 	}
 
 	fp = fc_frame_alloc(lport, sizeof(*adisc));
-	if (!fp)
+	if (!fp) {
+		rjt_data.reason = ELS_RJT_UNAB;
+		rjt_data.explan = ELS_EXPL_INSUF_RES;
+		lport->tt.seq_els_rsp_send(in_fp, ELS_LS_RJT, &rjt_data);
 		goto drop;
+	}
 	fc_adisc_fill(lport, fp);
 	adisc = fc_frame_payload_get(fp, sizeof(*adisc));
 	adisc->adisc_cmd = ELS_LS_ACC;
@@ -1783,14 +1791,17 @@ static void fc_rport_recv_plogi_req(struct fc_lport *lport,
 	 * Send LS_ACC.	 If this fails, the originator should retry.
 	 */
 	fp = fc_frame_alloc(lport, sizeof(*pl));
-	if (!fp)
-		goto out;
-
+	if (!fp) {
+		mutex_unlock(&rdata->rp_mutex);
+		rjt_data.reason = ELS_RJT_UNAB;
+		rjt_data.explan = ELS_EXPL_INSUF_RES;
+		goto reject;
+	}
 	fc_plogi_fill(lport, fp, ELS_LS_ACC);
 	fc_fill_reply_hdr(fp, rx_fp, FC_RCTL_ELS_REP, 0);
 	lport->tt.frame_send(lport, fp);
 	fc_rport_enter_prli(rdata);
-out:
+
 	mutex_unlock(&rdata->rp_mutex);
 	fc_frame_free(rx_fp);
 	return;
-- 
1.8.5.6

Re: [PATCH 17/22] libfc: Send LS_RJT responses on frame allocation failure

[Johannes Thumshirn]

On Wed, Aug 03, 2016 at 03:13:17PM +0200, Hannes Reinecke wrote:
> When we fail to allocate a frame we should be sending an LS_RJT
> response and not just silently drop the frame altogether.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>

Acked-by: Johannes Thumshirn <jth@kernel.org>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 18/22] libfc: don't advance state machine for incoming FLOGI

[Hannes Reinecke]

When we receive an FLOGI but have already sent our own we should
not advance the state machine but rather wait for our FLOGI to
return before continuing with PLOGI.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libfc/fc_rport.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
index 3edcc27..6a25771 100644
--- a/drivers/scsi/libfc/fc_rport.c
+++ b/drivers/scsi/libfc/fc_rport.c
@@ -753,6 +753,7 @@ static void fc_rport_flogi_resp(struct fc_seq *sp, struct fc_frame *fp,
 
 	flogi = fc_frame_payload_get(fp, sizeof(*flogi));
 	if (!flogi) {
+		FC_RPORT_DBG(rdata, "Bad FLOGI response\n");
 		err = -FC_EX_ALLOC_ERR;
 		goto bad;
 	}
@@ -772,7 +773,6 @@ put:
 	kref_put(&rdata->kref, lport->tt.rport_destroy);
 	return;
 bad:
-	FC_RPORT_DBG(rdata, "Bad FLOGI response\n");
 	fc_rport_error_retry(rdata, err);
 	goto out;
 }
@@ -918,11 +918,17 @@ static void fc_rport_recv_flogi_req(struct fc_lport *lport,
 	fc_fill_reply_hdr(fp, rx_fp, FC_RCTL_ELS_REP, 0);
 	lport->tt.frame_send(lport, fp);
 
-	if (rdata->ids.port_name < lport->wwpn)
-		fc_rport_enter_plogi(rdata);
-	else
-		fc_rport_state_enter(rdata, RPORT_ST_PLOGI_WAIT);
-
+	/*
+	 * Do not proceed with the state machine if our
+	 * FLOGI has crossed with an FLOGI from the
+	 * remote port; wait for the FLOGI response instead.
+	 */
+	if (rdata->rp_state != RPORT_ST_FLOGI) {
+		if (rdata->ids.port_name < lport->wwpn)
+			fc_rport_enter_plogi(rdata);
+		else
+			fc_rport_state_enter(rdata, RPORT_ST_PLOGI_WAIT);
+	}
 	mutex_unlock(&rdata->rp_mutex);
 	kref_put(&rdata->kref, lport->tt.rport_destroy);
 	fc_frame_free(rx_fp);
-- 
1.8.5.6

Re: [PATCH 18/22] libfc: don't advance state machine for incoming FLOGI

[Johannes Thumshirn]

On Wed, Aug 03, 2016 at 03:13:18PM +0200, Hannes Reinecke wrote:
> When we receive an FLOGI but have already sent our own we should
> not advance the state machine but rather wait for our FLOGI to
> return before continuing with PLOGI.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>

Acked-by: Johannes Thumshirn <jth@kernel.org>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 19/22] libfc: Implement RTV responder

[Hannes Reinecke]

The libfc stack generates an RTV request, so we should be implementing
an RTV responder, too.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libfc/fc_rport.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
index 6a25771..7fb6685 100644
--- a/drivers/scsi/libfc/fc_rport.c
+++ b/drivers/scsi/libfc/fc_rport.c
@@ -1330,6 +1330,40 @@ static void fc_rport_enter_rtv(struct fc_rport_priv *rdata)
 }
 
 /**
+ * fc_rport_recv_rtv_req() - Handler for Read Timeout Value (RTV) requests
+ * @rdata: The remote port that sent the RTV request
+ * @in_fp: The RTV request frame
+ *
+ * Locking Note:  Called with the lport and rport locks held.
+ */
+static void fc_rport_recv_rtv_req(struct fc_rport_priv *rdata,
+				  struct fc_frame *in_fp)
+{
+	struct fc_lport *lport = rdata->local_port;
+	struct fc_frame *fp;
+	struct fc_els_rtv_acc *rtv;
+	struct fc_seq_els_data rjt_data;
+
+	FC_RPORT_DBG(rdata, "Received RTV request\n");
+
+	fp = fc_frame_alloc(lport, sizeof(*rtv));
+	if (!fp) {
+		rjt_data.reason = ELS_RJT_UNAB;
+		rjt_data.reason = ELS_EXPL_INSUF_RES;
+		lport->tt.seq_els_rsp_send(in_fp, ELS_LS_RJT, &rjt_data);
+		goto drop;
+	}
+	rtv = fc_frame_payload_get(fp, sizeof(*rtv));
+	rtv->rtv_cmd = ELS_LS_ACC;
+	rtv->rtv_r_a_tov = lport->r_a_tov;
+	rtv->rtv_e_d_tov = lport->e_d_tov;
+	fc_fill_reply_hdr(fp, in_fp, FC_RCTL_ELS_REP, 0);
+	lport->tt.frame_send(lport, fp);
+drop:
+	fc_frame_free(in_fp);
+}
+
+/**
  * fc_rport_logo_resp() - Handler for logout (LOGO) responses
  * @sp:	       The sequence the LOGO was on
  * @fp:	       The LOGO response frame
@@ -1627,6 +1661,9 @@ static void fc_rport_recv_els_req(struct fc_lport *lport, struct fc_frame *fp)
 	case ELS_RLS:
 		fc_rport_recv_rls_req(rdata, fp);
 		break;
+	case ELS_RTV:
+		fc_rport_recv_rtv_req(rdata, fp);
+		break;
 	default:
 		fc_frame_free(fp);	/* can't happen */
 		break;
-- 
1.8.5.6

Re: [PATCH 19/22] libfc: Implement RTV responder

[Johannes Thumshirn]

On Wed, Aug 03, 2016 at 03:13:19PM +0200, Hannes Reinecke wrote:
> The libfc stack generates an RTV request, so we should be implementing
> an RTV responder, too.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>

Acked-by: Johannes Thumshirn <jth@kernel.org>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 20/22] libfc: Do not drop out-of-order frames

[Hannes Reinecke]

When receiving packets from the network we cannot guarantee any
frame ordering, so we should be receiving all valid frames and
let the upper layers deal with it.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libfc/fc_exch.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
index 26a43b3..3e9275f 100644
--- a/drivers/scsi/libfc/fc_exch.c
+++ b/drivers/scsi/libfc/fc_exch.c
@@ -1578,9 +1578,6 @@ static void fc_exch_recv_seq_resp(struct fc_exch_mgr *mp, struct fc_frame *fp)
 	if (fc_sof_is_init(sof)) {
 		sp->ssb_stat |= SSB_ST_RESP;
 		sp->id = fh->fh_seq_id;
-	} else if (sp->id != fh->fh_seq_id) {
-		atomic_inc(&mp->stats.seq_not_found);
-		goto rel;
 	}
 
 	f_ctl = ntoh24(fh->fh_f_ctl);
-- 
1.8.5.6

Re: [PATCH 20/22] libfc: Do not drop out-of-order frames

[Johannes Thumshirn]

On Wed, Aug 03, 2016 at 03:13:20PM +0200, Hannes Reinecke wrote:
> When receiving packets from the network we cannot guarantee any
> frame ordering, so we should be receiving all valid frames and
> let the upper layers deal with it.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>

Acked-by: Johannes Thumshirn <jth@kernel.org>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 21/22] libfc: reset timeout on queue full

[Hannes Reinecke]

When we're receiving a timeout we should be checking for queue
full status; if there are still some packets pending we should
be resetting the counter to ensure we're not missing out any
packets which are still queued.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libfc/fc_fcp.c | 21 ++++++++++++++++++---
 include/scsi/libfc.h        |  3 ++-
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/libfc/fc_fcp.c b/drivers/scsi/libfc/fc_fcp.c
index 7aa28a6..dfa1e12 100644
--- a/drivers/scsi/libfc/fc_fcp.c
+++ b/drivers/scsi/libfc/fc_fcp.c
@@ -254,8 +254,10 @@ static inline void fc_fcp_unlock_pkt(struct fc_fcp_pkt *fsp)
  */
 static void fc_fcp_timer_set(struct fc_fcp_pkt *fsp, unsigned long delay)
 {
-	if (!(fsp->state & FC_SRB_COMPL))
+	if (!(fsp->state & FC_SRB_COMPL)) {
 		mod_timer(&fsp->timer, jiffies + delay);
+		fsp->timer_delay = delay;
+	}
 }
 
 static void fc_fcp_abort_done(struct fc_fcp_pkt *fsp)
@@ -931,6 +933,12 @@ static void fc_fcp_resp(struct fc_fcp_pkt *fsp, struct fc_frame *fp)
 			 * Wait a at least one jiffy to see if it is delivered.
 			 * If this expires without data, we may do SRR.
 			 */
+			if (fsp->lp->qfull) {
+				FC_FCP_DBG(fsp, "tgt %6.6x data underrun, "
+					   "retry due to queue busy\n",
+					   fsp->rport->port_id);
+				return;
+			}
 			FC_FCP_DBG(fsp, "tgt %6.6x xfer len %zx data underrun "
 				   "len %x, data len %x\n",
 				   fsp->rport->port_id,
@@ -1431,8 +1439,15 @@ static void fc_fcp_timeout(unsigned long data)
 	if (fsp->cdb_cmd.fc_tm_flags)
 		goto unlock;
 
-	FC_FCP_DBG(fsp, "fcp timeout, flags %x state %x\n",
-		   rpriv->flags, fsp->state);
+	if (fsp->lp->qfull) {
+		FC_FCP_DBG(fsp, "fcp timeout, resetting timer delay %d\n",
+			   fsp->timer_delay);
+		setup_timer(&fsp->timer, fc_fcp_timeout, (unsigned long)fsp);
+		fc_fcp_timer_set(fsp, fsp->timer_delay);
+		goto unlock;
+	}
+	FC_FCP_DBG(fsp, "fcp timeout, delay %d flags %x state %x\n",
+		   fsp->timer_delay, rpriv->flags, fsp->state);
 	fsp->state |= FC_SRB_FCP_PROCESSING_TMO;
 
 	if (rpriv->flags & FC_RP_FLAGS_REC_SUPPORTED)
diff --git a/include/scsi/libfc.h b/include/scsi/libfc.h
index dc42d80..1fcbc9f 100644
--- a/include/scsi/libfc.h
+++ b/include/scsi/libfc.h
@@ -355,7 +355,8 @@ struct fc_fcp_pkt {
 
 	/* Timeout/error related information */
 	struct timer_list timer;
-	int	          wait_for_comp;
+	int		  wait_for_comp;
+	int		  timer_delay;
 	u32		  recov_retry;
 	struct fc_seq	  *recov_seq;
 	struct completion tm_done;
-- 
1.8.5.6

Re: [PATCH 21/22] libfc: reset timeout on queue full

[Johannes Thumshirn]

On Wed, Aug 03, 2016 at 03:13:21PM +0200, Hannes Reinecke wrote:
> When we're receiving a timeout we should be checking for queue
> full status; if there are still some packets pending we should
> be resetting the counter to ensure we're not missing out any
> packets which are still queued.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>

Maybe we can get the debug statements into one line, but anyways
Acked-by: Johannes Thumshirn <jth@kernel.org>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 22/22] fcoe: set default TC priority

[Hannes Reinecke]

If DCB is not enabled or compiled in we still should be setting
a sane default priority. So put FCoE frames in priority class
'interactive' and FIP frames in priority class 'besteffort'.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/fcoe/fcoe.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index c907661..f8ec94a 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -2168,6 +2168,8 @@ static bool fcoe_match(struct net_device *netdev)
  */
 static void fcoe_dcb_create(struct fcoe_interface *fcoe)
 {
+	int ctrl_prio = TC_PRIO_BESTEFFORT;
+	int fcoe_prio = TC_PRIO_INTERACTIVE;
 #ifdef CONFIG_DCB
 	int dcbx;
 	u8 fup, up;
@@ -2194,10 +2196,13 @@ static void fcoe_dcb_create(struct fcoe_interface *fcoe)
 			fup = dcb_getapp(netdev, &app);
 		}
 
-		fcoe->priority = ffs(up) ? ffs(up) - 1 : 0;
-		ctlr->priority = ffs(fup) ? ffs(fup) - 1 : fcoe->priority;
+		fcoe_prio = ffs(up) ? ffs(up) - 1 : 0;
+		ctlr_prio = ffs(fup) ? ffs(fup) - 1 : fcoe_prio;
+		prio_set = true;
 	}
 #endif
+	fcoe->priority = fcoe_prio;
+	ctlr->priority = ctlr_prio;
 }
 
 enum fcoe_create_link_state {
-- 
1.8.5.6

Re: [PATCH 22/22] fcoe: set default TC priority

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3038 bytes --]

Hi Hannes,

[auto build test ERROR on scsi/for-next]
[also build test ERROR on v4.7 next-20160803]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hannes-Reinecke/FCoE-VN2VN-fixes/20160803-211619
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: sparc64-allyesconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sparc64 

All error/warnings (new ones prefixed by >>):

   drivers/scsi/fcoe/fcoe.c: In function 'fcoe_dcb_create':
>> drivers/scsi/fcoe/fcoe.c:2200:3: error: 'ctlr_prio' undeclared (first use in this function)
      ctlr_prio = ffs(fup) ? ffs(fup) - 1 : fcoe_prio;
      ^
   drivers/scsi/fcoe/fcoe.c:2200:3: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/scsi/fcoe/fcoe.c:2201:3: error: 'prio_set' undeclared (first use in this function)
      prio_set = true;
      ^
>> drivers/scsi/fcoe/fcoe.c:2171:6: warning: unused variable 'ctrl_prio' [-Wunused-variable]
     int ctrl_prio = TC_PRIO_BESTEFFORT;
         ^

vim +/ctlr_prio +2200 drivers/scsi/fcoe/fcoe.c

  2165	 * @netdev: The net_device object of the L2 link that should be queried
  2166	 * @port: The fcoe_port to bind FCoE APP priority with
  2167	 * @
  2168	 */
  2169	static void fcoe_dcb_create(struct fcoe_interface *fcoe)
  2170	{
> 2171		int ctrl_prio = TC_PRIO_BESTEFFORT;
  2172		int fcoe_prio = TC_PRIO_INTERACTIVE;
  2173	#ifdef CONFIG_DCB
  2174		int dcbx;
  2175		u8 fup, up;
  2176		struct net_device *netdev = fcoe->realdev;
  2177		struct fcoe_ctlr *ctlr = fcoe_to_ctlr(fcoe);
  2178		struct dcb_app app = {
  2179					.priority = 0,
  2180					.protocol = ETH_P_FCOE
  2181				     };
  2182	
  2183		/* setup DCB priority attributes. */
  2184		if (netdev && netdev->dcbnl_ops && netdev->dcbnl_ops->getdcbx) {
  2185			dcbx = netdev->dcbnl_ops->getdcbx(netdev);
  2186	
  2187			if (dcbx & DCB_CAP_DCBX_VER_IEEE) {
  2188				app.selector = IEEE_8021QAZ_APP_SEL_ETHERTYPE;
  2189				up = dcb_ieee_getapp_mask(netdev, &app);
  2190				app.protocol = ETH_P_FIP;
  2191				fup = dcb_ieee_getapp_mask(netdev, &app);
  2192			} else {
  2193				app.selector = DCB_APP_IDTYPE_ETHTYPE;
  2194				up = dcb_getapp(netdev, &app);
  2195				app.protocol = ETH_P_FIP;
  2196				fup = dcb_getapp(netdev, &app);
  2197			}
  2198	
  2199			fcoe_prio = ffs(up) ? ffs(up) - 1 : 0;
> 2200			ctlr_prio = ffs(fup) ? ffs(fup) - 1 : fcoe_prio;
> 2201			prio_set = true;
  2202		}
  2203	#endif
  2204		fcoe->priority = fcoe_prio;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 46464 bytes --]

Re: [PATCH 00/22] FCoE VN2VN fixes

[Bart Van Assche]

On 08/03/16 06:13, Hannes Reinecke wrote:
> As usual, comments and reviews are welcome.

Great work :-) Have you been testing these patches against the LIO FCoE 
target driver? In that case you might need to port the following patch 
to LIO, a patch that fixes a subtle data corruption issue: 
https://sourceforge.net/p/scst/svn/4969.

Bart.

Re: [PATCH 00/22] FCoE VN2VN fixes

[Hannes Reinecke]

On 08/04/2016 04:59 PM, Bart Van Assche wrote:
> On 08/03/16 06:13, Hannes Reinecke wrote:
>> As usual, comments and reviews are welcome.
> 
> Great work :-) Have you been testing these patches against the LIO FCoE
> target driver? In that case you might need to port the following patch
> to LIO, a patch that fixes a subtle data corruption issue:
> https://sourceforge.net/p/scst/svn/4969.
> 
Indeed I have. With this patchset I've managed to run FCoE over virtio
with LIO running on the host. Quite cool, methinks :-)

And indeed, your patch seem to be helping with the issues I've seen
(mysterious out-of-order frames and wireshark complaining about invalid
frames).
Still a bit hackish, though. Can't we move the data destructor into the
skb itself and not free the data in the target core?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 00/22] FCoE VN2VN fixes

[Bart Van Assche]

On 08/05/2016 12:43 AM, Hannes Reinecke wrote:
> On 08/04/2016 04:59 PM, Bart Van Assche wrote:
>> On 08/03/16 06:13, Hannes Reinecke wrote:
>>> As usual, comments and reviews are welcome.
>>
>> Great work :-) Have you been testing these patches against the LIO FCoE
>> target driver? In that case you might need to port the following patch
>> to LIO, a patch that fixes a subtle data corruption issue:
>> https://sourceforge.net/p/scst/svn/4969.
>>
> Indeed I have. With this patchset I've managed to run FCoE over virtio
> with LIO running on the host. Quite cool, methinks :-)
>
> And indeed, your patch seem to be helping with the issues I've seen
> (mysterious out-of-order frames and wireshark complaining about invalid
> frames).
> Still a bit hackish, though. Can't we move the data destructor into the
> skb itself and not free the data in the target core?

Hello Hannes,

Several LIO drivers already support sending data asynchronously to the 
initiator, e.g. the ib_srpt and qla2x00t drivers. These drivers avoid 
that struct se_cmd and its data buffer are freed before sending finished 
by passing the TARGET_SCF_ACK_KREF flag to target_submit_cmd_map_sgls() 
and by calling target_put_sess_cmd() after the data transfer has 
finished. Since the tcm_fc driver already specifies TARGET_SCF_ACK_KREF 
when submitting commands to the LIO core I think what's needed to make 
use_sg=1 safe in the tcm_fc driver is for use_sg=1 to move the 
target_put_sess_cmd() call from ft_queue_status() into a callback 
function that is called when transmitting data has finished. Maybe the 
SKBTX_DEV_ZEROCOPY flag can be used to make the network stack invoke an 
appropriate callback function. See e.g. xenvif_zerocopy_callback() in 
the xen-netback driver.

Bart.

Re: [PATCH 00/22] FCoE VN2VN fixes

[Hannes Reinecke]

On 08/05/2016 05:33 PM, Bart Van Assche wrote:
> On 08/05/2016 12:43 AM, Hannes Reinecke wrote:
>> On 08/04/2016 04:59 PM, Bart Van Assche wrote:
>>> On 08/03/16 06:13, Hannes Reinecke wrote:
>>>> As usual, comments and reviews are welcome.
>>>
>>> Great work :-) Have you been testing these patches against the LIO FCoE
>>> target driver? In that case you might need to port the following patch
>>> to LIO, a patch that fixes a subtle data corruption issue:
>>> https://sourceforge.net/p/scst/svn/4969.
>>>
>> Indeed I have. With this patchset I've managed to run FCoE over virtio
>> with LIO running on the host. Quite cool, methinks :-)
>>
>> And indeed, your patch seem to be helping with the issues I've seen
>> (mysterious out-of-order frames and wireshark complaining about invalid
>> frames).
>> Still a bit hackish, though. Can't we move the data destructor into the
>> skb itself and not free the data in the target core?
>
> Hello Hannes,
>
> Several LIO drivers already support sending data asynchronously to the
> initiator, e.g. the ib_srpt and qla2x00t drivers. These drivers avoid
> that struct se_cmd and its data buffer are freed before sending finished
> by passing the TARGET_SCF_ACK_KREF flag to target_submit_cmd_map_sgls()
> and by calling target_put_sess_cmd() after the data transfer has
> finished. Since the tcm_fc driver already specifies TARGET_SCF_ACK_KREF
> when submitting commands to the LIO core I think what's needed to make
> use_sg=1 safe in the tcm_fc driver is for use_sg=1 to move the
> target_put_sess_cmd() call from ft_queue_status() into a callback
> function that is called when transmitting data has finished. Maybe the
> SKBTX_DEV_ZEROCOPY flag can be used to make the network stack invoke an
> appropriate callback function. See e.g. xenvif_zerocopy_callback() in
> the xen-netback driver.
>
Well, Johannes and me looked into that a bit closer; there actually _is_ 
a destructor callback in the sk_buff, so in theory we could hook into 
that. However, I'm not sure if that's working as intended; the FCoE 
stack does a lot of skb_clone(), and my setup even more so (tcm_fc over 
vlan over veth over bridge over tap into the guest).
So it's anyones guess at which point of the chain the original data will 
be freed. And whether the 'netdev' point still points to tcm_fcs netdev ...
Debugging continues.

Cheers,

Hannes


-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH 00/22] FCoE VN2VN fixes

[Bart Van Assche]

On 08/05/16 09:16, Hannes Reinecke wrote:
> On 08/05/2016 05:33 PM, Bart Van Assche wrote:
>> On 08/05/2016 12:43 AM, Hannes Reinecke wrote:
>>> On 08/04/2016 04:59 PM, Bart Van Assche wrote:
>>>> On 08/03/16 06:13, Hannes Reinecke wrote:
>>>>> As usual, comments and reviews are welcome.
>>>>
>>>> Great work :-) Have you been testing these patches against the LIO FCoE
>>>> target driver? In that case you might need to port the following patch
>>>> to LIO, a patch that fixes a subtle data corruption issue:
>>>> https://sourceforge.net/p/scst/svn/4969.
>>>>
>>> Indeed I have. With this patchset I've managed to run FCoE over virtio
>>> with LIO running on the host. Quite cool, methinks :-)
>>>
>>> And indeed, your patch seem to be helping with the issues I've seen
>>> (mysterious out-of-order frames and wireshark complaining about invalid
>>> frames).
>>> Still a bit hackish, though. Can't we move the data destructor into the
>>> skb itself and not free the data in the target core?
>>
>> Hello Hannes,
>>
>> Several LIO drivers already support sending data asynchronously to the
>> initiator, e.g. the ib_srpt and qla2x00t drivers. These drivers avoid
>> that struct se_cmd and its data buffer are freed before sending finished
>> by passing the TARGET_SCF_ACK_KREF flag to target_submit_cmd_map_sgls()
>> and by calling target_put_sess_cmd() after the data transfer has
>> finished. Since the tcm_fc driver already specifies TARGET_SCF_ACK_KREF
>> when submitting commands to the LIO core I think what's needed to make
>> use_sg=1 safe in the tcm_fc driver is for use_sg=1 to move the
>> target_put_sess_cmd() call from ft_queue_status() into a callback
>> function that is called when transmitting data has finished. Maybe the
>> SKBTX_DEV_ZEROCOPY flag can be used to make the network stack invoke an
>> appropriate callback function. See e.g. xenvif_zerocopy_callback() in
>> the xen-netback driver.
>>
> Well, Johannes and me looked into that a bit closer; there actually _is_
> a destructor callback in the sk_buff, so in theory we could hook into
> that. However, I'm not sure if that's working as intended; the FCoE
> stack does a lot of skb_clone(), and my setup even more so (tcm_fc over
> vlan over veth over bridge over tap into the guest).
> So it's anyones guess at which point of the chain the original data will
> be freed. And whether the 'netdev' point still points to tcm_fcs netdev ...
> Debugging continues.

Hi Hannes,

Does it really matter that the FCoE skb's get cloned? skb_clone() 
increases skb_shinfo(skb)->dataref. skb_release_data() only calls the 
skb destructor if skb_shinfo(skb)->dataref reaches zero. I think this 
means even if an skb gets cloned that the destructor is only called 
after the skb data has been sent.

Bart.