[PATCH v2 0/3] Successful NCQ commands sense_valid cleanups/fixes

[PATCH v2 0/3] Successful NCQ commands sense_valid cleanups/fixes

[Niklas Cassel]

Hello all,

Here comes some minor cleanups/fixes related to the sense_valid field in
the Successful NCQ commands log.


Kind regards,
Niklas


Changes since v1:
-Pick up tags.
-Added patch 3/3.
-Changed sense_valid to u32 (Igor).
-Clarified commit message in patch 2/3 (Hannes).


Niklas Cassel (3):
  ata: libata-sata: Save all fields from sense data descriptor
  ata: libata-sata: Simplify sense_valid fetching
  ata: libata-sata: Use BIT() macro

 drivers/ata/libata-sata.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

-- 
2.49.0

[PATCH v2 1/3] ata: libata-sata: Save all fields from sense data descriptor

[Niklas Cassel]

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 <cassel@kernel.org>
Reviewed-by: Igor Pylypiv <ipylypiv@google.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
 drivers/ata/libata-sata.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index ba300cc0a3a3..2e4463d3a356 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1510,6 +1510,8 @@ int ata_eh_get_ncq_success_sense(struct ata_link *link)
 	unsigned int err_mask, tag;
 	u8 *sense, sk = 0, asc = 0, ascq = 0;
 	u64 sense_valid, val;
+	u16 extended_sense;
+	bool aux_icc_valid;
 	int ret = 0;
 
 	err_mask = ata_read_log_page(dev, ATA_LOG_SENSE_NCQ, 0, buf, 2);
@@ -1529,6 +1531,8 @@ int ata_eh_get_ncq_success_sense(struct ata_link *link)
 
 	sense_valid = (u64)buf[8] | ((u64)buf[9] << 8) |
 		((u64)buf[10] << 16) | ((u64)buf[11] << 24);
+	extended_sense = get_unaligned_le16(&buf[14]);
+	aux_icc_valid = extended_sense & BIT(15);
 
 	ata_qc_for_each_raw(ap, qc, tag) {
 		if (!(qc->flags & ATA_QCFLAG_EH) ||
@@ -1556,6 +1560,17 @@ int ata_eh_get_ncq_success_sense(struct ata_link *link)
 			continue;
 		}
 
+		qc->result_tf.nsect = sense[6];
+		qc->result_tf.hob_nsect = sense[7];
+		qc->result_tf.lbal = sense[8];
+		qc->result_tf.lbam = sense[9];
+		qc->result_tf.lbah = sense[10];
+		qc->result_tf.hob_lbal = sense[11];
+		qc->result_tf.hob_lbam = sense[12];
+		qc->result_tf.hob_lbah = sense[13];
+		if (aux_icc_valid)
+			qc->result_tf.auxiliary = get_unaligned_le32(&sense[16]);
+
 		/* Set sense without also setting scsicmd->result */
 		scsi_build_sense_buffer(dev->flags & ATA_DFLAG_D_SENSE,
 					qc->scsicmd->sense_buffer, sk,
-- 
2.49.0

[PATCH v2 2/3] ata: libata-sata: Simplify sense_valid fetching

[Niklas Cassel]

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 <cassel@kernel.org>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
 drivers/ata/libata-sata.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 2e4463d3a356..076953d18db9 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1509,9 +1509,10 @@ int ata_eh_get_ncq_success_sense(struct ata_link *link)
 	struct ata_queued_cmd *qc;
 	unsigned int err_mask, tag;
 	u8 *sense, sk = 0, asc = 0, ascq = 0;
-	u64 sense_valid, val;
 	u16 extended_sense;
 	bool aux_icc_valid;
+	u32 sense_valid;
+	u64 val;
 	int ret = 0;
 
 	err_mask = ata_read_log_page(dev, ATA_LOG_SENSE_NCQ, 0, buf, 2);
@@ -1529,8 +1530,7 @@ int ata_eh_get_ncq_success_sense(struct ata_link *link)
 		return -EIO;
 	}
 
-	sense_valid = (u64)buf[8] | ((u64)buf[9] << 8) |
-		((u64)buf[10] << 16) | ((u64)buf[11] << 24);
+	sense_valid = get_unaligned_le32(&buf[8]);
 	extended_sense = get_unaligned_le16(&buf[14]);
 	aux_icc_valid = extended_sense & BIT(15);
 
-- 
2.49.0

[PATCH v2 3/3] ata: libata-sata: Use BIT() macro

[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 <ipylypiv@google.com>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-sata.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 076953d18db9..4e3034af285e 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -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;
-- 
2.49.0

Re: [PATCH v2 3/3] ata: libata-sata: Use BIT() macro

[Hannes Reinecke]

On 4/15/25 09:30, Niklas Cassel wrote:
> 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 <ipylypiv@google.com>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>   drivers/ata/libata-sata.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 076953d18db9..4e3034af285e 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -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;
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH v2 2/3] ata: libata-sata: Simplify sense_valid fetching

[Igor Pylypiv]

On Tue, Apr 15, 2025 at 09:30:16AM +0200, Niklas Cassel wrote:
> 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 <cassel@kernel.org>
> Reviewed-by: Hannes Reinecke <hare@suse.de>

Reviewed-by: Igor Pylypiv <ipylypiv@google.com>

Re: [PATCH v2 3/3] ata: libata-sata: Use BIT() macro

[Igor Pylypiv]

On Tue, Apr 15, 2025 at 09:30:17AM +0200, Niklas Cassel wrote:
> 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 <ipylypiv@google.com>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Igor Pylypiv <ipylypiv@google.com>

Thanks!

Re: [PATCH v2 3/3] ata: libata-sata: Use BIT() macro

[Damien Le Moal]

On 4/15/25 4:30 PM, Niklas Cassel wrote:
> 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 <ipylypiv@google.com>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

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.

> ---
>  drivers/ata/libata-sata.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 076953d18db9..4e3034af285e 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -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;


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 1/3] ata: libata-sata: Save all fields from sense data descriptor

[Damien Le Moal]

On 4/15/25 4:30 PM, Niklas Cassel wrote:
> 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 <cassel@kernel.org>
> Reviewed-by: Igor Pylypiv <ipylypiv@google.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>

Applied to for-6.15-fixes. Thanks !

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 3/3] ata: libata-sata: Use BIT() macro

[Niklas Cassel]

On Wed, Apr 16, 2025 at 05:32:11PM +0900, Damien Le Moal wrote:
> On 4/15/25 4:30 PM, Niklas Cassel wrote:
> > 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 <ipylypiv@google.com>
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> 
> 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.

Integer promotion will happen, so it is not a bug, and I change this in patch
3/3 anyway.

But sure, I can send a v3 where I change it to "1 << tag" in patch 2/3,
and then change it to use the BIT() macro in patch 3/3.


Kind regards,
Niklas