Successful NCQ commands sense_valid cleanups/fixes

When filling the taskfile result for a successful NCQ command, we use the SDB FIS from the FIS Receive Area, see e.g. ahci_qc_ncq_fill_rtf().

However, the SDB FIS only has fields STATUS and ERROR.

For a successful NCQ command that has sense data, we will have a successful sense data descriptor, in the Sense Data for Successful NCQ Commands log.

Since we have access to additional taskfile result fields, fill in these additional fields in qc->result_tf.

This matches how for failing/aborted NCQ commands, we will use e.g. ahci_qc_fill_rtf() to fill in some fields, but then for the command that actually caused the NCQ error, we will use ata_eh_read_log_10h(), which provides additional fields, saving additional fields/overriding the qc->result_tf that was fetched using ahci_qc_fill_rtf().

Fixes 18bd7718b5c4 ("scsi: ata: libata: Handle completion of CDL commands using policy 0xD")

Signed-off-by Niklas Cassel <[email protected]>

Reviewed-by Igor Pylypiv <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

While the SENSE DATA VALID field in the ACS-6 specification is 47 bits, we are currently only fetching 32 bits, because these are the only bits that we care about (these bits represent the tags (which can be 0-31)).

Thus, replace the existing logic with a simple get_unaligned_le32().

While at it, change the type of sense_valid to u32.

Signed-off-by Niklas Cassel <[email protected]>

Reviewed-by Hannes Reinecke <[email protected]>

The BIT() macro is commonly used in the kernel.
Make use of it when converting a tag, fetched from the Successful NCQ Commands log or the NCQ Command Error log, to a bit field. This makes the code easier to read.

Suggested-by Igor Pylypiv <[email protected]>

Signed-off-by Niklas Cassel <[email protected]>

---
  drivers/ata/libata-sata.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

drivers/ata/libata-sata.c +1 -1

index 076953d18db9..4e3034af285e 100644
@@ -1545,7 +1545,7 @@ int ata_eh_get_ncq_success_sense(struct ata_link *link)
  		 * If the command does not have any sense data, clear ATA_SENSE.
  		 * Keep ATA_QCFLAG_EH_SUCCESS_CMD so that command is finished.
  		 */
-		if (!(sense_valid & (1ULL << tag))) {
+		if (!(sense_valid & BIT(tag))) {
  			qc->result_tf.status &= ~ATA_SENSE;
  			continue;
  		}

The BIT() macro is commonly used in the kernel.
Make use of it when converting a tag, fetched from the Successful NCQ Commands log or the NCQ Command Error log, to a bit field. This makes the code easier to read.

Suggested-by Igor Pylypiv <[email protected]>

Signed-off-by Niklas Cassel <[email protected]>

The BIT() macro is commonly used in the kernel.
Make use of it when converting a tag, fetched from the Successful NCQ Commands log or the NCQ Command Error log, to a bit field. This makes the code easier to read.

Suggested-by Igor Pylypiv <[email protected]>

Signed-off-by Niklas Cassel <[email protected]>

---
 drivers/ata/libata-sata.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/ata/libata-sata.c +2 -2

index 076953d18db9..4e3034af285e 100644
@@ -1545,7 +1545,7 @@ int ata_eh_get_ncq_success_sense(struct ata_link *link)
 		 * If the command does not have any sense data, clear ATA_SENSE.
 		 * Keep ATA_QCFLAG_EH_SUCCESS_CMD so that command is finished.
 		 */
-		if (!(sense_valid & (1ULL << tag))) {
+		if (!(sense_valid & BIT(tag))) {
 			qc->result_tf.status &= ~ATA_SENSE;
 			continue;
 		}

@@ -1634,7 +1634,7 @@ void ata_eh_analyze_ncq_error(struct ata_link *link)
 		return;
 	}
 
-	if (!(link->sactive & (1 << tag))) {
+	if (!(link->sactive & BIT(tag))) {
 		ata_link_err(link, "log page 10h reported inactive tag %d\n",
 			     tag);
 		return;

Quoted 7 lines · Niklas Cassel

The BIT() macro is commonly used in the kernel.
Make use of it when converting a tag, fetched from the Successful NCQ Commands log or the NCQ Command Error log, to a bit field. This makes the code easier to read.

Suggested-by Igor Pylypiv <[email protected]>

Signed-off-by Niklas Cassel <[email protected]>

Patch 2 changed sense_valid from u64 to u32. So I think this patch must be squashed into patch 2 as otherwise, the first use of sense_valid doing:

	if (!(sense_valid & (1ULL << tag))) {

looks very wrong to me. Even though in practice it is not going to be an issue.