[PATCH] scsi_status() macro

[PATCH] scsi_status() macro

[Douglas Gilbert]

[-- 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)

Re: [PATCH] scsi_status() macro

[James Bottomley]

If you're doing this:

On Thu, 2003-05-15 at 18:50, Douglas Gilbert wrote:
> +#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

Re: [PATCH] scsi_status() macro

[Douglas Gilbert]

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

James Bottomley wrote:
> If you're doing this:
> 
> On Thu, 2003-05-15 at 18:50, Douglas Gilbert wrote:
> 
>>+#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.

A possible source of confusion with this proposed
patch is that 'scsi_status' is probably one of the
most common variable names in the SCSI subsystem.
As a macro it shouldn't clash but it may confuse.
Alternate patch using get_scsi_status() attached.


** SCSI-2 defines reserved as "set aside for future
standardization" but SPC-3 (spc3r12) nails it down:
"A reserved bit, byte, word or field shall be set to
zero, ...".

Doug Gilbert


[-- Attachment #2: scsi1_stat.txt --]
[-- Type: text/plain, Size: 855 bytes --]

14. Status 

A status byte shall be sent from the target to the initiator during the STATUS 
phase at the termination of each command as specified in Tables 14-1 and 14-2 
unless the command is cleared by an ABORT message, by a BUS DEVICE RESET 
message, or by a "hard" RESET condition.

                                  Table 14-1
                                 Status Byte 

==============================================================================
  Bit|   7    |   6    |   5    |   4    |   3    |   2    |   1    |   0    |
Byte |        |        |        |        |        |        |        |        |
==============================================================================
 0   |Reserved|  Vendor Unique  |        Status Byte Code           |   V    |
==============================================================================


[-- Attachment #3: scsi_h2569bk8cc2.diff --]
[-- Type: text/plain, Size: 1113 bytes --]

--- linux/drivers/scsi/scsi.h	2003-05-14 18:09:21.000000000 +1000
+++ linux/drivers/scsi/scsi.h2569bk8cc	2003-05-16 14:50:11.046698960 +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:
+ *	get_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 get_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)

Re: [PATCH] scsi_status() macro

[Andries.Brouwer]

    > 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

Re: [PATCH] scsi_status() macro

[Douglas Gilbert]

Andries.Brouwer@cwi.nl wrote:
>     > 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).)
<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.

In recent standards and drafts COMMAND TERMINATED is deprecated
but in the still important SCSI 2 standard it exists and
also implies that a sense buffer is available and should be
checked.

How long since anyone saw COMMAND TERMINATED? Well I'm
fighting with the ide-scsi driver (and probably losing)
and I see it often. I have a Creative x52 ATAPI CD
reader that once in a while when reading a data CD
with sg_dd fails with COMMAND_TERMINATED/overlapping_
commands. Since sg_dd sends SCSI reads in a purely
sequential order it points to something really sick
in the ide state machine (most likely caused by ide-scsi).
But I digress.

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.

Doug Gilbert

Re: [PATCH] scsi_status() macro

[Andries.Brouwer]

    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