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 
> ---
>  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 

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@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 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 
---
 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 = >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);
 }
 
 /**