public inbox for linux-scsi@vger.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH] scsi_status() macro
@ 2003-05-16 12:12 Andries.Brouwer
  2003-05-17  4:38 ` Douglas Gilbert
  0 siblings, 1 reply; 6+ messages in thread
From: Andries.Brouwer @ 2003-05-16 12:12 UTC (permalink / raw)
  To: James.Bottomley, dougg; +Cc: linux-scsi

    > If you're doing this:
    > 
    >>+#define scsi_status(result) ((result) & 0x7e)
    > 
    > because bit0 is reserved in SCSI-2, then it should be 0x3e because bits
    > 6 and 7 are also reserved.  Of course, this would clash with the SCSI-3
    > definition of TASK ABORTED, sigh.
    > 
    > Perhaps it's better just not to bother with the mask?

    James,
    The macro still gets rid of the upper bytes in
    scsi_cmnd::result so in needs to be at least
    a mask of 0xff.

    With a mask of 0x7e it is correct for SCSI-3,
    bit 6 is reserved in SCSI-2  and "vendor unique"
    in SCSI-1 and SCSI_1_CCS. I dug up a SCSI 1 draft and
    the Status byte table is attached. So as far as I
    can see our only exposure is to SCSI 1 and CCS
    devices.

Yes, SCSI-1 has RVVSSSSV with R=reserved, V=vendor unique, S=status.
Originally this was a bitmap, and it is still the case that codes
are not randomly assigned. (We have several places in the
kernel source that test (status & CHECK_CONDITION).)

I think it can be said that universally 0 means good status,
and good status is represented by 0.

The old bits 0,5,6 had a vendor-unique meaning,
and bit 5 now has a new meaning.

So, my version would have

#define scsi_status_byte(result)	((result) & 0xff)

Change all occurrences of status_byte() into scsi_status_byte()
and adapt the values tested against.

(Not that I think we should do this right now.
The patch would be very large, while the gain is very low.
This is the sort of minor cleanup better done at the start
of 2.7. For now I only plan to come with minimal fixes for
the places I noticed where X and X<<1 are confused.)

Andries

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] scsi_status() macro
@ 2003-05-17  4:59 Andries.Brouwer
  0 siblings, 0 replies; 6+ messages in thread
From: Andries.Brouwer @ 2003-05-17  4:59 UTC (permalink / raw)
  To: Andries.Brouwer, dougg; +Cc: James.Bottomley, linux-scsi

    From dougg@torque.net  Sat May 17 06:38:09 2003

    > Yes, SCSI-1 has RVVSSSSV with R=reserved, V=vendor unique, S=status.
    > Originally this was a bitmap, and it is still the case that codes
    > are not randomly assigned. (We have several places in the
    > kernel source that test (status & CHECK_CONDITION).)
    <snip>

    Andries,
    There is method in the madness with (status & CHECK_CONDITION).
    In SCSI-2 there is COMMAND TERMINATED status which has the
    value of 0x22. It is the only status in SCSI 2, 3 or the most
    recent drafts that has bit 1 set besides CHECK CONDITION.

    The bottom line:
        (status & CHECK_CONDITION)
    is obscure shorthand for:
        ((status == CHECK_CONDITION) || (status == COMMAND_TERMINATED))
    and that second check should not be dropped any time
    soon.

Yes, you are just saying what I said, namely that the values
are not randomly assigned but still behave like a bitmap.
The code "(status & CHECK_CONDITION)" would be altogether wrong
otherwise.
Still, I dislike such code - when a new code 0x16, or 0x42, is introduced,
do we expect it to describe a status that also requires the actions
that follow "if(status & CHECK_CONDITION)" ?
No SCSI standard describes status as a bitmap. This is from SASI times.

Andries

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] scsi_status() macro
@ 2003-05-15 23:50 Douglas Gilbert
  2003-05-16  1:11 ` James Bottomley
  0 siblings, 1 reply; 6+ messages in thread
From: Douglas Gilbert @ 2003-05-15 23:50 UTC (permalink / raw)
  To: linux-scsi

[-- Attachment #1: Type: text/plain, Size: 118 bytes --]

Here is a replacement macro for status_byte().
Leave the status_byte() macro and mark it as deprecated.

Doug Gilbert

[-- Attachment #2: scsi_h2569bk8cc.diff --]
[-- Type: text/plain, Size: 1105 bytes --]

--- linux/drivers/scsi/scsi.h	2003-05-14 18:09:21.000000000 +1000
+++ linux/drivers/scsi/scsi.h2569bk8cc	2003-05-16 09:47:11.469708960 +1000
@@ -90,12 +90,19 @@
  *
  *  These are set by:
  *
- *      status byte = set from target device
+ *      status byte = set from target device (SCSI status value)
  *      msg_byte    = return status from host adapter itself.
  *      host_byte   = set by low-level driver to indicate status.
  *      driver_byte = set by mid-level.
+ *
+ *  Notes about following macros:
+ *	scsi_status() returns a standard SCSI status value that may
+ *	be compared with the SAM_STAT_... series of defines.
+ *	status_byte() returns a shifted SCSI status value that matches
+ *	CHECK_CONDITION and friends. status_byte() is deprecated.
  */
-#define status_byte(result) (((result) >> 1) & 0x1f)
+#define scsi_status(result) ((result) & 0x7e)
+#define status_byte(result) (((result) >> 1) & 0x1f)	/* deprecated */
 #define msg_byte(result)    (((result) >> 8) & 0xff)
 #define host_byte(result)   (((result) >> 16) & 0xff)
 #define driver_byte(result) (((result) >> 24) & 0xff)

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2003-05-17  4:46 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2003-05-16 12:12 [PATCH] scsi_status() macro Andries.Brouwer
2003-05-17  4:38 ` Douglas Gilbert
  -- strict thread matches above, loose matches on Subject: below --
2003-05-17  4:59 Andries.Brouwer
2003-05-15 23:50 Douglas Gilbert
2003-05-16  1:11 ` James Bottomley
2003-05-16  5:32   ` Douglas Gilbert

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox