On Fri, 2017-06-23 at 15:02 +0200, Hannes Reinecke wrote:
> diff --git a/drivers/scsi/eata.c b/drivers/scsi/eata.c
> index 227dd2c..187bfaf 100644
> --- a/drivers/scsi/eata.c
> +++ b/drivers/scsi/eata.c
> @@ -507,7 +507,7 @@
> static int eata2x_release(struct Scsi_Host *);
> static int eata2x_queuecommand(struct Scsi_Host *, struct scsi_cmnd *);
> static int eata2x_eh_abort(struct scsi_cmnd *);
> -static int eata2x_eh_host_reset(struct scsi_cmnd *);
> +static int eata2x_eh_host_reset(struct Scsi_Host *);
> static int eata2x_bios_param(struct scsi_device *, struct block_device *,
> sector_t, int *);
> static int eata2x_slave_configure(struct scsi_device *);
> @@ -1896,21 +1896,16 @@ static int eata2x_eh_abort(struct scsi_cmnd *SCarg)
> panic("%s: abort, mbox %d, invalid cp_stat.\n", ha->board_name, i);
> }
>
> -static int eata2x_eh_host_reset(struct scsi_cmnd *SCarg)
> +static int eata2x_eh_host_reset(struct Scsi_Host *shost)
> {
> unsigned int i, time, k, c, limit = 0;
> - int arg_done = 0;
> struct scsi_cmnd *SCpnt;
> - struct Scsi_Host *shost = SCarg->device->host;
> struct hostdata *ha = (struct hostdata *)shost->hostdata;
>
> - scmd_printk(KERN_INFO, SCarg, "reset, enter.\n");
> + shost_printk(KERN_INFO, shost, "reset, enter.\n");
>
> spin_lock_irq(shost->host_lock);
>
> - if (SCarg->host_scribble == NULL)
> - printk("%s: reset, inactive.\n", ha->board_name);
> -
> if (ha->in_reset) {
> printk("%s: reset, exit, already in reset.\n", ha->board_name);
> spin_unlock_irq(shost->host_lock);
> @@ -1967,9 +1962,6 @@ static int eata2x_eh_host_reset(struct scsi_cmnd *SCarg)
> if (SCpnt->scsi_done == NULL)
> panic("%s: reset, mbox %d, SCpnt->scsi_done == NULL.\n",
> ha->board_name, i);
> -
> - if (SCpnt == SCarg)
> - arg_done = 1;
> }
>
> if (do_dma(shost->io_port, 0, RESET_PIO)) {
Please isolate the "arg_done" removal into a separate patch.
> @@ -865,23 +858,6 @@ static int __ips_eh_reset(struct scsi_cmnd *SC)
> if (!ha->active)
> return (FAILED);
>
> - /* See if the command is on the copp queue */
> - item = ha->copp_waitlist.head;
> - while ((item) && (item->scsi_cmd != SC))
> - item = item->next;
> -
> - if (item) {
> - /* Found it */
> - ips_removeq_copp(&ha->copp_waitlist, item);
> - return (SUCCESS);
> - }
> -
> - /* See if the command is on the wait queue */
> - if (ips_removeq_wait(&ha->scb_waitlist, SC)) {
> - /* command not sent yet */
> - return (SUCCESS);
> - }
> -
> /* An explanation for the casual observer:
> */
> /* Part of the function of a RAID controller is automatic error
> */
> /* detection and recovery. As such, the only problem that physically
> */
Shouldn't this change be isolated into a separate patch too?
> diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
> index 3c63c29..906570a 100644
> --- a/drivers/scsi/megaraid.c
> +++ b/drivers/scsi/megaraid.c
> @@ -1887,13 +1887,13 @@ static DEF_SCSI_QCMD(megaraid_queue)
>
>
> static int
> -megaraid_reset(struct scsi_cmnd *cmd)
> +megaraid_reset(struct Scsi_Host *shost)
> {
> adapter_t *adapter;
> megacmd_t mc;
> int rval;
>
> - adapter = (adapter_t *)cmd->device->host->hostdata;
> + adapter = (adapter_t *)shost->hostdata;
>
> #if MEGA_HAVE_CLUSTERING
> mc.cmd = MEGA_CLUSTER_CMD;
> @@ -1909,7 +1909,7 @@ static DEF_SCSI_QCMD(megaraid_queue)
>
> spin_lock_irq(&adapter->lock);
>
> - rval = megaraid_abort_and_reset(adapter, cmd, SCB_RESET);
> + rval = megaraid_abort_and_reset(adapter, NULL, SCB_RESET);
>
> /*
> * This is required here to complete any completed requests
> @@ -1948,7 +1948,7 @@ static DEF_SCSI_QCMD(megaraid_queue)
>
> scb = list_entry(pos, scb_t, list);
>
> - if (scb->cmd == cmd) { /* Found command */
> + if (!cmd || scb->cmd == cmd) { /* Found command */
>
> scb->state |= aor;
>
> @@ -1977,7 +1977,7 @@ static DEF_SCSI_QCMD(megaraid_queue)
> "%s-[%x], driver owner\n",
> (aor==SCB_ABORT) ? "ABORTING":"RESET",
> scb->idx);
> -
> + cmd = scb->cmd;
> mega_free_scb(adapter, scb);
This looks weird. Assigning scb->cmd to cmd will cause
megaraid_abort_and_reset()
to abort or reset the first command on pending_list if the cmd argument is NULL.
If the cmd argument is NULL, shouldn't all commands on pending_list be aborted
or
reset?
> -static int megasas_reset_bus_host(struct scsi_cmnd *scmd)
> +static int megasas_reset_bus_host(struct Scsi_Host *shost)
> {
> int ret;
> struct megasas_instance *instance;
>
> - instance = (struct megasas_instance *)scmd->device->host->hostdata;
> + instance = (struct megasas_instance *)shost->hostdata;
>
> - scmd_printk(KERN_INFO, scmd,
> + shost_printk(KERN_INFO, shost,
> "Controller reset is requested due to IO timeout\n"
> - "SCSI command pointer: (%p)\t SCSI host state: %d\t"
> + "SCSI host state: %d\t"
> " SCSI host busy: %d\t FW outstanding: %d\n",
> - scmd, scmd->device->host->shost_state,
> - atomic_read((atomic_t *)&scmd->device->host->host_busy),
> + shost->shost_state,
> + atomic_read((atomic_t *)&shost->host_busy),
> atomic_read(&instance->fw_outstanding));
Since host_busy already has type atomic_t and since you have to touch this
code anyway, how about leaving out the (atomic_t *) cast?
> --- a/drivers/scsi/qla1280.c
> +++ b/drivers/scsi/qla1280.c
> @@ -1047,13 +1047,28 @@ static void qla1280_mailbox_timeout(unsigned long
> __data)
> * Reset the specified adapter (both channels)
> **************************************************************************/
> static int
> -qla1280_eh_adapter_reset(struct scsi_cmnd *cmd)
> +qla1280_eh_adapter_reset(struct Scsi_Host *shost)
> {
> int rc;
> + struct scsi_qla_host *ha = (struct scsi_qla_host *)shost->hostdata;
>
> - spin_lock_irq(cmd->device->host->host_lock);
> - rc = qla1280_error_action(cmd, ADAPTER_RESET);
> - spin_unlock_irq(cmd->device->host->host_lock);
> + spin_lock_irq(shost->host_lock);
> + if (qla1280_verbose) {
> + printk(KERN_INFO
> + "scsi(%ld): Issued ADAPTER RESET\n",
> + ha->host_no);
> + printk(KERN_INFO "scsi(%ld): I/O processing will "
> + "continue automatically\n", ha->host_no);
> + }
> + ha->flags.reset_active = 1;
> +
> + if (qla1280_abort_isp(ha) != 0) { /* it's dead */
> + rc = FAILED;
> + }
> +
> + ha->flags.reset_active = 0;
> +
> + spin_unlock_irq(shost->host_lock);
>
> return rc;
> }
This change introduces some code duplication. Have you considered to isolate
the ADAPTER_RESET code from qla1280_error_action() into a new function in a
separate patch such that that new function can be called here instead of
duplicating the ADAPTER_RESET code?
Bart.
