On 4/27/21 6:39 AM, Finn Thain wrote:
On Mon, 26 Apr 2021, Hannes Reinecke wrote:
On 4/24/21 11:20 AM, Finn Thain wrote:
On Fri, 23 Apr 2021, Hannes Reinecke wrote:
Instead of setting the message byte translate it to the appropriate
host byte. As error recovery would return DID_ERROR for any non-zero
message byte the translation doesn't change the error handling.
Signed-off-by: Hannes Reinecke <hare@xxxxxxx>
---
drivers/scsi/wd33c93.c | 46 ++++++++++++++++++++++++------------------
1 file changed, 26 insertions(+), 20 deletions(-)
diff --git a/drivers/scsi/wd33c93.c b/drivers/scsi/wd33c93.c
index a23277bb870e..187b363db00e 100644
--- a/drivers/scsi/wd33c93.c
+++ b/drivers/scsi/wd33c93.c
@@ -1176,13 +1176,14 @@ wd33c93_intr(struct Scsi_Host *instance)
if (cmd->SCp.Status == ILLEGAL_STATUS_BYTE)
cmd->SCp.Status = lun;
if (cmd->cmnd[0] == REQUEST_SENSE
- && cmd->SCp.Status != SAM_STAT_GOOD)
- cmd->result =
- (cmd->
- result & 0x00ffff) | (DID_ERROR << 16);
- else
- cmd->result =
- cmd->SCp.Status | (cmd->SCp.Message << 8);
+ && cmd->SCp.Status != SAM_STAT_GOOD) {
+ set_host_byte(cmd, DID_ERROR);
+ set_status_byte(cmd, SAM_STAT_GOOD);
+ } else {
+ set_host_byte(cmd, DID_OK);
+ translate_msg_byte(cmd, cmd->SCp.Message);
+ set_status_byte(cmd, cmd->SCp.Status);
+ }
cmd->scsi_done(cmd);
/* We are no longer connected to a target - check to see if
@@ -1262,11 +1263,15 @@ wd33c93_intr(struct Scsi_Host *instance)
hostdata->connected = NULL;
hostdata->busy[cmd->device->id] &= ~(1 << (cmd->device->lun & 0xff));
hostdata->state = S_UNCONNECTED;
- if (cmd->cmnd[0] == REQUEST_SENSE && cmd->SCp.Status != SAM_STAT_GOOD)
- cmd->result =
- (cmd->result & 0x00ffff) | (DID_ERROR << 16);
- else
- cmd->result = cmd->SCp.Status | (cmd->SCp.Message << 8);
+ if (cmd->cmnd[0] == REQUEST_SENSE &&
+ cmd->SCp.Status != SAM_STAT_GOOD) {
+ set_host_byte(cmd, DID_ERROR);
+ set_status_byte(cmd, SAM_STAT_GOOD);
+ } else {
+ set_host_byte(cmd, DID_OK);
+ translate_msg_byte(cmd, cmd->SCp.Message);
+ set_status_byte(cmd, cmd->SCp.Status);
+ }
cmd->scsi_done(cmd);
/* We are no longer connected to a target - check to see if
@@ -1295,14 +1300,15 @@ wd33c93_intr(struct Scsi_Host *instance)
hostdata->busy[cmd->device->id] &= ~(1 << (cmd->device->lun & 0xff));
hostdata->state = S_UNCONNECTED;
DB(DB_INTR, printk(":%d", cmd->SCp.Status))
- if (cmd->cmnd[0] == REQUEST_SENSE
- && cmd->SCp.Status != SAM_STAT_GOOD)
- cmd->result =
- (cmd->
- result & 0x00ffff) | (DID_ERROR << 16);
- else
- cmd->result =
- cmd->SCp.Status | (cmd->SCp.Message << 8);
+ if (cmd->cmnd[0] == REQUEST_SENSE
+ && cmd->SCp.Status != SAM_STAT_GOOD) {
+ set_host_byte(cmd, DID_ERROR);
+ set_status_byte(cmd, SAM_STAT_GOOD);
+ } else {
+ set_host_byte(cmd, DID_OK);
+ translate_msg_byte(cmd, cmd->SCp.Message);
+ set_status_byte(cmd, cmd->SCp.Status);
+ }
cmd->scsi_done(cmd);
break;
case S_PRE_TMP_DISC:
I think these three hunks all have the same mistake, which would force
SAM_STAT_GOOD.
Which mistake was that again?
I noticed that the old code,
cmd->result = (cmd->result & 0x00ffff) | (DID_ERROR << 16);
preserves the status byte whereas the new code clobbers it. This was not
mentioned in the commit log.
Now that I've looked a bit deeper and failed to find any
scsi_eh_prep_cmnd()/scsi_eh_restore_cmnd() that would complicate the
cmd->cmnd[0] == REQUEST_SENSE comparison, I now think clobbering the
status byte is harmless (though redundant).
So please disregard my objection. Sorry for the noise.
Ah. Right. Guess we are both right, then.
Yes, you are right in your objection that my code clobbers the status
byte in the DID_ERROR case.
But that would be irrelevant as SCSI EH will disregard the status code
anyway if the host byte is set.
But in either case, I'll be fixup up the patch to not clobber the status
code here.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@xxxxxxx +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
