[PATCH 00/10] scsi: Fix internal host code use

[PATCH 00/10] scsi: Fix internal host code use

[Mike Christie]

The following patches made over Martin's 5.20 staging branch fix an issue
where we probably intended the host codes:

DID_TARGET_FAILURE
DID_NEXUS_FAILURE
DID_ALLOC_FAILURE
DID_MEDIUM_ERROR

to be internal to scsi-ml, but at some point drivers started using them
and the driver writers never updated scsi-ml.

The problem with drivers using them to tell scsi-ml there was an error
is:

1. scsi_result_to_blk_status clears those codes, so they are not
propagated upwards. SG IO/passthrough users will then not see an error
and think a command was successful.

2. The SCSI error handler runs because scsi_decide_disposition has no
case statements for them and we return FAILED.

This patchset converts the drivers to stop using these codes, and then
moves them to scsi_priv.h in a new error byte so they can only be used
by scsi-ml.

[PATCH 01/10] scsi: xen: Drop use of internal host codes.

[Mike Christie]

The error codes:

DID_TARGET_FAILURE
DID_NEXUS_FAILURE
DID_ALLOC_FAILURE
DID_MEDIUM_ERROR

are internal to the SCSI layer. Drivers must not use them because:

1. They are not propagated upwards, so SG IO/passthrough users will not
see an error and think a command was successful.

xen-scsiback will never see this error and should not try to send it.

2. There is no handling for them in scsi_decide_disposition so if
xen-scsifront were to return the error to scsi-ml then it kicks off the
error handler which is definitely not what we want.

This patch remove the use from xen-scsifront/back.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/xen-scsifront.c       |  8 --------
 drivers/xen/xen-scsiback.c         | 12 ------------
 include/xen/interface/io/vscsiif.h | 10 +---------
 3 files changed, 1 insertion(+), 29 deletions(-)

diff --git a/drivers/scsi/xen-scsifront.c b/drivers/scsi/xen-scsifront.c
index 51afc66e839d..66b316d173b0 100644
--- a/drivers/scsi/xen-scsifront.c
+++ b/drivers/scsi/xen-scsifront.c
@@ -289,14 +289,6 @@ static unsigned int scsifront_host_byte(int32_t rslt)
 		return DID_TRANSPORT_DISRUPTED;
 	case XEN_VSCSIIF_RSLT_HOST_TRANSPORT_FAILFAST:
 		return DID_TRANSPORT_FAILFAST;
-	case XEN_VSCSIIF_RSLT_HOST_TARGET_FAILURE:
-		return DID_TARGET_FAILURE;
-	case XEN_VSCSIIF_RSLT_HOST_NEXUS_FAILURE:
-		return DID_NEXUS_FAILURE;
-	case XEN_VSCSIIF_RSLT_HOST_ALLOC_FAILURE:
-		return DID_ALLOC_FAILURE;
-	case XEN_VSCSIIF_RSLT_HOST_MEDIUM_ERROR:
-		return DID_MEDIUM_ERROR;
 	case XEN_VSCSIIF_RSLT_HOST_TRANSPORT_MARGINAL:
 		return DID_TRANSPORT_MARGINAL;
 	default:
diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index 7a0c93acc2c5..e98c88a960d8 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -333,18 +333,6 @@ static int32_t scsiback_result(int32_t result)
 	case DID_TRANSPORT_FAILFAST:
 		host_status = XEN_VSCSIIF_RSLT_HOST_TRANSPORT_FAILFAST;
 		break;
-	case DID_TARGET_FAILURE:
-		host_status = XEN_VSCSIIF_RSLT_HOST_TARGET_FAILURE;
-		break;
-	case DID_NEXUS_FAILURE:
-		host_status = XEN_VSCSIIF_RSLT_HOST_NEXUS_FAILURE;
-		break;
-	case DID_ALLOC_FAILURE:
-		host_status = XEN_VSCSIIF_RSLT_HOST_ALLOC_FAILURE;
-		break;
-	case DID_MEDIUM_ERROR:
-		host_status = XEN_VSCSIIF_RSLT_HOST_MEDIUM_ERROR;
-		break;
 	case DID_TRANSPORT_MARGINAL:
 		host_status = XEN_VSCSIIF_RSLT_HOST_TRANSPORT_MARGINAL;
 		break;
