Re: [PATCH 08/10] scsi: Add error codes for internal scsi-ml use.

[Bart Van Assche <bvanassche@acm.org>]

On 8/3/22 20:40, Mike Christie wrote:
> If a driver returns:
> 
> DID_TARGET_FAILURE
> DID_NEXUS_FAILURE
> DID_ALLOC_FAILURE
> DID_MEDIUM_ERROR
> 
> we hit a couple bugs:
> 
> 1. The SCSI error handler runs because scsi_decide_disposition has no
> case statements for them and we return FAILED.
> 
> 2. For SG IO the userspace app gets a success status instead of failed,
> because scsi_result_to_blk_status clears those errors.
> 
> This patch adds a new internal error code byte for use by scsi-ml. It
> will be used instead of the above error codes, so we don't have to play
> that clearing the host code game in scsi_result_to_blk_status and
> drivers cannot accidentally use them.
> 
> The next patch will then remove the internal users of the above codes and
> convert us to use the new ones.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>   drivers/scsi/scsi_error.c |  5 +++++
>   drivers/scsi/scsi_lib.c   | 22 ++++++++++++++++++++++
>   drivers/scsi/scsi_priv.h  | 11 +++++++++++
>   3 files changed, 38 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 947d98a0565f..4adadd3fb410 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -514,6 +514,11 @@ static void scsi_report_sense(struct scsi_device *sdev,
>   	}
>   }
>   
> +static inline void set_scsi_ml_byte(struct scsi_cmnd *cmd, u8 status)
> +{
> +	cmd->result = (cmd->result & 0xffff00ff) | (status << 8);
> +}
> +
>   /**
>    * scsi_check_sense - Examine scsi cmd sense
>    * @scmd:	Cmd to have sense checked.
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 2aca0a838ca5..eaf4865a2cb6 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -576,6 +576,11 @@ static bool scsi_end_request(struct request *req, blk_status_t error,
>   	return false;
>   }
>   
> +static inline u8 get_scsi_ml_byte(int result)
> +{
> +	return (result >> 8) & 0xff;
> +}
> +
>   /**
>    * scsi_result_to_blk_status - translate a SCSI result code into blk_status_t
>    * @cmd:	SCSI command
> @@ -586,6 +591,23 @@ static bool scsi_end_request(struct request *req, blk_status_t error,
>    */
>   static blk_status_t scsi_result_to_blk_status(struct scsi_cmnd *cmd, int result)
>   {
> +	/*
> +	 * Check the scsi-ml byte first in case we converted a host or status
> +	 * byte.
> +	 */
> +	switch (get_scsi_ml_byte(result)) {
> +	case SCSIML_STAT_OK:
> +		break;
> +	case SCSIML_STAT_RESV_CONFLICT:
> +		return BLK_STS_NEXUS;
> +	case SCSIML_STAT_SPACE_ALLOC:
> +		return BLK_STS_NOSPC;
> +	case SCSIML_STAT_MED_ERROR:
> +		return BLK_STS_MEDIUM;
> +	case SCSIML_STAT_TGT_FAILURE:
> +		return BLK_STS_TARGET;
> +	}
> +
>   	switch (host_byte(result)) {
>   	case DID_OK:
>   		if (scsi_status_is_good(result))
> diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
> index 5c4786310a31..9d2d32bf0171 100644
> --- a/drivers/scsi/scsi_priv.h
> +++ b/drivers/scsi/scsi_priv.h
> @@ -18,6 +18,17 @@ struct scsi_nl_hdr;
>   
>   #define SCSI_CMD_RETRIES_NO_LIMIT -1
>   
> +/*
> + * Error codes used by scsi-ml internally. These must not be used by drivers.
> + */
> +enum scsi_ml_status {
> +	SCSIML_STAT_OK			= 0x00,
> +	SCSIML_STAT_RESV_CONFLICT	= 0x01,	/* Reservation conflict */
> +	SCSIML_STAT_SPACE_ALLOC		= 0x02,	/* Space allocation on the dev failed */
> +	SCSIML_STAT_MED_ERROR		= 0x03,	/* Medium error */
> +	SCSIML_STAT_TGT_FAILURE		= 0x04,	/* Permanent target failure */
> +};

How about changing "SPACE_ALLOC" into "ENOSPC"?

Anyway:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>