[RFC 0/9] zfcp: decouple scsi_eh callbacks from scsi_cmnd

[RFC 0/9] zfcp: decouple scsi_eh callbacks from scsi_cmnd

[Steffen Maier]

This is an early request for comments.
The patch set serves as a zfcp preparation step for Hannes' series
"[PATCH 00/47] SCSI EH argument reshuffle part II"
http://www.spinics.net/lists/linux-scsi/msg111165.html
or
http://marc.info/?l=linux-scsi&m=150091945302995&w=2

The series is based on 18 preceding zfcp patches,
including some stable regression bugfixes for zfcp tracing.
Hence it might not apply cleanly.
However, we plan to post the 18 preceding patches soon for integration
and I would like to get those in first.

Please do not apply to any tree that will merge into upstream yet,
as it's not ready for prime time.
It only builds (after each patch; sparse and checkpatch clean)
but it has not seen any function testing yet.

There are still some open questions:
* Search victim scsi_device in target_reset_handler just to get a LUN?
  (Even if we do, zfcp_fsf_fcp_handler_common() should not print that LUN!)
  http://www.spinics.net/lists/linux-scsi/msg111164.html
  http://www.spinics.net/lists/linux-scsi/msg111166.html
* Exact rport blocking logic in host_reset_handler.
  http://www.spinics.net/lists/linux-scsi/msg111165.html


Steffen Maier (9):
  zfcp: drop unsuitable scsi_cmnd usage from SCSI traces for scsi_eh /
    TMF
  zfcp: decouple TMF response handler from scsi_cmnd
  zfcp: split FCP_CMND IU setup between SCSI I/O and TMF again
  zfcp: decouple FSF request setup of TMF from scsi_cmnd
  zfcp: decouple SCSI setup of TMF from scsi_cmnd
  scsi: fc: start decoupling fc_block_scsi_eh from scsi_cmnd
  zfcp: use fc_block_rport for TMFs and host reset to decouple from
    scsi_cmnd
  zfcp: fix waiting for rport(s) unblock in eh_host_reset_handler
  zfcp: decouple our scsi_eh callbacks from scsi_cmnd

 drivers/s390/scsi/zfcp_dbf.c     |  51 ++++++++++++-------
 drivers/s390/scsi/zfcp_dbf.h     |  26 +++++++---
 drivers/s390/scsi/zfcp_ext.h     |  12 +++--
 drivers/s390/scsi/zfcp_fc.h      |  24 ++++++---
 drivers/s390/scsi/zfcp_fsf.c     | 106 +++++++++++++++++++++++++--------------
 drivers/s390/scsi/zfcp_scsi.c    | 105 +++++++++++++++++++++++++++++---------
 drivers/scsi/scsi_transport_fc.c |  31 ++++++++++--
 include/scsi/scsi_transport_fc.h |   1 +
 8 files changed, 252 insertions(+), 104 deletions(-)

-- 
2.11.2

[RFC 1/9] zfcp: drop unsuitable scsi_cmnd usage from SCSI traces for scsi_eh / TMF

[Steffen Maier]

The SCSI command pointer passed to scsi_eh callbacks is just one arbitrary
command of potentially many that are in the eh queue to be processed.
The command is only used to indirectly pass the TMF scope in terms of
SCSI ID/target and SCSI LUN for LUN reset.

Hence, zfcp had filled in SCSI trace record fields which do not really
belong to the TMF. This was confusing.

Therefore, refactor the TMF tracing to work without SCSI command
and instead pass explicit arguments for SCSI ID and SCSI LUN.
As context we now need a pointer to zfcp_adapter.
To make it even clearer, we set all bits to 1 for the fields, which do
not belong to the TMF, to indicate that these fields are invalid.

The old zfcp_dbf_scsi() became zfcp_dbf_scsi_common() to now handle both
SCSI commands and TMFs. The old argument scsi_cmnd is now optional and
can be NULL with TMFs. Two new arguments scsi_id and scsi_lun are
optional and only used if scsi_cmnd is NULL, i.e. with TMFs.

Signed-off-by: Steffen Maier <maier@linux.vnet.ibm.com>
---
 drivers/s390/scsi/zfcp_dbf.c  | 51 ++++++++++++++++++++++++++++---------------
 drivers/s390/scsi/zfcp_dbf.h  | 26 +++++++++++++++-------
 drivers/s390/scsi/zfcp_ext.h  |  8 ++++---
 drivers/s390/scsi/zfcp_scsi.c | 20 ++++++++++++-----
 4 files changed, 71 insertions(+), 34 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
index 8227076c9cbb..dca624aaa7c0 100644
--- a/drivers/s390/scsi/zfcp_dbf.c
+++ b/drivers/s390/scsi/zfcp_dbf.c
@@ -577,16 +577,19 @@ void zfcp_dbf_san_in_els(char *tag, struct zfcp_fsf_req *fsf)
 }
 
 /**
- * zfcp_dbf_scsi - trace event for scsi commands
- * @tag: identifier for event
- * @sc: pointer to struct scsi_cmnd
- * @fsf: pointer to struct zfcp_fsf_req
+ * zfcp_dbf_scsi_common() - Common trace event helper for scsi.
+ * @tag: Identifier for event.
+ * @level: trace level of event.
+ * @adapter: Pointer to zfcp adapter as context for this event.
+ * @sc: Pointer to SCSI command, or NULL with task management function (TMF).
+ * @fsf: Pointer to FSF request, or NULL.
+ * @scsi_id: SCSI ID/target to indicate scope, only for TMF.
+ * @scsi_lun: SCSI LUN if TMF is Logical Unit Reset, else %ZFCP_DBF_INVALID_LUN.
  */