diff --git a/include/xen/interface/io/vscsiif.h b/include/xen/interface/io/vscsiif.h
index 7ea4dc9611c4..44eb1f34f1a0 100644
--- a/include/xen/interface/io/vscsiif.h
+++ b/include/xen/interface/io/vscsiif.h
@@ -316,16 +316,8 @@ struct vscsiif_response {
 #define XEN_VSCSIIF_RSLT_HOST_TRANSPORT_DISRUPTED 14
 /* Transport class fastfailed */
 #define XEN_VSCSIIF_RSLT_HOST_TRANSPORT_FAILFAST  15
-/* Permanent target failure */
-#define XEN_VSCSIIF_RSLT_HOST_TARGET_FAILURE      16
-/* Permanent nexus failure on path */
-#define XEN_VSCSIIF_RSLT_HOST_NEXUS_FAILURE       17
-/* Space allocation on device failed */
-#define XEN_VSCSIIF_RSLT_HOST_ALLOC_FAILURE       18
-/* Medium error */
-#define XEN_VSCSIIF_RSLT_HOST_MEDIUM_ERROR        19
 /* Transport marginal errors */
-#define XEN_VSCSIIF_RSLT_HOST_TRANSPORT_MARGINAL  20
+#define XEN_VSCSIIF_RSLT_HOST_TRANSPORT_MARGINAL  16
 
 /* Result values of reset operations */
 #define XEN_VSCSIIF_RSLT_RESET_SUCCESS  0x2002
-- 
2.25.1

[PATCH 02/10] scsi: storvsc: Drop DID_TARGET_FAILURE use.

[Mike Christie]

DID_TARGET_FAILURE is internal to the SCSI layer. Drivers must not use it
because:

1. It's not propagated upwards, so SG IO/passthrough users will not see an
error and think a command was successful.

2. There is no handling for them in scsi_decide_disposition so it results
in the scsi eh running.

It looks like the driver wanted a hard failure so this swaps it with
DID_BAD_TARGET.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/storvsc_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index fe000da11332..25c44c87c972 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1029,7 +1029,7 @@ static void storvsc_handle_error(struct vmscsi_request *vm_srb,
 	 */
 	wrk = kmalloc(sizeof(struct storvsc_scan_work), GFP_ATOMIC);
 	if (!wrk) {
-		set_host_byte(scmnd, DID_TARGET_FAILURE);
+		set_host_byte(scmnd, DID_BAD_TARGET);
 		return;
 	}
 
-- 
2.25.1

[PATCH 03/10] scsi: uas: Drop DID_TARGET_FAILURE use.

[Mike Christie]

DID_TARGET_FAILURE is internal to the SCSI layer. Drivers must not use it
because:

1. It's not propagated upwards, so SG IO/passthrough users will not see an
error and think a command was successful.

2. There is no handling for them in scsi_decide_disposition so it results
in the scsi eh running.

It looks like the driver wanted a hard failure so this swaps it with
DID_BAD_TARGET which gives us that behavior and the error looks like it's
for a case where the target did not support a TMF we wanted to use (maybe
not a bad target but disappointing so close enough).

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/usb/storage/uas.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 84dc270f6f73..de3836412bf3 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -283,7 +283,7 @@ static bool uas_evaluate_response_iu(struct response_iu *riu, struct scsi_cmnd *
 		set_host_byte(cmnd, DID_OK);
 		break;
 	case RC_TMF_NOT_SUPPORTED:
-		set_host_byte(cmnd, DID_TARGET_FAILURE);
+		set_host_byte(cmnd, DID_BAD_TARGET);
 		break;
 	default:
 		uas_log_cmd_state(cmnd, "response iu", response_code);
-- 
2.25.1

[PATCH 04/10] scsi: virtio_scsi: Drop DID_TARGET_FAILURE use.

[Mike Christie]

DID_TARGET_FAILURE is internal to the SCSI layer. Drivers must not use it
because:

1. It's not propagated upwards, so SG IO/passthrough users will not see an
error and think a command was successful.

2. There is no handling for them in scsi_decide_disposition so it results
in the scsi eh running.

It looks like virtio_scsi gets this when something like qemu returns
VIRTIO_SCSI_S_TARGET_FAILURE. It looks like qemu returns that error code
if a host OS returns it, but this shouldn't happen for linux since we
never propagate that error to userspace.

This has us use DID_BAD_TARGET in case some other virt layer is returning
it. In that case we will still get a hard error like before and it conveys
something unexpected happened.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/virtio_scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 578c4b6d0f7d..112d8c3962b0 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -141,7 +141,7 @@ static void virtscsi_complete_cmd(struct virtio_scsi *vscsi, void *buf)
 		set_host_byte(sc, DID_TRANSPORT_DISRUPTED);
 		break;
 	case VIRTIO_SCSI_S_TARGET_FAILURE:
-		set_host_byte(sc, DID_TARGET_FAILURE);
+		set_host_byte(sc, DID_BAD_TARGET);
 		break;
 	case VIRTIO_SCSI_S_NEXUS_FAILURE:
 		set_host_byte(sc, DID_NEXUS_FAILURE);
-- 
2.25.1

[PATCH 05/10] scsi: virtio_scsi: Drop DID_NEXUS_FAILURE use.

[Mike Christie]

DID_NEXUS_FAILURE is internal to the SCSI layer. Drivers must not use it
because:

1. It's not propagated upwards, so SG IO/passthrough users will not see an
error and think a command was successful.

2. There is no handling for them in scsi_decide_disposition so it results
in the scsi eh running.

It looks like virtio_scsi gets this when something like qemu returns
VIRTIO_SCSI_S_NEXUS_FAILURE. It looks like qemu returns that error code
if host OS returns DID_NEXUS_FAILURE (qemu's internal
SCSI_HOST_RESERVATION_ERROR maps to DID_NEXUS_FAILURE). This shouldn't
happen for linux since we don't propagate that error code to userspace.

This has us convert VIRTIO_SCSI_S_NEXUS_FAILURE to a
SAM_STAT_RESERVATION_CONFLICT in case some other virt layer is returning
it. In that case we will still get the reservation confict failure we
expect.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/virtio_scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 112d8c3962b0..00cf6743db8c 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -144,7 +144,7 @@ static void virtscsi_complete_cmd(struct virtio_scsi *vscsi, void *buf)
 		set_host_byte(sc, DID_BAD_TARGET);
 		break;
 	case VIRTIO_SCSI_S_NEXUS_FAILURE:
-		set_host_byte(sc, DID_NEXUS_FAILURE);
+		set_status_byte(sc, SAM_STAT_RESERVATION_CONFLICT);
 		break;
 	default:
 		scmd_printk(KERN_WARNING, sc, "Unknown response %d",
-- 
2.25.1

[PATCH 06/10] scsi: qla2xxx: Drop DID_TARGET_FAILURE use.

[Mike Christie]

DID_TARGET_FAILURE is internal to the SCSI layer. Drivers must not use it
because:

1. It's not propagated upwards, so SG IO/passthrough users will not see an
error and think a command was successful.

2. There is no handling for them in scsi_decide_disposition so it results
in the scsi eh running.

This has qla2xxx use DID_NO_CONNECT because it looks like we hit this
error when we can't find a port. It will give us the same hard error
behavior and it seems to match the error where we can't find the
endpoint.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/qla2xxx/qla_edif.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/qla_edif.c b/drivers/scsi/qla2xxx/qla_edif.c
index 400a8b6f3982..00ccc41cef14 100644
--- a/drivers/scsi/qla2xxx/qla_edif.c
+++ b/drivers/scsi/qla2xxx/qla_edif.c
@@ -1551,7 +1551,7 @@ qla24xx_sadb_update(struct bsg_job *bsg_job)
 		ql_dbg(ql_dbg_edif, vha, 0x70a3, "Failed to find port= %06x\n",
 		    sa_frame.port_id.b24);
 		rval = -EINVAL;
-		SET_DID_STATUS(bsg_reply->result, DID_TARGET_FAILURE);
+		SET_DID_STATUS(bsg_reply->result, DID_NO_CONNECT);
 		goto done;
 	}
 
-- 
2.25.1

[PATCH 07/10] scsi: cxlflash: Drop DID_ALLOC_FAILURE use.

[Mike Christie]

DID_ALLOC_FAILURE is internal to the SCSI layer. Drivers must not use it
because:

1. It's not propagated upwards, so SG IO/passthrough users will not see an
error and think a command was successful.

2. There is no handling for them in scsi_decide_disposition so it results
in the scsi eh running.

By the code comment, it looks like the driver wanted a retryable error
code, so this has it use DID_ERROR.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/cxlflash/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c
index e7be95ee7d64..cd1324ec742d 100644
--- a/drivers/scsi/cxlflash/main.c
+++ b/drivers/scsi/cxlflash/main.c
@@ -132,7 +132,7 @@ static void process_cmd_err(struct afu_cmd *cmd, struct scsi_cmnd *scp)
 			break;
 		case SISL_AFU_RC_OUT_OF_DATA_BUFS:
 			/* Retry */
-			scp->result = (DID_ALLOC_FAILURE << 16);
+			scp->result = (DID_ERROR << 16);
 			break;
 		default:
 			scp->result = (DID_ERROR << 16);
-- 
2.25.1

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

[Mike Christie]

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 */
+};
+
 /*
  * Scsi Error Handler Flags
  */
-- 
2.25.1

[PATCH 09/10] scsi: Convert scsi_decide_disposition to use SCSIML_STAT

[Mike Christie]

Don't use:

DID_TARGET_FAILURE
DID_NEXUS_FAILURE
DID_ALLOC_FAILURE
DID_MEDIUM_ERROR

Instead use the scsi-ml internal values.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/scsi/scsi_error.c | 12 ++++++------
 drivers/scsi/scsi_lib.c   | 24 +++++-------------------
 2 files changed, 11 insertions(+), 25 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 4adadd3fb410..dd6a31dce3eb 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -649,7 +649,7 @@ enum scsi_disposition scsi_check_sense(struct scsi_cmnd *scmd)
 	case DATA_PROTECT:
 		if (sshdr.asc == 0x27 && sshdr.ascq == 0x07) {
 			/* Thin provisioning hard threshold reached */
-			set_host_byte(scmd, DID_ALLOC_FAILURE);
+			set_scsi_ml_byte(scmd, SCSIML_STAT_SPACE_ALLOC);
 			return SUCCESS;
 		}
 		fallthrough;
@@ -657,14 +657,14 @@ enum scsi_disposition scsi_check_sense(struct scsi_cmnd *scmd)
 	case VOLUME_OVERFLOW:
 	case MISCOMPARE:
 	case BLANK_CHECK:
-		set_host_byte(scmd, DID_TARGET_FAILURE);
+		set_scsi_ml_byte(scmd, SCSIML_STAT_TGT_FAILURE);
 		return SUCCESS;
 
 	case MEDIUM_ERROR:
 		if (sshdr.asc == 0x11 || /* UNRECOVERED READ ERR */
 		    sshdr.asc == 0x13 || /* AMNF DATA FIELD */
 		    sshdr.asc == 0x14) { /* RECORD NOT FOUND */
-			set_host_byte(scmd, DID_MEDIUM_ERROR);
+			set_scsi_ml_byte(scmd, SCSIML_STAT_MED_ERROR);
 			return SUCCESS;
 		}
 		return NEEDS_RETRY;
@@ -673,7 +673,7 @@ enum scsi_disposition scsi_check_sense(struct scsi_cmnd *scmd)
 		if (scmd->device->retry_hwerror)
 			return ADD_TO_MLQUEUE;
 		else
-			set_host_byte(scmd, DID_TARGET_FAILURE);
+			set_scsi_ml_byte(scmd, SCSIML_STAT_TGT_FAILURE);
 		fallthrough;
 
 	case ILLEGAL_REQUEST:
@@ -683,7 +683,7 @@ enum scsi_disposition scsi_check_sense(struct scsi_cmnd *scmd)
 		    sshdr.asc == 0x24 || /* Invalid field in cdb */
 		    sshdr.asc == 0x26 || /* Parameter value invalid */
 		    sshdr.asc == 0x27) { /* Write protected */
-			set_host_byte(scmd, DID_TARGET_FAILURE);
+			set_scsi_ml_byte(scmd, SCSIML_STAT_TGT_FAILURE);
 		}
 		return SUCCESS;
 
@@ -1988,7 +1988,7 @@ enum scsi_disposition scsi_decide_disposition(struct scsi_cmnd *scmd)
 	case SAM_STAT_RESERVATION_CONFLICT:
 		sdev_printk(KERN_INFO, scmd->device,
 			    "reservation conflict\n");
-		set_host_byte(scmd, DID_NEXUS_FAILURE);
+		set_scsi_ml_byte(scmd, SCSIML_STAT_RESV_CONFLICT);
 		return SUCCESS; /* causes immediate i/o error */
 	}
 	return FAILED;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index eaf4865a2cb6..92a643ff64e4 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -583,13 +583,11 @@ static inline u8 get_scsi_ml_byte(int result)
 
 /**
  * scsi_result_to_blk_status - translate a SCSI result code into blk_status_t
- * @cmd:	SCSI command
  * @result:	scsi error code
  *
- * Translate a SCSI result code into a blk_status_t value. May reset the host
- * byte of @cmd->result.
+ * Translate a SCSI result code into a blk_status_t value.
  */
