Re: [PATCH 24/47] zfcp: use scsi device as argument for zfcp_task_mgmt_function()

Steffen Maier
2017-07-24T17:15:22Z

On 06/28/2017 10:32 AM, Hannes Reinecke wrote:
> zfcp_task_mgmt_function() is only used for lun and device reset,
> so it should be using the scsi device as an argument, not the
> scsi command.
>
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
> drivers/s390/scsi/zfcp_ext.h | 2 +-
> drivers/s390/scsi/zfcp_fc.h | 9 +--------
> drivers/s390/scsi/zfcp_fsf.c | 21 +++++++++++----------
> drivers/s390/scsi/zfcp_scsi.c | 12 ++++++------
> 4 files changed, 19 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/s390/scsi/zfcp_ext.h b/drivers/s390/scsi/zfcp_ext.h
> index 57f1d4a..64d4db7 100644
> --- a/drivers/s390/scsi/zfcp_ext.h
> +++ b/drivers/s390/scsi/zfcp_ext.h
> @@ -117,7 +117,7 @@ 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 scsi_device *, u8);
> 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_fc.h b/drivers/s390/scsi/zfcp_fc.h
> index df2b541..f08eaf9 100644
> --- a/drivers/s390/scsi/zfcp_fc.h
> +++ b/drivers/s390/scsi/zfcp_fc.h
> @@ -206,19 +206,12 @@ 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)
> {
> 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)

When I wrote my zfcp decoupling patch series I did a lot of git history
research in order to double check if my changes are OK.

Here, I figured that I'd like to separately revert 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. Even though it was one of the first commits I
posted upstream as newbie zfcp maintainer... little did I know.

> diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
> index 27ff38f..c4bd3d4 100644
> --- a/drivers/s390/scsi/zfcp_fsf.c
> +++ b/drivers/s390/scsi/zfcp_fsf.c
> @@ -2036,10 +2036,9 @@ static void zfcp_fsf_req_trace(struct zfcp_fsf_req *req, struct scsi_cmnd *scsi)

> -static void zfcp_fsf_fcp_handler_common(struct zfcp_fsf_req *req)
> +static void zfcp_fsf_fcp_handler_common(struct zfcp_fsf_req *req,
> + struct scsi_device *sdev)

> @@ -2120,7 +2119,7 @@ static void zfcp_fsf_fcp_cmnd_handler(struct zfcp_fsf_req *req)

> - zfcp_fsf_fcp_handler_common(req);
> + zfcp_fsf_fcp_handler_common(req, scpnt->device);

> @@ -2296,8 +2295,9 @@ static void zfcp_fsf_fcp_task_mgmt_handler(struct zfcp_fsf_req *req)
> {

> + struct scsi_device *sdev = req->data;
>
> - zfcp_fsf_fcp_handler_common(req);
> + zfcp_fsf_fcp_handler_common(req, sdev);

Moving the resolving of req->data into the callers of
zfcp_fsf_fcp_handler_common() I did the same way in my own patch series.

> @@ -2313,12 +2313,12 @@ static void zfcp_fsf_fcp_task_mgmt_handler(struct zfcp_fsf_req *req)

> -struct zfcp_fsf_req *zfcp_fsf_fcp_task_mgmt(struct scsi_cmnd *scmnd,
> +struct zfcp_fsf_req *zfcp_fsf_fcp_task_mgmt(struct scsi_device *sdev,
> u8 tm_flags)

> @@ -2338,7 +2338,7 @@ struct zfcp_fsf_req *zfcp_fsf_fcp_task_mgmt(struct scsi_cmnd *scmnd,

> - req->data = scmnd;
> + req->data = sdev;

> diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
> index 5fada93..dd7bea0 100644
> --- a/drivers/s390/scsi/zfcp_scsi.c
> +++ b/drivers/s390/scsi/zfcp_scsi.c
> @@ -259,17 +259,17 @@ 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)
> +static int zfcp_task_mgmt_function(struct scsi_device *sdev, u8 tm_flags)
> {
> - struct zfcp_scsi_dev *zfcp_sdev = sdev_to_zfcp(scpnt->device);
> + struct zfcp_scsi_dev *zfcp_sdev = sdev_to_zfcp(sdev);
> struct zfcp_adapter *adapter = zfcp_sdev->port->adapter;
> - struct fc_port *rport = zfcp_sdev->port->rport;

This smells very odd: fc_port instead of fc_*r*port.
Looks like this was introduced buggy in "[PATCH 15/47]
scsi_transport_fc: Use fc_rport as argument for fc_block_scsi_eh" and
should be avoided for bisectability.

> + struct fc_rport *rport = zfcp_sdev->port->rport;

> - fsf_req = zfcp_fsf_fcp_task_mgmt(scpnt, tm_flags);
> + fsf_req = zfcp_fsf_fcp_task_mgmt(sdev, tm_flags);

> static int zfcp_scsi_eh_device_reset_handler(struct scsi_cmnd *scpnt)
> {
> - return zfcp_task_mgmt_function(scpnt, FCP_TMF_LUN_RESET);
> + return zfcp_task_mgmt_function(scpnt->device, 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);
> + return zfcp_task_mgmt_function(scpnt->device, FCP_TMF_TGT_RESET);
> }

Won't the target_reset_handler eventually have scsi_target as argument
and thus we will not have any particular scsi_device to pass anymore
later in the patch set?

In my patch series, I replaced scsi_cmnd with a mandatory zfcp_port and
an optional(!) scsi_device for the TMF call chains in zfcp.
I thought scsi_device must be optional because it only exists for LUN
reset but not for target reset.
Does this make sense?

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