2003-05-15 22:32:24

by Andries E. Brouwer

Subject: [patch] NCR5380.c fix

Several SCSI drivers confuse CHECK_CONDITION and CHECK_CONDITION << 1.
One of them is NCR5380.c. Below a patch adding status_byte() twice.

(On the other hand, sun3_NCR5380.c does this right, and generally
looks better. Maybe they can be merged eventually.)

Andries

(This was for 2.5.68. I think 2.5.69 will differ by 1 line.)

diff -u --recursive --new-file -X /linux/dontdiff a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
--- a/drivers/scsi/NCR5380.c Wed Mar 5 04:29:03 2003
+++ b/drivers/scsi/NCR5380.c Fri May 16 01:42:51 2003
@@ -2466,11 +2466,11 @@

if (cmd->cmnd[0] != REQUEST_SENSE)
cmd->result = cmd->SCp.Status | (cmd->SCp.Message << 8);
- else if (cmd->SCp.Status != GOOD)
+ else if (status_byte(cmd->SCp.Status) != GOOD)
cmd->result = (cmd->result & 0x00ffff) | (DID_ERROR << 16);

#ifdef AUTOSENSE
- if ((cmd->cmnd[0] != REQUEST_SENSE) && (cmd->SCp.Status == CHECK_CONDITION)) {
+ if ((cmd->cmnd[0] != REQUEST_SENSE) && (status_byte(cmd->SCp.Status) == CHECK_CONDITION)) {
dprintk(NDEBUG_AUTOSENSE, ("scsi%d : performing request sense\n", instance->host_no));
cmd->cmnd[0] = REQUEST_SENSE;
cmd->cmnd[1] &= 0xe0;

2003-05-15 22:39:30

by James Bottomley

Subject: Re: [patch] NCR5380.c fix

On Thu, 2003-05-15 at 17:45, [email protected] wrote:
> - else if (cmd->SCp.Status != GOOD)
> + else if (status_byte(cmd->SCp.Status) != GOOD)

Well...if we're doing it this way, any reason not to use the newly
minted SAM_STAT_GOOD and SAM_STAT_CHECK_CONDITION?

James

2003-05-15 23:03:09

by Douglas Gilbert

Subject: Re: [patch] NCR5380.c fix

[email protected] wrote:
> Several SCSI drivers confuse CHECK_CONDITION and CHECK_CONDITION << 1.
<snip>

Linux has always had SCSI status values that are masked
(reasonable) and shifted one bit right (bizarre) from
the equivalent values in the SCSI standards. This
has tricked lots of people.

So in lk 2.5 saner defines (with long-winded names)
have been introduced:
....
#define SAM_STAT_CHECK_CONDITION 0x02
....

The appropriate mask is now 0x7e since Linux uses
the upper bytes and the vendor could (but seldom)
use bits 0 and 7. We should have a constant or macro
for this mask.

So in lk 2.5 your check could read:

if ((cmd->SCp.Status & 0x7e) == SAM_STAT_CHECK_CONDITION)


Aside: "SAM" stands for SCSI Architecture Model which is the
modern standard that defines SCSI status values.

Doug Gilbert

2003-05-15 23:00:17

by James Bottomley

Subject: Re: [patch] NCR5380.c fix

On Thu, 2003-05-15 at 18:10, [email protected] wrote:
> I wouldn't mind merging these three and choosing the SCSI_STATUS_ prefix.

I'll go for that if you want to do the honours.

James

2003-05-15 22:57:15

by Andries E. Brouwer

Subject: Re: [patch] NCR5380.c fix

> any reason not to use the newly minted SAM_STAT_GOOD and
> SAM_STAT_CHECK_CONDITION?

(i) this brings NCR5380.c a tiny little bit closer to sun3_NCR5380.c
(ii) I am not so happy with SAM_STAT_GOOD etc.

Maybe it is just me, but I do not immediately think of SCSI given SAM.
We have sym_defs.h with

#define S_GOOD (0x00)
#define S_CHECK_COND (0x02)
...

and aiclib.h with

#define SCSI_STATUS_OK 0x00
#define SCSI_STATUS_CHECK_COND 0x02
...

and scsi.h with

#define SAM_STAT_GOOD 0x00
#define SAM_STAT_CHECK_CONDITION 0x02
...

I wouldn't mind merging these three and choosing the SCSI_STATUS_ prefix.

Andries