-void zfcp_dbf_scsi(char *tag, int level, struct scsi_cmnd *sc,
-		   struct zfcp_fsf_req *fsf)
+void zfcp_dbf_scsi_common(char *tag, int level, struct zfcp_adapter *adapter,
+			  struct scsi_cmnd *sc, struct zfcp_fsf_req *fsf,
+			  unsigned int scsi_id, u64 scsi_lun)
 {
-	struct zfcp_adapter *adapter =
-		(struct zfcp_adapter *) sc->device->host->hostdata[0];
 	struct zfcp_dbf *dbf = adapter->dbf;
 	struct zfcp_dbf_scsi *rec = &dbf->scsi_buf;
 	struct fcp_resp_with_ext *fcp_rsp;
@@ -598,16 +601,28 @@ void zfcp_dbf_scsi(char *tag, int level, struct scsi_cmnd *sc,
 
 	memcpy(rec->tag, tag, ZFCP_DBF_TAG_LEN);
 	rec->id = ZFCP_DBF_SCSI_CMND;
-	rec->scsi_result = sc->result;
-	rec->scsi_retries = sc->retries;
-	rec->scsi_allowed = sc->allowed;
-	rec->scsi_id = sc->device->id;
-	rec->scsi_lun = (u32)sc->device->lun;
-	rec->scsi_lun_64_hi = (u32)(sc->device->lun >> 32);
-	rec->host_scribble = (unsigned long)sc->host_scribble;
-
-	memcpy(rec->scsi_opcode, sc->cmnd,
-	       min((int)sc->cmd_len, ZFCP_DBF_SCSI_OPCODE));
+	if (sc) {
+		rec->scsi_result = sc->result;
+		rec->scsi_retries = sc->retries;
+		rec->scsi_allowed = sc->allowed;
+		rec->scsi_id = sc->device->id;
+		rec->scsi_lun = (u32)sc->device->lun;
+		rec->scsi_lun_64_hi = (u32)(sc->device->lun >> 32);
+		rec->host_scribble = (unsigned long)sc->host_scribble;
+
+		memcpy(rec->scsi_opcode, sc->cmnd,
+		       min_t(int, sc->cmd_len, ZFCP_DBF_SCSI_OPCODE));
+	} else {
+		rec->scsi_result = ~0;
+		rec->scsi_retries = ~0;
+		rec->scsi_allowed = ~0;
+		rec->scsi_id = scsi_id;
+		rec->scsi_lun = (u32)scsi_lun;
+		rec->scsi_lun_64_hi = (u32)(scsi_lun >> 32);
+		rec->host_scribble = ~0;
+
+		memset(rec->scsi_opcode, 0xff, ZFCP_DBF_SCSI_OPCODE);
+	}
 
 	if (fsf) {
 		rec->fsf_req_id = fsf->req_id;
diff --git a/drivers/s390/scsi/zfcp_dbf.h b/drivers/s390/scsi/zfcp_dbf.h
index 3508c00458f4..6e29e7cccbc4 100644
--- a/drivers/s390/scsi/zfcp_dbf.h
+++ b/drivers/s390/scsi/zfcp_dbf.h
@@ -358,7 +358,8 @@ void _zfcp_dbf_scsi(char *tag, int level, struct scsi_cmnd *scmd,
 					scmd->device->host->hostdata[0];
 
 	if (debug_level_enabled(adapter->dbf->scsi, level))
-		zfcp_dbf_scsi(tag, level, scmd, req);
+		zfcp_dbf_scsi_common(tag, level, adapter, scmd, req,
+				     ~0, ZFCP_DBF_INVALID_LUN);
 }
 
 /**
@@ -401,16 +402,24 @@ void zfcp_dbf_scsi_abort(char *tag, struct scsi_cmnd *scmd,
 }
 
 /**
- * zfcp_dbf_scsi_devreset - trace event for Logical Unit or Target Reset
- * @tag: tag indicating success or failure of reset operation
- * @scmnd: SCSI command which caused this error recovery
- * @flag: indicates type of reset (Target Reset, Logical Unit Reset)
+ * zfcp_dbf_scsi_devreset() - Trace event for Logical Unit or Target Reset.
+ * @tag: Tag indicating success or failure of reset operation.
+ * @adapter: Pointer to zfcp adapter as context for this event.
+ * @flag: Indicates type of reset (Target Reset, Logical Unit Reset).
+ * @fsf_req: Pointer to FSF request representing the TMF, or NULL.
+ * @scsi_id: SCSI ID/target to indicate scope of task management function (TMF).
+ * @scsi_lun: SCSI LUN if TMF is Logical Unit Reset, else %ZFCP_DBF_INVALID_LUN.
  */
 static inline
-void zfcp_dbf_scsi_devreset(char *tag, struct scsi_cmnd *scmnd, u8 flag,
-			    struct zfcp_fsf_req *fsf_req)
+void zfcp_dbf_scsi_devreset(char *tag, struct zfcp_adapter *adapter, u8 flag,
+			    struct zfcp_fsf_req *fsf_req,
+			    unsigned int scsi_id, u64 scsi_lun)
 {
 	char tmp_tag[ZFCP_DBF_TAG_LEN];
+	static int const level = 1;
+
+	if (unlikely(!debug_level_enabled(adapter->dbf->scsi, level)))
+		return;
 
 	if (flag == FCP_TMF_TGT_RESET)
 		memcpy(tmp_tag, "tr_", 3);
@@ -418,7 +427,8 @@ void zfcp_dbf_scsi_devreset(char *tag, struct scsi_cmnd *scmnd, u8 flag,
 		memcpy(tmp_tag, "lr_", 3);
 
 	memcpy(&tmp_tag[3], tag, 4);
-	_zfcp_dbf_scsi(tmp_tag, 1, scmnd, fsf_req);
+	zfcp_dbf_scsi_common(tmp_tag, level, adapter, NULL, fsf_req,
+			     scsi_id, scsi_lun);
 }
 
 /**
diff --git a/drivers/s390/scsi/zfcp_ext.h b/drivers/s390/scsi/zfcp_ext.h
index a9e968717dd9..f3101bc5d1bc 100644
--- a/drivers/s390/scsi/zfcp_ext.h
+++ b/drivers/s390/scsi/zfcp_ext.h
@@ -3,7 +3,7 @@
  *
  * External function declarations.
  *
- * Copyright IBM Corp. 2002, 2016
+ * Copyright IBM Corp. 2002, 2017
  */
 
 #ifndef ZFCP_EXT_H
@@ -46,8 +46,10 @@ extern void zfcp_dbf_hba_basic(char *, struct zfcp_adapter *);
 extern void zfcp_dbf_san_req(char *, struct zfcp_fsf_req *, u32);
 extern void zfcp_dbf_san_res(char *, struct zfcp_fsf_req *);
 extern void zfcp_dbf_san_in_els(char *, struct zfcp_fsf_req *);
-extern void zfcp_dbf_scsi(char *, int, struct scsi_cmnd *,
-			  struct zfcp_fsf_req *);
+extern void zfcp_dbf_scsi_common(char *tag, int level,
+				 struct zfcp_adapter *adapter,
+				 struct scsi_cmnd *sc, struct zfcp_fsf_req *fsf,
+				 unsigned int scsi_id, u64 scsi_lun);
 
 /* zfcp_erp.c */
 extern void zfcp_erp_set_adapter_status(struct zfcp_adapter *, u32);
diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
index ec3ddd1d31d5..cd0f811452b7 100644
--- a/drivers/s390/scsi/zfcp_scsi.c
+++ b/drivers/s390/scsi/zfcp_scsi.c
@@ -262,10 +262,15 @@ static int zfcp_task_mgmt_function(struct scsi_cmnd *scpnt, u8 tm_flags)
 {
 	struct zfcp_scsi_dev *zfcp_sdev = sdev_to_zfcp(scpnt->device);
 	struct zfcp_adapter *adapter = zfcp_sdev->port->adapter;
+	unsigned int scsi_id = zfcp_sdev->port->starget_id;
+	u64 scsi_lun = ZFCP_DBF_INVALID_LUN;
 	struct zfcp_fsf_req *fsf_req = NULL;
 	int retval = SUCCESS, ret;
 	int retry = 3;
 
+	if (tm_flags == FCP_TMF_LUN_RESET)
+		scsi_lun = scpnt->device->lun;
+
 	while (retry--) {
 		fsf_req = zfcp_fsf_fcp_task_mgmt(scpnt, tm_flags);
 		if (fsf_req)
@@ -274,28 +279,33 @@ static int zfcp_task_mgmt_function(struct scsi_cmnd *scpnt, u8 tm_flags)
 		zfcp_erp_wait(adapter);
 		ret = fc_block_scsi_eh(scpnt);
 		if (ret) {
-			zfcp_dbf_scsi_devreset("fiof", scpnt, tm_flags, NULL);
+			zfcp_dbf_scsi_devreset("fiof", adapter, tm_flags, NULL,
+					       scsi_id, scsi_lun);
 			return ret;
 		}
 
 		if (!(atomic_read(&adapter->status) &
 		      ZFCP_STATUS_COMMON_RUNNING)) {
-			zfcp_dbf_scsi_devreset("nres", scpnt, tm_flags, NULL);
+			zfcp_dbf_scsi_devreset("nres", adapter, tm_flags, NULL,
+					       scsi_id, scsi_lun);
 			return SUCCESS;
 		}
 	}
 	if (!fsf_req) {
-		zfcp_dbf_scsi_devreset("reqf", scpnt, tm_flags, NULL);
+		zfcp_dbf_scsi_devreset("reqf", adapter, tm_flags, NULL,
+				       scsi_id, scsi_lun);
 		return FAILED;
 	}
 
 	wait_for_completion(&fsf_req->completion);
 
 	if (fsf_req->status & ZFCP_STATUS_FSFREQ_TMFUNCFAILED) {
-		zfcp_dbf_scsi_devreset("fail", scpnt, tm_flags, fsf_req);
+		zfcp_dbf_scsi_devreset("fail", adapter, tm_flags, fsf_req,
+				       scsi_id, scsi_lun);
 		retval = FAILED;
 	} else {
-		zfcp_dbf_scsi_devreset("okay", scpnt, tm_flags, fsf_req);
+		zfcp_dbf_scsi_devreset("okay", adapter, tm_flags, fsf_req,
+				       scsi_id, scsi_lun);
 		zfcp_scsi_forget_cmnds(zfcp_sdev, tm_flags);
 	}
 
-- 
2.11.2

Re: [RFC 1/9] zfcp: drop unsuitable scsi_cmnd usage from SCSI traces for scsi_eh / TMF

[Hannes Reinecke]

On 07/25/2017 04:14 PM, Steffen Maier wrote:
> The SCSI command pointer passed to scsi_eh callbacks is just one arbitrary
> command of potentially many that are in the eh queue to be processed.
> The command is only used to indirectly pass the TMF scope in terms of
> SCSI ID/target and SCSI LUN for LUN reset.
> 
> Hence, zfcp had filled in SCSI trace record fields which do not really
> belong to the TMF. This was confusing.
> 
> Therefore, refactor the TMF tracing to work without SCSI command
> and instead pass explicit arguments for SCSI ID and SCSI LUN.
> As context we now need a pointer to zfcp_adapter.
> To make it even clearer, we set all bits to 1 for the fields, which do
> not belong to the TMF, to indicate that these fields are invalid.
> 
> The old zfcp_dbf_scsi() became zfcp_dbf_scsi_common() to now handle both
> SCSI commands and TMFs. The old argument scsi_cmnd is now optional and
> can be NULL with TMFs. Two new arguments scsi_id and scsi_lun are
> optional and only used if scsi_cmnd is NULL, i.e. with TMFs.
> 
> Signed-off-by: Steffen Maier <maier@linux.vnet.ibm.com>
> ---
>  drivers/s390/scsi/zfcp_dbf.c  | 51 ++++++++++++++++++++++++++++---------------
>  drivers/s390/scsi/zfcp_dbf.h  | 26 +++++++++++++++-------
>  drivers/s390/scsi/zfcp_ext.h  |  8 ++++---
>  drivers/s390/scsi/zfcp_scsi.c | 20 ++++++++++++-----
>  4 files changed, 71 insertions(+), 34 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

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)

[RFC 2/9] zfcp: decouple TMF response handler from scsi_cmnd

[Steffen Maier]

Do not get scsi_device via req->data any more, but pass an optional(!)
scsi_device to zfcp_fsf_fcp_handler_common(). The latter must now guard
any access to scsi_device as it can be NULL.

Since we always have at least a zfcp port as scope, pass this as mandatory
argument to zfcp_fsf_fcp_handler_common() because we cannot get it through
scsi_device => zfcp_scsi_dev => port any more.

Hence, the callers of zfcp_fsf_fcp_handler_common() must resolve req->data.

TMF handling now has different context data in fsf_req->data
depending on the TMF scope in fcp_cmnd->fc_tm_flags:
* scsi_device if FCP_TMF_LUN_RESET,
* zfcp_port if FCP_TMF_TGT_RESET.

Signed-off-by: Steffen Maier <maier@linux.vnet.ibm.com>
---
 drivers/s390/scsi/zfcp_fsf.c | 72 ++++++++++++++++++++++++++++----------------
 1 file changed, 46 insertions(+), 26 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
index 69d1dc3ec79d..8b2b2ea552d6 100644
--- a/drivers/s390/scsi/zfcp_fsf.c
+++ b/drivers/s390/scsi/zfcp_fsf.c
@@ -2035,27 +2035,30 @@ static void zfcp_fsf_req_trace(struct zfcp_fsf_req *req, struct scsi_cmnd *scsi)
 			    sizeof(blktrc));
 }
 
-static void zfcp_fsf_fcp_handler_common(struct zfcp_fsf_req *req)
+/**
+ * zfcp_fsf_fcp_handler_common() - FCP response handler common to I/O and TMF.
+ * @req: Pointer to FSF request.
+ * @port: Pointer to zfcp port.
+ * @sdev: Pointer to SCSI device, or %NULL with Target Reset TMF.
+ */
+static void zfcp_fsf_fcp_handler_common(struct zfcp_fsf_req *req,
+					struct zfcp_port *port,
+					struct scsi_device *sdev)
 {
-	struct scsi_cmnd *scmnd = req->data;
-	struct scsi_device *sdev = scmnd->device;
-	struct zfcp_scsi_dev *zfcp_sdev;
 	struct fsf_qtcb_header *header = &req->qtcb->header;
 
 	if (unlikely(req->status & ZFCP_STATUS_FSFREQ_ERROR))
 		return;
 
-	zfcp_sdev = sdev_to_zfcp(sdev);
-
 	switch (header->fsf_status) {
 	case FSF_HANDLE_MISMATCH:
 	case FSF_PORT_HANDLE_NOT_VALID:
-		zfcp_erp_adapter_reopen(zfcp_sdev->port->adapter, 0, "fssfch1");
+		zfcp_erp_adapter_reopen(req->adapter, 0, "fssfch1");
 		req->status |= ZFCP_STATUS_FSFREQ_ERROR;
 		break;
 	case FSF_FCPLUN_NOT_VALID:
 	case FSF_LUN_HANDLE_NOT_VALID:
-		zfcp_erp_port_reopen(zfcp_sdev->port, 0, "fssfch2");
+		zfcp_erp_port_reopen(port, 0, "fssfch2");
 		req->status |= ZFCP_STATUS_FSFREQ_ERROR;
 		break;
 	case FSF_SERVICE_CLASS_NOT_SUPPORTED:
@@ -2066,10 +2069,10 @@ static void zfcp_fsf_fcp_handler_common(struct zfcp_fsf_req *req)
 			"Incorrect direction %d, LUN 0x%016Lx on port "
 			"0x%016Lx closed\n",
 			req->qtcb->bottom.io.data_direction,
-			(unsigned long long)zfcp_scsi_dev_lun(sdev),
-			(unsigned long long)zfcp_sdev->port->wwpn);
-		zfcp_erp_adapter_shutdown(zfcp_sdev->port->adapter, 0,
-					  "fssfch3");
+			sdev ? (unsigned long long)zfcp_scsi_dev_lun(sdev) :
+			ZFCP_DBF_INVALID_LUN,
+			(unsigned long long)port->wwpn);
+		zfcp_erp_adapter_shutdown(req->adapter, 0, "fssfch3");
 		req->status |= ZFCP_STATUS_FSFREQ_ERROR;
 		break;
 	case FSF_CMND_LENGTH_NOT_VALID:
@@ -2077,29 +2080,32 @@ static void zfcp_fsf_fcp_handler_common(struct zfcp_fsf_req *req)
 			"Incorrect CDB length %d, LUN 0x%016Lx on "
 			"port 0x%016Lx closed\n",
 			req->qtcb->bottom.io.fcp_cmnd_length,
-			(unsigned long long)zfcp_scsi_dev_lun(sdev),
-			(unsigned long long)zfcp_sdev->port->wwpn);
-		zfcp_erp_adapter_shutdown(zfcp_sdev->port->adapter, 0,
-					  "fssfch4");
+			sdev ? (unsigned long long)zfcp_scsi_dev_lun(sdev) :
+			ZFCP_DBF_INVALID_LUN,
+			(unsigned long long)port->wwpn);
+		zfcp_erp_adapter_shutdown(req->adapter, 0, "fssfch4");
 		req->status |= ZFCP_STATUS_FSFREQ_ERROR;
 		break;
 	case FSF_PORT_BOXED:
