Re: [PATCH 1/2] scsi: core: avoid leaving shost->last_reset with stale value if EH does not run

["Martin K. Petersen" <martin.petersen@oracle.com>]

Ewan,

No particular objections to the functional change, although I would
appreciate if Hannes would have a look.

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

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

-- 
Martin K. Petersen	Oracle Linux Engineering