On 10/3/22 10:52, Mike Christie wrote:
+static enum scsi_disposition scsi_check_passthrough(struct scsi_cmnd *scmd)
+{
+ struct scsi_failure *failure;
+ struct scsi_sense_hdr sshdr;
+ enum scsi_disposition ret;
+ u8 status;
How about using type enum sam_status instead of u8 for 'status'?
+ int i;
+
+
A single blank line to separate declarations and code please.
+ if (!scmd->result || !scmd->failures)
+ return SCSI_RETURN_NOT_HANDLED;
A comment that explains what the "not handled" return value indicates
would be welcome, e.g. above the scsi_check_passthrough() function.
+ if (((__get_status_byte(failure->result)) !=
+ SAM_STAT_CHECK_CONDITION) ||
+ (failure->sense == SCMD_FAILURE_SENSE_ANY))
+ goto maybe_retry;
Please do not use more parentheses than necessary. I think that 6
parentheses can be left out from the if-expression without affecting the
readability of the above code.
@@ -1608,6 +1608,7 @@ static blk_status_t scsi_prepare_cmd(struct request *req)
/* Usually overridden by the ULP */
cmd->allowed = 0;
+ cmd->failures = NULL;
memset(cmd->cmnd, 0, sizeof(cmd->cmnd));
return scsi_cmd_to_driver(cmd)->init_command(cmd);
}
Shouldn't the cmd->failures assignment be moved into scsi_initialize_rq()
rather than keeping it in scsi_prepare_cmd() such that the value set by
scsi_execute() is preserved?
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index bac55decf900..9ab97e48c5c2 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -65,6 +65,24 @@ enum scsi_cmnd_submitter {
SUBMITTED_BY_SCSI_RESET_IOCTL = 2,
} __packed;
+#define SCMD_FAILURE_NONE 0
+#define SCMD_FAILURE_ANY -1
This is the same value as -EPERM. Would something like 0x7fffffff be a better
choice for SCMD_FAILURE_ANY?
+struct scsi_failure {
+ int result;
+ u8 sense;
+ u8 asc;
+ u8 ascq;
+
+ s8 allowed;
+ s8 retries;
+};
Why has 'retries' type s8 instead of u8?
Thanks,
Bart.