-static blk_status_t scsi_result_to_blk_status(struct scsi_cmnd *cmd, int result)
+static blk_status_t scsi_result_to_blk_status(int result)
 {
 	/*
 	 * Check the scsi-ml byte first in case we converted a host or status
@@ -616,18 +614,6 @@ static blk_status_t scsi_result_to_blk_status(struct scsi_cmnd *cmd, int result)
 	case DID_TRANSPORT_FAILFAST:
 	case DID_TRANSPORT_MARGINAL:
 		return BLK_STS_TRANSPORT;
-	case DID_TARGET_FAILURE:
-		set_host_byte(cmd, DID_OK);
-		return BLK_STS_TARGET;
-	case DID_NEXUS_FAILURE:
-		set_host_byte(cmd, DID_OK);
-		return BLK_STS_NEXUS;
-	case DID_ALLOC_FAILURE:
-		set_host_byte(cmd, DID_OK);
-		return BLK_STS_NOSPC;
-	case DID_MEDIUM_ERROR:
-		set_host_byte(cmd, DID_OK);
-		return BLK_STS_MEDIUM;
 	default:
 		return BLK_STS_IOERR;
 	}
@@ -715,7 +701,7 @@ static void scsi_io_completion_action(struct scsi_cmnd *cmd, int result)
 	if (sense_valid)
 		sense_current = !scsi_sense_is_deferred(&sshdr);
 
-	blk_stat = scsi_result_to_blk_status(cmd, result);
+	blk_stat = scsi_result_to_blk_status(result);
 
 	if (host_byte(result) == DID_RESET) {
 		/* Third party bus reset or reset for error recovery
@@ -893,14 +879,14 @@ static int scsi_io_completion_nz_result(struct scsi_cmnd *cmd, int result,
 					     SCSI_SENSE_BUFFERSIZE);
 		}
 		if (sense_current)
-			*blk_statp = scsi_result_to_blk_status(cmd, result);
+			*blk_statp = scsi_result_to_blk_status(result);
 	} else if (blk_rq_bytes(req) == 0 && sense_current) {
 		/*
 		 * Flush commands do not transfers any data, and thus cannot use
 		 * good_bytes != blk_rq_bytes(req) as the signal for an error.
 		 * This sets *blk_statp explicitly for the problem case.
 		 */
-		*blk_statp = scsi_result_to_blk_status(cmd, result);
+		*blk_statp = scsi_result_to_blk_status(result);
 	}
 	/*
 	 * Recovered errors need reporting, but they're always treated as
-- 
2.25.1

[PATCH 10/10] scsi: Remove useless host error codes.

[Mike Christie]

The host codes that were supposed to only be used for internal use are
now not used, so remove them.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 include/scsi/scsi_status.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/scsi/scsi_status.h b/include/scsi/scsi_status.h
index 31d30cee1869..9cb85262de64 100644
--- a/include/scsi/scsi_status.h
+++ b/include/scsi/scsi_status.h
@@ -62,12 +62,12 @@ enum scsi_host_status {
 					 * recover the link. Transport class will
 					 * retry or fail IO */
 	DID_TRANSPORT_FAILFAST = 0x0f, /* Transport class fastfailed the io */
