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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

Doug Gilbert





[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux