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

[Steffen Maier <maier@linux.ibm.com>]

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.

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