Hello Hannes,
On Mon, Oct 23, 2023 at 11:28:28AM +0200, Hannes Reinecke wrote:
> diff --git a/Documentation/scsi/scsi_mid_low_api.rst b/Documentation/scsi/scsi_mid_low_api.rst
> index 022198c51350..96983bb1cc45 100644
> --- a/Documentation/scsi/scsi_mid_low_api.rst
> +++ b/Documentation/scsi/scsi_mid_low_api.rst
> @@ -777,9 +777,9 @@ Details::
>
> /**
> * eh_host_reset_handler - reset host (host bus adapter)
> - * @scp: SCSI host that contains this device should be reset
> + * @shp: SCSI host that should be reset
> *
> - * Returns SUCCESS if command aborted else FAILED
> + * Returns SUCCESS if host has been reset else FAILED
This should also mention FAST_IO_FAIL now that we touch this documentation
anyway. Same for the other callbacks that you change later in the patchset.
> *
> * Locks: None held
> *
> diff --git a/drivers/message/fusion/mptscsih.c b/drivers/message/fusion/mptscsih.c
> index 9080a73b4ea6..4f9729cf4098 100644
> --- a/drivers/message/fusion/mptscsih.c
> +++ b/drivers/message/fusion/mptscsih.c
> @@ -1955,34 +1955,27 @@ mptscsih_bus_reset(struct scsi_cmnd * SCpnt)
>
> /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
> /**
> - * mptscsih_host_reset - Perform a SCSI host adapter RESET (new_eh variant)
> - * @SCpnt: Pointer to scsi_cmnd structure, IO which reset is due to
> + * mptscsih_host_reset - Perform a SCSI host adapter RESET
> + * @sh: Pointer to Scsi_Host structure, which is reset due to
> *
> * (linux scsi_host_template.eh_host_reset_handler routine)
> *
> * Returns SUCCESS or FAILED.
> */
> int
> -mptscsih_host_reset(struct scsi_cmnd *SCpnt)
> +mptscsih_host_reset(struct Scsi_Host *sh)
> {
> - MPT_SCSI_HOST * hd;
> + MPT_SCSI_HOST * hd = shost_priv(sh);
> int status = SUCCESS;
> MPT_ADAPTER *ioc;
> int retval;
>
> - /* If we can't locate the host to reset, then we failed. */
> - if ((hd = shost_priv(SCpnt->device->host)) == NULL){
> - printk(KERN_ERR MYNAM ": host reset: "
> - "Can't locate host! (sc=%p)\n", SCpnt);
> - return FAILED;
> - }
> -
> /* make sure we have no outstanding commands at this stage */
> mptscsih_flush_running_cmds(hd);
>
> ioc = hd->ioc;
> - printk(MYIOC_s_INFO_FMT "attempting host reset! (sc=%p)\n",
> - ioc->name, SCpnt);
> + printk(MYIOC_s_INFO_FMT "attempting host reset!\n",
> + ioc->name);
Nitpick: you could remove the line-break here now.
>
> /* If our attempts to reset the host failed, then return a failed
> * status. The host will be taken off line by the SCSI mid-layer.
> @@ -1993,8 +1986,8 @@ mptscsih_host_reset(struct scsi_cmnd *SCpnt)
> else
> status = SUCCESS;
>
> - printk(MYIOC_s_INFO_FMT "host reset: %s (sc=%p)\n",
> - ioc->name, ((retval == 0) ? "SUCCESS" : "FAILED" ), SCpnt);
> + printk(MYIOC_s_INFO_FMT "host reset: %s\n",
> + ioc->name, (retval == 0 ? "SUCCESS" : "FAILED" ));
^
Nitpick: IFF you remove the line-break above, maybe also remove the Plenk
here.
>
> return status;
> }
> diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
> index f925f8664c2c..6dbffb1bc293 100644
> --- a/drivers/scsi/3w-9xxx.c
> +++ b/drivers/scsi/3w-9xxx.c
> @@ -1717,18 +1717,15 @@ static int twa_scsi_biosparam(struct scsi_device *sdev, struct block_device *bde
> } /* End twa_scsi_biosparam() */
>
> /* This is the new scsi eh reset function */
> -static int twa_scsi_eh_reset(struct scsi_cmnd *SCpnt)
> +static int twa_scsi_eh_reset(struct Scsi_Host *shost)
> {
> - TW_Device_Extension *tw_dev = NULL;
> + TW_Device_Extension *tw_dev = shost_priv(shost);
> int retval = FAILED;
>
> - tw_dev = (TW_Device_Extension *)SCpnt->device->host->hostdata;
> -
> tw_dev->num_resets++;
>
> - sdev_printk(KERN_WARNING, SCpnt->device,
> - "WARNING: (0x%02X:0x%04X): Command (0x%x) timed out, resetting card.\n",
> - TW_DRIVER, 0x2c, SCpnt->cmnd[0]);
> + shost_printk(KERN_WARNING, shost,
> + "WARNING: Command timed out, resetting card.");
Why remove the end-of-line `\n`? IIRC `printk()` generally doesn't append one
automatically.
>
> /* Make sure we are not issuing an ioctl or resetting from ioctl */
> mutex_lock(&tw_dev->ioctl_lock);
> diff --git a/drivers/scsi/3w-sas.c b/drivers/scsi/3w-sas.c
> index 55989eaa2d9f..c18a07591505 100644
> --- a/drivers/scsi/3w-sas.c
> +++ b/drivers/scsi/3w-sas.c
> @@ -1423,18 +1423,15 @@ static int twl_scsi_biosparam(struct scsi_device *sdev, struct block_device *bde
> } /* End twl_scsi_biosparam() */
>
> /* This is the new scsi eh reset function */
> -static int twl_scsi_eh_reset(struct scsi_cmnd *SCpnt)
> +static int twl_scsi_eh_reset(struct Scsi_Host *shost)
> {
> - TW_Device_Extension *tw_dev = NULL;
> + TW_Device_Extension *tw_dev = shost_priv(shost);
> int retval = FAILED;
>
> - tw_dev = (TW_Device_Extension *)SCpnt->device->host->hostdata;
> -
> tw_dev->num_resets++;
>
> - sdev_printk(KERN_WARNING, SCpnt->device,
> - "WARNING: (0x%02X:0x%04X): Command (0x%x) timed out, resetting card.\n",
> - TW_DRIVER, 0x2c, SCpnt->cmnd[0]);
> + shost_printk(KERN_WARNING, shost,
> + "WARNING: Command timed out, resetting card.");
...
>
> /* Make sure we are not issuing an ioctl or resetting from ioctl */
> mutex_lock(&tw_dev->ioctl_lock);
> diff --git a/drivers/scsi/3w-xxxx.c b/drivers/scsi/3w-xxxx.c
> index f39c9ec2e781..190597e6b3d2 100644
> --- a/drivers/scsi/3w-xxxx.c
> +++ b/drivers/scsi/3w-xxxx.c
> @@ -1366,25 +1366,22 @@ static int tw_scsi_biosparam(struct scsi_device *sdev, struct block_device *bdev
> } /* End tw_scsi_biosparam() */
>
> /* This is the new scsi eh reset function */
> -static int tw_scsi_eh_reset(struct scsi_cmnd *SCpnt)
> +static int tw_scsi_eh_reset(struct Scsi_Host *shost)
> {
> - TW_Device_Extension *tw_dev=NULL;
> + TW_Device_Extension *tw_dev = shost_priv(shost);
> int retval = FAILED;
>
> - tw_dev = (TW_Device_Extension *)SCpnt->device->host->hostdata;
> -
> tw_dev->num_resets++;
>
> - sdev_printk(KERN_WARNING, SCpnt->device,
> - "WARNING: Command (0x%x) timed out, resetting card.\n",
> - SCpnt->cmnd[0]);
> + shost_printk(KERN_WARNING, shost,
> + "WARNING: Command timed out, resetting card.");
...
>
> /* Make sure we are not issuing an ioctl or resetting from ioctl */
> mutex_lock(&tw_dev->ioctl_lock);
>
> /* Now reset the card and some of the device extension data */
> if (tw_reset_device_extension(tw_dev)) {
> - printk(KERN_WARNING "3w-xxxx: scsi%d: Reset failed.\n", tw_dev->host->host_no);
> + shost_printk(KERN_WARNING, shost, "Reset failed.\n");
> goto out;
> }
>
> diff --git a/drivers/scsi/arm/fas216.c b/drivers/scsi/arm/fas216.c
> index e6289c6af5ef..efa6f2527428 100644
> --- a/drivers/scsi/arm/fas216.c
> +++ b/drivers/scsi/arm/fas216.c
> @@ -2649,16 +2649,16 @@ static void fas216_init_chip(FAS216_Info *info)
> }
>
> /**
> - * fas216_eh_host_reset - Reset the host associated with this command
> - * @SCpnt: command specifing host to reset
> + * fas216_eh_host_reset - Reset the host
Nitpick:
I think kdoc style wants the function to have `()` appended (now that you
change that line anyway).
* fas216_eh_host_reset() - Reset the host.
> + * @shost: host to reset
> *
> - * Reset the host associated with this command.
> + * Reset the specified host.
> * Returns: FAILED if unable to reset.
> * Notes: io_request_lock is taken, and irqs are disabled
> */
> -int fas216_eh_host_reset(struct scsi_cmnd *SCpnt)
> +int fas216_eh_host_reset(struct Scsi_Host *shost)
> {
> - FAS216_Info *info = (FAS216_Info *)SCpnt->device->host->hostdata;
> + FAS216_Info *info = shost_priv(shost);
>
> spin_lock_irq(info->host->host_lock);
>
> diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c
> index 10cf5775a939..1faf4566b884 100644
> --- a/drivers/scsi/ips.c
> +++ b/drivers/scsi/ips.c
> @@ -829,11 +829,11 @@ int ips_eh_abort(struct scsi_cmnd *SC)
> /* NOTE: this routine is called under the io_request_lock spinlock */
> /* */
> /****************************************************************************/
> -static int __ips_eh_reset(struct scsi_cmnd *SC)
> +static int __ips_eh_reset(struct Scsi_Host *shost)
> {
> int ret;
> int i;
> - ips_ha_t *ha;
> + ips_ha_t *ha = shost_priv(shost);
> ips_scb_t *scb;
>
> METHOD_TRACE("ips_eh_reset", 1);
> @@ -842,20 +842,6 @@ static int __ips_eh_reset(struct scsi_cmnd *SC)
> return (FAILED);
> #else
>
> - if (!SC) {
> - DEBUG(1, "Reset called with NULL scsi command");
> -
> - return (FAILED);
> - }
> -
> - ha = (ips_ha_t *) SC->device->host->hostdata;
> -
> - if (!ha) {
I wonder whether we really know `ha` is always set here? At least from the
changeset it doesn't appear obvious to me. We get `ha` via the provided
`shost` and `shost_priv()`, but that doesn't necessarily mean the pointer is
not NULL.
> - DEBUG(1, "Reset called with NULL ha struct");
> -
> - return (FAILED);
> - }
> -
> if (!ha->active)
> return (FAILED);
>
> diff --git a/drivers/scsi/megaraid.h b/drivers/scsi/megaraid.h
> index 013fbfb911b9..43acad67d95f 100644
> --- a/drivers/scsi/megaraid.h
> +++ b/drivers/scsi/megaraid.h
> @@ -2502,8 +2502,8 @@ megaraid_abort_handler(struct scsi_cmnd *scp)
> }
>
> /**
> - * megaraid_reset_handler - device reset handler for mailbox based driver
...
> - * @scp : reference command
> + * megaraid_reset_handler - host reset handler for mailbox based driver
> + * @shost : host to reset
> *
> * Reset handler for the mailbox based controller. First try to find out if
> * the FW is still live, in which case the outstanding commands counter mut go
> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
> index 3d4f13da1ae8..cdd56144c841 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -2890,21 +2890,18 @@ static int megasas_wait_for_outstanding(struct megasas_instance *instance)
>
> /**
> * megasas_generic_reset - Generic reset routine
> - * @scmd: Mid-layer SCSI command
> + * @shost: Mid-layer SCSI host
> *
> - * This routine implements a generic reset handler for device, bus and host
> - * reset requests. Device, bus and host specific reset handlers can use this
> + * This routine implements a generic reset handler for host
> + * reset requests. Host specific reset handlers can use this
> * function after they do their specific tasks.
Nitpick: this comment really does sound sorta strange now, especially since
this function is only called from one place.
> */
> -static int megasas_generic_reset(struct scsi_cmnd *scmd)
> +static int megasas_generic_reset(struct Scsi_Host *shost)
> {
> int ret_val;
> - struct megasas_instance *instance;
> -
> - instance = (struct megasas_instance *)scmd->device->host->hostdata;
> + struct megasas_instance *instance = shost_priv(shost);
>
> - scmd_printk(KERN_NOTICE, scmd, "megasas: RESET cmd=%x retries=%x\n",
> - scmd->cmnd[0], scmd->retries);
> + shost_printk(KERN_NOTICE, shost, "megasas: RESET\n");
>
> if (atomic_read(&instance->adprecovery) == MEGASAS_HW_CRITICAL_ERROR) {
> dev_err(&instance->pdev->dev, "cannot recover from previous reset failures\n");
> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 040031eb0c12..d52412870b54 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -4028,9 +4028,9 @@ static int mpi3mr_eh_host_reset(struct scsi_cmnd *scmd)
>
> retval = SUCCESS;
> out:
> - sdev_printk(KERN_INFO, scmd->device,
> - "Host reset is %s for scmd(%p)\n",
> - ((retval == SUCCESS) ? "SUCCESS" : "FAILED"), scmd);
> + shost_printk(KERN_INFO, shost,
> + "Host reset is %s\n",
> + ((retval == SUCCESS) ? "SUCCESS" : "FAILED"));
Nitpick: superfluous parentheses.
>
> return retval;
> }
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index 7e103d711825..94a4bd5d2841 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -1731,8 +1725,8 @@ qla2xxx_eh_host_reset(struct scsi_cmnd *cmd)
>
> eh_host_reset_lock:
> ql_log(ql_log_info, vha, 0x8017,
> - "ADAPTER RESET %s nexus=%ld:%d:%llu.\n",
> - (ret == FAILED) ? "FAILED" : "SUCCEEDED", vha->host_no, id, lun);
> + "ADAPTER RESET %s host=%ld.\n",
> + (ret == FAILED) ? "FAILED" : "SUCCEEDED", vha->host_no);
...
>
> return ret;
> }
> diff --git a/drivers/scsi/snic/snic_scsi.c b/drivers/scsi/snic/snic_scsi.c
> index c38f648da3d7..36298dbadb14 100644
> --- a/drivers/scsi/snic/snic_scsi.c
> +++ b/drivers/scsi/snic/snic_scsi.c
> @@ -2306,34 +2313,6 @@ snic_reset(struct Scsi_Host *shost)
> ret = SUCCESS;
>
> reset_end:
> - return ret;
> -} /* end of snic_reset */
> -
> -/*
> - * SCSI Error handling calls driver's eh_host_reset if all prior
> - * error handling levels return FAILED.
> - *
> - * Host Reset is the highest level of error recovery. If this fails, then
> - * host is offlined by SCSI.
> - */
> -int
> -snic_host_reset(struct scsi_cmnd *sc)
> -{
> - struct Scsi_Host *shost = sc->device->host;
> - u32 start_time = jiffies;
> - int ret;
> -
> - SNIC_SCSI_DBG(shost,
> - "host reset:sc %p sc_cmd 0x%x req %p tag %d flags 0x%llx\n",
> - sc, sc->cmnd[0], scsi_cmd_to_rq(sc),
> - snic_cmd_tag(sc), CMD_FLAGS(sc));
> -
> - ret = snic_reset(shost);
> -
> - SNIC_TRC(shost->host_no, snic_cmd_tag(sc), (ulong) sc,
> - jiffies_to_msecs(jiffies - start_time),
> - 0, SNIC_TRC_CMD(sc), SNIC_TRC_CMD_STATE_FLAGS(sc));
Is it ok to loose those twp debug/logging statements? They seem to have some
information about timing.
> -
> return ret;
> } /* end of snic_host_reset */
>
--
Best Regards, Benjamin Block / Linux on IBM Z Kernel Development
IBM Deutschland Research & Development GmbH / https://www.ibm.com/privacy
Vors. Aufs.-R.: Gregor Pillen / Geschäftsführung: David Faller
Sitz der Ges.: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294
