diff mbox series

[06/16] qedf: use fc rport as argument for qedf_initiate_tmf()

Message ID 20220524061602.86760-7-hare@suse.de (mailing list archive)
State Deferred
Headers show
Series scsi: EH rework prep patches, part 1 | expand

Commit Message

Hannes Reinecke May 24, 2022, 6:15 a.m. UTC 
When sending a TMF we're only concerned with the rport and the LUN ID,
so use struct fc_rport as argument for qedf_initiate_tmf().

Signed-off-by: Hannes Reinecke <hare@suse.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Cc: Saurav Kashyap <skashyap@marvell.com>
---
 drivers/scsi/qedf/qedf.h      |  3 +-
 drivers/scsi/qedf/qedf_io.c   | 69 ++++++++++-------------------------
 drivers/scsi/qedf/qedf_main.c | 19 +++++-----
 3 files changed, 31 insertions(+), 60 deletions(-)

Comments

Steffen Maier May 24, 2022, 11:16 a.m. UTC | #1 
On 5/24/22 08:15, Hannes Reinecke wrote:
> When sending a TMF we're only concerned with the rport and the LUN ID,
> so use struct fc_rport as argument for qedf_initiate_tmf().
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Cc: Saurav Kashyap <skashyap@marvell.com>
> ---
>   drivers/scsi/qedf/qedf.h      |  3 +-
>   drivers/scsi/qedf/qedf_io.c   | 69 ++++++++++-------------------------
>   drivers/scsi/qedf/qedf_main.c | 19 +++++-----
>   3 files changed, 31 insertions(+), 60 deletions(-)


> @@ -2432,33 +2425,10 @@ int qedf_initiate_tmf(struct scsi_cmnd *sc_cmd, u8 tm_flags)
>   		 (tm_flags == FCP_TMF_TGT_RESET) ? "TARGET RESET" :
>   		 "LUN RESET");
> 
> -	if (qedf_priv(sc_cmd)->io_req) {
> -		io_req = qedf_priv(sc_cmd)->io_req;
> -		ref_cnt = kref_read(&io_req->refcount);
> -		QEDF_ERR(NULL,
> -			 "orig io_req = %p xid = 0x%x ref_cnt = %d.\n",
> -			 io_req, io_req->xid, ref_cnt);
> -	}
> -
> -	rval = fc_remote_port_chkready(rport);
> -	if (rval) {
> -		QEDF_ERR(NULL, "device_reset rport not ready\n");
> -		rc = FAILED;
> -		goto tmf_err;
> -	}
> -
> -	rc = fc_block_scsi_eh(sc_cmd);
> +	rc = fc_block_rport(rport);


> diff --git a/drivers/scsi/qedf/qedf_main.c b/drivers/scsi/qedf/qedf_main.c
> index 18dc68d577b6..85ccfbc5cd26 100644
> --- a/drivers/scsi/qedf/qedf_main.c
> +++ b/drivers/scsi/qedf/qedf_main.c
> @@ -773,7 +773,7 @@ static int qedf_eh_abort(struct scsi_cmnd *sc_cmd)
>   		goto drop_rdata_kref;
>   	}
> 
> -	rc = fc_block_scsi_eh(sc_cmd);
> +	rc = fc_block_rport(rport);
>   	if (rc)
>   		goto drop_rdata_kref;
> 

Why replace the fc_block helper here in the abort handler?
Isn't the scope of the abort hander exactly the one scsi_cmnd?
The description of this patch is about changes to TMF (so I understand the 
change in qedf_initiate_tmf() above for device or target reset), i.e. not abort.
Admittedly, it shouldn't be a problem functional-wise, as fc_block_scsi_eh() 
delegates internally to fc_block_rport(), but it looks odd to me nonetheless.

This change seems inconsistent with the other patches in this series.
You don't plan to get rid of fc_block_scsi_eh(), do you?

Oh, later in patch 09 you write:
"Use fc_block_rport() instead of fc_block_scsi_eh() as the latter
will be deprecated."

