[24/39] wd33c93: translate message byte to host byte

Message ID	20210423113944.42672-25-hare@suse.de (mailing list archive)
State	Superseded
Headers	show Return-Path: <linux-scsi-owner@kernel.org> From: Hannes Reinecke <hare@suse.de> To: "Martin K. Petersen" <martin.petersen@oracle.com> Cc: Christoph Hellwig <hch@lst.de>, James Bottomley <james.bottomley@hansenpartnership.com>, linux-scsi@vger.kernel.org, Bart van Assche <bvanassche@acm.org>, Hannes Reinecke <hare@suse.de> Subject: [PATCH 24/39] wd33c93: translate message byte to host byte Date: Fri, 23 Apr 2021 13:39:29 +0200 Message-Id: <20210423113944.42672-25-hare@suse.de> In-Reply-To: <20210423113944.42672-1-hare@suse.de> References: <20210423113944.42672-1-hare@suse.de> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	SCSI result cleanup, part 2 \| expand [RFC,PATCHv2,00/39] SCSI result cleanup, part 2 [01/39] st: return error code in st_scsi_execute() [02/39] scsi_ioctl: return error code when blk_rq_map_kern() fails [03/39] scsi_dh_alua: do not interpret DRIVER_ERROR [04/39] scsi: Fixup calling convention for scsi_mode_sense() [05/39] scsi: stop using DRIVER_ERROR [06/39] scsi: introduce scsi_build_sense() [07/39] scsi: introduce scsi_status_is_check_condition() [08/39] scsi: Kill DRIVER_SENSE [09/39] scsi: do not use DRIVER_INVALID [10/39] scsi_error: use DID_TIME_OUT instead of DRIVER_TIMEOUT [11/39] xen-scsiback: use DID_ERROR instead of DRIVER_ERROR [12/39] xen-scsifront: compability status handling [13/39] scsi: Drop the now obsolete driver_byte definitions [14/39] NCR5380: Fold SCSI message ABORT onto DID_ABORT [15/39] scsi: add get_{status,host}_byte() accessor function [16/39] scsi: add translate_msg_byte() [17/39] dc395: use standard macros to set SCSI result [18/39] dc395: translate message bytes [19/39] qlogicfas408: make ql_pcmd() a void function [20/39] qlogicfas408: whitespace cleanup [21/39] qlogicfas408: translate message to host byte status [22/39] nsp32: whitespace cleanup [23/39] nsp32: do not set message byte [24/39] wd33c93: translate message byte to host byte [25/39] mesh: translate message to host byte status [26/39] acornscsi: remove acornscsi_reportstatus() [27/39] acornscsi: translate message byte to host byte [28/39] aha152x: modify done() to use separate status bytes [29/39] aha152x: do not set message byte when calling scsi_done() [30/39] advansys: do not set message byte in SCSI status [31/39] fas216: translate message to host byte status [32/39] fas216: Use get_status_byte() to avoid using linux-specific status codes [33/39] FlashPoint: Use standard SCSI definitions [34/39] fdomain: drop last argument to fdomain_finish_cmd() [35/39] fdomain: translate message to host byte status [36/39] scsi: drop message byte helper [37/39] scsi: kill message byte [38/39] target: use standard SAM status types [39/39] scsi: drop obsolete linux-specific SCSI status codes

Message ID

20210423113944.42672-25-hare@suse.de (mailing list archive)

State

Superseded

Headers

From: Hannes Reinecke <hare@suse.de>
To: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>,
        James Bottomley <james.bottomley@hansenpartnership.com>,
        linux-scsi@vger.kernel.org, Bart van Assche <bvanassche@acm.org>,
        Hannes Reinecke <hare@suse.de>
Subject: [PATCH 24/39] wd33c93: translate message byte to host byte
Date: Fri, 23 Apr 2021 13:39:29 +0200
Message-Id: <20210423113944.42672-25-hare@suse.de>
In-Reply-To: <20210423113944.42672-1-hare@suse.de>
References: <20210423113944.42672-1-hare@suse.de>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Series

SCSI result cleanup, part 2 | expand

Commit Message

Hannes Reinecke April 23, 2021, 11:39 a.m. UTC

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@suse.de>
---
 drivers/scsi/wd33c93.c | 46 ++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 20 deletions(-)

Comments

Finn Thain April 24, 2021, 9:20 a.m. UTC | #1

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@suse.de>
> ---
>  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.

Hannes Reinecke April 26, 2021, 9:07 a.m. UTC | #2

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@suse.de>
>> ---
>>  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?

Cheers,

Hannes

Finn Thain April 27, 2021, 4:39 a.m. UTC | #3

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@suse.de>
> >> ---
> >>  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.

Hannes Reinecke April 27, 2021, 6:11 a.m. UTC | #4

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@suse.de>
>>>> ---
>>>>   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

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:

[24/39] wd33c93: translate message byte to host byte

Commit Message

Comments

Patch