Re: [PATCH 14/47] scsi: Use Scsi_Host as argument for eh_host_reset_handler

Steffen Maier
2017-07-24T16:55:14Z

On 06/28/2017 10:32 AM, Hannes Reinecke wrote:
> Issuing a host reset should not rely on any commands.
> So use Scsi_Host as argument for eh_host_reset_handler.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
> Documentation/scsi/scsi_eh.txt | 2 +-
> Documentation/scsi/scsi_mid_low_api.txt | 4 +--

> drivers/s390/scsi/zfcp_scsi.c | 3 +-

> drivers/scsi/scsi_debug.c | 16 +++++-----
> drivers/scsi/scsi_error.c | 2 +-

> include/scsi/scsi_host.h | 2 +-
> 81 files changed, 290 insertions(+), 380 deletions(-)
>
> diff --git a/Documentation/scsi/scsi_eh.txt b/Documentation/scsi/scsi_eh.txt
> index 11e447b..78cd6b9 100644
> --- a/Documentation/scsi/scsi_eh.txt
> +++ b/Documentation/scsi/scsi_eh.txt
> @@ -208,7 +208,7 @@ considered to fail always.
> int (* eh_abort_handler)(struct scsi_cmnd *);
> int (* eh_device_reset_handler)(struct scsi_cmnd *);
> int (* eh_bus_reset_handler)(struct scsi_cmnd *);
> -int (* eh_host_reset_handler)(struct scsi_cmnd *);
> +int (* eh_host_reset_handler)(struct Scsi_Host *);
>
> Higher-severity actions are taken only when lower-severity actions
> cannot recover some of failed scmds. Also, note that failure of the
> diff --git a/Documentation/scsi/scsi_mid_low_api.txt b/Documentation/scsi/scsi_mid_low_api.txt
> index 6338400..ad517d5 100644
> --- a/Documentation/scsi/scsi_mid_low_api.txt
> +++ b/Documentation/scsi/scsi_mid_low_api.txt
> @@ -891,7 +891,7 @@ Details:
>
> /**
> * eh_host_reset_handler - reset host (host bus adapter)
> - * @scp: SCSI host that contains this device should be reset
> + * @shost: SCSI host that should be reset
> *
> * Returns SUCCESS if command aborted else FAILED
> *
> @@ -908,7 +908,7 @@ Details:
> *
> * Optionally defined in: LLD
> **/
> - int eh_host_reset_handler(struct scsi_cmnd * scp)
> + int eh_host_reset_handler(struct Scsi_Host * shost)
>
>
> /**

> diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
> index 3d18659..d7d4a63 100644
> --- a/drivers/s390/scsi/zfcp_scsi.c
> +++ b/drivers/s390/scsi/zfcp_scsi.c
> @@ -309,9 +309,8 @@ static int zfcp_scsi_eh_target_reset_handler(struct scsi_cmnd *scpnt)
> return zfcp_task_mgmt_function(scpnt, FCP_TMF_TGT_RESET);
> }
>
> -static int zfcp_scsi_eh_host_reset_handler(struct scsi_cmnd *scpnt)
> +static int zfcp_scsi_eh_host_reset_handler(struct Scsi_Host *host)
> {
> - struct Scsi_Host *host = scpnt->device->host;
> struct zfcp_adapter *adapter = (struct zfcp_adapter *)host->hostdata[0];
> int ret = 0;
> unsigned long flags;

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index dc095a2..ebf525b 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -3878,27 +3878,27 @@ static int scsi_debug_bus_reset(struct scsi_cmnd * SCpnt)
> return SUCCESS;
> }
>
> -static int scsi_debug_host_reset(struct scsi_cmnd * SCpnt)
> +static int scsi_debug_host_reset(struct Scsi_Host * shost)
> {
> struct sdebug_host_info * sdbg_host;
> struct sdebug_dev_info *devip;
> int k = 0;
>
> ++num_host_resets;
> - if ((SCpnt->device) && (SDEBUG_OPT_ALL_NOISE & sdebug_opts))
> - sdev_printk(KERN_INFO, SCpnt->device, "%s\n", __func__);
> - spin_lock(&sdebug_host_list_lock);
> - list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
> + if (SDEBUG_OPT_ALL_NOISE & sdebug_opts)
> + shost_printk(KERN_INFO, shost, "%s\n", __func__);
> + spin_lock(&sdebug_host_list_lock);
> + list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
> list_for_each_entry(devip, &sdbg_host->dev_info_list,
> dev_list) {
> set_bit(SDEBUG_UA_BUS_RESET, devip->uas_bm);
> ++k;
> }
> - }
> - spin_unlock(&sdebug_host_list_lock);
> + }
> + spin_unlock(&sdebug_host_list_lock);
> stop_all_queued();
> if (SDEBUG_OPT_RESET_NOISE & sdebug_opts)
> - sdev_printk(KERN_INFO, SCpnt->device,
> + shost_printk(KERN_INFO, shost,
> "%s: %d device(s) found\n", __func__, k);
> return SUCCESS;
> }

Mechanically, the change looks good.

However, is it correct that it's been looping over all its shosts and
doing something even for those that do not match SCpnt or now shost?
I thought it should only reset on the scope but it seems to call
stop_all_queued(void) which might reset all shosts?
But then again the loop seems about unit attentions so the loop may be
correct.
Anyway, this is not related to your change.

The other quoted parts incl. zfcp look good to me.

> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index ac31964..808167f 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -745,7 +745,7 @@ static int scsi_try_host_reset(struct scsi_cmnd *scmd)
> if (!hostt->eh_host_reset_handler)
> return FAILED;
>
> - rtn = hostt->eh_host_reset_handler(scmd);
> + rtn = hostt->eh_host_reset_handler(host);
>
> if (rtn == SUCCESS) {
> if (!hostt->skip_settle_delay)

> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index afb0481..b85f8a5 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -148,7 +148,7 @@ struct scsi_host_template {
> int (* eh_device_reset_handler)(struct scsi_cmnd *);
> int (* eh_target_reset_handler)(struct scsi_cmnd *);
> int (* eh_bus_reset_handler)(struct scsi_cmnd *);
> - int (* eh_host_reset_handler)(struct scsi_cmnd *);
> + int (* eh_host_reset_handler)(struct Scsi_Host *);
>
> /*
> * Before the mid layer attempts to scan for a new device where none
>

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