| Message ID | 20220804034100.121125-9-michael.christie@oracle.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: Fix internal host code use | expand |
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>
On 8/9/22 3:13 PM, Bart Van Assche wrote: > > How about changing "SPACE_ALLOC" into "ENOSPC"? I have to resend for some other comment, so I'll do that as well.
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 */ +}; + /* * Scsi Error Handler Flags */
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(+)