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.
