Re: [PATCH 14/42] scsi: add scsi_result_is_good()

[Douglas Gilbert <dgilbert@interlog.com>]

On 2021-04-22 11:56 a.m., Douglas Gilbert wrote:
> On 2021-04-22 4:42 a.m., Hannes Reinecke wrote:
>> On 4/21/21 11:58 PM, Douglas Gilbert wrote:
>>> On 2021-04-21 5:10 p.m., Bart Van Assche wrote:
>>>> On 4/21/21 10:47 AM, Hannes Reinecke wrote:
>>>>> +static inline bool scsi_result_is_good(struct scsi_cmnd *cmd)
>>>>> +{
>>>>> +    return (cmd->result == 0);
>>>>> +}
>>>>
>>>> Do we really need an inline function to compare an integer with zero? How 
>>>> about open-coding this comparison in the callers of this function?
>>>>
>>>
>>> Please don't open code it. Please fix it!
>>> Spot the difference:
>>>
>>> static inline int scsi_status_is_good(int status)
>>> {
>>>          /*
>>>           * 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));
>>> }
>>>
>>> In sg3_utils' library I ignore the last three SAM_STATs. Not sure if ignoring
>>> bit 0 is still required.
>>>
>>> Without considering SAM_STAT_CONDITION_MET a good status, someone will be
>>> scratching their head wondering why so many PRE-FETCH commands fail.
>>>
>>> That command can be used when a sequence of READs to consecutive LBAs is
>>> followed by a different (i.e. non-consecutive) READ. That last READ could
>>> be preceded by PRE-FETCH(new_LBA, IMMED). Assuming there is processing
>>> of the data from the consecutive LBAs to be done, when the time comes
>>> for READ(new_LBA) the probability of its data being in the disk's cache is
>>> increased.
>>>
>> That would be a change in behaviour.
>> Current code doesn't check for CONDITION_MET, so this change shouldn't do it, 
>> neither. Idea was that this patchset shouldn't change the current behaviour.
>>
>> While your argument might be valid, it definitely is a different story and 
>> would need to be address with a different patchset.
> 
> Okay. May I suggest a "FIX_ME" comment? And again, please don't open code it.
> 
> In driver manuals Seagate often list the PRE-FETCH command as optional. As
> in: pay us some extra money and we will put it in. That suggests to me some
> big OEM likes PRE-FETCH. Where I found it supported in WDC manuals, they
> didn't support the IMMED bit which sort of defeats the purpose of it IMO.

And PRE-FETCH has another (sneaky) use. You might think that when a LBA is
unmapped, then if its data is in the cache, it would be removed. So what
does SBC-4 say about reading unmapped LBAs (sbc4r22 4.7.4.4 table 10):
   "user data set to a vendor-specific value that is not obtained from
    any other LBA; and" (... place 0xff bytes in the PI)

Spot the weasel word: _other_ ! So it can read the former data in that
now unmapped LBA. So the second usage of PRE-FETCH is to prevent that.
Even more worryingly, it looks like PRE-FETCH may be needed after a
cryptographic erase (via the SANITIZE command)! SYNCHRONIZE CACHE only
talks about the write side of the cache, not removing stale read data.

Doug Gilbert