-	DID_TARGET_FAILURE = 0x10, /* Permanent target failure, do not retry on
-				    * other paths */
-	DID_NEXUS_FAILURE = 0x11,  /* Permanent nexus failure, retry on other
-				    * paths might yield different results */
-	DID_ALLOC_FAILURE = 0x12,  /* Space allocation on the device failed */
-	DID_MEDIUM_ERROR = 0x13,  /* Medium error */
+	/*
+	 * We used to have DID_TARGET_FAILURE, DID_NEXUS_FAILURE,
+	 * DID_ALLOC_FAILURE and DID_MEDIUM_ERROR at 0x10 - 0x13. For compat
+	 * with userspace apps that parse the host byte for SG IO, we leave
+	 * that block of codes unused and start at 0x14 below.
+	 */
 	DID_TRANSPORT_MARGINAL = 0x14, /* Transport marginal errors */
 };
 
-- 
2.25.1

Re: [PATCH 01/10] scsi: xen: Drop use of internal host codes.

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2158 bytes --]

On 04.08.22 05:40, Mike Christie wrote:
> The error codes:
> 
> DID_TARGET_FAILURE
> DID_NEXUS_FAILURE
> DID_ALLOC_FAILURE
> DID_MEDIUM_ERROR
> 
> are internal to the SCSI layer. Drivers must not use them because:
> 
> 1. They are not propagated upwards, so SG IO/passthrough users will not
> see an error and think a command was successful.
> 
> xen-scsiback will never see this error and should not try to send it.
> 
> 2. There is no handling for them in scsi_decide_disposition so if
> xen-scsifront were to return the error to scsi-ml then it kicks off the
> error handler which is definitely not what we want.
> 
> This patch remove the use from xen-scsifront/back.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>   drivers/scsi/xen-scsifront.c       |  8 --------
>   drivers/xen/xen-scsiback.c         | 12 ------------
>   include/xen/interface/io/vscsiif.h | 10 +---------
>   3 files changed, 1 insertion(+), 29 deletions(-)
> 

...