-		zfcp_erp_set_port_status(zfcp_sdev->port,
+		zfcp_erp_set_port_status(port,
 					 ZFCP_STATUS_COMMON_ACCESS_BOXED);
-		zfcp_erp_port_reopen(zfcp_sdev->port,
+		zfcp_erp_port_reopen(port,
 				     ZFCP_STATUS_COMMON_ERP_FAILED, "fssfch5");
 		req->status |= ZFCP_STATUS_FSFREQ_ERROR;
 		break;
 	case FSF_LUN_BOXED:
-		zfcp_erp_set_lun_status(sdev, ZFCP_STATUS_COMMON_ACCESS_BOXED);
-		zfcp_erp_lun_reopen(sdev, ZFCP_STATUS_COMMON_ERP_FAILED,
-				    "fssfch6");
+		if (sdev) {
+			zfcp_erp_set_lun_status(
+				sdev, ZFCP_STATUS_COMMON_ACCESS_BOXED);
+			zfcp_erp_lun_reopen(sdev, ZFCP_STATUS_COMMON_ERP_FAILED,
+					    "fssfch6");
+		}
 		req->status |= ZFCP_STATUS_FSFREQ_ERROR;
 		break;
 	case FSF_ADAPTER_STATUS_AVAILABLE:
 		if (header->fsf_status_qual.word[0] ==
 		    FSF_SQ_INVOKE_LINK_TEST_PROCEDURE)
-			zfcp_fc_test_link(zfcp_sdev->port);
+			zfcp_fc_test_link(port);
 		req->status |= ZFCP_STATUS_FSFREQ_ERROR;
 		break;
 	}
@@ -2108,6 +2114,7 @@ static void zfcp_fsf_fcp_handler_common(struct zfcp_fsf_req *req)
 static void zfcp_fsf_fcp_cmnd_handler(struct zfcp_fsf_req *req)
 {
 	struct scsi_cmnd *scpnt;
+	struct scsi_device *sdev;
 	struct fcp_resp_with_ext *fcp_rsp;
 	unsigned long flags;
 
@@ -2119,7 +2126,8 @@ static void zfcp_fsf_fcp_cmnd_handler(struct zfcp_fsf_req *req)
 		return;
 	}
 
-	zfcp_fsf_fcp_handler_common(req);
+	sdev = scpnt->device;
+	zfcp_fsf_fcp_handler_common(req, sdev_to_zfcp(sdev)->port, sdev);
 
 	if (unlikely(req->status & ZFCP_STATUS_FSFREQ_ERROR)) {
 		set_host_byte(scpnt, DID_TRANSPORT_DISRUPTED);
@@ -2296,10 +2304,21 @@ int zfcp_fsf_fcp_cmnd(struct scsi_cmnd *scsi_cmnd)
 
 static void zfcp_fsf_fcp_task_mgmt_handler(struct zfcp_fsf_req *req)
 {
+	struct fcp_cmnd *fcp_cmnd;
+	struct zfcp_port *port;
+	struct scsi_device *sdev;
 	struct fcp_resp_with_ext *fcp_rsp;
 	struct fcp_resp_rsp_info *rsp_info;
 
-	zfcp_fsf_fcp_handler_common(req);
+	fcp_cmnd = &req->qtcb->bottom.io.fcp_cmnd.iu;
+	if (fcp_cmnd->fc_tm_flags & FCP_TMF_LUN_RESET) {
+		sdev = req->data;
+		port = sdev_to_zfcp(sdev)->port;
+	} else {
+		sdev = NULL;
+		port = req->data;
+	}
+	zfcp_fsf_fcp_handler_common(req, port, sdev);
 
 	fcp_rsp = &req->qtcb->bottom.io.fcp_rsp.iu;
 	rsp_info = (struct fcp_resp_rsp_info *) &fcp_rsp[1];
@@ -2340,7 +2359,9 @@ struct zfcp_fsf_req *zfcp_fsf_fcp_task_mgmt(struct scsi_cmnd *scmnd,
 		goto out;
 	}
 
-	req->data = scmnd;
+	fcp_cmnd = &req->qtcb->bottom.io.fcp_cmnd.iu;
+	req->data = (fcp_cmnd->fc_tm_flags & FCP_TMF_LUN_RESET) ?
+		scmnd->device : (void *)sdev_to_zfcp(scmnd->device)->port;
 	req->handler = zfcp_fsf_fcp_task_mgmt_handler;
 	req->qtcb->header.lun_handle = zfcp_sdev->lun_handle;
 	req->qtcb->header.port_handle = zfcp_sdev->port->handle;
@@ -2350,7 +2371,6 @@ struct zfcp_fsf_req *zfcp_fsf_fcp_task_mgmt(struct scsi_cmnd *scmnd,
 
 	zfcp_qdio_set_sbale_last(qdio, &req->qdio_req);
 
-	fcp_cmnd = &req->qtcb->bottom.io.fcp_cmnd.iu;
 	zfcp_fc_scsi_to_fcp(fcp_cmnd, scmnd, tm_flags);
 
 	zfcp_fsf_start_timer(req, ZFCP_SCSI_ER_TIMEOUT);
-- 
2.11.2

Re: [RFC 2/9] zfcp: decouple TMF response handler from scsi_cmnd

[Hannes Reinecke]

On 07/25/2017 04:14 PM, Steffen Maier wrote:
> Do not get scsi_device via req->data any more, but pass an optional(!)
> scsi_device to zfcp_fsf_fcp_handler_common(). The latter must now guard
> any access to scsi_device as it can be NULL.
> 
> Since we always have at least a zfcp port as scope, pass this as mandatory
> argument to zfcp_fsf_fcp_handler_common() because we cannot get it through
> scsi_device => zfcp_scsi_dev => port any more.
> 
> Hence, the callers of zfcp_fsf_fcp_handler_common() must resolve req->data.
> 
> TMF handling now has different context data in fsf_req->data
> depending on the TMF scope in fcp_cmnd->fc_tm_flags:
> * scsi_device if FCP_TMF_LUN_RESET,
> * zfcp_port if FCP_TMF_TGT_RESET.
> 
> Signed-off-by: Steffen Maier <maier@linux.vnet.ibm.com>
> ---
>  drivers/s390/scsi/zfcp_fsf.c | 72 ++++++++++++++++++++++++++++----------------
>  1 file changed, 46 insertions(+), 26 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

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: [RFC 2/9] zfcp: decouple TMF response handler from scsi_cmnd

[Steffen Maier]

Just for the records, in case anyone wants to resurrect this later on:
This patch is buggy.

On 07/25/2017 04:14 PM, Steffen Maier wrote:
> Do not get scsi_device via req->data any more, but pass an optional(!)
> scsi_device to zfcp_fsf_fcp_handler_common(). The latter must now guard
> any access to scsi_device as it can be NULL.
> 
> Since we always have at least a zfcp port as scope, pass this as mandatory
> argument to zfcp_fsf_fcp_handler_common() because we cannot get it through
> scsi_device => zfcp_scsi_dev => port any more.
> 
> Hence, the callers of zfcp_fsf_fcp_handler_common() must resolve req->data.
> 
> TMF handling now has different context data in fsf_req->data
> depending on the TMF scope in fcp_cmnd->fc_tm_flags:
> * scsi_device if FCP_TMF_LUN_RESET,
> * zfcp_port if FCP_TMF_TGT_RESET.
> 
> Signed-off-by: Steffen Maier <maier@linux.vnet.ibm.com>
> ---
>   drivers/s390/scsi/zfcp_fsf.c | 72 ++++++++++++++++++++++++++++----------------
>   1 file changed, 46 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
> index 69d1dc3ec79d..8b2b2ea552d6 100644
> --- a/drivers/s390/scsi/zfcp_fsf.c
> +++ b/drivers/s390/scsi/zfcp_fsf.c

> @@ -2296,10 +2304,21 @@ int zfcp_fsf_fcp_cmnd(struct scsi_cmnd *scsi_cmnd)
> 
>   static void zfcp_fsf_fcp_task_mgmt_handler(struct zfcp_fsf_req *req)
>   {
> +	struct fcp_cmnd *fcp_cmnd;
> +	struct zfcp_port *port;
> +	struct scsi_device *sdev;
>   	struct fcp_resp_with_ext *fcp_rsp;
>   	struct fcp_resp_rsp_info *rsp_info;
> 
> -	zfcp_fsf_fcp_handler_common(req);
> +	fcp_cmnd = &req->qtcb->bottom.io.fcp_cmnd.iu;
> +	if (fcp_cmnd->fc_tm_flags & FCP_TMF_LUN_RESET) {
> +		sdev = req->data;
> +		port = sdev_to_zfcp(sdev)->port;

Below described bug causes, in case of a LUN reset, a wrong type 
interpretation because we interpret req->data as scsi_device but the 
request function had assigned a zfcp_port to it. Dereferencing the port 
field leads to a kernel page fault in (Soft)IRQ context ending up in a 
panic.

> +	} else { > +		sdev = NULL;
> +		port = req->data;
> +	}
> +	zfcp_fsf_fcp_handler_common(req, port, sdev);
> 
>   	fcp_rsp = &req->qtcb->bottom.io.fcp_rsp.iu;
>   	rsp_info = (struct fcp_resp_rsp_info *) &fcp_rsp[1];
> @@ -2340,7 +2359,9 @@ struct zfcp_fsf_req *zfcp_fsf_fcp_task_mgmt(struct scsi_cmnd *scmnd,
>   		goto out;
>   	}
> 
> -	req->data = scmnd;
> +	fcp_cmnd = &req->qtcb->bottom.io.fcp_cmnd.iu;

While I moved the pointer assignment here,
the memory it points to is only filled in below with:
zfcp_fc_scsi_to_fcp(fcp_cmnd, scmnd, tm_flags).
The still freshly allocated QTCB is pre-initialized with zero.
Hence, the subsequent boolean expression always evaluates to false
since no flag is set yet.
Thus, a LUN reset erroneously has:
req->data = (void *)sdev_to_zfcp(scmnd->device)->port.

A fix would be to base the boolean expression on function argument 
tm_flags rather than the QTCB content:
(tm_flags & FCP_TMF_LUN_RESET).
To not confuse people, I would also undo the move of the fcp_cmnd 
pointer assignment.

I won't send a new version with this fix,
because it turned out the FCP channel always requires a valid LUN handle 
(even for a target reset), so I'm settling on scsi_device as common 
context for any TMF, similarly like Hannes did.
Once I've successfully completed function test of v2 of my patch set,
I'm going to re-submit the full refactored set.

> +	req->data = (fcp_cmnd->fc_tm_flags & FCP_TMF_LUN_RESET) ?
> +		scmnd->device : (void *)sdev_to_zfcp(scmnd->device)->port;
>   	req->handler = zfcp_fsf_fcp_task_mgmt_handler;
>   	req->qtcb->header.lun_handle = zfcp_sdev->lun_handle;
>   	req->qtcb->header.port_handle = zfcp_sdev->port->handle;
> @@ -2350,7 +2371,6 @@ struct zfcp_fsf_req *zfcp_fsf_fcp_task_mgmt(struct scsi_cmnd *scmnd,
> 
>   	zfcp_qdio_set_sbale_last(qdio, &req->qdio_req);
> 
> -	fcp_cmnd = &req->qtcb->bottom.io.fcp_cmnd.iu;
>   	zfcp_fc_scsi_to_fcp(fcp_cmnd, scmnd, tm_flags);
> 
>   	zfcp_fsf_start_timer(req, ZFCP_SCSI_ER_TIMEOUT);
> 

-- 
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

[RFC 3/9] zfcp: split FCP_CMND IU setup between SCSI I/O and TMF again

[Steffen Maier]

This reverts commit 2443c8b23aea ("[SCSI] zfcp: Merge FCP task management
setup with regular FCP command setup"), because this introduced a
dependency on the unsuitable SCSI command for scsi_eh / TMF.

Signed-off-by: Steffen Maier <maier@linux.vnet.ibm.com>
---
 drivers/s390/scsi/zfcp_fc.h  | 22 ++++++++++++++--------
 drivers/s390/scsi/zfcp_fsf.c |  4 ++--
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_fc.h b/drivers/s390/scsi/zfcp_fc.h
index 41f22d3dc6d1..24949868d027 100644
--- a/drivers/s390/scsi/zfcp_fc.h
+++ b/drivers/s390/scsi/zfcp_fc.h
@@ -206,21 +206,14 @@ struct zfcp_fc_wka_ports {
  * zfcp_fc_scsi_to_fcp - setup FCP command with data from scsi_cmnd
  * @fcp: fcp_cmnd to setup
  * @scsi: scsi_cmnd where to get LUN, task attributes/flags and CDB
- * @tm: task management flags to setup task management command
  */
 static inline
-void zfcp_fc_scsi_to_fcp(struct fcp_cmnd *fcp, struct scsi_cmnd *scsi,
-			 u8 tm_flags)
+void zfcp_fc_scsi_to_fcp(struct fcp_cmnd *fcp, struct scsi_cmnd *scsi)
 {
 	u32 datalen;
 
 	int_to_scsilun(scsi->device->lun, (struct scsi_lun *) &fcp->fc_lun);
 
-	if (unlikely(tm_flags)) {
-		fcp->fc_tm_flags = tm_flags;
-		return;
-	}
-
 	fcp->fc_pri_ta = FCP_PTA_SIMPLE;
 
 	if (scsi->sc_data_direction == DMA_FROM_DEVICE)
@@ -240,6 +233,19 @@ void zfcp_fc_scsi_to_fcp(struct fcp_cmnd *fcp, struct scsi_cmnd *scsi,
 }
 
 /**
+ * zfcp_fc_fcp_tm() - Setup FCP command as task management command.
+ * @fcp: Pointer to FCP_CMND IU to set up.
+ * @dev: Pointer to SCSI_device where to send the task management command.
+ * @tm_flags: Task management flags to setup tm command.
+ */
+static inline
+void zfcp_fc_fcp_tm(struct fcp_cmnd *fcp, struct scsi_device *dev, u8 tm_flags)
+{
+	int_to_scsilun(dev->lun, (struct scsi_lun *) &fcp->fc_lun);
+	fcp->fc_tm_flags = tm_flags;
+}
+
+/**
  * zfcp_fc_evap_fcp_rsp - evaluate FCP RSP IU and update scsi_cmnd accordingly
  * @fcp_rsp: FCP RSP IU to evaluate
  * @scsi: SCSI command where to update status and sense buffer
diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
index 8b2b2ea552d6..f221a34c26df 100644
--- a/drivers/s390/scsi/zfcp_fsf.c
+++ b/drivers/s390/scsi/zfcp_fsf.c
@@ -2265,7 +2265,7 @@ int zfcp_fsf_fcp_cmnd(struct scsi_cmnd *scsi_cmnd)
 
 	BUILD_BUG_ON(sizeof(struct fcp_cmnd) > FSF_FCP_CMND_SIZE);
 	fcp_cmnd = &req->qtcb->bottom.io.fcp_cmnd.iu;
-	zfcp_fc_scsi_to_fcp(fcp_cmnd, scsi_cmnd, 0);
+	zfcp_fc_scsi_to_fcp(fcp_cmnd, scsi_cmnd);
 
 	if ((scsi_get_prot_op(scsi_cmnd) != SCSI_PROT_NORMAL) &&
 	    scsi_prot_sg_count(scsi_cmnd)) {
@@ -2371,7 +2371,7 @@ struct zfcp_fsf_req *zfcp_fsf_fcp_task_mgmt(struct scsi_cmnd *scmnd,
 
 	zfcp_qdio_set_sbale_last(qdio, &req->qdio_req);
 
-	zfcp_fc_scsi_to_fcp(fcp_cmnd, scmnd, tm_flags);
+	zfcp_fc_fcp_tm(fcp_cmnd, scmnd->device, tm_flags);
 
 	zfcp_fsf_start_timer(req, ZFCP_SCSI_ER_TIMEOUT);
 	if (!zfcp_fsf_req_send(req))
-- 
2.11.2

Re: [RFC 3/9] zfcp: split FCP_CMND IU setup between SCSI I/O and TMF again

[Hannes Reinecke]

On 07/25/2017 04:14 PM, Steffen Maier wrote:
> This reverts commit 2443c8b23aea ("[SCSI] zfcp: Merge FCP task management
> setup with regular FCP command setup"), because this introduced a
> dependency on the unsuitable SCSI command for scsi_eh / TMF.
> 
> Signed-off-by: Steffen Maier <maier@linux.vnet.ibm.com>
> ---
>  drivers/s390/scsi/zfcp_fc.h  | 22 ++++++++++++++--------
>  drivers/s390/scsi/zfcp_fsf.c |  4 ++--
>  2 files changed, 16 insertions(+), 10 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

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)

[RFC 4/9] zfcp: decouple FSF request setup of TMF from scsi_cmnd

[Steffen Maier]

The scsi_device argument of zfcp_fc_fcp_tm() can now be NULL.

In zfcp_fsf_fcp_task_mgmt() resolve the still old argument scsi_cmnd
into scsi_device very early and only depend on scsi_device and derived
objects in the function body.

Scsi_device and derived zfcp_scsi_dev can later be NULL for the
target reset case, so do not depend on them unconditionally.
For the generic case, rather change to using zfcp_port directly.

This prepares to later change the function signature replacing the
scsi_cmnd argument with zfcp_port and an
optional scsi_device which can be NULL.

Signed-off-by: Steffen Maier <maier@linux.vnet.ibm.com>
---
 drivers/s390/scsi/zfcp_fc.h  |  6 ++++--
 drivers/s390/scsi/zfcp_fsf.c | 25 +++++++++++++++++--------
 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_fc.h b/drivers/s390/scsi/zfcp_fc.h
index 24949868d027..0e5b01c33873 100644
--- a/drivers/s390/scsi/zfcp_fc.h
+++ b/drivers/s390/scsi/zfcp_fc.h
@@ -235,13 +235,15 @@ void zfcp_fc_scsi_to_fcp(struct fcp_cmnd *fcp, struct scsi_cmnd *scsi)
 /**
  * zfcp_fc_fcp_tm() - Setup FCP command as task management command.
  * @fcp: Pointer to FCP_CMND IU to set up.
- * @dev: Pointer to SCSI_device where to send the task management command.
+ * @dev: Pointer to SCSI device if LUN Reset TMF, or %NULL.
  * @tm_flags: Task management flags to setup tm command.
  */
 static inline
 void zfcp_fc_fcp_tm(struct fcp_cmnd *fcp, struct scsi_device *dev, u8 tm_flags)
 {
-	int_to_scsilun(dev->lun, (struct scsi_lun *) &fcp->fc_lun);
+	if (dev)
+		int_to_scsilun(dev->lun, (struct scsi_lun *) &fcp->fc_lun);
+
 	fcp->fc_tm_flags = tm_flags;
 }
 
diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
index f221a34c26df..2dc7d2a6f6ea 100644
--- a/drivers/s390/scsi/zfcp_fsf.c
+++ b/drivers/s390/scsi/zfcp_fsf.c
@@ -2339,13 +2339,19 @@ struct zfcp_fsf_req *zfcp_fsf_fcp_task_mgmt(struct scsi_cmnd *scmnd,
 {
 	struct zfcp_fsf_req *req = NULL;
 	struct fcp_cmnd *fcp_cmnd;
-	struct zfcp_scsi_dev *zfcp_sdev = sdev_to_zfcp(scmnd->device);
-	struct zfcp_qdio *qdio = zfcp_sdev->port->adapter->qdio;
+	struct scsi_device *sdev = scmnd->device;
+	struct zfcp_scsi_dev *zfcp_sdev = sdev_to_zfcp(sdev);
+	struct zfcp_port *port = zfcp_sdev->port;
+	struct zfcp_qdio *qdio = port->adapter->qdio;
 
-	if (unlikely(!(atomic_read(&zfcp_sdev->status) &
+	if (unlikely(!(atomic_read(&port->status) &
 		       ZFCP_STATUS_COMMON_UNBLOCKED)))
 		return NULL;
 
+	if (unlikely(zfcp_sdev && !(atomic_read(&zfcp_sdev->status) &
+				    ZFCP_STATUS_COMMON_UNBLOCKED)))
+		return NULL;
+
 	spin_lock_irq(&qdio->req_q_lock);
 	if (zfcp_qdio_sbal_get(qdio))
 		goto out;
@@ -2360,18 +2366,21 @@ struct zfcp_fsf_req *zfcp_fsf_fcp_task_mgmt(struct scsi_cmnd *scmnd,
 	}
 
 	fcp_cmnd = &req->qtcb->bottom.io.fcp_cmnd.iu;
-	req->data = (fcp_cmnd->fc_tm_flags & FCP_TMF_LUN_RESET) ?
-		scmnd->device : (void *)sdev_to_zfcp(scmnd->device)->port;
+	if (fcp_cmnd->fc_tm_flags & FCP_TMF_LUN_RESET) {
+		req->data = sdev;
+		req->qtcb->header.lun_handle = zfcp_sdev->lun_handle;
+	} else
+		req->data = port;
+
 	req->handler = zfcp_fsf_fcp_task_mgmt_handler;
-	req->qtcb->header.lun_handle = zfcp_sdev->lun_handle;
-	req->qtcb->header.port_handle = zfcp_sdev->port->handle;
+	req->qtcb->header.port_handle = port->handle;
 	req->qtcb->bottom.io.data_direction = FSF_DATADIR_CMND;
 	req->qtcb->bottom.io.service_class = FSF_CLASS_3;
 	req->qtcb->bottom.io.fcp_cmnd_length = FCP_CMND_LEN;
 
 	zfcp_qdio_set_sbale_last(qdio, &req->qdio_req);
 
-	zfcp_fc_fcp_tm(fcp_cmnd, scmnd->device, tm_flags);
+	zfcp_fc_fcp_tm(fcp_cmnd, sdev, tm_flags);
 
 	zfcp_fsf_start_timer(req, ZFCP_SCSI_ER_TIMEOUT);
 	if (!zfcp_fsf_req_send(req))
-- 
2.11.2

Re: [RFC 4/9] zfcp: decouple FSF request setup of TMF from scsi_cmnd

[Hannes Reinecke]

On 07/25/2017 04:14 PM, Steffen Maier wrote:
> The scsi_device argument of zfcp_fc_fcp_tm() can now be NULL.
> 
> In zfcp_fsf_fcp_task_mgmt() resolve the still old argument scsi_cmnd
> into scsi_device very early and only depend on scsi_device and derived
> objects in the function body.
> 
> Scsi_device and derived zfcp_scsi_dev can later be NULL for the
> target reset case, so do not depend on them unconditionally.
> For the generic case, rather change to using zfcp_port directly.
> 
> This prepares to later change the function signature replacing the
> scsi_cmnd argument with zfcp_port and an
> optional scsi_device which can be NULL.
> 
> Signed-off-by: Steffen Maier <maier@linux.vnet.ibm.com>
> ---
>  drivers/s390/scsi/zfcp_fc.h  |  6 ++++--
>  drivers/s390/scsi/zfcp_fsf.c | 25 +++++++++++++++++--------
>  2 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/s390/scsi/zfcp_fc.h b/drivers/s390/scsi/zfcp_fc.h
> index 24949868d027..0e5b01c33873 100644
> --- a/drivers/s390/scsi/zfcp_fc.h
> +++ b/drivers/s390/scsi/zfcp_fc.h
> @@ -235,13 +235,15 @@ void zfcp_fc_scsi_to_fcp(struct fcp_cmnd *fcp, struct scsi_cmnd *scsi)
>  /**
>   * zfcp_fc_fcp_tm() - Setup FCP command as task management command.
>   * @fcp: Pointer to FCP_CMND IU to set up.
> - * @dev: Pointer to SCSI_device where to send the task management command.
> + * @dev: Pointer to SCSI device if LUN Reset TMF, or %NULL.
>   * @tm_flags: Task management flags to setup tm command.
>   */
>  static inline
>  void zfcp_fc_fcp_tm(struct fcp_cmnd *fcp, struct scsi_device *dev, u8 tm_flags)
>  {
> -	int_to_scsilun(dev->lun, (struct scsi_lun *) &fcp->fc_lun);
> +	if (dev)
> +		int_to_scsilun(dev->lun, (struct scsi_lun *) &fcp->fc_lun);
> +
>  	fcp->fc_tm_flags = tm_flags;
>  }
>  
Hmm. This function is becoming so small, _and_ with a conditional to
boot. Maybe you should simply open-coding it?

> diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
> index f221a34c26df..2dc7d2a6f6ea 100644
> --- a/drivers/s390/scsi/zfcp_fsf.c
> +++ b/drivers/s390/scsi/zfcp_fsf.c
> @@ -2339,13 +2339,19 @@ struct zfcp_fsf_req *zfcp_fsf_fcp_task_mgmt(struct scsi_cmnd *scmnd,
>  {
>  	struct zfcp_fsf_req *req = NULL;
>  	struct fcp_cmnd *fcp_cmnd;
> -	struct zfcp_scsi_dev *zfcp_sdev = sdev_to_zfcp(scmnd->device);
> -	struct zfcp_qdio *qdio = zfcp_sdev->port->adapter->qdio;
> +	struct scsi_device *sdev = scmnd->device;
> +	struct zfcp_scsi_dev *zfcp_sdev = sdev_to_zfcp(sdev);
> +	struct zfcp_port *port = zfcp_sdev->port;
> +	struct zfcp_qdio *qdio = port->adapter->qdio;
>  
> -	if (unlikely(!(atomic_read(&zfcp_sdev->status) &
> +	if (unlikely(!(atomic_read(&port->status) &
>  		       ZFCP_STATUS_COMMON_UNBLOCKED)))
>  		return NULL;
>  
> +	if (unlikely(zfcp_sdev && !(atomic_read(&zfcp_sdev->status) &
> +				    ZFCP_STATUS_COMMON_UNBLOCKED)))
> +		return NULL;
> +
>  	spin_lock_irq(&qdio->req_q_lock);
>  	if (zfcp_qdio_sbal_get(qdio))
>  		goto out;
> @@ -2360,18 +2366,21 @@ struct zfcp_fsf_req *zfcp_fsf_fcp_task_mgmt(struct scsi_cmnd *scmnd,
>  	}
>  
>  	fcp_cmnd = &req->qtcb->bottom.io.fcp_cmnd.iu;
> -	req->data = (fcp_cmnd->fc_tm_flags & FCP_TMF_LUN_RESET) ?
> -		scmnd->device : (void *)sdev_to_zfcp(scmnd->device)->port;
> +	if (fcp_cmnd->fc_tm_flags & FCP_TMF_LUN_RESET) {
> +		req->data = sdev;
> +		req->qtcb->header.lun_handle = zfcp_sdev->lun_handle;
> +	} else
> +		req->data = port;
> +
>  	req->handler = zfcp_fsf_fcp_task_mgmt_handler;
> -	req->qtcb->header.lun_handle = zfcp_sdev->lun_handle;
> -	req->qtcb->header.port_handle = zfcp_sdev->port->handle;
> +	req->qtcb->header.port_handle = port->handle;
>  	req->qtcb->bottom.io.data_direction = FSF_DATADIR_CMND;
>  	req->qtcb->bottom.io.service_class = FSF_CLASS_3;
>  	req->qtcb->bottom.io.fcp_cmnd_length = FCP_CMND_LEN;
>  
>  	zfcp_qdio_set_sbale_last(qdio, &req->qdio_req);
>  
> -	zfcp_fc_fcp_tm(fcp_cmnd, scmnd->device, tm_flags);
> +	zfcp_fc_fcp_tm(fcp_cmnd, sdev, tm_flags);
>  
>  	zfcp_fsf_start_timer(req, ZFCP_SCSI_ER_TIMEOUT);
>  	if (!zfcp_fsf_req_send(req))
> 
Otherwise:

Reviewed-by: Hannes Reinecke <hare@suse.com>

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: [RFC 4/9] zfcp: decouple FSF request setup of TMF from scsi_cmnd

[Steffen Maier]

Just for the records: There's another bug below.

On 07/25/2017 04:14 PM, Steffen Maier wrote:
> The scsi_device argument of zfcp_fc_fcp_tm() can now be NULL.
> 
> In zfcp_fsf_fcp_task_mgmt() resolve the still old argument scsi_cmnd
> into scsi_device very early and only depend on scsi_device and derived
> objects in the function body.
> 
> Scsi_device and derived zfcp_scsi_dev can later be NULL for the
> target reset case, so do not depend on them unconditionally.
> For the generic case, rather change to using zfcp_port directly.
> 
> This prepares to later change the function signature replacing the
> scsi_cmnd argument with zfcp_port and an
> optional scsi_device which can be NULL.
> 
> Signed-off-by: Steffen Maier <maier@linux.vnet.ibm.com>
> ---
>   drivers/s390/scsi/zfcp_fc.h  |  6 ++++--
>   drivers/s390/scsi/zfcp_fsf.c | 25 +++++++++++++++++--------
>   2 files changed, 21 insertions(+), 10 deletions(-)

> diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
> index f221a34c26df..2dc7d2a6f6ea 100644
> --- a/drivers/s390/scsi/zfcp_fsf.c
> +++ b/drivers/s390/scsi/zfcp_fsf.c
> @@ -2339,13 +2339,19 @@ struct zfcp_fsf_req *zfcp_fsf_fcp_task_mgmt(struct scsi_cmnd *scmnd,
>   {
>   	struct zfcp_fsf_req *req = NULL;
>   	struct fcp_cmnd *fcp_cmnd;
> -	struct zfcp_scsi_dev *zfcp_sdev = sdev_to_zfcp(scmnd->device);
> -	struct zfcp_qdio *qdio = zfcp_sdev->port->adapter->qdio;
> +	struct scsi_device *sdev = scmnd->device;
> +	struct zfcp_scsi_dev *zfcp_sdev = sdev_to_zfcp(sdev);

BUG: must not unconditionally dereference sdev which can be NULL later 
on in the patch set!

Fix: +	struct zfcp_scsi_dev *zfcp_sdev = sdev ? sdev_to_zfcp(sdev) : NULL;

Fix is no longer necessary in my reworked v2 (always having a non-NULL 
sdev) to be sent when I successfully completed function test.

> +	struct zfcp_port *port = zfcp_sdev->port;

This line was removed in the subsequent patch 5/9, so here the 
unconditional deref is OK because here in this patch we still get a 
non-NULL sdev. (The line is just argument lifting preparing for the 
function argument replacement in 5/9.)

Other accesses to sdev or zfcp_sdev were properly guarded with this patch.

-- 
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

[RFC 5/9] zfcp: decouple SCSI setup of TMF from scsi_cmnd

[Steffen Maier]

Actually change the signature of zfcp_fsf_fcp_task_mgmt().
Since it was prepared in the previous patch, we only need to delete
some local auto variables which are now the intended arguments.

Refactor zfcp_scsi_forget_cmnds() to now take a mandatory zfcp_port
and an optional zfcp_scsi_dev, which can be NULL for target reset,
instead of a mandatory zfcp_scsi_dev.

Prepare zfcp_fsf_fcp_task_mgmt's caller zfcp_task_mgmt_function()
to have its function body only depend on a mandatory zfcp_port
and an optional scsi_device, which can be NULL for target reset.

Signed-off-by: Steffen Maier <maier@linux.vnet.ibm.com>
---
 drivers/s390/scsi/zfcp_ext.h  |  4 +++-
 drivers/s390/scsi/zfcp_fsf.c  | 15 ++++++++-------
 drivers/s390/scsi/zfcp_scsi.c | 28 +++++++++++++++++++---------
 3 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_ext.h b/drivers/s390/scsi/zfcp_ext.h
index f3101bc5d1bc..26772b0c1c39 100644
--- a/drivers/s390/scsi/zfcp_ext.h
+++ b/drivers/s390/scsi/zfcp_ext.h
@@ -118,7 +118,9 @@ extern int zfcp_fsf_send_els(struct zfcp_adapter *, u32,
 			     struct zfcp_fsf_ct_els *, unsigned int);
 extern int zfcp_fsf_fcp_cmnd(struct scsi_cmnd *);
 extern void zfcp_fsf_req_free(struct zfcp_fsf_req *);
-extern struct zfcp_fsf_req *zfcp_fsf_fcp_task_mgmt(struct scsi_cmnd *, u8);
+extern struct zfcp_fsf_req *zfcp_fsf_fcp_task_mgmt(struct zfcp_port *port,
+						   struct scsi_device *sdev,
+						   u8 tm_flags);
 extern struct zfcp_fsf_req *zfcp_fsf_abort_fcp_cmnd(struct scsi_cmnd *);
 extern void zfcp_fsf_reqid_check(struct zfcp_qdio *, int);
 
diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
index 2dc7d2a6f6ea..7cc2d7ee1f56 100644
--- a/drivers/s390/scsi/zfcp_fsf.c
+++ b/drivers/s390/scsi/zfcp_fsf.c
@@ -2329,19 +2329,20 @@ static void zfcp_fsf_fcp_task_mgmt_handler(struct zfcp_fsf_req *req)
 }
 
 /**
- * zfcp_fsf_fcp_task_mgmt - send SCSI task management command
- * @scmnd: SCSI command to send the task management command for
- * @tm_flags: unsigned byte for task management flags
- * Returns: on success pointer to struct fsf_req, NULL otherwise
+ * zfcp_fsf_fcp_task_mgmt() - Send SCSI task management command (TMF).
+ * @port: Pointer to zfcp port as scope for TMF.
+ * @sdev: Pointer to scsi device if LUN Reset TMF, or %NULL.
+ * @tm_flags: Unsigned byte for task management flags.
+ *
+ * Return: On success pointer to struct fsf_req, %NULL otherwise.
  */
-struct zfcp_fsf_req *zfcp_fsf_fcp_task_mgmt(struct scsi_cmnd *scmnd,
+struct zfcp_fsf_req *zfcp_fsf_fcp_task_mgmt(struct zfcp_port *port,
+					    struct scsi_device *sdev,
 					    u8 tm_flags)
 {
 	struct zfcp_fsf_req *req = NULL;
 	struct fcp_cmnd *fcp_cmnd;
-	struct scsi_device *sdev = scmnd->device;
 	struct zfcp_scsi_dev *zfcp_sdev = sdev_to_zfcp(sdev);
-	struct zfcp_port *port = zfcp_sdev->port;
 	struct zfcp_qdio *qdio = port->adapter->qdio;
 
 	if (unlikely(!(atomic_read(&port->status) &
diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
index cd0f811452b7..05c823ccb959 100644
--- a/drivers/s390/scsi/zfcp_scsi.c
+++ b/drivers/s390/scsi/zfcp_scsi.c
@@ -234,12 +234,20 @@ static void zfcp_scsi_forget_cmnd(struct zfcp_fsf_req *old_req, void *data)
 	old_req->data = NULL;
 }
 
-static void zfcp_scsi_forget_cmnds(struct zfcp_scsi_dev *zsdev, u8 tm_flags)
+/**
+ * zfcp_scsi_forget_cmnds() - Forget pending SCSI requests on given scope.
+ * @port: Pointer to zfcp port indicating scope.
+ * @zsdev: Pointer to zfcp scsi dev as scope, or %NULL if scope is only port.
+ * @tm_flags: Task management flags,
+ *            here we only handle %FCP_TMF_TGT_RESET or %FCP_TMF_LUN_RESET.
+ */
+static void zfcp_scsi_forget_cmnds(struct zfcp_port *port,
+				   struct zfcp_scsi_dev *zsdev, u8 tm_flags)
 {
-	struct zfcp_adapter *adapter = zsdev->port->adapter;
+	struct zfcp_adapter *adapter = port->adapter;
 	struct zfcp_scsi_req_filter filter = {
 		.tmf_scope = FCP_TMF_TGT_RESET,
-		.port_handle = zsdev->port->handle,
+		.port_handle = port->handle,
 	};
 	unsigned long flags;
 
@@ -260,19 +268,21 @@ static void zfcp_scsi_forget_cmnds(struct zfcp_scsi_dev *zsdev, u8 tm_flags)
 
 static int zfcp_task_mgmt_function(struct scsi_cmnd *scpnt, u8 tm_flags)
 {
-	struct zfcp_scsi_dev *zfcp_sdev = sdev_to_zfcp(scpnt->device);
-	struct zfcp_adapter *adapter = zfcp_sdev->port->adapter;
-	unsigned int scsi_id = zfcp_sdev->port->starget_id;
+	struct scsi_device *sdev = scpnt->device;
+	struct zfcp_scsi_dev *zfcp_sdev = sdev_to_zfcp(sdev);
+	struct zfcp_port *port = zfcp_sdev->port;
+	struct zfcp_adapter *adapter = port->adapter;
+	unsigned int scsi_id = port->starget_id;
 	u64 scsi_lun = ZFCP_DBF_INVALID_LUN;
 	struct zfcp_fsf_req *fsf_req = NULL;
 	int retval = SUCCESS, ret;
 	int retry = 3;
 
 	if (tm_flags == FCP_TMF_LUN_RESET)
-		scsi_lun = scpnt->device->lun;
+		scsi_lun = sdev->lun;
 
 	while (retry--) {
-		fsf_req = zfcp_fsf_fcp_task_mgmt(scpnt, tm_flags);
+		fsf_req = zfcp_fsf_fcp_task_mgmt(port, sdev, tm_flags);
 		if (fsf_req)
 			break;
 
@@ -306,7 +316,7 @@ static int zfcp_task_mgmt_function(struct scsi_cmnd *scpnt, u8 tm_flags)
 	} else {
 		zfcp_dbf_scsi_devreset("okay", adapter, tm_flags, fsf_req,
 				       scsi_id, scsi_lun);
-		zfcp_scsi_forget_cmnds(zfcp_sdev, tm_flags);
+		zfcp_scsi_forget_cmnds(port, zfcp_sdev, tm_flags);
 	}
 
 	zfcp_fsf_req_free(fsf_req);
-- 
2.11.2

Re: [RFC 5/9] zfcp: decouple SCSI setup of TMF from scsi_cmnd

[Hannes Reinecke]

On 07/25/2017 04:14 PM, Steffen Maier wrote:
> Actually change the signature of zfcp_fsf_fcp_task_mgmt().
> Since it was prepared in the previous patch, we only need to delete
> some local auto variables which are now the intended arguments.
> 
> Refactor zfcp_scsi_forget_cmnds() to now take a mandatory zfcp_port
> and an optional zfcp_scsi_dev, which can be NULL for target reset,
> instead of a mandatory zfcp_scsi_dev.
> 
> Prepare zfcp_fsf_fcp_task_mgmt's caller zfcp_task_mgmt_function()
> to have its function body only depend on a mandatory zfcp_port
> and an optional scsi_device, which can be NULL for target reset.
> 
> Signed-off-by: Steffen Maier <maier@linux.vnet.ibm.com>
> ---
>  drivers/s390/scsi/zfcp_ext.h  |  4 +++-
>  drivers/s390/scsi/zfcp_fsf.c  | 15 ++++++++-------
>  drivers/s390/scsi/zfcp_scsi.c | 28 +++++++++++++++++++---------
>  3 files changed, 30 insertions(+), 17 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

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)

[RFC 6/9] scsi: fc: start decoupling fc_block_scsi_eh from scsi_cmnd

[Steffen Maier]

Scsi_cmnd is an unsuitable argument for eh_device_reset_handler(),
eh_target_reset_handler(), and eh_host_reset_handler()
which do not have the scope of one single SCSI command.
These callbacks tend to use fc_block_scsi_eh() requiring scsi_cmnd.
In order to start decoupling above eh callbacks from scsi_cmnd,
introduce a new variant of the function called fc_block_rport()
taking an fc_rport as argument.
Refactor the old fc_block_scsi_eh() to simply delegate to fc_block_rport().

Signed-off-by: Steffen Maier <maier@linux.vnet.ibm.com>
---
 drivers/scsi/scsi_transport_fc.c | 31 ++++++++++++++++++++++++++-----
 include/scsi/scsi_transport_fc.h |  1 +
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index d4cf32d55546..3594043834c7 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -3272,8 +3272,8 @@ fc_scsi_scan_rport(struct work_struct *work)
 }
 
 /**
- * fc_block_scsi_eh - Block SCSI eh thread for blocked fc_rport
- * @cmnd: SCSI command that scsi_eh is trying to recover
+ * fc_block_rport() - Block SCSI eh thread for blocked fc_rport.
+ * @rport: Remote port that scsi_eh is trying to recover.
  *
  * This routine can be called from a FC LLD scsi_eh callback. It
  * blocks the scsi_eh thread until the fc_rport leaves the
@@ -3285,10 +3285,9 @@ fc_scsi_scan_rport(struct work_struct *work)
  *	    FAST_IO_FAIL if the fast_io_fail_tmo fired, this should be
  *	    passed back to scsi_eh.
  */
-int fc_block_scsi_eh(struct scsi_cmnd *cmnd)
+int fc_block_rport(struct fc_rport *rport)
 {
-	struct Scsi_Host *shost = cmnd->device->host;
-	struct fc_rport *rport = starget_to_rport(scsi_target(cmnd->device));
+	struct Scsi_Host *shost = rport_to_shost(rport);
 	unsigned long flags;
 
 	spin_lock_irqsave(shost->host_lock, flags);
@@ -3305,6 +3304,28 @@ int fc_block_scsi_eh(struct scsi_cmnd *cmnd)
 
 	return 0;
 }
+EXPORT_SYMBOL(fc_block_rport);
+
+/**
+ * fc_block_scsi_eh - Block SCSI eh thread for blocked fc_rport
+ * @cmnd: SCSI command that scsi_eh is trying to recover
+ *
+ * This routine can be called from a FC LLD scsi_eh callback. It
+ * blocks the scsi_eh thread until the fc_rport leaves the
+ * FC_PORTSTATE_BLOCKED, or the fast_io_fail_tmo fires. This is
+ * necessary to avoid the scsi_eh failing recovery actions for blocked
+ * rports which would lead to offlined SCSI devices.
+ *
+ * Returns: 0 if the fc_rport left the state FC_PORTSTATE_BLOCKED.
+ *	    FAST_IO_FAIL if the fast_io_fail_tmo fired, this should be
+ *	    passed back to scsi_eh.
+ */
+int fc_block_scsi_eh(struct scsi_cmnd *cmnd)
+{
+	struct fc_rport *rport = starget_to_rport(scsi_target(cmnd->device));
+
+	return fc_block_rport(rport);
+}
 EXPORT_SYMBOL(fc_block_scsi_eh);
 
 /**
diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
index 6e208bb32c78..d8cae7bd8161 100644
--- a/include/scsi/scsi_transport_fc.h
+++ b/include/scsi/scsi_transport_fc.h
@@ -808,6 +808,7 @@ void fc_host_post_vendor_event(struct Scsi_Host *shost, u32 event_number,
 struct fc_vport *fc_vport_create(struct Scsi_Host *shost, int channel,
 		struct fc_vport_identifiers *);
 int fc_vport_terminate(struct fc_vport *vport);
+int fc_block_rport(struct fc_rport *rport);
 int fc_block_scsi_eh(struct scsi_cmnd *cmnd);
 enum blk_eh_timer_return fc_eh_timed_out(struct scsi_cmnd *scmd);
 
-- 
2.11.2

Re: [RFC 6/9] scsi: fc: start decoupling fc_block_scsi_eh from scsi_cmnd

[Hannes Reinecke]

On 07/25/2017 04:14 PM, Steffen Maier wrote:
> Scsi_cmnd is an unsuitable argument for eh_device_reset_handler(),
> eh_target_reset_handler(), and eh_host_reset_handler()
> which do not have the scope of one single SCSI command.
> These callbacks tend to use fc_block_scsi_eh() requiring scsi_cmnd.
> In order to start decoupling above eh callbacks from scsi_cmnd,
> introduce a new variant of the function called fc_block_rport()
> taking an fc_rport as argument.
> Refactor the old fc_block_scsi_eh() to simply delegate to fc_block_rport().
> 
> Signed-off-by: Steffen Maier <maier@linux.vnet.ibm.com>
> ---
>  drivers/scsi/scsi_transport_fc.c | 31 ++++++++++++++++++++++++++-----
>  include/scsi/scsi_transport_fc.h |  1 +
>  2 files changed, 27 insertions(+), 5 deletions(-)
> 
Very good.
I need that for my patchset as well.
Martin, would it be possible to apply this independent of this patchset?
It would help me a lot when redrafting my patchset.

Reviewed-by: Hannes Reinecke <hare@suse.com>

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: [RFC 6/9] scsi: fc: start decoupling fc_block_scsi_eh from scsi_cmnd

[Martin K. Petersen]

Steffen,

> Scsi_cmnd is an unsuitable argument for eh_device_reset_handler(),
> eh_target_reset_handler(), and eh_host_reset_handler() which do not
> have the scope of one single SCSI command.  These callbacks tend to
> use fc_block_scsi_eh() requiring scsi_cmnd.  In order to start
> decoupling above eh callbacks from scsi_cmnd, introduce a new variant
> of the function called fc_block_rport() taking an fc_rport as
> argument.  Refactor the old fc_block_scsi_eh() to simply delegate to
> fc_block_rport().

Applied to 4.14/scsi-queue. Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

[RFC 7/9] zfcp: use fc_block_rport for TMFs and host reset to decouple from scsi_cmnd

[Steffen Maier]

Signed-off-by: Steffen Maier <maier@linux.vnet.ibm.com>
---
 drivers/s390/scsi/zfcp_scsi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
index 05c823ccb959..8e96196fa877 100644
--- a/drivers/s390/scsi/zfcp_scsi.c
+++ b/drivers/s390/scsi/zfcp_scsi.c
@@ -287,7 +287,7 @@ static int zfcp_task_mgmt_function(struct scsi_cmnd *scpnt, u8 tm_flags)
 			break;
 
 		zfcp_erp_wait(adapter);
-		ret = fc_block_scsi_eh(scpnt);
+		ret = port->rport ? fc_block_rport(port->rport) : 0;
 		if (ret) {
 			zfcp_dbf_scsi_devreset("fiof", adapter, tm_flags, NULL,
 					       scsi_id, scsi_lun);
@@ -337,11 +337,13 @@ static int zfcp_scsi_eh_host_reset_handler(struct scsi_cmnd *scpnt)
 {
 	struct zfcp_scsi_dev *zfcp_sdev = sdev_to_zfcp(scpnt->device);
 	struct zfcp_adapter *adapter = zfcp_sdev->port->adapter;
+	struct zfcp_port *port;
 	int ret;
 
 	zfcp_erp_adapter_reopen(adapter, 0, "schrh_1");
 	zfcp_erp_wait(adapter);
-	ret = fc_block_scsi_eh(scpnt);
+	port = zfcp_sdev->port;
+	ret = port->rport ? fc_block_rport(port->rport) : 0;
 	if (ret)
 		return ret;
 
-- 
2.11.2

Re: [RFC 7/9] zfcp: use fc_block_rport for TMFs and host reset to decouple from scsi_cmnd

[Hannes Reinecke]

On 07/25/2017 04:14 PM, Steffen Maier wrote:
> Signed-off-by: Steffen Maier <maier@linux.vnet.ibm.com>
> ---
>  drivers/s390/scsi/zfcp_scsi.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
> index 05c823ccb959..8e96196fa877 100644
> --- a/drivers/s390/scsi/zfcp_scsi.c
> +++ b/drivers/s390/scsi/zfcp_scsi.c
> @@ -287,7 +287,7 @@ static int zfcp_task_mgmt_function(struct scsi_cmnd *scpnt, u8 tm_flags)
>  			break;
>  
>  		zfcp_erp_wait(adapter);
> -		ret = fc_block_scsi_eh(scpnt);
> +		ret = port->rport ? fc_block_rport(port->rport) : 0;
>  		if (ret) {
>  			zfcp_dbf_scsi_devreset("fiof", adapter, tm_flags, NULL,
>  					       scsi_id, scsi_lun);> @@ -337,11 +337,13 @@ static int
zfcp_scsi_eh_host_reset_handler(struct scsi_cmnd *scpnt)
>  {
>  	struct zfcp_scsi_dev *zfcp_sdev = sdev_to_zfcp(scpnt->device);
>  	struct zfcp_adapter *adapter = zfcp_sdev->port->adapter;
> +	struct zfcp_port *port;
>  	int ret;
>  
>  	zfcp_erp_adapter_reopen(adapter, 0, "schrh_1");
>  	zfcp_erp_wait(adapter);
> -	ret = fc_block_scsi_eh(scpnt);
> +	port = zfcp_sdev->port;
> +	ret = port->rport ? fc_block_rport(port->rport) : 0;
>  	if (ret)
>  		return ret;
>  
> 

Hmm. So there is a chance where ->rport is simply NULL, in which case we
won't be calling fc_block_rport().
However, we _do_ continue with TMF even then.
Wasn't that precisely the point of fc_block_rport() to figure out if the
rport is valid?
So shouldn't we rather return FAILED or something here?
If not, why do we continue sending TMFs to a non-existing port?

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)

[RFC 8/9] zfcp: fix waiting for rport(s) unblock in eh_host_reset_handler

[Steffen Maier]

v2.6.30 commit 63caf367e1c9 ("[SCSI] zfcp: Improve reliability of SCSI eh
handlers in zfcp") added calls to zfcp_erp_wait() within
eh_abort_handler(), eh_device_reset_handler(), eh_target_reset_handler()
in order to synchronize with zfcp recovery completion before returning
from a scsi_eh callback (e.g. with SUCCESS) to prevent eh escalation.

v2.6.33 commit af4de36d911a ("[SCSI] zfcp: Block scsi_eh thread for rport
state BLOCKED") introduced the use of fc_block_scsi_eh() for
eh_abort_handler(), eh_device_reset_handler(), eh_target_reset_handler(),
and eh_host_reset_handler(), because zfcp_erp_wait() from above commit is
not sufficient.
The use in zfcp_task_mgmt_function() is correct even for a LUN reset,
as described in commit 6f2ce1c6af37 ("scsi: zfcp: fix rport unblock race
with LUN recovery").
However, the one call in zfcp_scsi_eh_host_reset_handler() waiting for
just one arbitrary port of the arbitrary scsi_cmnd seems insufficient
as the preceding adapter recovery could have recovered multiple ports
for which we all should wait to unblock (or have run into FAST_IO_FAIL).

Therefore, we now wait for all ports of the adapter with this fix.

NB: We cannot easily wait for an event because there is a time window
between zfcp_erp_wait() returned and zfcp_erp_try_rport_unblock() as part
of zfcp_erp_action_cleanup() actually scheduled rport_work which will
unblock an rport in zfcp_scsi_rport_work() asynchronously. Hence a
flush_work() could come early before queue_work() was even done.

v2.6.35 commit a1dbfddd02d2 ("[SCSI] zfcp: Pass return code from
fc_block_scsi_eh to scsi eh") fixed v2.6.33 for the FAST_IO_FAIL case.

Signed-off-by: Steffen Maier <maier@linux.vnet.ibm.com>
Fixes: af4de36d911a ("[SCSI] zfcp: Block scsi_eh thread for rport state BLOCKED")
Fixes: a1dbfddd02d2 ("[SCSI] zfcp: Pass return code from fc_block_scsi_eh to scsi eh")
---
 drivers/s390/scsi/zfcp_scsi.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
index 8e96196fa877..11cf33ea8c14 100644
--- a/drivers/s390/scsi/zfcp_scsi.c
+++ b/drivers/s390/scsi/zfcp_scsi.c
@@ -338,16 +338,29 @@ static int zfcp_scsi_eh_host_reset_handler(struct scsi_cmnd *scpnt)
 	struct zfcp_scsi_dev *zfcp_sdev = sdev_to_zfcp(scpnt->device);
 	struct zfcp_adapter *adapter = zfcp_sdev->port->adapter;
 	struct zfcp_port *port;
-	int ret;
+	int ret = SUCCESS;
 
 	zfcp_erp_adapter_reopen(adapter, 0, "schrh_1");
 	zfcp_erp_wait(adapter);
-	port = zfcp_sdev->port;
-	ret = port->rport ? fc_block_rport(port->rport) : 0;
-	if (ret)
-		return ret;
+	/* after internal recovery, wait for async unblock of rport(s) */
+	read_lock(&adapter->port_list_lock);
+	list_for_each_entry(port, &adapter->port_list, list) {
+		int fc_ret;
+
+		if (!port->rport)
+			continue;
+
+		fc_ret = fc_block_rport(port->rport);
+		/* Any rport ran into fast_io_fail_tmo: FAST_IO_FAIL.
+		 * To let pending requests bubble up, even if too many
+		 * because of other rports without this timeout.
+		 */
+		if (fc_ret)
+			ret = fc_ret;
+	}
+	read_unlock(&adapter->port_list_lock);
 
-	return SUCCESS;
+	return ret;
 }
 
 struct scsi_transport_template *zfcp_scsi_transport_template;
-- 
2.11.2

Re: [RFC 8/9] zfcp: fix waiting for rport(s) unblock in eh_host_reset_handler

[Hannes Reinecke]

On 07/25/2017 04:14 PM, Steffen Maier wrote:
> v2.6.30 commit 63caf367e1c9 ("[SCSI] zfcp: Improve reliability of SCSI eh
> handlers in zfcp") added calls to zfcp_erp_wait() within
> eh_abort_handler(), eh_device_reset_handler(), eh_target_reset_handler()
> in order to synchronize with zfcp recovery completion before returning
> from a scsi_eh callback (e.g. with SUCCESS) to prevent eh escalation.
> 
> v2.6.33 commit af4de36d911a ("[SCSI] zfcp: Block scsi_eh thread for rport
> state BLOCKED") introduced the use of fc_block_scsi_eh() for
> eh_abort_handler(), eh_device_reset_handler(), eh_target_reset_handler(),
> and eh_host_reset_handler(), because zfcp_erp_wait() from above commit is
> not sufficient.
> The use in zfcp_task_mgmt_function() is correct even for a LUN reset,
> as described in commit 6f2ce1c6af37 ("scsi: zfcp: fix rport unblock race
> with LUN recovery").
> However, the one call in zfcp_scsi_eh_host_reset_handler() waiting for
> just one arbitrary port of the arbitrary scsi_cmnd seems insufficient
> as the preceding adapter recovery could have recovered multiple ports
> for which we all should wait to unblock (or have run into FAST_IO_FAIL).
> 
> Therefore, we now wait for all ports of the adapter with this fix.
> 
> NB: We cannot easily wait for an event because there is a time window
> between zfcp_erp_wait() returned and zfcp_erp_try_rport_unblock() as part
> of zfcp_erp_action_cleanup() actually scheduled rport_work which will
> unblock an rport in zfcp_scsi_rport_work() asynchronously. Hence a
> flush_work() could come early before queue_work() was even done.
> 
> v2.6.35 commit a1dbfddd02d2 ("[SCSI] zfcp: Pass return code from
> fc_block_scsi_eh to scsi eh") fixed v2.6.33 for the FAST_IO_FAIL case.
> 
> Signed-off-by: Steffen Maier <maier@linux.vnet.ibm.com>
> Fixes: af4de36d911a ("[SCSI] zfcp: Block scsi_eh thread for rport state BLOCKED")
> Fixes: a1dbfddd02d2 ("[SCSI] zfcp: Pass return code from fc_block_scsi_eh to scsi eh")
> ---
>  drivers/s390/scsi/zfcp_scsi.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
> index 8e96196fa877..11cf33ea8c14 100644
> --- a/drivers/s390/scsi/zfcp_scsi.c
> +++ b/drivers/s390/scsi/zfcp_scsi.c
> @@ -338,16 +338,29 @@ static int zfcp_scsi_eh_host_reset_handler(struct scsi_cmnd *scpnt)
>  	struct zfcp_scsi_dev *zfcp_sdev = sdev_to_zfcp(scpnt->device);
>  	struct zfcp_adapter *adapter = zfcp_sdev->port->adapter;
>  	struct zfcp_port *port;
> -	int ret;
> +	int ret = SUCCESS;
>  
>  	zfcp_erp_adapter_reopen(adapter, 0, "schrh_1");
>  	zfcp_erp_wait(adapter);
> -	port = zfcp_sdev->port;
> -	ret = port->rport ? fc_block_rport(port->rport) : 0;
> -	if (ret)
> -		return ret;
> +	/* after internal recovery, wait for async unblock of rport(s) */
> +	read_lock(&adapter->port_list_lock);
> +	list_for_each_entry(port, &adapter->port_list, list) {
> +		int fc_ret;
> +
> +		if (!port->rport)
> +			continue;
> +
> +		fc_ret = fc_block_rport(port->rport);
> +		/* Any rport ran into fast_io_fail_tmo: FAST_IO_FAIL.
> +		 * To let pending requests bubble up, even if too many
> +		 * because of other rports without this timeout.
> +		 */
> +		if (fc_ret)
> +			ret = fc_ret;
> +	}
> +	read_unlock(&adapter->port_list_lock);
>  
> -	return SUCCESS;
> +	return ret;
>  }
>  
>  struct scsi_transport_template *zfcp_scsi_transport_template;
> 
:-)

Reviewed-by: Hannes Reinecke <hare@suse.com>

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)

[RFC 9/9] zfcp: decouple our scsi_eh callbacks from scsi_cmnd

[Steffen Maier]

zfcp_scsi_eh_device_reset_handler() now only depends on scsi_device.
zfcp_scsi_eh_target_reset_handler() now only depends on scsi_target.
zfcp_scsi_eh_host_reset_handler() now only depends on Scsi_Host.
All derive other objects from these intended callback arguments.

Actually change the signature of zfcp_task_mgmt_function() used by
zfcp_scsi_eh_device_reset_handler() & zfcp_scsi_eh_target_reset_handler().
Since it was prepared in a previous patch, we only need to delete
some local auto variables which are now the intended arguments.

Signed-off-by: Steffen Maier <maier@linux.vnet.ibm.com>
---
 drivers/s390/scsi/zfcp_scsi.c | 40 ++++++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
index 11cf33ea8c14..4cb38cfd46e3 100644
--- a/drivers/s390/scsi/zfcp_scsi.c
+++ b/drivers/s390/scsi/zfcp_scsi.c
@@ -266,11 +266,17 @@ static void zfcp_scsi_forget_cmnds(struct zfcp_port *port,
 	write_unlock_irqrestore(&adapter->abort_lock, flags);
 }
 
-static int zfcp_task_mgmt_function(struct scsi_cmnd *scpnt, u8 tm_flags)
+/**
+ * zfcp_task_mgmt_function() - Synchronously send a task management function.
+ * @port: Pointer to zfcp port indicating scope.
+ * @sdev: Pointer to SCSI device as scope, or %NULL if scope is only port.
+ * @tm_flags: Task management flags,
+ *            here we only handle %FCP_TMF_TGT_RESET or %FCP_TMF_LUN_RESET.
+ */
+static int zfcp_task_mgmt_function(struct zfcp_port *port,
+				   struct scsi_device *sdev, u8 tm_flags)
 {
-	struct scsi_device *sdev = scpnt->device;
-	struct zfcp_scsi_dev *zfcp_sdev = sdev_to_zfcp(sdev);
-	struct zfcp_port *port = zfcp_sdev->port;
+	struct zfcp_scsi_dev *zfcp_sdev = sdev ? sdev_to_zfcp(sdev) : NULL;
 	struct zfcp_adapter *adapter = port->adapter;
 	unsigned int scsi_id = port->starget_id;
 	u64 scsi_lun = ZFCP_DBF_INVALID_LUN;
@@ -325,18 +331,36 @@ static int zfcp_task_mgmt_function(struct scsi_cmnd *scpnt, u8 tm_flags)
 
 static int zfcp_scsi_eh_device_reset_handler(struct scsi_cmnd *scpnt)
 {
-	return zfcp_task_mgmt_function(scpnt, FCP_TMF_LUN_RESET);
+	struct scsi_device *sdev = scpnt->device;
+	struct zfcp_scsi_dev *zfcp_sdev = sdev_to_zfcp(sdev);
+	struct zfcp_port *port = zfcp_sdev->port;
+
+	return zfcp_task_mgmt_function(port, sdev, FCP_TMF_LUN_RESET);
 }
 
 static int zfcp_scsi_eh_target_reset_handler(struct scsi_cmnd *scpnt)
 {
-	return zfcp_task_mgmt_function(scpnt, FCP_TMF_TGT_RESET);
+	struct scsi_target *starget = scsi_target(scpnt->device);
+	struct fc_rport *rport = starget_to_rport(starget);
+	struct zfcp_adapter *adapter =
+		(struct zfcp_adapter *)rport_to_shost(rport)->hostdata[0];
+	struct zfcp_port *port = zfcp_get_port_by_wwpn(adapter,
+						       rport->port_name);
+	if (!port) {
+		zfcp_dbf_scsi_devreset("nopt", adapter, FCP_TMF_TGT_RESET,
+				       NULL, port->starget_id,
+				       ZFCP_DBF_INVALID_LUN);
+		return 0;
+	}
+
+	return zfcp_task_mgmt_function(port, NULL, FCP_TMF_TGT_RESET);
 }
 
 static int zfcp_scsi_eh_host_reset_handler(struct scsi_cmnd *scpnt)
 {
-	struct zfcp_scsi_dev *zfcp_sdev = sdev_to_zfcp(scpnt->device);
-	struct zfcp_adapter *adapter = zfcp_sdev->port->adapter;
+	struct Scsi_Host *shost = scpnt->device->host;
+	struct zfcp_adapter *adapter =
+		(struct zfcp_adapter *)shost->hostdata[0];
 	struct zfcp_port *port;
 	int ret = SUCCESS;
 
-- 
2.11.2

Re: [RFC 9/9] zfcp: decouple our scsi_eh callbacks from scsi_cmnd

[Hannes Reinecke]

On 07/25/2017 04:14 PM, Steffen Maier wrote:
> zfcp_scsi_eh_device_reset_handler() now only depends on scsi_device.
> zfcp_scsi_eh_target_reset_handler() now only depends on scsi_target.
> zfcp_scsi_eh_host_reset_handler() now only depends on Scsi_Host.
> All derive other objects from these intended callback arguments.
> 
> Actually change the signature of zfcp_task_mgmt_function() used by
> zfcp_scsi_eh_device_reset_handler() & zfcp_scsi_eh_target_reset_handler().
> Since it was prepared in a previous patch, we only need to delete
> some local auto variables which are now the intended arguments.
> 
> Signed-off-by: Steffen Maier <maier@linux.vnet.ibm.com>
> ---
>  drivers/s390/scsi/zfcp_scsi.c | 40 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 32 insertions(+), 8 deletions(-)
> 
Thank you for doing this.

Reviewed-by: Hannes Reinecke <hare@suse.com>

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)