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