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@xxxxxxxx>
Reviewed-by: Christoph Hellwig <hch@xxxxxx>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@xxxxxxx>
Cc: Saurav Kashyap <skashyap@xxxxxxxxxxx>
---
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.
--
Mit freundlichen Gruessen / Kind regards
Steffen Maier
Linux on IBM Z and LinuxONE
https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschaeftsfuehrung: David Faller
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
