Hello Hannes,

On Fri, Jan 27, 2023 at 04:34:59PM +0100, Hannes Reinecke wrote:

quoted

@@ -691,6 +708,15 @@ enum scsi_disposition scsi_check_sense(struct scsi_cmnd *scmd)
  		}
  		return SUCCESS;
+	case COMPLETED:
+		if (sshdr.asc == 0x55 && sshdr.ascq == 0x0a) {
+			set_scsi_ml_byte(scmd, SCSIML_STAT_DL_TIMEOUT);
+			req->cmd_flags |= REQ_FAILFAST_DEV;
+			req->rq_flags |= RQF_QUIET;
+			return SUCCESS;

You can kill this line, will be done anyway.

Thanks, we will fix in next revision.

quoted

+		}
+		return SUCCESS;
+
  	default:
  		return SUCCESS;
  	}

@@ -785,6 +811,14 @@ static enum scsi_disposition scsi_eh_completed_normally(struct scsi_cmnd *scmd)
  	switch (get_status_byte(scmd)) {
  	case SAM_STAT_GOOD:
  		scsi_handle_queue_ramp_up(scmd->device);
+		if (scmd->sense_buffer && SCSI_SENSE_VALID(scmd))
+			/*
+			 * If we have sense data, call scsi_check_sense() in
+			 * order to set the correct SCSI ML byte (if any).
+			 * No point in checking the return value, since the
+			 * command has already completed successfully.
+			 */
+			scsi_check_sense(scmd);

I am every so slightly nervous here.
We never checked the sense code for GOOD status, so heaven knows if there
are devices out there which return something here.

Well, right now we have had quite the opposite problem, to even allow
sense data while not being a CHECK_COMMAND, since they have been very
intertwined historically.

That alone makes me seriously doubt that this is going to be a problem.

But if there is a device that returns sense data when it shouldn't,
that is obviously a spec violation, and I assume that we would deal
with it in the same way we usually do, i.e. a device that does not
follow the spec has to be quirked. However, right now, I think that
it is sheer speculation that such a device even exists.

And you have checked that we've cleared the sense code before submitting (or
retrying, even), right?

Yes, scsi_queue_rq() does an unconditionall memset(cmd->sense_buffer, ...).

A retried command will call scsi_queue_insert() -> blk_mq_requeue_request(),
which will eventually reach blk_mq_dispatch_rq_list(), which will again lead
to scsi_queue_insert() getting called (which will memset the sense_buffer).

quoted

  		fallthrough;
  	case SAM_STAT_COMMAND_TERMINATED:
  		return SUCCESS;

@@ -1807,6 +1841,10 @@ bool scsi_noretry_cmd(struct scsi_cmnd *scmd)
  		return !!(req->cmd_flags & REQ_FAILFAST_DRIVER);
  	}
+	/* Never retry commands aborted due to a duration limit timeout */
+	if (scsi_ml_byte(scmd->result) == SCSIML_STAT_DL_TIMEOUT)
+		return true;
+
  	if (!scsi_status_is_check_condition(scmd->result))
  		return false;

@@ -1966,6 +2004,14 @@ enum scsi_disposition scsi_decide_disposition(struct scsi_cmnd *scmd)
  		if (scmd->cmnd[0] == REPORT_LUNS)
  			scmd->device->sdev_target->expecting_lun_change = 0;
  		scsi_handle_queue_ramp_up(scmd->device);
+		if (scmd->sense_buffer && SCSI_SENSE_VALID(scmd))
+			/*
+			 * If we have sense data, call scsi_check_sense() in
+			 * order to set the correct SCSI ML byte (if any).
+			 * No point in checking the return value, since the
+			 * command has already completed successfully.
+			 */
+			scsi_check_sense(scmd);
  		fallthrough;
  	case SAM_STAT_COMMAND_TERMINATED:
  		return SUCCESS;

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e1a021dd4da2..406952e72a68 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c

@@ -600,6 +600,8 @@ static blk_status_t scsi_result_to_blk_status(int result)
  		return BLK_STS_MEDIUM;
  	case SCSIML_STAT_TGT_FAILURE:
  		return BLK_STS_TARGET;
+	case SCSIML_STAT_DL_TIMEOUT:
+		return BLK_STS_DURATION_LIMIT;
  	}
  	switch (host_byte(result)) {

@@ -797,6 +799,8 @@ static void scsi_io_completion_action(struct scsi_cmnd *cmd, int result)
  				blk_stat = BLK_STS_ZONE_OPEN_RESOURCE;
  			}
  			break;
+		case COMPLETED:
+			fallthrough;
  		default:
  			action = ACTION_FAIL;
  			break;

diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 74324fba4281..f42388ecb024 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h

@@ -27,6 +27,7 @@ enum scsi_ml_status {
  	SCSIML_STAT_NOSPC		= 0x02,	/* Space allocation on the dev failed */
  	SCSIML_STAT_MED_ERROR		= 0x03,	/* Medium error */
  	SCSIML_STAT_TGT_FAILURE		= 0x04,	/* Permanent target failure */
+	SCSIML_STAT_DL_TIMEOUT		= 0x05, /* Command Duration Limit timeout */
  };
  static inline u8 scsi_ml_byte(int result)

Cheers,

Hannes