Why?
Then all FCP HBA abort handlers need to convert their scsi_cmnd to fc_rport 
themselves.
E.g. zfcp_scsi_eh_abort_handler() does not deal with fc_rport so far as there 
was no need. From what I've seen you haven't touched that part in
[PATCH v2 03/24] zfcp: open-code fc_block_scsi_eh() for host reset
when zfcp was still part of the series, so I smell an inconsistency.

Haven't seen that coming. Did I miss this having been announced explicitly 
somewhere else earlier? If not, I would find it interesting to being made 
explicit. In retrospectice, I found some hints hidden in individual patches of 
earlier version in this series, but I had completely missed it.
Also, it seems a separate topic, not necessarily related to changing the 
scsi_eh callback API for dev/target/bus/host reset to get rid of their 
unsuitable argument scsi_cmnd.
Hannes Reinecke May 24, 2022, 11:26 a.m. UTC | #2 
On 5/24/22 13:16, Steffen Maier wrote:
> On 5/24/22 08:15, Hannes Reinecke wrote:
>> When sending a TMF we're only concerned with the rport and the LUN ID,
>> so use struct fc_rport as argument for qedf_initiate_tmf().
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>> Cc: Saurav Kashyap <skashyap@marvell.com>
>> ---
>>   drivers/scsi/qedf/qedf.h      |  3 +-
>>   drivers/scsi/qedf/qedf_io.c   | 69 ++++++++++-------------------------
>>   drivers/scsi/qedf/qedf_main.c | 19 +++++-----
>>   3 files changed, 31 insertions(+), 60 deletions(-)
> 
> 
>> @@ -2432,33 +2425,10 @@ int qedf_initiate_tmf(struct scsi_cmnd 
>> *sc_cmd, u8 tm_flags)
>>            (tm_flags == FCP_TMF_TGT_RESET) ? "TARGET RESET" :
>>            "LUN RESET");
>>
>> -    if (qedf_priv(sc_cmd)->io_req) {
>> -        io_req = qedf_priv(sc_cmd)->io_req;
>> -        ref_cnt = kref_read(&io_req->refcount);
>> -        QEDF_ERR(NULL,
>> -             "orig io_req = %p xid = 0x%x ref_cnt = %d.\n",
>> -             io_req, io_req->xid, ref_cnt);
>> -    }
>> -
>> -    rval = fc_remote_port_chkready(rport);
>> -    if (rval) {
>> -        QEDF_ERR(NULL, "device_reset rport not ready\n");
>> -        rc = FAILED;
>> -        goto tmf_err;
>> -    }
>> -
>> -    rc = fc_block_scsi_eh(sc_cmd);
>> +    rc = fc_block_rport(rport);
> 
> 
>> diff --git a/drivers/scsi/qedf/qedf_main.c 
>> b/drivers/scsi/qedf/qedf_main.c
>> index 18dc68d577b6..85ccfbc5cd26 100644
>> --- a/drivers/scsi/qedf/qedf_main.c
>> +++ b/drivers/scsi/qedf/qedf_main.c
>> @@ -773,7 +773,7 @@ static int qedf_eh_abort(struct scsi_cmnd *sc_cmd)
>>           goto drop_rdata_kref;
>>       }
>>
>> -    rc = fc_block_scsi_eh(sc_cmd);
>> +    rc = fc_block_rport(rport);
>>       if (rc)
>>           goto drop_rdata_kref;
>>
> 
> Why replace the fc_block helper here in the abort handler?
> Isn't the scope of the abort hander exactly the one scsi_cmnd?
> The description of this patch is about changes to TMF (so I understand 
> the change in qedf_initiate_tmf() above for device or target reset), 
> i.e. not abort.
> Admittedly, it shouldn't be a problem functional-wise, as 
> fc_block_scsi_eh() delegates internally to fc_block_rport(), but it 
> looks odd to me nonetheless.
> 
> This change seems inconsistent with the other patches in this series.
> You don't plan to get rid of fc_block_scsi_eh(), do you?
> 
> Oh, later in patch 09 you write:
> "Use fc_block_rport() instead of fc_block_scsi_eh() as the latter
> will be deprecated."
> 
> Why?

Yeah, sorry, that is misleading. I had an earlier patchset where I tried 
to phase it out, only to find out that this is not the best of ideas.

So no worries, everything will stay as-is in that area.

Will be modifying the patch (and the description) for the next round.

