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.