> diff --git a/include/xen/interface/io/vscsiif.h b/include/xen/interface/io/vscsiif.h
> index 7ea4dc9611c4..44eb1f34f1a0 100644
> --- a/include/xen/interface/io/vscsiif.h
> +++ b/include/xen/interface/io/vscsiif.h
> @@ -316,16 +316,8 @@ struct vscsiif_response {
>   #define XEN_VSCSIIF_RSLT_HOST_TRANSPORT_DISRUPTED 14
>   /* Transport class fastfailed */
>   #define XEN_VSCSIIF_RSLT_HOST_TRANSPORT_FAILFAST  15
> -/* Permanent target failure */
> -#define XEN_VSCSIIF_RSLT_HOST_TARGET_FAILURE      16
> -/* Permanent nexus failure on path */
> -#define XEN_VSCSIIF_RSLT_HOST_NEXUS_FAILURE       17
> -/* Space allocation on device failed */
> -#define XEN_VSCSIIF_RSLT_HOST_ALLOC_FAILURE       18
> -/* Medium error */
> -#define XEN_VSCSIIF_RSLT_HOST_MEDIUM_ERROR        19
>   /* Transport marginal errors */
> -#define XEN_VSCSIIF_RSLT_HOST_TRANSPORT_MARGINAL  20
> +#define XEN_VSCSIIF_RSLT_HOST_TRANSPORT_MARGINAL  16

Please drop the modifications of this header.

This is a copy of the master from the Xen repository. It might be used
in multiple OS'es across many releases, so it needs to be regarded as ABI.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 00/10] scsi: Fix internal host code use

[Oliver Neukum]

On 04.08.22 05:40, Mike Christie wrote:
> The following patches made over Martin's 5.20 staging branch fix an issue
> where we probably intended the host codes:
> 
> DID_TARGET_FAILURE
> DID_NEXUS_FAILURE
> DID_ALLOC_FAILURE
> DID_MEDIUM_ERROR
> 
> to be internal to scsi-ml, but at some point drivers started using them
> and the driver writers never updated scsi-ml.

Hi,

this approach drops useful information, though. If a device
reports such specific an error condition, why not use that
information?

	Regards
		Oliver

Re: [PATCH 01/10] scsi: xen: Drop use of internal host codes.

[Mike Christie]

On 8/4/22 1:18 AM, Juergen Gross wrote:
> On 04.08.22 05:40, Mike Christie wrote:
>> The error codes:
>>
>> DID_TARGET_FAILURE
>> DID_NEXUS_FAILURE
>> DID_ALLOC_FAILURE
>> DID_MEDIUM_ERROR
>>
>> are internal to the SCSI layer. Drivers must not use them because:
>>
>> 1. They are not propagated upwards, so SG IO/passthrough users will not
>> see an error and think a command was successful.
>>
>> xen-scsiback will never see this error and should not try to send it.
>>
>> 2. There is no handling for them in scsi_decide_disposition so if
>> xen-scsifront were to return the error to scsi-ml then it kicks off the
>> error handler which is definitely not what we want.
>>
>> This patch remove the use from xen-scsifront/back.
>>
>> Signed-off-by: Mike Christie <michael.christie@oracle.com>
>> ---
>>   drivers/scsi/xen-scsifront.c       |  8 --------
>>   drivers/xen/xen-scsiback.c         | 12 ------------
>>   include/xen/interface/io/vscsiif.h | 10 +---------
>>   3 files changed, 1 insertion(+), 29 deletions(-)
>>
> 
> ...
> 
>> diff --git a/include/xen/interface/io/vscsiif.h b/include/xen/interface/io/vscsiif.h
>> index 7ea4dc9611c4..44eb1f34f1a0 100644
>> --- a/include/xen/interface/io/vscsiif.h
>> +++ b/include/xen/interface/io/vscsiif.h
>> @@ -316,16 +316,8 @@ struct vscsiif_response {
>>   #define XEN_VSCSIIF_RSLT_HOST_TRANSPORT_DISRUPTED 14
>>   /* Transport class fastfailed */
>>   #define XEN_VSCSIIF_RSLT_HOST_TRANSPORT_FAILFAST  15
>> -/* Permanent target failure */
>> -#define XEN_VSCSIIF_RSLT_HOST_TARGET_FAILURE      16
>> -/* Permanent nexus failure on path */
>> -#define XEN_VSCSIIF_RSLT_HOST_NEXUS_FAILURE       17
>> -/* Space allocation on device failed */
>> -#define XEN_VSCSIIF_RSLT_HOST_ALLOC_FAILURE       18
>> -/* Medium error */
>> -#define XEN_VSCSIIF_RSLT_HOST_MEDIUM_ERROR        19
>>   /* Transport marginal errors */
>> -#define XEN_VSCSIIF_RSLT_HOST_TRANSPORT_MARGINAL  20
>> +#define XEN_VSCSIIF_RSLT_HOST_TRANSPORT_MARGINAL  16
> 
> Please drop the modifications of this header.
> 
> This is a copy of the master from the Xen repository. It might be used
> in multiple OS'es across many releases, so it needs to be regarded as ABI.
> 

How do you want to handle scsifront_host_byte?

xen-scsiback.c will never return those error codes, but can some other
implementation? Do you want me to add translation from the XEN_VSCSIIF
to some other DID codes?

Re: [PATCH 01/10] scsi: xen: Drop use of internal host codes.

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2718 bytes --]

On 04.08.22 18:28, Mike Christie wrote:
> On 8/4/22 1:18 AM, Juergen Gross wrote:
>> On 04.08.22 05:40, Mike Christie wrote:
>>> The error codes:
>>>
>>> DID_TARGET_FAILURE
>>> DID_NEXUS_FAILURE
>>> DID_ALLOC_FAILURE
>>> DID_MEDIUM_ERROR
>>>
>>> are internal to the SCSI layer. Drivers must not use them because:
>>>
>>> 1. They are not propagated upwards, so SG IO/passthrough users will not
>>> see an error and think a command was successful.
>>>
>>> xen-scsiback will never see this error and should not try to send it.
>>>
>>> 2. There is no handling for them in scsi_decide_disposition so if
>>> xen-scsifront were to return the error to scsi-ml then it kicks off the
>>> error handler which is definitely not what we want.
>>>
>>> This patch remove the use from xen-scsifront/back.
>>>
>>> Signed-off-by: Mike Christie <michael.christie@oracle.com>
>>> ---
>>>    drivers/scsi/xen-scsifront.c       |  8 --------
>>>    drivers/xen/xen-scsiback.c         | 12 ------------
>>>    include/xen/interface/io/vscsiif.h | 10 +---------
>>>    3 files changed, 1 insertion(+), 29 deletions(-)
>>>
>>
>> ...
>>
>>> diff --git a/include/xen/interface/io/vscsiif.h b/include/xen/interface/io/vscsiif.h
>>> index 7ea4dc9611c4..44eb1f34f1a0 100644
>>> --- a/include/xen/interface/io/vscsiif.h
>>> +++ b/include/xen/interface/io/vscsiif.h
>>> @@ -316,16 +316,8 @@ struct vscsiif_response {
>>>    #define XEN_VSCSIIF_RSLT_HOST_TRANSPORT_DISRUPTED 14
>>>    /* Transport class fastfailed */
>>>    #define XEN_VSCSIIF_RSLT_HOST_TRANSPORT_FAILFAST  15
>>> -/* Permanent target failure */
>>> -#define XEN_VSCSIIF_RSLT_HOST_TARGET_FAILURE      16
>>> -/* Permanent nexus failure on path */
>>> -#define XEN_VSCSIIF_RSLT_HOST_NEXUS_FAILURE       17
>>> -/* Space allocation on device failed */
>>> -#define XEN_VSCSIIF_RSLT_HOST_ALLOC_FAILURE       18
>>> -/* Medium error */
>>> -#define XEN_VSCSIIF_RSLT_HOST_MEDIUM_ERROR        19
>>>    /* Transport marginal errors */
>>> -#define XEN_VSCSIIF_RSLT_HOST_TRANSPORT_MARGINAL  20
>>> +#define XEN_VSCSIIF_RSLT_HOST_TRANSPORT_MARGINAL  16
>>
>> Please drop the modifications of this header.
>>
>> This is a copy of the master from the Xen repository. It might be used
>> in multiple OS'es across many releases, so it needs to be regarded as ABI.
>>
> 
> How do you want to handle scsifront_host_byte?
> 
> xen-scsiback.c will never return those error codes, but can some other
> implementation? Do you want me to add translation from the XEN_VSCSIIF
> to some other DID codes?

Its fine for scsifront to use the "default:" fallback in this case.


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 00/10] scsi: Fix internal host code use

[Mike Christie]

On 8/4/22 1:55 AM, Oliver Neukum wrote:
> 
> 
> On 04.08.22 05:40, Mike Christie wrote:
>> The following patches made over Martin's 5.20 staging branch fix an issue
>> where we probably intended the host codes:
>>
>> DID_TARGET_FAILURE
>> DID_NEXUS_FAILURE
>> DID_ALLOC_FAILURE
>> DID_MEDIUM_ERROR
>>
>> to be internal to scsi-ml, but at some point drivers started using them
>> and the driver writers never updated scsi-ml.
> 
> Hi,
> 
> this approach drops useful information, though. If a device
> reports such specific an error condition, why not use that
> information

Is there a specific patch/case/code you are concerned?

I think in most cases the drivers were not using the correct
error code or they were stretching in trying to find a code
already.

The only ones that I thought were questionable were:

1. storvsc_drv: Used DID_TARGET_FAILURE for a local allocation
failure when they wanted to handle lun removal/scanning from a
worker thread.

I don't think DID_TARGET_FAILURE is right here. The driver wants
to just not retry this command. It's not really a perm target
failure like DID_TARGET_FAILURE is documented as. The failure
is just that the driver can't allocate some mem to perform lun
management.

I think either:

1. When we hit that failure path that we want to keep the 
DID_NO_CONNECT/DID_REQUEUE and not overwrite them.

Or

2. I used DID_BAD_TARGET to try and keep the spirit of their
DID_TARGET_FAILURE use where we couldn't handle an operation on
it's behalf. So the target itself is not bad but our processing
for it was so I thought it was close enough.

Note that I think the root issue is that the driver should
not be handling UAs and doing LUN scanning/removal and should
have added code to scsi-ml so it can be handled for everyone.
So really that code should not exist but that is a larger
change. I didn't want to add a new error code because of this.

2. uas: Used DID_TARGET_FAILURE when a TMF was not supported.
Again I don't think that code was right because it's not
a perm target failure. It is something that we don't want to
retry on another path but I don't think that comes up for
this driver ever.

I think DID_BAD_TARGET is ok'ish for this one. It's not a bad
target, but the target doesn't support what we needed and
DID_BAD_TARGET still conveys what we wanted and gives us the
same behavior.

3. cxlflash: DID_ALLOC_FAILURE was wrong in this case because
they wanted a retryable error. DID_ALLOC_FAILURE was for when
we are doing provisioning and couldn't allocate space on the
device, and is not retrable.

DID_ERROR gives them the behavior they want. It does lose info
but that's just how drivers ask scsi-ml to retry errors we don't
have codes for. We could add a new code but I don't think it
was worth it since we don't do that for every other driver and
their retryable errors. If there are drivers that have the same
issue then I'm for adding a new code.

Re: [PATCH 04/10] scsi: virtio_scsi: Drop DID_TARGET_FAILURE use.

[Paolo Bonzini]

On 8/4/22 05:40, Mike Christie wrote:
> DID_TARGET_FAILURE is internal to the SCSI layer. Drivers must not use it
> because:
> 
> 1. It's not propagated upwards, so SG IO/passthrough users will not see an
> error and think a command was successful.
> 
> 2. There is no handling for them in scsi_decide_disposition so it results
> in the scsi eh running.
> 
> It looks like virtio_scsi gets this when something like qemu returns
> VIRTIO_SCSI_S_TARGET_FAILURE. It looks like qemu returns that error code
> if a host OS returns it, but this shouldn't happen for linux since we
> never propagate that error to userspace.
> 
> This has us use DID_BAD_TARGET in case some other virt layer is returning
> it. In that case we will still get a hard error like before and it conveys
> something unexpected happened.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>   drivers/scsi/virtio_scsi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 578c4b6d0f7d..112d8c3962b0 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -141,7 +141,7 @@ static void virtscsi_complete_cmd(struct virtio_scsi *vscsi, void *buf)
>   		set_host_byte(sc, DID_TRANSPORT_DISRUPTED);
>   		break;
>   	case VIRTIO_SCSI_S_TARGET_FAILURE:
> -		set_host_byte(sc, DID_TARGET_FAILURE);
> +		set_host_byte(sc, DID_BAD_TARGET);
>   		break;
>   	case VIRTIO_SCSI_S_NEXUS_FAILURE:
>   		set_host_byte(sc, DID_NEXUS_FAILURE);

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH 05/10] scsi: virtio_scsi: Drop DID_NEXUS_FAILURE use.

