On Fri, 2018-04-06 at 09:45 +0200, Johannes Thumshirn wrote:
> On 05/04/18 19:49, Martin K. Petersen wrote:
> > Longer term I'd really like to see the command result integer
> > host/driver/msg/status stuff cleaned up. It's super arcane and the
> > associated naming schemes make it a very error-prone interface.
> >
>
> I did start a series [1] for this but than got distracted by more urgent
> things. I can pick it up again I think.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/jth/linux.git/log/?h=iscsi_DID_REQUEUE
Hello Johannes,
Since modifying how SCSI results are communicated from SCSI LLDs to the SCSI
core requires to touch all SCSI LLDs, please consider to include the following
changes in your patch series:
- Remove the deprecated SCSI status codes (GOOD, CHECK_CONDITION, etc.) and
use the SAM_STAT_* symbolic names instead.
- Ensure that assigning an int to scsi_cmnd.result triggers a compiler error.
There is code in the Linux kernel that mixes traditional error codes (-EIO)
with the SCSI result codes (DID_ERROR << 16). I think most of that code is
buggy and that it should be fixed. Changing the type of scsi_cmnd.result is
one way to find such code. How about something like the following (untested)
patch?
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index cb85eddb47ea..27f1c347f0ea 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -18,6 +18,27 @@ enum scsi_timeouts {
SCSI_DEFAULT_EH_TIMEOUT = 10 * HZ,
};
+/*
+ * Note: .result is signed because SCSI LLDs store a SCSI result in
+ * .result while the bsg driver stores a negative error code in .result.
+ */
+typedef union {
+ s32 result;
+ struct {
+#if defined(__BIG_ENDIAN)
+ u8 driver_byte;
+ u8 host_byte;
+ u8 msg_byte;
+ u8 status_byte;
+#elif defined(__LITTLE_ENDIAN)
+ u8 status_byte;
+ u8 msg_byte;
+ u8 host_byte;
+ u8 driver_byte;
+#endif
+ };
+} scsi_result;
+
/*
* DIX-capable adapters effectively support infinite chaining for the
* protection information scatterlist
@@ -38,8 +59,10 @@ enum scsi_timeouts {
* This returns true for known good conditions that may be treated as
* command completed normally
*/
-static inline int scsi_status_is_good(int status)
+static inline int scsi_status_is_good(scsi_result result)
{
+ u8 status = result.status_byte;
+
/*
* FIXME: bit0 is listed as reserved in SCSI-2, but is
* significant in SCSI-3. For now, we follow the SCSI-2
@@ -205,10 +228,10 @@ static inline int scsi_is_wlun(u64 lun)
* host_byte = set by low-level driver to indicate status.
* driver_byte = set by mid-level.
*/
-#define status_byte(result) (((result) >> 1) & 0x7f)
-#define msg_byte(result) (((result) >> 8) & 0xff)
-#define host_byte(result) (((result) >> 16) & 0xff)
-#define driver_byte(result) (((result) >> 24) & 0xff)
+#define status_byte(result) ((result).status_byte >> 1)
+#define msg_byte(result) ((result).msg_byte)
+#define host_byte(result) ((result).host_byte)
+#define driver_byte(result) ((result).driver_byte)
#define sense_class(sense) (((sense) >> 4) & 0x7)
#define sense_error(sense) ((sense) & 0xf)
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 2280b2351739..cbc5df948dd3 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -144,7 +144,7 @@ struct scsi_cmnd {
* obtained by scsi_malloc is guaranteed
* to be at an address < 16Mb). */
- int result; /* Status code from lower level driver */
+ scsi_result result; /* Status code from lower level driver */
int flags; /* Command flags */
unsigned char tag; /* SCSI-II queued command tag */
Bart.
