On Wed, 2022-09-28 at 21:53 -0500, Mike Christie wrote:
> For passthrough, we don't retry any error we get a check condition
> for.
> This results in a lot of callers driving their own retries for those
> types
> of errors and retrying all errors, and there has been a request to
> retry
> specific host byte errors.
>
> This adds the core code to allow passthrough users to specify what
> errors
> they want scsi-ml to retry for them. We can then convert users to
> drop
> their sense parsing and retry handling.
>
> Signed-off-by: Mike Christie <michael.christie@xxxxxxxxxx>
I like the general approach. A few remarks below.
> ---
> drivers/scsi/scsi_error.c | 63
> +++++++++++++++++++++++++++++++++++++++
> drivers/scsi/scsi_lib.c | 1 +
> include/scsi/scsi_cmnd.h | 26 +++++++++++++++-
> 3 files changed, 89 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 3f630798d1eb..4bf7b65bc63d 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -1831,6 +1831,63 @@ bool scsi_noretry_cmd(struct scsi_cmnd *scmd)
> return false;
> }
>
> +static enum scsi_disposition scsi_check_passthrough(struct scsi_cmnd
> *scmd)
> +{
> + struct scsi_failure *failure;
> + struct scsi_sense_hdr sshdr;
> + enum scsi_disposition ret;
> + int i = 0;
> +
> + if (!scmd->result || !scmd->failures)
> + return SCSI_RETURN_NOT_HANDLED;
> +
> + while (1) {
> + failure = &scmd->failures[i++];
Style nit: a for loop would express the logic better, IMO.
> + if (!failure->result)
> + break;
> +
> + if (failure->result == SCMD_FAILURE_ANY)
> + goto maybe_retry;
> +
> + if (host_byte(scmd->result) & host_byte(failure-
> >result)) {
> + goto maybe_retry;
Using "&" here needs explanation. The host byte is not a bit mask.
> + } else if (get_status_byte(scmd) &
> + __get_status_byte(failure->result)) {
See above.
> + if (get_status_byte(scmd) !=
> SAM_STAT_CHECK_CONDITION ||
> + failure->sense == SCMD_FAILURE_SENSE_ANY)
> + goto maybe_retry;
> +
> + ret = scsi_start_sense_processing(scmd,
> &sshdr);
> + if (ret == NEEDS_RETRY)
> + goto maybe_retry;
> + else if (ret != SUCCESS)
> + return ret;
> +
> + if (failure->sense != sshdr.sense_key)
> + continue;
> +
> + if (failure->asc == SCMD_FAILURE_ASC_ANY)
> + goto maybe_retry;
> +
> + if (failure->asc != sshdr.asc)
> + continue;
> +
> + if (failure->ascq == SCMD_FAILURE_ASCQ_ANY ||
> + failure->ascq == sshdr.ascq)
> + goto maybe_retry;
> + }
> + }
> +
> + return SCSI_RETURN_NOT_HANDLED;
> +
> +maybe_retry:
> + if (failure->allowed == SCMD_FAILURE_NO_LIMIT ||
> + ++failure->retries <= failure->allowed)
> + return NEEDS_RETRY;
> +
> + return SUCCESS;
> +}
> +
> /**
> * scsi_decide_disposition - Disposition a cmd on return from LLD.
> * @scmd: SCSI cmd to examine.
> @@ -1859,6 +1916,12 @@ enum scsi_disposition
> scsi_decide_disposition(struct scsi_cmnd *scmd)
> return SUCCESS;
> }
>
> + if (blk_rq_is_passthrough(scsi_cmd_to_rq(scmd))) {
> + rtn = scsi_check_passthrough(scmd);
> + if (rtn != SCSI_RETURN_NOT_HANDLED)
> + return rtn;
> + }
> +
> /*
> * first check the host byte, to see if there is anything in
> there
> * that would indicate what we need to do.
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 497efc0da259..56aefe38d69b 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -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);
> }
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index bac55decf900..9fb85932d5b9 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -65,6 +65,23 @@ enum scsi_cmnd_submitter {
> SUBMITTED_BY_SCSI_RESET_IOCTL = 2,
> } __packed;
>
> +#define SCMD_FAILURE_NONE 0
> +#define SCMD_FAILURE_ANY -1
> +#define SCMD_FAILURE_SENSE_ANY 0xff
> +#define SCMD_FAILURE_ASC_ANY 0xff
> +#define SCMD_FAILURE_ASCQ_ANY 0xff
> +#define SCMD_FAILURE_NO_LIMIT -1
> +
> +struct scsi_failure {
> + int result;
> + u8 sense;
> + u8 asc;
> + u8 ascq;
> +
> + s8 allowed;
> + s8 retries;
> +};
> +
> struct scsi_cmnd {
> struct scsi_device *device;
> struct list_head eh_entry; /* entry for the host
> eh_abort_list/eh_cmd_q */
> @@ -85,6 +102,8 @@ struct scsi_cmnd {
>
> int retries;
> int allowed;
> + /* optional array of failures that passthrough users want
> retried */
> + struct scsi_failure *failures;
>
> unsigned char prot_op;
> unsigned char prot_type;
> @@ -330,9 +349,14 @@ static inline void set_status_byte(struct
> scsi_cmnd *cmd, char status)
> cmd->result = (cmd->result & 0xffffff00) | status;
> }
>
> +static inline u8 __get_status_byte(int result)
> +{
> + return result & 0xff;
> +}
> +
> static inline u8 get_status_byte(struct scsi_cmnd *cmd)
> {
> - return cmd->result & 0xff;
> + return __get_status_byte(cmd->result);
> }
>
> static inline void set_host_byte(struct scsi_cmnd *cmd, char status)