[Paolo Bonzini]

On 8/4/22 05:40, Mike Christie wrote:
> DID_NEXUS_FAILURE is internal to the SCSI layer. Drivers must not use it
> because:
> 
> 1. It's not propagated upwards, so SG IO/passthrough users will not see an
> error and think a command was successful.
> 
> 2. There is no handling for them in scsi_decide_disposition so it results
> in the scsi eh running.
> 
> It looks like virtio_scsi gets this when something like qemu returns
> VIRTIO_SCSI_S_NEXUS_FAILURE. It looks like qemu returns that error code
> if host OS returns DID_NEXUS_FAILURE (qemu's internal
> SCSI_HOST_RESERVATION_ERROR maps to DID_NEXUS_FAILURE). This shouldn't
> happen for linux since we don't propagate that error code to userspace.
> 
> This has us convert VIRTIO_SCSI_S_NEXUS_FAILURE to a
> SAM_STAT_RESERVATION_CONFLICT in case some other virt layer is returning
> it. In that case we will still get the reservation confict failure we
> expect.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>   drivers/scsi/virtio_scsi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 112d8c3962b0..00cf6743db8c 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -144,7 +144,7 @@ static void virtscsi_complete_cmd(struct virtio_scsi *vscsi, void *buf)
>   		set_host_byte(sc, DID_BAD_TARGET);
>   		break;
>   	case VIRTIO_SCSI_S_NEXUS_FAILURE:
> -		set_host_byte(sc, DID_NEXUS_FAILURE);
> +		set_status_byte(sc, SAM_STAT_RESERVATION_CONFLICT);
>   		break;
>   	default:
>   		scmd_printk(KERN_WARNING, sc, "Unknown response %d",

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH 03/10] scsi: uas: Drop DID_TARGET_FAILURE use.

