Re: [QUESTION]: Why did we clear the lowest bit of SCSI command's status in scsi_status_is_good

[Wenchao Hao <haowenchao@huawei.com>]

On 2022/11/28 20:52, James Bottomley wrote:
> On Mon, 2022-11-28 at 11:58 +0800, Wenchao Hao wrote:
>> static inline bool scsi_status_is_good(int
>> status)                                                              
>>                                                                      
>>                           
>> {
>>         if (status < 0)
>>                 return false;
>>
>>         if (host_byte(status) == DID_NO_CONNECT)
>>                 return false;
>>
>>         /*  
>>          * FIXME: bit0 is listed as reserved in SCSI-2, but is
>>          * significant in SCSI-3.  For now, we follow the SCSI-2
>>          * behaviour and ignore reserved bits.
>>          */
>>         status &= 0xfe;
>>         return ((status == SAM_STAT_GOOD) ||
>>                 (status == SAM_STAT_CONDITION_MET) ||
>>                 /* Next two "intermediate" statuses are obsolete in
>> SAM-4 */
>>                 (status == SAM_STAT_INTERMEDIATE) ||
>>                 (status == SAM_STAT_INTERMEDIATE_CONDITION_MET) ||
>>                 /* FIXME: this is obsolete in SAM-3 */
>>                 (status == SAM_STAT_COMMAND_TERMINATED));
>> }
>> We have function defined in include/scsi/scsi.h as above, which is
>> used to check if the status in result is good.
>>
>> But the function cleared the lowest bit of SCSI command's status,
>> which would translate an undefined status to good in some condition,
>> for example the status is 0x1.
>>
>> This code seems introduced in this patch:
>> https://lore.kernel.org/all/1052403312.2097.35.camel@mulgrave/
>>
>> Is anyone who knows why did we clear the lowest bit? 
> 
> It says why in the comment you quote above ... what is unclear about
> it?
> 
>> We found some firmware or drivers would return status which did not
>> defined in SAM. Now SCSI middle level do not have an uniform behavior
>> for these undefined status. I want to change the logic to return
>> error for all status which did not defined in SAM or define a method
>> in host template to let drivers to judge what to do in this
>> condition.
> 
> Why? The general rule of thumb is be strict in what you emit and
> generous in what you receive (which is why reserved bits are ignored).
> Is the drive you refer to above not working in some way, in which case
> detail it so people can understand the actual problem.
> 
> James
> 
> .


We come with an issue with megaraid_sas driver. Where scsi_cmnd is completed with result's
status byte set to 1, scsi_io_completion() would finish this scsi_cmnd's request with BLK_STS_OK.
The path is:

scsi_io_completion()
	scsi_io_completion_nz_result()
		scsi_status_is_good()	->  return true
		result = 0;
		*blk_statp = BLK_STS_OK
	scsi_end_request() with BLK_STS_OK

While the scsi_cmnd result is actually wrong and should be finished with BLK_STS_IOERR.


What's more, according to code analysis, we found for scsi_cmnd which completed with status byte
undefined in SAM, the scsi_status_is_good() behave differently. For example, if status byte is in
0x1, 0x3, 0x5, 0x9 and so on, it would return true; while for status in other reserved field,
it would return false. 

So I think we should respect the SAM and drop the line "status &= 0xfe" in scsi_status_is_good()
to tell the caller the status is not good for all status which is not defined in SAM, so
scsi_result_to_blk_status() would return BLK_STS_IOERR on this condition.