On Wed, 2021-10-27 at 23:29 -0400, Martin K. Petersen wrote:
> Ewan,
>
> No particular objections to the functional change, although I would
> appreciate if Hannes would have a look.
Absolutely.
>
> > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> > index b6c86cc..9f4001e 100644
> > --- a/drivers/scsi/scsi_error.c
> > +++ b/drivers/scsi/scsi_error.c
> > @@ -151,9 +151,11 @@ scmd_eh_abort_handler(struct work_struct *work)
> > struct scsi_cmnd *scmd =
> > container_of(work, struct scsi_cmnd, abort_work.work);
> > struct scsi_device *sdev = scmd->device;
> > + struct Scsi_Host *shost = sdev->host;
> > enum scsi_disposition rtn;
> > + unsigned long flags;
> >
> > - if (scsi_host_eh_past_deadline(sdev->host)) {
> > + if (scsi_host_eh_past_deadline(shost)) {
> > SCSI_LOG_ERROR_RECOVERY(3,
> > scmd_printk(KERN_INFO, scmd,
> > "eh timeout, not aborting\n"));
>
> Changing sdev->host to shost makes this patch harder to review and
> creates unnecessary churn for a stable fix. Maybe postpone that
> particular cleanup?
Sure. I added shost because the later code I added would have used
sdev->shost a lot, seemed like I should have changed all uses.
But I can it the other way, it will end up being the same after the
subsequent patch.
>
> > @@ -175,12 +177,42 @@ scmd_eh_abort_handler(struct work_struct *work)
> > SCSI_LOG_ERROR_RECOVERY(3,
> > scmd_printk(KERN_WARNING, scmd,
> > "retry aborted command\n"));
> > +
> > + spin_lock_irqsave(shost->host_lock, flags);
> > + list_del_init(&scmd->eh_entry);
> > +
> > + /*
> > + * If the abort succeeds, and there is no further
> > + * EH action, clear the ->last_reset time.
> > + */
> > + if (list_empty(&shost->eh_abort_list) &&
> > + list_empty(&shost->eh_cmd_q))
> > + if (shost->eh_deadline != -1)
> > + shost->last_reset = 0;
> > +
> > + spin_unlock_irqrestore(shost->host_lock, flags);
> > +
>
> Perhaps you could introduce a scsi_host_clear_last_reset() helper to
> avoid duplicating this code?
I removed the duplication in the subsequent patch, the reason it is duplicated
here is to keep the control flow the same and avoid taking the ->host_lock twice.
-Ewan
>