[Mike Christie]

Adding Hans and Gerd. Sorry, I messed up the cc originally.

On 8/3/22 10:40 PM, Mike Christie wrote:
> DID_TARGET_FAILURE is internal to the SCSI layer. Drivers must not use it
> because:
> 
> 1. It's not propagated upwards, so SG IO/passthrough users will not see an
> error and think a command was successful.
> 
> 2. There is no handling for them in scsi_decide_disposition so it results
> in the scsi eh running.
> 
> It looks like the driver wanted a hard failure so this swaps it with
> DID_BAD_TARGET which gives us that behavior and the error looks like it's
> for a case where the target did not support a TMF we wanted to use (maybe
> not a bad target but disappointing so close enough).
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>  drivers/usb/storage/uas.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 84dc270f6f73..de3836412bf3 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -283,7 +283,7 @@ static bool uas_evaluate_response_iu(struct response_iu *riu, struct scsi_cmnd *
>  		set_host_byte(cmnd, DID_OK);
>  		break;
>  	case RC_TMF_NOT_SUPPORTED:
> -		set_host_byte(cmnd, DID_TARGET_FAILURE);
> +		set_host_byte(cmnd, DID_BAD_TARGET);
>  		break;
>  	default:
>  		uas_log_cmd_state(cmnd, "response iu", response_code);

Re: [PATCH 03/10] scsi: uas: Drop DID_TARGET_FAILURE use.

[Hans de Goede]

Hi,

On 8/4/22 20:59, Mike Christie wrote:
> Adding Hans and Gerd. Sorry, I messed up the cc originally.
> 
> On 8/3/22 10:40 PM, Mike Christie wrote:
>> DID_TARGET_FAILURE is internal to the SCSI layer. Drivers must not use it
>> because:
>>
>> 1. It's not propagated upwards, so SG IO/passthrough users will not see an
>> error and think a command was successful.
>>
>> 2. There is no handling for them in scsi_decide_disposition so it results
>> in the scsi eh running.
>>
>> It looks like the driver wanted a hard failure so this swaps it with
>> DID_BAD_TARGET which gives us that behavior and the error looks like it's
>> for a case where the target did not support a TMF we wanted to use (maybe
>> not a bad target but disappointing so close enough).
>>
>> Signed-off-by: Mike Christie <michael.christie@oracle.com>

The suggested change sounds reasonable to me. Note I never touched
the part of the driver being changed here and I'm not a SCSI expert,
so this is just my 2 cents on the change really.

Regards,

Hans




>> ---
>>  drivers/usb/storage/uas.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
>> index 84dc270f6f73..de3836412bf3 100644
>> --- a/drivers/usb/storage/uas.c
>> +++ b/drivers/usb/storage/uas.c
>> @@ -283,7 +283,7 @@ static bool uas_evaluate_response_iu(struct response_iu *riu, struct scsi_cmnd *
>>  		set_host_byte(cmnd, DID_OK);
>>  		break;
>>  	case RC_TMF_NOT_SUPPORTED:
>> -		set_host_byte(cmnd, DID_TARGET_FAILURE);
>> +		set_host_byte(cmnd, DID_BAD_TARGET);
>>  		break;
>>  	default:
>>  		uas_log_cmd_state(cmnd, "response iu", response_code);
> 

Re: [PATCH 05/10] scsi: virtio_scsi: Drop DID_NEXUS_FAILURE use.

[Michael S. Tsirkin]

On Wed, Aug 03, 2022 at 10:40:55PM -0500, Mike Christie wrote:
> DID_NEXUS_FAILURE is internal to the SCSI layer. Drivers must not use it
> because:
> 
> 1. It's not propagated upwards, so SG IO/passthrough users will not see an
> error and think a command was successful.
> 
> 2. There is no handling for them in scsi_decide_disposition so it results
> in the scsi eh running.
> 
> It looks like virtio_scsi gets this when something like qemu returns
> VIRTIO_SCSI_S_NEXUS_FAILURE. It looks like qemu returns that error code
> if host OS returns DID_NEXUS_FAILURE (qemu's internal
> SCSI_HOST_RESERVATION_ERROR maps to DID_NEXUS_FAILURE). This shouldn't
> happen for linux since we don't propagate that error code to userspace.
> 
> This has us convert VIRTIO_SCSI_S_NEXUS_FAILURE to a
> SAM_STAT_RESERVATION_CONFLICT in case some other virt layer is returning
> it. In that case we will still get the reservation confict failure we
> expect.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>

If everyone drops it, so should virtio

so

Acked-by: Michael S. Tsirkin <mst@redhat.com>

but pls merge with rest of the patchset.

> ---
>  drivers/scsi/virtio_scsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 112d8c3962b0..00cf6743db8c 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -144,7 +144,7 @@ static void virtscsi_complete_cmd(struct virtio_scsi *vscsi, void *buf)
>  		set_host_byte(sc, DID_BAD_TARGET);
>  		break;
>  	case VIRTIO_SCSI_S_NEXUS_FAILURE:
> -		set_host_byte(sc, DID_NEXUS_FAILURE);
> +		set_status_byte(sc, SAM_STAT_RESERVATION_CONFLICT);
>  		break;
>  	default:
>  		scmd_printk(KERN_WARNING, sc, "Unknown response %d",
> -- 
> 2.25.1

Re: [PATCH 04/10] scsi: virtio_scsi: Drop DID_TARGET_FAILURE use.

[Michael S. Tsirkin]

On Wed, Aug 03, 2022 at 10:40:54PM -0500, Mike Christie wrote:
> DID_TARGET_FAILURE is internal to the SCSI layer. Drivers must not use it
> because:
> 
> 1. It's not propagated upwards, so SG IO/passthrough users will not see an
> error and think a command was successful.
> 
> 2. There is no handling for them in scsi_decide_disposition so it results
> in the scsi eh running.
> 
> It looks like virtio_scsi gets this when something like qemu returns
> VIRTIO_SCSI_S_TARGET_FAILURE. It looks like qemu returns that error code
> if a host OS returns it, but this shouldn't happen for linux since we
> never propagate that error to userspace.
> 
> This has us use DID_BAD_TARGET in case some other virt layer is returning
> it. In that case we will still get a hard error like before and it conveys
> something unexpected happened.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

pls merge with rest of patchset.

> ---
>  drivers/scsi/virtio_scsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 578c4b6d0f7d..112d8c3962b0 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -141,7 +141,7 @@ static void virtscsi_complete_cmd(struct virtio_scsi *vscsi, void *buf)
>  		set_host_byte(sc, DID_TRANSPORT_DISRUPTED);
>  		break;
>  	case VIRTIO_SCSI_S_TARGET_FAILURE:
> -		set_host_byte(sc, DID_TARGET_FAILURE);
> +		set_host_byte(sc, DID_BAD_TARGET);
>  		break;
>  	case VIRTIO_SCSI_S_NEXUS_FAILURE:
>  		set_host_byte(sc, DID_NEXUS_FAILURE);
> -- 
> 2.25.1

Re: [PATCH 10/10] scsi: Remove useless host error codes.

[Bart Van Assche]

On 8/3/22 20:41, Mike Christie wrote:
> The host codes that were supposed to only be used for internal use are
> now not used, so remove them.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>   include/scsi/scsi_status.h | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/include/scsi/scsi_status.h b/include/scsi/scsi_status.h
> index 31d30cee1869..9cb85262de64 100644
> --- a/include/scsi/scsi_status.h
> +++ b/include/scsi/scsi_status.h
> @@ -62,12 +62,12 @@ enum scsi_host_status {
>   					 * recover the link. Transport class will
>   					 * retry or fail IO */
>   	DID_TRANSPORT_FAILFAST = 0x0f, /* Transport class fastfailed the io */
> -	DID_TARGET_FAILURE = 0x10, /* Permanent target failure, do not retry on
> -				    * other paths */
> -	DID_NEXUS_FAILURE = 0x11,  /* Permanent nexus failure, retry on other
> -				    * paths might yield different results */
> -	DID_ALLOC_FAILURE = 0x12,  /* Space allocation on the device failed */
> -	DID_MEDIUM_ERROR = 0x13,  /* Medium error */
> +	/*
> +	 * We used to have DID_TARGET_FAILURE, DID_NEXUS_FAILURE,
> +	 * DID_ALLOC_FAILURE and DID_MEDIUM_ERROR at 0x10 - 0x13. For compat
> +	 * with userspace apps that parse the host byte for SG IO, we leave
> +	 * that block of codes unused and start at 0x14 below.
> +	 */
>   	DID_TRANSPORT_MARGINAL = 0x14, /* Transport marginal errors */
>   };

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

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

[Bart Van Assche]

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>

Re: [PATCH 09/10] scsi: Convert scsi_decide_disposition to use SCSIML_STAT

[Bart Van Assche]

On 8/3/22 20:40, Mike Christie wrote:
> Don't use:
> 
> DID_TARGET_FAILURE
> DID_NEXUS_FAILURE
> DID_ALLOC_FAILURE
> DID_MEDIUM_ERROR
> 
> Instead use the scsi-ml internal values.

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

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

[Mike Christie]

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.