Re: [PATCH 01/25] ata: scsi: rename flag ATA_QCFLAG_FAILED to ATA_QCFLAG_EH

[John Garry <john.g.garry@oracle.com>]

On 08/12/2022 10:59, Niklas Cassel wrote:
> The name ATA_QCFLAG_FAILED is misleading since it does not mean that a
> QC was actually an error.

"QC was actually an error" sounds odd. Maybe "...it does not mean that a
QC completed in error or not at all". I'm not sure on a good suggestion.

> It means that libata decided to schedule EH
> for the QC, so the QC is now owned by the libata error handler (EH).
> 
> The normal execution path is responsible for not accessing a QC owned
> by EH. libata core enforces the rule by returning NULL from
> ata_qc_from_tag() for QCs owned by EH.
> 
> It is quite easy to mistake that a QC marked with ATA_QCFLAG_FAILED was
> an error. However, a QC that was actually an error is instead indicated
> by having qc->err_mask set. E.g. when we have a NCQ error, we abort all
> QCs, which currently will mark all QCs as ATA_QCFLAG_FAILED. However, it
> will only be a single QC that is an error (i.e. has qc->err_mask set).
> 
> Rename ATA_QCFLAG_FAILED to ATA_QCFLAG_EH to more clearly highlight that
> this flag simply means that a QC is now owned by EH. This new name will
> not mislead to think that the QC was an error (which is instead
> indicated by having qc->err_mask set).
> 
> This also makes it more obvious that the EH code skips all QCs that do
> not have ATA_QCFLAG_EH set (rather than ATA_QCFLAG_FAILED), since the EH
> code should simply only care about QCs that are owned by EH itself.
> 
> Signed-off-by: Niklas Cassel<niklas.cassel@wdc.com>

FWIW,

Reviewed-by: John Garry <john.g.garry@oracle.com>