Cheers,

Hannes
diff mbox series

Patch

diff --git a/drivers/scsi/qedf/qedf.h b/drivers/scsi/qedf/qedf.h
index c5c0bbdafc4e..fcc887c6c40e 100644
--- a/drivers/scsi/qedf/qedf.h
+++ b/drivers/scsi/qedf/qedf.h
@@ -112,6 +112,7 @@  struct qedf_ioreq {
 #define QEDF_CMD_ERR_SCSI_DONE		0x5
 	u8 io_req_flags;
 	uint8_t tm_flags;
+	u64 tm_lun;
 	struct qedf_rport *fcport;
 #define	QEDF_CMD_ST_INACTIVE		0
 #define	QEDFC_CMD_ST_IO_ACTIVE		1
@@ -522,7 +523,7 @@  extern int qedf_initiate_cleanup(struct qedf_ioreq *io_req,
 	bool return_scsi_cmd_on_abts);
 extern void qedf_process_cleanup_compl(struct qedf_ctx *qedf,
 	struct fcoe_cqe *cqe, struct qedf_ioreq *io_req);
-extern int qedf_initiate_tmf(struct scsi_cmnd *sc_cmd, u8 tm_flags);
+extern int qedf_initiate_tmf(struct fc_rport *rport, u64 lun, u8 tm_flags);
 extern void qedf_process_tmf_compl(struct qedf_ctx *qedf, struct fcoe_cqe *cqe,
 	struct qedf_ioreq *io_req);
 extern void qedf_process_cqe(struct qedf_ctx *qedf, struct fcoe_cqe *cqe);
diff --git a/drivers/scsi/qedf/qedf_io.c b/drivers/scsi/qedf/qedf_io.c
index 2ec1f710fd1d..2248f660d227 100644
--- a/drivers/scsi/qedf/qedf_io.c
+++ b/drivers/scsi/qedf/qedf_io.c
@@ -546,7 +546,7 @@  static int qedf_build_bd_list_from_sg(struct qedf_ioreq *io_req)
 }
 
 static void qedf_build_fcp_cmnd(struct qedf_ioreq *io_req,
-				  struct fcp_cmnd *fcp_cmnd)
+				struct fcp_cmnd *fcp_cmnd)
 {
 	struct scsi_cmnd *sc_cmd = io_req->sc_cmd;
 
@@ -554,8 +554,12 @@  static void qedf_build_fcp_cmnd(struct qedf_ioreq *io_req,
 	memset(fcp_cmnd, 0, FCP_CMND_LEN);
 
 	/* 8 bytes: SCSI LUN info */
-	int_to_scsilun(sc_cmd->device->lun,
-			(struct scsi_lun *)&fcp_cmnd->fc_lun);
+	if (io_req->cmd_type == QEDF_TASK_MGMT_CMD)
+		int_to_scsilun(io_req->tm_lun,
+			       (struct scsi_lun *)&fcp_cmnd->fc_lun);
+	else
+		int_to_scsilun(sc_cmd->device->lun,
+			       (struct scsi_lun *)&fcp_cmnd->fc_lun);
 
 	/* 4 bytes: flag info */
 	fcp_cmnd->fc_pri_ta = 0;
@@ -1096,7 +1100,7 @@  static void qedf_parse_fcp_rsp(struct qedf_ioreq *io_req,
 	}
 
 	/* The sense buffer can be NULL for TMF commands */
-	if (sc_cmd->sense_buffer) {
+	if (sc_cmd && sc_cmd->sense_buffer) {
 		memset(sc_cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
 		if (fcp_sns_len)
 			memcpy(sc_cmd->sense_buffer, sense_data,
@@ -2282,7 +2286,7 @@  void qedf_process_cleanup_compl(struct qedf_ctx *qedf, struct fcoe_cqe *cqe,
 	complete(&io_req->cleanup_done);
 }
 
-static int qedf_execute_tmf(struct qedf_rport *fcport, struct scsi_cmnd *sc_cmd,
+static int qedf_execute_tmf(struct qedf_rport *fcport, u64 tm_lun,
 	uint8_t tm_flags)
 {
 	struct qedf_ioreq *io_req;
@@ -2292,17 +2296,10 @@  static int qedf_execute_tmf(struct qedf_rport *fcport, struct scsi_cmnd *sc_cmd,
 	int rc = 0;
 	uint16_t xid;
 	int tmo = 0;
-	int lun = 0;
 	unsigned long flags;
 	struct fcoe_wqe *sqe;
 	u16 sqe_idx;
 
-	if (!sc_cmd) {
-		QEDF_ERR(&qedf->dbg_ctx, "sc_cmd is NULL\n");
-		return FAILED;
-	}
-
-	lun = (int)sc_cmd->device->lun;
 	if (!test_bit(QEDF_RPORT_SESSION_READY, &fcport->flags)) {
 		QEDF_ERR(&(qedf->dbg_ctx), "fcport not offloaded\n");
 		rc = FAILED;
@@ -2322,7 +2319,7 @@  static int qedf_execute_tmf(struct qedf_rport *fcport, struct scsi_cmnd *sc_cmd,
 		qedf->target_resets++;
 
 	/* Initialize rest of io_req fields */
-	io_req->sc_cmd = sc_cmd;
+	io_req->sc_cmd = NULL;
 	io_req->fcport = fcport;
 	io_req->cmd_type = QEDF_TASK_MGMT_CMD;
 
@@ -2336,6 +2333,7 @@  static int qedf_execute_tmf(struct qedf_rport *fcport, struct scsi_cmnd *sc_cmd,
 
 	/* Default is to return a SCSI command when an error occurs */
 	io_req->return_scsi_cmd_on_abts = false;
+	io_req->tm_lun = tm_lun;
 
 	/* Obtain exchange id */
 	xid = io_req->xid;
@@ -2390,7 +2388,7 @@  static int qedf_execute_tmf(struct qedf_rport *fcport, struct scsi_cmnd *sc_cmd,
 
 
 	if (tm_flags == FCP_TMF_LUN_RESET)
-		qedf_flush_active_ios(fcport, lun);
+		qedf_flush_active_ios(fcport, tm_lun);
 	else
 		qedf_flush_active_ios(fcport, -1);
 
@@ -2405,23 +2403,18 @@  static int qedf_execute_tmf(struct qedf_rport *fcport, struct scsi_cmnd *sc_cmd,
 	return rc;
 }
 
-int qedf_initiate_tmf(struct scsi_cmnd *sc_cmd, u8 tm_flags)
+int qedf_initiate_tmf(struct fc_rport *rport, u64 lun, u8 tm_flags)
 {
-	struct fc_rport *rport = starget_to_rport(scsi_target(sc_cmd->device));
 	struct fc_rport_libfc_priv *rp = rport->dd_data;
 	struct qedf_rport *fcport = (struct qedf_rport *)&rp[1];
-	struct qedf_ctx *qedf;
-	struct fc_lport *lport = shost_priv(sc_cmd->device->host);
+	struct qedf_ctx *qedf = fcport->qedf;
+	struct fc_lport *lport = rp->local_port;
 	int rc = SUCCESS;
-	int rval;
-	struct qedf_ioreq *io_req = NULL;
-	int ref_cnt = 0;
 	struct fc_rport_priv *rdata = fcport->rdata;
 
 	QEDF_ERR(NULL,
-		 "tm_flags 0x%x sc_cmd %p op = 0x%02x target_id = 0x%x lun=%d\n",
-		 tm_flags, sc_cmd, sc_cmd->cmd_len ? sc_cmd->cmnd[0] : 0xff,
-		 rport->scsi_target_id, (int)sc_cmd->device->lun);
+		 "tm_flags 0x%x target_id = 0x%x lun=%llu\n",
+		 tm_flags, rport->scsi_target_id, lun);
 
 	if (!rdata || !kref_get_unless_zero(&rdata->kref)) {
 		QEDF_ERR(NULL, "stale rport\n");
@@ -2432,33 +2425,10 @@  int qedf_initiate_tmf(struct scsi_cmnd *sc_cmd, u8 tm_flags)
 		 (tm_flags == FCP_TMF_TGT_RESET) ? "TARGET RESET" :
 		 "LUN RESET");
 
-	if (qedf_priv(sc_cmd)->io_req) {
-		io_req = qedf_priv(sc_cmd)->io_req;
-		ref_cnt = kref_read(&io_req->refcount);
-		QEDF_ERR(NULL,
-			 "orig io_req = %p xid = 0x%x ref_cnt = %d.\n",
-			 io_req, io_req->xid, ref_cnt);
-	}
-
-	rval = fc_remote_port_chkready(rport);
-	if (rval) {
-		QEDF_ERR(NULL, "device_reset rport not ready\n");
-		rc = FAILED;
-		goto tmf_err;
-	}
-
-	rc = fc_block_scsi_eh(sc_cmd);
+	rc = fc_block_rport(rport);
 	if (rc)
 		goto tmf_err;
 
-	if (!fcport) {
-		QEDF_ERR(NULL, "device_reset: rport is NULL\n");
-		rc = FAILED;
-		goto tmf_err;
-	}
-
-	qedf = fcport->qedf;
-
 	if (!qedf) {
 		QEDF_ERR(NULL, "qedf is NULL.\n");
 		rc = FAILED;
@@ -2495,7 +2465,7 @@  int qedf_initiate_tmf(struct scsi_cmnd *sc_cmd, u8 tm_flags)
 		goto tmf_err;
 	}
 
-	rc = qedf_execute_tmf(fcport, sc_cmd, tm_flags);
+	rc = qedf_execute_tmf(fcport, lun, tm_flags);
 
 tmf_err:
 	kref_put(&rdata->kref, fc_rport_destroy);
@@ -2512,7 +2482,6 @@  void qedf_process_tmf_compl(struct qedf_ctx *qedf, struct fcoe_cqe *cqe,
 	fcp_rsp = &cqe->cqe_info.rsp_info;
 	qedf_parse_fcp_rsp(io_req, fcp_rsp);
 
-	io_req->sc_cmd = NULL;
 	complete(&io_req->tm_done);
 }
 
diff --git a/drivers/scsi/qedf/qedf_main.c b/drivers/scsi/qedf/qedf_main.c
index 18dc68d577b6..85ccfbc5cd26 100644
--- a/drivers/scsi/qedf/qedf_main.c
+++ b/drivers/scsi/qedf/qedf_main.c
@@ -773,7 +773,7 @@  static int qedf_eh_abort(struct scsi_cmnd *sc_cmd)
 		goto drop_rdata_kref;
 	}
 
-	rc = fc_block_scsi_eh(sc_cmd);
+	rc = fc_block_rport(rport);
 	if (rc)
 		goto drop_rdata_kref;
 
@@ -857,18 +857,19 @@  static int qedf_eh_abort(struct scsi_cmnd *sc_cmd)
 
 static int qedf_eh_target_reset(struct scsi_cmnd *sc_cmd)
 {
-	QEDF_ERR(NULL, "%d:0:%d:%lld: TARGET RESET Issued...",
-		 sc_cmd->device->host->host_no, sc_cmd->device->id,
-		 sc_cmd->device->lun);
-	return qedf_initiate_tmf(sc_cmd, FCP_TMF_TGT_RESET);
+	struct scsi_target *starget = scsi_target(sc_cmd->device);
+	struct fc_rport *rport = starget_to_rport(starget);
+
+	QEDF_ERR(NULL, "TARGET RESET Issued...");
+	return qedf_initiate_tmf(rport, 0, FCP_TMF_TGT_RESET);
 }
 
 static int qedf_eh_device_reset(struct scsi_cmnd *sc_cmd)
 {
-	QEDF_ERR(NULL, "%d:0:%d:%lld: LUN RESET Issued... ",
-		 sc_cmd->device->host->host_no, sc_cmd->device->id,
-		 sc_cmd->device->lun);
-	return qedf_initiate_tmf(sc_cmd, FCP_TMF_LUN_RESET);
+	struct fc_rport *rport = starget_to_rport(scsi_target(sc_cmd->device));
+
+	QEDF_ERR(NULL, "LUN RESET Issued...\n");
+	return qedf_initiate_tmf(rport, sc_cmd->device->lun, FCP_TMF_LUN_RESET);
 }
 
 bool qedf_wait_for_upload(struct qedf_ctx *qedf)