[PATCH v1 0/4] ATA PASS-THROUGH sense data fixes

[PATCH v1 0/4] ATA PASS-THROUGH sense data fixes

[Igor Pylypiv]

This patch series is fixing a few ATA PASS-THROUGH issues:

1. Not reporting the sense data when ATA error happens and CK_COND is 0.
2. Generating "fake" sense data based on ATA status/error registers even
   though a valid sense data was already fetched from a disk.
3. Fixed format sense data was using incorrect field offsets for ATA
   PASS-THROUGH commands.

Igor Pylypiv (4):
  ata: libata: Remove redundant sense_buffer memsets
  ata: libata-scsi: Generate ATA PT sense data when ATA ERR/DF are set
  ata: libata-scsi: Report valid sense data for ATA PT if present
  ata: libata-scsi: Fix offsets for the fixed format sense data

 drivers/ata/libata-eh.c   |  2 --
 drivers/ata/libata-scsi.c | 53 ++++++++++++++++++++-------------------
 2 files changed, 27 insertions(+), 28 deletions(-)

-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH v1 1/4] ata: libata: Remove redundant sense_buffer memsets

[Igor Pylypiv]

scsi_queue_rq() memsets sense_buffer before a command is dispatched.

Libata is not memsetting sense_buffer before setting sense data that
was obtained from a disk so there should be no reason to do a memset
for ATA PASS-THROUGH / ATAPI.

Memsetting the sense_buffer in ata_gen_passthru_sense() is erasing valid
sense data that was previously obtained from a disk. A follow-up patch
will modify ata_gen_passthru_sense() to stop generating sense data based
on ATA status register bits if a valid sense data is already present.

Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
 drivers/ata/libata-eh.c   | 2 --
 drivers/ata/libata-scsi.c | 4 ----
 2 files changed, 6 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 214b935c2ced..b5e05efe73f6 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1479,8 +1479,6 @@ unsigned int atapi_eh_request_sense(struct ata_device *dev,
 	struct ata_port *ap = dev->link->ap;
 	struct ata_taskfile tf;
 
-	memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE);
-
 	/* initialize sense_buf with the error register,
 	 * for the case where they are -not- overwritten
 	 */
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index cdf29b178ddc..032cf11d0bcc 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -858,8 +858,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
 	unsigned char *desc = sb + 8;
 	u8 sense_key, asc, ascq;
 
-	memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
-
 	/*
 	 * Use ata_to_sense_error() to map status register bits
 	 * onto sense key, asc & ascq.
@@ -953,8 +951,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
 	u64 block;
 	u8 sense_key, asc, ascq;
 
-	memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
-
 	if (ata_dev_disabled(dev)) {
 		/* Device disabled after error recovery */
 		/* LOGICAL UNIT NOT READY, HARD RESET REQUIRED */
-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH v1 2/4] ata: libata-scsi: Generate ATA PT sense data when ATA ERR/DF are set

[Igor Pylypiv]

SCSI/ATA Translation-5 (SAT-5) Table 209 — "ATA command results"
specifies that SATL shall generate sense data for ATA PASS-THROUGH
commands when either CK_COND is set or when ATA_ERR or ATA_DF status
bits are set.

ata_eh_analyze_tf() sets AC_ERR_DEV bit in qc->err_mask when ATA_ERR
or ATA_DF bits are set. It looks like qc->err_mask can be used as
an error indicator but ata_eh_link_autopsy() clears AC_ERR_DEV bit
when ATA_QCFLAG_SENSE_VALID is set. This effectively clears the error
indication if no other bits were set in qc->err_mask.

ata_scsi_qc_complete() should not use qc->err_mask for ATA PASS-THROUGH
commands because qc->err_mask can be zero (i.e. "no error") even when
the corresponding command has failed with ATA_ERR/ATA_DF bits set.

Additionally, the presence of valid sense data (ATA_QCFLAG_SENSE_VALID)
should not prevent SATL from generating sense data for ATA PASS-THROUGH.

Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
 drivers/ata/libata-scsi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 032cf11d0bcc..79e8103ef3a9 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1632,8 +1632,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 		!(qc->flags & ATA_QCFLAG_SENSE_VALID);
 
 	/* For ATA pass thru (SAT) commands, generate a sense block if
-	 * user mandated it or if there's an error.  Note that if we
-	 * generate because the user forced us to [CK_COND =1], a check
+	 * user mandated it or if ATA_ERR or ATA_DF bits are set. Note that
+	 * if we generate because the user forced us to [CK_COND=1], a check
 	 * condition is generated and the ATA register values are returned
 	 * whether the command completed successfully or not. If there
 	 * was no error, we use the following sense data:
@@ -1641,7 +1641,7 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 	 * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
 	 */
 	if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
-	    ((cdb[2] & 0x20) || need_sense))
+	    ((cdb[2] & 0x20) || (qc->result_tf.status & (ATA_ERR | ATA_DF))))
 		ata_gen_passthru_sense(qc);
 	else if (need_sense)
 		ata_gen_ata_sense(qc);
-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH v1 3/4] ata: libata-scsi: Report valid sense data for ATA PT if present

[Igor Pylypiv]

Do not generate sense data from ATA status/error registers
if valid sense data is already present.

Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
 drivers/ata/libata-scsi.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 79e8103ef3a9..4bfe47e7d266 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -858,12 +858,17 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
 	unsigned char *desc = sb + 8;
 	u8 sense_key, asc, ascq;
 
-	/*
-	 * Use ata_to_sense_error() to map status register bits
-	 * onto sense key, asc & ascq.
-	 */
-	if (qc->err_mask ||
-	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
+	if (qc->flags & ATA_QCFLAG_SENSE_VALID) {
+		/*
+		 * Do not generate sense data from ATA status/error
+		 * registers if valid sense data is already present.
+		 */
+	} else if (qc->err_mask ||
+		   tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
+		/*
+		 * Use ata_to_sense_error() to map status register bits
+		 * onto sense key, asc & ascq.
+		 */
 		ata_to_sense_error(qc->ap->print_id, tf->status, tf->error,
 				   &sense_key, &asc, &ascq);
 		ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);
-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH v1 4/4] ata: libata-scsi: Fix offsets for the fixed format sense data

[Igor Pylypiv]

Correct the ATA PASS-THROUGH fixed format sense data offsets to conform
to SPC-6 and SAT-5 specifications. Additionally, set the VALID bit to
indicate that the INFORMATION field contains valid information.

INFORMATION
===========

SAT-5 Table 212 — "Fixed format sense data INFORMATION field for the ATA
PASS-THROUGH commands" defines the following format:

+------+------------+
| Byte |   Field    |
+------+------------+
|    0 | ERROR      |
|    1 | STATUS     |
|    2 | DEVICE     |
|    3 | COUNT(7:0) |
+------+------------+

SPC-6 Table 48 - "Fixed format sense data" specifies that the INFORMATION
field starts at byte 3 in sense buffer resulting in the following offsets
for the ATA PASS-THROUGH commands:

+------------+-------------------------+
|   Field    |  Offset in sense buffer |
+------------+-------------------------+
| ERROR      |  3                      |
| STATUS     |  4                      |
| DEVICE     |  5                      |
| COUNT(7:0) |  6                      |
+------------+-------------------------+

COMMAND-SPECIFIC INFORMATION
============================

SAT-5 Table 213 - "Fixed format sense data COMMAND-SPECIFIC INFORMATION
field for ATA PASS-THROUGH" defines the following format:

+------+-------------------+
| Byte |        Field      |
+------+-------------------+
|    0 | FLAGS | LOG INDEX |
|    1 | LBA (7:0)         |
|    2 | LBA (15:8)        |
|    3 | LBA (23:16)       |
+------+-------------------+

SPC-6 Table 48 - "Fixed format sense data" specifies that
the COMMAND-SPECIFIC-INFORMATION field starts at byte 8
in sense buffer resulting in the following offsets for
the ATA PASS-THROUGH commands:

Offsets of these fields in the fixed sense format are as follows:

+-------------------+-------------------------+
|       Field       |  Offset in sense buffer |
+-------------------+-------------------------+
| FLAGS | LOG INDEX |  8                      |
| LBA (7:0)         |  9                      |
| LBA (15:8)        |  10                     |
| LBA (23:16)       |  11                     |
+-------------------+-------------------------+

Reported-by: Akshat Jain <akshatzen@google.com>
Fixes: 11093cb1ef56 ("libata-scsi: generate correct ATA pass-through sense")
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
 drivers/ata/libata-scsi.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 4bfe47e7d266..8588512f5975 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -855,7 +855,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
 	struct scsi_cmnd *cmd = qc->scsicmd;
 	struct ata_taskfile *tf = &qc->result_tf;
 	unsigned char *sb = cmd->sense_buffer;
-	unsigned char *desc = sb + 8;
 	u8 sense_key, asc, ascq;
 
 	if (qc->flags & ATA_QCFLAG_SENSE_VALID) {
@@ -880,8 +879,9 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
 		scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
 	}
 
-	if ((cmd->sense_buffer[0] & 0x7f) >= 0x72) {
+	if ((sb[0] & 0x7f) >= 0x72) {
 		u8 len;
+		unsigned char *desc;
 
 		/* descriptor format */
 		len = sb[7];
@@ -919,21 +919,21 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
 		}
 	} else {
 		/* Fixed sense format */
-		desc[0] = tf->error;
-		desc[1] = tf->status;
-		desc[2] = tf->device;
-		desc[3] = tf->nsect;
-		desc[7] = 0;
+		sb[0] |= 0x80;
+		sb[3] = tf->error;
+		sb[4] = tf->status;
+		sb[5] = tf->device;
+		sb[6] = tf->nsect;
 		if (tf->flags & ATA_TFLAG_LBA48)  {
-			desc[8] |= 0x80;
+			sb[8] |= 0x80;
 			if (tf->hob_nsect)
-				desc[8] |= 0x40;
+				sb[8] |= 0x40;
 			if (tf->hob_lbal || tf->hob_lbam || tf->hob_lbah)
-				desc[8] |= 0x20;
+				sb[8] |= 0x20;
 		}
-		desc[9] = tf->lbal;
-		desc[10] = tf->lbam;
-		desc[11] = tf->lbah;
+		sb[9] = tf->lbal;
+		sb[10] = tf->lbam;
+		sb[11] = tf->lbah;
 	}
 }
 
-- 
2.45.2.627.g7a2c4fd464-goog

Re: [PATCH v1 1/4] ata: libata: Remove redundant sense_buffer memsets

[Damien Le Moal]

On 6/15/24 04:18, Igor Pylypiv wrote:
> scsi_queue_rq() memsets sense_buffer before a command is dispatched.
> 
> Libata is not memsetting sense_buffer before setting sense data that
> was obtained from a disk so there should be no reason to do a memset
> for ATA PASS-THROUGH / ATAPI.

This sentence is not very clear at all... I assume that the first part of the
sentence is for non passthrough commands. In this case, libata does not clear
the sense buffer because the scsi layer did that already, in scsi_queue_rq() as
noted above.

For passthrough commands, the same should be happening as well since passthrough
commands are also executed through blk_execute_rq() -> scsi_queue_rq(). So I do
not really understand (but I do agree that the memset() in libata seem useless).

> Memsetting the sense_buffer in ata_gen_passthru_sense() is erasing valid
> sense data that was previously obtained from a disk. A follow-up patch
> will modify ata_gen_passthru_sense() to stop generating sense data based
> on ATA status register bits if a valid sense data is already present.

This fix should come first in the series, since that commit will likely need to
go into current rc and cc-stable. And that will simplify this patch as well.

> 
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>  drivers/ata/libata-eh.c   | 2 --
>  drivers/ata/libata-scsi.c | 4 ----
>  2 files changed, 6 deletions(-)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 214b935c2ced..b5e05efe73f6 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -1479,8 +1479,6 @@ unsigned int atapi_eh_request_sense(struct ata_device *dev,
>  	struct ata_port *ap = dev->link->ap;
>  	struct ata_taskfile tf;
>  
> -	memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE);
> -
>  	/* initialize sense_buf with the error register,
>  	 * for the case where they are -not- overwritten
>  	 */
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index cdf29b178ddc..032cf11d0bcc 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -858,8 +858,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
>  	unsigned char *desc = sb + 8;
>  	u8 sense_key, asc, ascq;
>  
> -	memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
> -
>  	/*
>  	 * Use ata_to_sense_error() to map status register bits
>  	 * onto sense key, asc & ascq.
> @@ -953,8 +951,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
>  	u64 block;
>  	u8 sense_key, asc, ascq;
>  
> -	memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
> -
>  	if (ata_dev_disabled(dev)) {
>  		/* Device disabled after error recovery */
>  		/* LOGICAL UNIT NOT READY, HARD RESET REQUIRED */

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v1 2/4] ata: libata-scsi: Generate ATA PT sense data when ATA ERR/DF are set

[Damien Le Moal]

On 6/15/24 04:18, Igor Pylypiv wrote:
> SCSI/ATA Translation-5 (SAT-5) Table 209 — "ATA command results"
> specifies that SATL shall generate sense data for ATA PASS-THROUGH
> commands when either CK_COND is set or when ATA_ERR or ATA_DF status
> bits are set.
> 
> ata_eh_analyze_tf() sets AC_ERR_DEV bit in qc->err_mask when ATA_ERR
> or ATA_DF bits are set. It looks like qc->err_mask can be used as
> an error indicator but ata_eh_link_autopsy() clears AC_ERR_DEV bit
> when ATA_QCFLAG_SENSE_VALID is set. This effectively clears the error
> indication if no other bits were set in qc->err_mask.
> 
> ata_scsi_qc_complete() should not use qc->err_mask for ATA PASS-THROUGH
> commands because qc->err_mask can be zero (i.e. "no error") even when
> the corresponding command has failed with ATA_ERR/ATA_DF bits set.

If there was a failed command, qc->err_mask will be non-zero since the command
completion will be signaled by an error interrupt. So I am confused here. Are
you seeing this happening in practice ? If yes, doing what with which  adapter ?

> Additionally, the presence of valid sense data (ATA_QCFLAG_SENSE_VALID)
> should not prevent SATL from generating sense data for ATA PASS-THROUGH.

Same here, this is strange: ATA_QCFLAG_SENSE_VALID indicates that we have sense
data for the command, regardless if the command is passthrough or not. So if
that flag is set, we should not overwrite the already valid sense data with
sense data generated from the error and status bits.
Do you see an issue without this change ? If yes, what are the adapter and
operations you are running ?

> 
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>  drivers/ata/libata-scsi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 032cf11d0bcc..79e8103ef3a9 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1632,8 +1632,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
>  		!(qc->flags & ATA_QCFLAG_SENSE_VALID);
>  
>  	/* For ATA pass thru (SAT) commands, generate a sense block if
> -	 * user mandated it or if there's an error.  Note that if we
> -	 * generate because the user forced us to [CK_COND =1], a check
> +	 * user mandated it or if ATA_ERR or ATA_DF bits are set. Note that
> +	 * if we generate because the user forced us to [CK_COND=1], a check
>  	 * condition is generated and the ATA register values are returned
>  	 * whether the command completed successfully or not. If there
>  	 * was no error, we use the following sense data:
> @@ -1641,7 +1641,7 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
>  	 * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
>  	 */
>  	if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
> -	    ((cdb[2] & 0x20) || need_sense))
> +	    ((cdb[2] & 0x20) || (qc->result_tf.status & (ATA_ERR | ATA_DF))))
>  		ata_gen_passthru_sense(qc);
>  	else if (need_sense)
>  		ata_gen_ata_sense(qc);

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v1 3/4] ata: libata-scsi: Report valid sense data for ATA PT if present

[Damien Le Moal]

On 6/15/24 04:18, Igor Pylypiv wrote:
> Do not generate sense data from ATA status/error registers
> if valid sense data is already present.

This kind of contradicts what you said in patch 2... So I am really confused now.
Though this patch actually looks good to me, modulo the comment below.
But shouldn't this be squashed with patch 2 ?

> 
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>  drivers/ata/libata-scsi.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 79e8103ef3a9..4bfe47e7d266 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -858,12 +858,17 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
>  	unsigned char *desc = sb + 8;
>  	u8 sense_key, asc, ascq;
>  
> -	/*
> -	 * Use ata_to_sense_error() to map status register bits
> -	 * onto sense key, asc & ascq.
> -	 */
> -	if (qc->err_mask ||
> -	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
> +	if (qc->flags & ATA_QCFLAG_SENSE_VALID) {
> +		/*
> +		 * Do not generate sense data from ATA status/error
> +		 * registers if valid sense data is already present.
> +		 */

The empty "if" here is really horrible. Please revert the condition and add it
as a "&&" in the below if.

> +	} else if (qc->err_mask ||
> +		   tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
> +		/*
> +		 * Use ata_to_sense_error() to map status register bits
> +		 * onto sense key, asc & ascq.
> +		 */
>  		ata_to_sense_error(qc->ap->print_id, tf->status, tf->error,
>  				   &sense_key, &asc, &ascq);
>  		ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v1 4/4] ata: libata-scsi: Fix offsets for the fixed format sense data

[Damien Le Moal]

On 6/15/24 04:18, Igor Pylypiv wrote:
> Correct the ATA PASS-THROUGH fixed format sense data offsets to conform
> to SPC-6 and SAT-5 specifications. Additionally, set the VALID bit to
> indicate that the INFORMATION field contains valid information.
> 
> INFORMATION
> ===========
> 
> SAT-5 Table 212 — "Fixed format sense data INFORMATION field for the ATA
> PASS-THROUGH commands" defines the following format:
> 
> +------+------------+
> | Byte |   Field    |
> +------+------------+
> |    0 | ERROR      |
> |    1 | STATUS     |
> |    2 | DEVICE     |
> |    3 | COUNT(7:0) |
> +------+------------+
> 
> SPC-6 Table 48 - "Fixed format sense data" specifies that the INFORMATION
> field starts at byte 3 in sense buffer resulting in the following offsets
> for the ATA PASS-THROUGH commands:
> 
> +------------+-------------------------+
> |   Field    |  Offset in sense buffer |
> +------------+-------------------------+
> | ERROR      |  3                      |
> | STATUS     |  4                      |
> | DEVICE     |  5                      |
> | COUNT(7:0) |  6                      |
> +------------+-------------------------+
> 
> COMMAND-SPECIFIC INFORMATION
> ============================
> 
> SAT-5 Table 213 - "Fixed format sense data COMMAND-SPECIFIC INFORMATION
> field for ATA PASS-THROUGH" defines the following format:
> 
> +------+-------------------+
> | Byte |        Field      |
> +------+-------------------+
> |    0 | FLAGS | LOG INDEX |
> |    1 | LBA (7:0)         |
> |    2 | LBA (15:8)        |
> |    3 | LBA (23:16)       |
> +------+-------------------+
> 
> SPC-6 Table 48 - "Fixed format sense data" specifies that
> the COMMAND-SPECIFIC-INFORMATION field starts at byte 8
> in sense buffer resulting in the following offsets for
> the ATA PASS-THROUGH commands:
> 
> Offsets of these fields in the fixed sense format are as follows:
> 
> +-------------------+-------------------------+
> |       Field       |  Offset in sense buffer |
> +-------------------+-------------------------+
> | FLAGS | LOG INDEX |  8                      |
> | LBA (7:0)         |  9                      |
> | LBA (15:8)        |  10                     |
> | LBA (23:16)       |  11                     |
> +-------------------+-------------------------+
> 
> Reported-by: Akshat Jain <akshatzen@google.com>
> Fixes: 11093cb1ef56 ("libata-scsi: generate correct ATA pass-through sense")
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>  drivers/ata/libata-scsi.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 4bfe47e7d266..8588512f5975 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -855,7 +855,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
>  	struct scsi_cmnd *cmd = qc->scsicmd;
>  	struct ata_taskfile *tf = &qc->result_tf;
>  	unsigned char *sb = cmd->sense_buffer;
> -	unsigned char *desc = sb + 8;
>  	u8 sense_key, asc, ascq;
>  
>  	if (qc->flags & ATA_QCFLAG_SENSE_VALID) {
> @@ -880,8 +879,9 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
>  		scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
>  	}
>  
> -	if ((cmd->sense_buffer[0] & 0x7f) >= 0x72) {
> +	if ((sb[0] & 0x7f) >= 0x72) {
>  		u8 len;
> +		unsigned char *desc;

Please move this declaration before the "u8 len" one.
Otherwise, this seems OK but needs a Cc: stable@vger.kernel.org tag added.

>  
>  		/* descriptor format */
>  		len = sb[7];
> @@ -919,21 +919,21 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
>  		}
>  	} else {
>  		/* Fixed sense format */
> -		desc[0] = tf->error;
> -		desc[1] = tf->status;
> -		desc[2] = tf->device;
> -		desc[3] = tf->nsect;
> -		desc[7] = 0;
> +		sb[0] |= 0x80;
> +		sb[3] = tf->error;
> +		sb[4] = tf->status;
> +		sb[5] = tf->device;
> +		sb[6] = tf->nsect;
>  		if (tf->flags & ATA_TFLAG_LBA48)  {
> -			desc[8] |= 0x80;
> +			sb[8] |= 0x80;
>  			if (tf->hob_nsect)
> -				desc[8] |= 0x40;
> +				sb[8] |= 0x40;>  			if (tf->hob_lbal || tf->hob_lbam || tf->hob_lbah)
> -				desc[8] |= 0x20;
> +				sb[8] |= 0x20;
>  		}
> -		desc[9] = tf->lbal;
> -		desc[10] = tf->lbam;
> -		desc[11] = tf->lbah;
> +		sb[9] = tf->lbal;
> +		sb[10] = tf->lbam;
> +		sb[11] = tf->lbah;
>  	}
>  }
>  

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v1 1/4] ata: libata: Remove redundant sense_buffer memsets

[Niklas Cassel]

On Fri, Jun 14, 2024 at 07:18:32PM +0000, Igor Pylypiv wrote:
> scsi_queue_rq() memsets sense_buffer before a command is dispatched.
> 
> Libata is not memsetting sense_buffer before setting sense data that
> was obtained from a disk so there should be no reason to do a memset
> for ATA PASS-THROUGH / ATAPI.
> 
> Memsetting the sense_buffer in ata_gen_passthru_sense() is erasing valid
> sense data that was previously obtained from a disk. A follow-up patch
> will modify ata_gen_passthru_sense() to stop generating sense data based
> on ATA status register bits if a valid sense data is already present.
> 
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>  drivers/ata/libata-eh.c   | 2 --
>  drivers/ata/libata-scsi.c | 4 ----
>  2 files changed, 6 deletions(-)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 214b935c2ced..b5e05efe73f6 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -1479,8 +1479,6 @@ unsigned int atapi_eh_request_sense(struct ata_device *dev,
>  	struct ata_port *ap = dev->link->ap;
>  	struct ata_taskfile tf;
>  
> -	memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE);
> -

Are you sure that this is safe?

atapi_eh_request_sense() is called both by:
ata_eh_analyze_tf():
tmp = atapi_eh_request_sense(.., qc->scsicmd->sense_buffer, ..)

and by:
atapi_eh_clear_ua():
atapi_eh_request_sense(.., sense_buffer, ..);
where sense_buffer is dev->link->ap->sector_buf.


Wouldn't a better fix be for ata_gen_* functions to return early if
ATA_QCFLAG_SENSE_VALID is set?


>  	/* initialize sense_buf with the error register,
>  	 * for the case where they are -not- overwritten
>  	 */
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index cdf29b178ddc..032cf11d0bcc 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -858,8 +858,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
>  	unsigned char *desc = sb + 8;
>  	u8 sense_key, asc, ascq;
>  
> -	memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
> -
>  	/*
>  	 * Use ata_to_sense_error() to map status register bits
>  	 * onto sense key, asc & ascq.
> @@ -953,8 +951,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
>  	u64 block;
>  	u8 sense_key, asc, ascq;
>  
> -	memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
> -
>  	if (ata_dev_disabled(dev)) {
>  		/* Device disabled after error recovery */
>  		/* LOGICAL UNIT NOT READY, HARD RESET REQUIRED */
> -- 
> 2.45.2.627.g7a2c4fd464-goog
> 

Re: [PATCH v1 3/4] ata: libata-scsi: Report valid sense data for ATA PT if present

[Niklas Cassel]

On Fri, Jun 14, 2024 at 07:18:34PM +0000, Igor Pylypiv wrote:
> Do not generate sense data from ATA status/error registers
> if valid sense data is already present.
> 
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>  drivers/ata/libata-scsi.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 79e8103ef3a9..4bfe47e7d266 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -858,12 +858,17 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
>  	unsigned char *desc = sb + 8;
>  	u8 sense_key, asc, ascq;

Like I suggested in the earlier patch,

can't you do a:

if (qc->flags & ATA_QCFLAG_SENSE_VALID)
	return;

here instead?


>  
> -	/*
> -	 * Use ata_to_sense_error() to map status register bits
> -	 * onto sense key, asc & ascq.
> -	 */
> -	if (qc->err_mask ||
> -	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
> +	if (qc->flags & ATA_QCFLAG_SENSE_VALID) {
> +		/*
> +		 * Do not generate sense data from ATA status/error
> +		 * registers if valid sense data is already present.
> +		 */
> +	} else if (qc->err_mask ||
> +		   tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
> +		/*
> +		 * Use ata_to_sense_error() to map status register bits
> +		 * onto sense key, asc & ascq.
> +		 */
>  		ata_to_sense_error(qc->ap->print_id, tf->status, tf->error,
>  				   &sense_key, &asc, &ascq);
>  		ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);
> -- 
> 2.45.2.627.g7a2c4fd464-goog
> 

Re: [PATCH v1 2/4] ata: libata-scsi: Generate ATA PT sense data when ATA ERR/DF are set

[Niklas Cassel]

On Fri, Jun 14, 2024 at 07:18:33PM +0000, Igor Pylypiv wrote:
> SCSI/ATA Translation-5 (SAT-5) Table 209 — "ATA command results"
> specifies that SATL shall generate sense data for ATA PASS-THROUGH
> commands when either CK_COND is set or when ATA_ERR or ATA_DF status
> bits are set.
> 
> ata_eh_analyze_tf() sets AC_ERR_DEV bit in qc->err_mask when ATA_ERR
> or ATA_DF bits are set. It looks like qc->err_mask can be used as
> an error indicator but ata_eh_link_autopsy() clears AC_ERR_DEV bit
> when ATA_QCFLAG_SENSE_VALID is set. This effectively clears the error
> indication if no other bits were set in qc->err_mask.

The reason why libata clears the err_mask when having sense data,
is because the upper layer, i.e. SCSI, should determine what to do
with the command, if there is sense data.

For a non-passthrough command, this will be done by
scsi_io_completion_action():
https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L1084-L1087


However, if there is any bits set in cmd->result,
scsi_io_completion_nz_result() will be called:
https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L1052-L1053

which will do the following for a passthrough command:
https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L969-L978
which will set blk_stat.

After that, scsi_io_completion() which check blk_stat and if it is a
scsi_noretry_cmd():
https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L1073-L1078

A passthrough command will return true for scsi_noretry_cmd(), so
scsi_io_completion_action() should NOT get called for a passthough command.

So IIUC, for a non-passthrough command, scsi_io_completion_action() will
decide what to do depending on the sense data, but a passthrough command will
get finished with the sense data, leaving the user to decide what to do.


> 
> ata_scsi_qc_complete() should not use qc->err_mask for ATA PASS-THROUGH
> commands because qc->err_mask can be zero (i.e. "no error") even when
> the corresponding command has failed with ATA_ERR/ATA_DF bits set.
> 
> Additionally, the presence of valid sense data (ATA_QCFLAG_SENSE_VALID)
> should not prevent SATL from generating sense data for ATA PASS-THROUGH.
> 
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>  drivers/ata/libata-scsi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 032cf11d0bcc..79e8103ef3a9 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1632,8 +1632,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
>  		!(qc->flags & ATA_QCFLAG_SENSE_VALID);
>  
>  	/* For ATA pass thru (SAT) commands, generate a sense block if
> -	 * user mandated it or if there's an error.  Note that if we
> -	 * generate because the user forced us to [CK_COND =1], a check
> +	 * user mandated it or if ATA_ERR or ATA_DF bits are set. Note that
> +	 * if we generate because the user forced us to [CK_COND=1], a check
>  	 * condition is generated and the ATA register values are returned
>  	 * whether the command completed successfully or not. If there
>  	 * was no error, we use the following sense data:
> @@ -1641,7 +1641,7 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
>  	 * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
>  	 */
>  	if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
> -	    ((cdb[2] & 0x20) || need_sense))
> +	    ((cdb[2] & 0x20) || (qc->result_tf.status & (ATA_ERR | ATA_DF))))

qc->result_tf can only be used if qc->flags has ATA_QCFLAG_RESULT_TF set,
otherwise it can contain bogus data.
You don't seem to check if ATA_QCFLAG_RESULT_TF is set.

ata_scsi_pass_thru() does set ATA_QCFLAG_RESULT_TF.


>  		ata_gen_passthru_sense(qc);
>  	else if (need_sense)
>  		ata_gen_ata_sense(qc);
> -- 
> 2.45.2.627.g7a2c4fd464-goog
> 

Re: [PATCH v1 2/4] ata: libata-scsi: Generate ATA PT sense data when ATA ERR/DF are set

[Niklas Cassel]

On Mon, Jun 17, 2024 at 01:29:25PM +0200, Niklas Cassel wrote:
> 
> However, if there is any bits set in cmd->result,
> scsi_io_completion_nz_result() will be called:
> https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L1052-L1053

If forgot to say that, if ATA_ERR and Sense Data Available bit is set:
ata_eh_analyze_tf() will call scsi_check_sense() which will set the
SCSI ML bit for many ASC/ASCQ codes.


If Sense Data Available bit is set, but ATA_ERR is not set:
-For non-NCQ commands, ata_eh_read_sense_success_non_ncq() will call
scsi_check_sense() which will set the SCSI ML bit for many ASC/ASCQ codes.

-For NCQ commands, ata_eh_read_sense_success_ncq_log() will call
scsi_check_sense() which will set the SCSI ML bit for many ASC/ASCQ codes.



The SCSI ML bit is stored in cmd->result, so if scsi_check_sense() did
set the SCSI ML bit, cmd->result will be non-zero, which means that once
scsi_io_completion() is called, it will call scsi_io_completion_nz_result().


Kind regards,
Niklas

Re: [PATCH v1 3/4] ata: libata-scsi: Report valid sense data for ATA PT if present

[Igor Pylypiv]

On Mon, Jun 17, 2024 at 12:49:47PM +0200, Niklas Cassel wrote:
> On Fri, Jun 14, 2024 at 07:18:34PM +0000, Igor Pylypiv wrote:
> > Do not generate sense data from ATA status/error registers
> > if valid sense data is already present.
> > 
> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > ---
> >  drivers/ata/libata-scsi.c | 17 +++++++++++------
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index 79e8103ef3a9..4bfe47e7d266 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -858,12 +858,17 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> >  	unsigned char *desc = sb + 8;
> >  	u8 sense_key, asc, ascq;
> 
> Like I suggested in the earlier patch,
> 
> can't you do a:
> 
> if (qc->flags & ATA_QCFLAG_SENSE_VALID)
> 	return;
> 
> here instead?
> 

We need to populate the "ATA Status Return sense data descriptor" as per SAT-5
"Table 209 — ATA command results". By returning early we are skipping the code
that copies ATA output fields into descriptor/fixed sense data buffer.

> 
> >  
> > -	/*
> > -	 * Use ata_to_sense_error() to map status register bits
> > -	 * onto sense key, asc & ascq.
> > -	 */
> > -	if (qc->err_mask ||
> > -	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
> > +	if (qc->flags & ATA_QCFLAG_SENSE_VALID) {
> > +		/*
> > +		 * Do not generate sense data from ATA status/error
> > +		 * registers if valid sense data is already present.
> > +		 */
> > +	} else if (qc->err_mask ||
> > +		   tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
> > +		/*
> > +		 * Use ata_to_sense_error() to map status register bits
> > +		 * onto sense key, asc & ascq.
> > +		 */
> >  		ata_to_sense_error(qc->ap->print_id, tf->status, tf->error,
> >  				   &sense_key, &asc, &ascq);
> >  		ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);
> > -- 
> > 2.45.2.627.g7a2c4fd464-goog
> > 

Re: [PATCH v1 3/4] ata: libata-scsi: Report valid sense data for ATA PT if present

[Igor Pylypiv]

On Mon, Jun 17, 2024 at 08:25:54AM +0900, Damien Le Moal wrote:
> On 6/15/24 04:18, Igor Pylypiv wrote:
> > Do not generate sense data from ATA status/error registers
> > if valid sense data is already present.
> 
> This kind of contradicts what you said in patch 2... So I am really confused now.

Sorry about the confustion. I think the problem is that I was using "sense data"
to describe two different things:
#1. SK/ASC/ASCQ
#2. ATA Status Return sense data descriptor

Both #1 and #2 need to be populated into sense buffer. The problem with
the current code is that we can only have either valid #1 or valid #2 but
not both at the same time.

> Though this patch actually looks good to me, modulo the comment below.
> But shouldn't this be squashed with patch 2 ?

Yes, that's a good point. Let me factor out the sense data descriptor
population code into a separate function and then squash this patch with
the patch 2.

>
> > 
> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > ---
> >  drivers/ata/libata-scsi.c | 17 +++++++++++------
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index 79e8103ef3a9..4bfe47e7d266 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -858,12 +858,17 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> >  	unsigned char *desc = sb + 8;
> >  	u8 sense_key, asc, ascq;
> >  
> > -	/*
> > -	 * Use ata_to_sense_error() to map status register bits
> > -	 * onto sense key, asc & ascq.
> > -	 */
> > -	if (qc->err_mask ||
> > -	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
> > +	if (qc->flags & ATA_QCFLAG_SENSE_VALID) {
> > +		/*
> > +		 * Do not generate sense data from ATA status/error
> > +		 * registers if valid sense data is already present.
> > +		 */
> 
> The empty "if" here is really horrible. Please revert the condition and add it
> as a "&&" in the below if.
>
Adding the condition to the below if will change the code flow and we'll end
up executing scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D) when
ATA_QCFLAG_SENSE_VALID is set, which is not what we want.

I agree about horrible :)

Perhaps I should have factored out the descriptor population code into
a separate function to make the code correct and not so horrible. Let me
do that in v2.

> > +	} else if (qc->err_mask ||
> > +		   tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
> > +		/*
> > +		 * Use ata_to_sense_error() to map status register bits
> > +		 * onto sense key, asc & ascq.
> > +		 */
> >  		ata_to_sense_error(qc->ap->print_id, tf->status, tf->error,
> >  				   &sense_key, &asc, &ascq);
> >  		ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);
> 
> -- 
> Damien Le Moal
> Western Digital Research
>
Thank you,
Igor 

Re: [PATCH v1 2/4] ata: libata-scsi: Generate ATA PT sense data when ATA ERR/DF are set

[Igor Pylypiv]

On Mon, Jun 17, 2024 at 08:22:07AM +0900, Damien Le Moal wrote:
> On 6/15/24 04:18, Igor Pylypiv wrote:
> > SCSI/ATA Translation-5 (SAT-5) Table 209 — "ATA command results"
> > specifies that SATL shall generate sense data for ATA PASS-THROUGH
> > commands when either CK_COND is set or when ATA_ERR or ATA_DF status
> > bits are set.
> > 
> > ata_eh_analyze_tf() sets AC_ERR_DEV bit in qc->err_mask when ATA_ERR
> > or ATA_DF bits are set. It looks like qc->err_mask can be used as
> > an error indicator but ata_eh_link_autopsy() clears AC_ERR_DEV bit
> > when ATA_QCFLAG_SENSE_VALID is set. This effectively clears the error
> > indication if no other bits were set in qc->err_mask.
> > 
> > ata_scsi_qc_complete() should not use qc->err_mask for ATA PASS-THROUGH
> > commands because qc->err_mask can be zero (i.e. "no error") even when
> > the corresponding command has failed with ATA_ERR/ATA_DF bits set.
> 
> If there was a failed command, qc->err_mask will be non-zero since the command
> completion will be signaled by an error interrupt. So I am confused here. Are
> you seeing this happening in practice ? If yes, doing what with which  adapter ?
>

Yes, this is happening in practice with the PM8006 adapter. ata_eh_analyze_tf()
sets AC_ERR_DEV and later ata_eh_link_autopsy() clears AC_ERR_DEV making
qc->err_mask == 0.

ata_eh_link_autopsy()
  \
   `ata_eh_analyze_tf()
            if (stat & (ATA_ERR | ATA_DF)) {                                        
                    qc->err_mask |= AC_ERR_DEV;     <<< set AC_ERR_DEV                                   
                    /*                                                              
                     * Sense data reporting does not work if the                    
                     * device fault bit is set.                                     
                     */                                                             
                     if (stat & ATA_DF)                                              
                         stat &= ~ATA_SENSE;                                     
    ...

<cont. ata_eh_link_autopsy()>
        /*                                                              
         * SENSE_VALID trumps dev/unknown error and revalidation. Upper 
         * layers will determine whether the command is worth retrying  
         * based on the sense data and device class/type. Otherwise,    
         * determine directly if the command is worth retrying using its
         * error mask and flags.                                        
         */                                                             
        if (qc->flags & ATA_QCFLAG_SENSE_VALID)                         
                qc->err_mask &= ~(AC_ERR_DEV | AC_ERR_OTHER); <<< clear AC_ERR_DEV



> > Additionally, the presence of valid sense data (ATA_QCFLAG_SENSE_VALID)
> > should not prevent SATL from generating sense data for ATA PASS-THROUGH.
> 
> Same here, this is strange: ATA_QCFLAG_SENSE_VALID indicates that we have sense
> data for the command, regardless if the command is passthrough or not. So if
> that flag is set, we should not overwrite the already valid sense data with
> sense data generated from the error and status bits.

Sorry about the confusion. I meant that the "ATA Status Return sense data
descriptor" is not getting populated when ATA_QCFLAG_SENSE_VALID is set
and CK_COND is 0.

What you described is true when CK_COND is 0, however, when CK_COND is 1,
existing code will call ata_gen_passthru_sense() without checking
the "need_sense" value. This will generate fake sk/asc/ascq regardless of
the ATA_QCFLAG_SENSE_VALID flag.

> Do you see an issue without this change ? If yes, what are the adapter and
> operations you are running ?
> 
Yes, we see the issue on PM8006 adapter.

Any failed ATA PASS-THROUGH command with CK_COND=1 has this issue. For example,
Sending READ SECTOR(S) EXT via ATA PASS-THROUGH with CK_COND=1 to an lba
that was previously corrupted by WRITE UNCORRECTABLE EXT is supposed to
return "READ ERROR - LBA MARKED BAD BY APPLICATION CLIENT" but instead it
returns generated "UNRECOVERED READ ERROR - AUTO REALLOCATE FAILED".

> > 
> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > ---
> >  drivers/ata/libata-scsi.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index 032cf11d0bcc..79e8103ef3a9 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -1632,8 +1632,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> >  		!(qc->flags & ATA_QCFLAG_SENSE_VALID);
> >  
> >  	/* For ATA pass thru (SAT) commands, generate a sense block if
> > -	 * user mandated it or if there's an error.  Note that if we
> > -	 * generate because the user forced us to [CK_COND =1], a check
> > +	 * user mandated it or if ATA_ERR or ATA_DF bits are set. Note that
> > +	 * if we generate because the user forced us to [CK_COND=1], a check
> >  	 * condition is generated and the ATA register values are returned
> >  	 * whether the command completed successfully or not. If there
> >  	 * was no error, we use the following sense data:
> > @@ -1641,7 +1641,7 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> >  	 * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
> >  	 */
> >  	if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
> > -	    ((cdb[2] & 0x20) || need_sense))
> > +	    ((cdb[2] & 0x20) || (qc->result_tf.status & (ATA_ERR | ATA_DF))))
> >  		ata_gen_passthru_sense(qc);
> >  	else if (need_sense)
> >  		ata_gen_ata_sense(qc);
> 
> -- 
> Damien Le Moal
> Western Digital Research
>

Thank you,
Igor 

Re: [PATCH v1 4/4] ata: libata-scsi: Fix offsets for the fixed format sense data

[Igor Pylypiv]

On Mon, Jun 17, 2024 at 08:37:09AM +0900, Damien Le Moal wrote:
> On 6/15/24 04:18, Igor Pylypiv wrote:
> > Correct the ATA PASS-THROUGH fixed format sense data offsets to conform
> > to SPC-6 and SAT-5 specifications. Additionally, set the VALID bit to
> > indicate that the INFORMATION field contains valid information.
> > 
> > INFORMATION
> > ===========
> > 
> > SAT-5 Table 212 — "Fixed format sense data INFORMATION field for the ATA
> > PASS-THROUGH commands" defines the following format:
> > 
> > +------+------------+
> > | Byte |   Field    |
> > +------+------------+
> > |    0 | ERROR      |
> > |    1 | STATUS     |
> > |    2 | DEVICE     |
> > |    3 | COUNT(7:0) |
> > +------+------------+
> > 
> > SPC-6 Table 48 - "Fixed format sense data" specifies that the INFORMATION
> > field starts at byte 3 in sense buffer resulting in the following offsets
> > for the ATA PASS-THROUGH commands:
> > 
> > +------------+-------------------------+
> > |   Field    |  Offset in sense buffer |
> > +------------+-------------------------+
> > | ERROR      |  3                      |
> > | STATUS     |  4                      |
> > | DEVICE     |  5                      |
> > | COUNT(7:0) |  6                      |
> > +------------+-------------------------+
> > 
> > COMMAND-SPECIFIC INFORMATION
> > ============================
> > 
> > SAT-5 Table 213 - "Fixed format sense data COMMAND-SPECIFIC INFORMATION
> > field for ATA PASS-THROUGH" defines the following format:
> > 
> > +------+-------------------+
> > | Byte |        Field      |
> > +------+-------------------+
> > |    0 | FLAGS | LOG INDEX |
> > |    1 | LBA (7:0)         |
> > |    2 | LBA (15:8)        |
> > |    3 | LBA (23:16)       |
> > +------+-------------------+
> > 
> > SPC-6 Table 48 - "Fixed format sense data" specifies that
> > the COMMAND-SPECIFIC-INFORMATION field starts at byte 8
> > in sense buffer resulting in the following offsets for
> > the ATA PASS-THROUGH commands:
> > 
> > Offsets of these fields in the fixed sense format are as follows:
> > 
> > +-------------------+-------------------------+
> > |       Field       |  Offset in sense buffer |
> > +-------------------+-------------------------+
> > | FLAGS | LOG INDEX |  8                      |
> > | LBA (7:0)         |  9                      |
> > | LBA (15:8)        |  10                     |
> > | LBA (23:16)       |  11                     |
> > +-------------------+-------------------------+
> > 
> > Reported-by: Akshat Jain <akshatzen@google.com>
> > Fixes: 11093cb1ef56 ("libata-scsi: generate correct ATA pass-through sense")
> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > ---
> >  drivers/ata/libata-scsi.c | 26 +++++++++++++-------------
> >  1 file changed, 13 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index 4bfe47e7d266..8588512f5975 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -855,7 +855,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> >  	struct scsi_cmnd *cmd = qc->scsicmd;
> >  	struct ata_taskfile *tf = &qc->result_tf;
> >  	unsigned char *sb = cmd->sense_buffer;
> > -	unsigned char *desc = sb + 8;
> >  	u8 sense_key, asc, ascq;
> >  
> >  	if (qc->flags & ATA_QCFLAG_SENSE_VALID) {
> > @@ -880,8 +879,9 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> >  		scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
> >  	}
> >  
> > -	if ((cmd->sense_buffer[0] & 0x7f) >= 0x72) {
> > +	if ((sb[0] & 0x7f) >= 0x72) {
> >  		u8 len;
> > +		unsigned char *desc;
> 
> Please move this declaration before the "u8 len" one.

Will do in v2. Could you please explain why this declaration order is preferred?

> Otherwise, this seems OK but needs a Cc: stable@vger.kernel.org tag added.

Ack, will add in v2.
> 
> >  
> >  		/* descriptor format */
> >  		len = sb[7];
> > @@ -919,21 +919,21 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> >  		}
> >  	} else {
> >  		/* Fixed sense format */
> > -		desc[0] = tf->error;
> > -		desc[1] = tf->status;
> > -		desc[2] = tf->device;
> > -		desc[3] = tf->nsect;
> > -		desc[7] = 0;
> > +		sb[0] |= 0x80;
> > +		sb[3] = tf->error;
> > +		sb[4] = tf->status;
> > +		sb[5] = tf->device;
> > +		sb[6] = tf->nsect;
> >  		if (tf->flags & ATA_TFLAG_LBA48)  {
> > -			desc[8] |= 0x80;
> > +			sb[8] |= 0x80;
> >  			if (tf->hob_nsect)
> > -				desc[8] |= 0x40;
> > +				sb[8] |= 0x40;>  			if (tf->hob_lbal || tf->hob_lbam || tf->hob_lbah)
> > -				desc[8] |= 0x20;
> > +				sb[8] |= 0x20;
> >  		}
> > -		desc[9] = tf->lbal;
> > -		desc[10] = tf->lbam;
> > -		desc[11] = tf->lbah;
> > +		sb[9] = tf->lbal;
> > +		sb[10] = tf->lbam;
> > +		sb[11] = tf->lbah;
> >  	}
> >  }
> >  
> 
> -- 
> Damien Le Moal
> Western Digital Research
>

Thank you,
Igor 

Re: [PATCH v1 3/4] ata: libata-scsi: Report valid sense data for ATA PT if present

[Damien Le Moal]

On 6/18/24 09:02, Igor Pylypiv wrote:
> On Mon, Jun 17, 2024 at 08:25:54AM +0900, Damien Le Moal wrote:
>> On 6/15/24 04:18, Igor Pylypiv wrote:
>>> Do not generate sense data from ATA status/error registers
>>> if valid sense data is already present.
>>
>> This kind of contradicts what you said in patch 2... So I am really confused now.
> 
> Sorry about the confustion. I think the problem is that I was using "sense data"
> to describe two different things:
> #1. SK/ASC/ASCQ
> #2. ATA Status Return sense data descriptor
> 
> Both #1 and #2 need to be populated into sense buffer. The problem with
> the current code is that we can only have either valid #1 or valid #2 but
> not both at the same time.
> 
>> Though this patch actually looks good to me, modulo the comment below.
>> But shouldn't this be squashed with patch 2 ?
> 
> Yes, that's a good point. Let me factor out the sense data descriptor
> population code into a separate function and then squash this patch with
> the patch 2.
> 
>>
>>>
>>> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
>>> ---
>>>  drivers/ata/libata-scsi.c | 17 +++++++++++------
>>>  1 file changed, 11 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>>> index 79e8103ef3a9..4bfe47e7d266 100644
>>> --- a/drivers/ata/libata-scsi.c
>>> +++ b/drivers/ata/libata-scsi.c
>>> @@ -858,12 +858,17 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
>>>  	unsigned char *desc = sb + 8;
>>>  	u8 sense_key, asc, ascq;
>>>  
>>> -	/*
>>> -	 * Use ata_to_sense_error() to map status register bits
>>> -	 * onto sense key, asc & ascq.
>>> -	 */
>>> -	if (qc->err_mask ||
>>> -	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
>>> +	if (qc->flags & ATA_QCFLAG_SENSE_VALID) {
>>> +		/*
>>> +		 * Do not generate sense data from ATA status/error
>>> +		 * registers if valid sense data is already present.
>>> +		 */
>>
>> The empty "if" here is really horrible. Please revert the condition and add it
>> as a "&&" in the below if.
>>
> Adding the condition to the below if will change the code flow and we'll end
> up executing scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D) when
> ATA_QCFLAG_SENSE_VALID is set, which is not what we want.

I did say "reverse the condition" :)
So that if would be done only if ATA_QCFLAG_SENSE_VALID is *not* set.

> 
> I agree about horrible :)
> 
> Perhaps I should have factored out the descriptor population code into
> a separate function to make the code correct and not so horrible. Let me
> do that in v2.
> 
>>> +	} else if (qc->err_mask ||
>>> +		   tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
>>> +		/*
>>> +		 * Use ata_to_sense_error() to map status register bits
>>> +		 * onto sense key, asc & ascq.
>>> +		 */
>>>  		ata_to_sense_error(qc->ap->print_id, tf->status, tf->error,
>>>  				   &sense_key, &asc, &ascq);
>>>  		ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);
>>
>> -- 
>> Damien Le Moal
>> Western Digital Research
>>
> Thank you,
> Igor 
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v1 1/4] ata: libata: Remove redundant sense_buffer memsets

[Igor Pylypiv]

On Mon, Jun 17, 2024 at 08:13:51AM +0900, Damien Le Moal wrote:
> On 6/15/24 04:18, Igor Pylypiv wrote:
> > scsi_queue_rq() memsets sense_buffer before a command is dispatched.
> > 
> > Libata is not memsetting sense_buffer before setting sense data that
> > was obtained from a disk so there should be no reason to do a memset
> > for ATA PASS-THROUGH / ATAPI.
> 
> This sentence is not very clear at all... I assume that the first part of the
> sentence is for non passthrough commands. In this case, libata does not clear
> the sense buffer because the scsi layer did that already, in scsi_queue_rq() as
> noted above.
> 
> For passthrough commands, the same should be happening as well since passthrough
> commands are also executed through blk_execute_rq() -> scsi_queue_rq(). So I do
> not really understand (but I do agree that the memset() in libata seem useless).

Thanks! I'll update the commit message to make it more clear.

> 
> > Memsetting the sense_buffer in ata_gen_passthru_sense() is erasing valid
> > sense data that was previously obtained from a disk. A follow-up patch
> > will modify ata_gen_passthru_sense() to stop generating sense data based
> > on ATA status register bits if a valid sense data is already present.
> 
> This fix should come first in the series, since that commit will likely need to
> go into current rc and cc-stable. And that will simplify this patch as well.
>
Ack. I'll reorder the commits.

> > 
> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > ---
> >  drivers/ata/libata-eh.c   | 2 --
> >  drivers/ata/libata-scsi.c | 4 ----
> >  2 files changed, 6 deletions(-)
> > 
> > diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> > index 214b935c2ced..b5e05efe73f6 100644
> > --- a/drivers/ata/libata-eh.c
> > +++ b/drivers/ata/libata-eh.c
> > @@ -1479,8 +1479,6 @@ unsigned int atapi_eh_request_sense(struct ata_device *dev,
> >  	struct ata_port *ap = dev->link->ap;
> >  	struct ata_taskfile tf;
> >  
> > -	memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE);
> > -
> >  	/* initialize sense_buf with the error register,
> >  	 * for the case where they are -not- overwritten
> >  	 */
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index cdf29b178ddc..032cf11d0bcc 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -858,8 +858,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> >  	unsigned char *desc = sb + 8;
> >  	u8 sense_key, asc, ascq;
> >  
> > -	memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
> > -
> >  	/*
> >  	 * Use ata_to_sense_error() to map status register bits
> >  	 * onto sense key, asc & ascq.
> > @@ -953,8 +951,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
> >  	u64 block;
> >  	u8 sense_key, asc, ascq;
> >  
> > -	memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
> > -
> >  	if (ata_dev_disabled(dev)) {
> >  		/* Device disabled after error recovery */
> >  		/* LOGICAL UNIT NOT READY, HARD RESET REQUIRED */
> 
> -- 
> Damien Le Moal
> Western Digital Research
> 

Thank you,
Igor

Re: [PATCH v1 1/4] ata: libata: Remove redundant sense_buffer memsets

[Igor Pylypiv]

On Mon, Jun 17, 2024 at 12:41:26PM +0200, Niklas Cassel wrote:
> On Fri, Jun 14, 2024 at 07:18:32PM +0000, Igor Pylypiv wrote:
> > scsi_queue_rq() memsets sense_buffer before a command is dispatched.
> > 
> > Libata is not memsetting sense_buffer before setting sense data that
> > was obtained from a disk so there should be no reason to do a memset
> > for ATA PASS-THROUGH / ATAPI.
> > 
> > Memsetting the sense_buffer in ata_gen_passthru_sense() is erasing valid
> > sense data that was previously obtained from a disk. A follow-up patch
> > will modify ata_gen_passthru_sense() to stop generating sense data based
> > on ATA status register bits if a valid sense data is already present.
> > 
> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > ---
> >  drivers/ata/libata-eh.c   | 2 --
> >  drivers/ata/libata-scsi.c | 4 ----
> >  2 files changed, 6 deletions(-)
> > 
> > diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> > index 214b935c2ced..b5e05efe73f6 100644
> > --- a/drivers/ata/libata-eh.c
> > +++ b/drivers/ata/libata-eh.c
> > @@ -1479,8 +1479,6 @@ unsigned int atapi_eh_request_sense(struct ata_device *dev,
> >  	struct ata_port *ap = dev->link->ap;
> >  	struct ata_taskfile tf;
> >  
> > -	memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE);
> > -
> 
> Are you sure that this is safe?
> 
> atapi_eh_request_sense() is called both by:
> ata_eh_analyze_tf():
> tmp = atapi_eh_request_sense(.., qc->scsicmd->sense_buffer, ..)
> 
> and by:
> atapi_eh_clear_ua():
> atapi_eh_request_sense(.., sense_buffer, ..);
> where sense_buffer is dev->link->ap->sector_buf.
> 

Thanks for pointing this out, Niklas!

ata_eh_analyze_tf() case is safe because qc->scsicmd->sense_buffer is cleared
by scsi_queue_rq().

atapi_eh_clear_ua() case is safe right now because the sense buffer contents
are not being used. However, someone might start using the sense data in
the future so it is not safe to leave it as-is.

There's one more place where this function is being called:

zpready():
atapi_eh_request_sense(..., sense_buf, ...);
where sense_buffer is dev->link->ap->sector_buf.

This one is actually using the obtained sense buffer so it would be
a nasty bug if we don't do a memset().

I think we should explicitly memset buffers before passing them to
atapi_eh_request_sense() in atapi_eh_clear_ua() and zpready() so that
atapi_eh_request_sense() can have the same behavior as ata_eh_request_sense()
with regards to sense buffer expectations i.e. both functions will expect
buffers that are already memeset to zero.

> 
> Wouldn't a better fix be for ata_gen_* functions to return early if
> ATA_QCFLAG_SENSE_VALID is set?
> 

It would be possible to return early if ATA_QCFLAG_SENSE_VALID is set once
we factor out "ATA Status Return sense data descriptor" population out of
ata_gen_passthru_sense() into a separate function. I'll factor out the
descriptor population code in v2.

I think that it is still benefitial to remove the redundant memset() from
the ata_eh_analyze_tf() -> atapi_eh_request_sense() path?

> 
> >  	/* initialize sense_buf with the error register,
> >  	 * for the case where they are -not- overwritten
> >  	 */
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index cdf29b178ddc..032cf11d0bcc 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -858,8 +858,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> >  	unsigned char *desc = sb + 8;
> >  	u8 sense_key, asc, ascq;
> >  
> > -	memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
> > -
> >  	/*
> >  	 * Use ata_to_sense_error() to map status register bits
> >  	 * onto sense key, asc & ascq.
> > @@ -953,8 +951,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
> >  	u64 block;
> >  	u8 sense_key, asc, ascq;
> >  
> > -	memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
> > -
> >  	if (ata_dev_disabled(dev)) {
> >  		/* Device disabled after error recovery */
> >  		/* LOGICAL UNIT NOT READY, HARD RESET REQUIRED */
> > -- 
> > 2.45.2.627.g7a2c4fd464-goog
> >

Thank you,
Igor 

Re: [PATCH v1 2/4] ata: libata-scsi: Generate ATA PT sense data when ATA ERR/DF are set

[Igor Pylypiv]

On Mon, Jun 17, 2024 at 01:29:25PM +0200, Niklas Cassel wrote:
> On Fri, Jun 14, 2024 at 07:18:33PM +0000, Igor Pylypiv wrote:
> > SCSI/ATA Translation-5 (SAT-5) Table 209 — "ATA command results"
> > specifies that SATL shall generate sense data for ATA PASS-THROUGH
> > commands when either CK_COND is set or when ATA_ERR or ATA_DF status
> > bits are set.
> > 
> > ata_eh_analyze_tf() sets AC_ERR_DEV bit in qc->err_mask when ATA_ERR
> > or ATA_DF bits are set. It looks like qc->err_mask can be used as
> > an error indicator but ata_eh_link_autopsy() clears AC_ERR_DEV bit
> > when ATA_QCFLAG_SENSE_VALID is set. This effectively clears the error
> > indication if no other bits were set in qc->err_mask.
> 
> The reason why libata clears the err_mask when having sense data,
> is because the upper layer, i.e. SCSI, should determine what to do
> with the command, if there is sense data.
> 
> For a non-passthrough command, this will be done by
> scsi_io_completion_action():
> https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L1084-L1087
> 
> 
> However, if there is any bits set in cmd->result,
> scsi_io_completion_nz_result() will be called:
> https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L1052-L1053
> 
> which will do the following for a passthrough command:
> https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L969-L978
> which will set blk_stat.
> 
> After that, scsi_io_completion() which check blk_stat and if it is a
> scsi_noretry_cmd():
> https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L1073-L1078
> 
> A passthrough command will return true for scsi_noretry_cmd(), so
> scsi_io_completion_action() should NOT get called for a passthough command.
> 
> So IIUC, for a non-passthrough command, scsi_io_completion_action() will
> decide what to do depending on the sense data, but a passthrough command will
> get finished with the sense data, leaving the user to decide what to do.
>

Thank you for the detailed explanation, Niklas!
I was looking at a related logic in ata_eh_link_report():
https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-eh.c#L2359-L2360
 
Is my understanding correct that if we have ATA_QCFLAG_SENSE_VALID set and 
qc->err_mask is zero then we don't want to report the error to user since
SCSI might decide that it is not an error based on the sense data?

> 
> > 
> > ata_scsi_qc_complete() should not use qc->err_mask for ATA PASS-THROUGH
> > commands because qc->err_mask can be zero (i.e. "no error") even when
> > the corresponding command has failed with ATA_ERR/ATA_DF bits set.
> > 
> > Additionally, the presence of valid sense data (ATA_QCFLAG_SENSE_VALID)
> > should not prevent SATL from generating sense data for ATA PASS-THROUGH.
> > 
> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > ---
> >  drivers/ata/libata-scsi.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index 032cf11d0bcc..79e8103ef3a9 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -1632,8 +1632,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> >  		!(qc->flags & ATA_QCFLAG_SENSE_VALID);
> >  
> >  	/* For ATA pass thru (SAT) commands, generate a sense block if
> > -	 * user mandated it or if there's an error.  Note that if we
> > -	 * generate because the user forced us to [CK_COND =1], a check
> > +	 * user mandated it or if ATA_ERR or ATA_DF bits are set. Note that
> > +	 * if we generate because the user forced us to [CK_COND=1], a check
> >  	 * condition is generated and the ATA register values are returned
> >  	 * whether the command completed successfully or not. If there
> >  	 * was no error, we use the following sense data:
> > @@ -1641,7 +1641,7 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> >  	 * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
> >  	 */
> >  	if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
> > -	    ((cdb[2] & 0x20) || need_sense))
> > +	    ((cdb[2] & 0x20) || (qc->result_tf.status & (ATA_ERR | ATA_DF))))
> 
> qc->result_tf can only be used if qc->flags has ATA_QCFLAG_RESULT_TF set,
> otherwise it can contain bogus data.
> You don't seem to check if ATA_QCFLAG_RESULT_TF is set.
>
> ata_scsi_pass_thru() does set ATA_QCFLAG_RESULT_TF.
> 

Thanks for pointing this out! Looks like ATA PASS-TRHOUGH case is fine
since the flag is always set by ata_scsi_pass_thru() as you pointed out.
Do we still want to add the check even though we know that it is always
set by ata_scsi_pass_thru()?

If the answer is "yes", I wonder if we should use the ATA_QCFLAG_RTF_FILLED
flag instead? Currently it is used for ahci only but looks like it can be
expanded to other drivers. inic_qc_fill_rtf() will benefit from this change
because it is not always setting the status/error values:
https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/sata_inic162x.c#L583-L586

For the non passthough case qc->result_tf in ata_gen_ata_sense() is also valid
because fill_result_tf() is being called for failed commands regardless of
the ATA_QCFLAG_RESULT_TF flag:
https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-core.c#L4856-L4873

In this case using ATA_QCFLAG_RTF_FILLED will be more accurate because
fill_result_tf() is being called even when ATA_QCFLAG_RESULT_TF is not set.

With that said I'm not sure if it makes sense to update all of the ATA
error handling to start checking for the ATA_QCFLAG_RTF_FILLED flag.

What are your thoughts on this?

> 
> >  		ata_gen_passthru_sense(qc);
> >  	else if (need_sense)
> >  		ata_gen_ata_sense(qc);
> > -- 
> > 2.45.2.627.g7a2c4fd464-goog
> > 

Thank you,
Igor

Re: [PATCH v1 1/4] ata: libata: Remove redundant sense_buffer memsets

[Niklas Cassel]

On Tue, Jun 18, 2024 at 07:58:56PM +0000, Igor Pylypiv wrote:
> 
> I think we should explicitly memset buffers before passing them to
> atapi_eh_request_sense() in atapi_eh_clear_ua() and zpready() so that
> atapi_eh_request_sense() can have the same behavior as ata_eh_request_sense()
> with regards to sense buffer expectations i.e. both functions will expect
> buffers that are already memeset to zero.

Well, you could argue that:
static bool ata_eh_request_sense(struct ata_queued_cmd *qc)
doesn't take a sense_buffer, but:

unsigned int atapi_eh_request_sense(struct ata_device *dev,
                                    u8 *sense_buf, u8 dfl_sense_key)

does, so it makes sense for atapi_eh_request_sense() to memset() the buffer.


> I think that it is still benefitial to remove the redundant memset() from
> the ata_eh_analyze_tf() -> atapi_eh_request_sense() path?

atapi_eh_request_sense() should only be called when ATA_SENSE bit is set,
so this is only called in special circumstances, so it is not like the
memset() is in the hot path.

If you ask me, I think that the current code is fine.


Kind regards,
Niklas

Re: [PATCH v1 2/4] ata: libata-scsi: Generate ATA PT sense data when ATA ERR/DF are set

[Niklas Cassel]

On Tue, Jun 18, 2024 at 09:51:50PM +0000, Igor Pylypiv wrote:
> On Mon, Jun 17, 2024 at 01:29:25PM +0200, Niklas Cassel wrote:
> > On Fri, Jun 14, 2024 at 07:18:33PM +0000, Igor Pylypiv wrote:
> > > SCSI/ATA Translation-5 (SAT-5) Table 209 — "ATA command results"
> > > specifies that SATL shall generate sense data for ATA PASS-THROUGH
> > > commands when either CK_COND is set or when ATA_ERR or ATA_DF status
> > > bits are set.
> > > 
> > > ata_eh_analyze_tf() sets AC_ERR_DEV bit in qc->err_mask when ATA_ERR
> > > or ATA_DF bits are set. It looks like qc->err_mask can be used as
> > > an error indicator but ata_eh_link_autopsy() clears AC_ERR_DEV bit
> > > when ATA_QCFLAG_SENSE_VALID is set. This effectively clears the error
> > > indication if no other bits were set in qc->err_mask.
> > 
> > The reason why libata clears the err_mask when having sense data,
> > is because the upper layer, i.e. SCSI, should determine what to do
> > with the command, if there is sense data.
> > 
> > For a non-passthrough command, this will be done by
> > scsi_io_completion_action():
> > https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L1084-L1087
> > 
> > 
> > However, if there is any bits set in cmd->result,
> > scsi_io_completion_nz_result() will be called:
> > https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L1052-L1053
> > 
> > which will do the following for a passthrough command:
> > https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L969-L978
> > which will set blk_stat.
> > 
> > After that, scsi_io_completion() which check blk_stat and if it is a
> > scsi_noretry_cmd():
> > https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L1073-L1078
> > 
> > A passthrough command will return true for scsi_noretry_cmd(), so
> > scsi_io_completion_action() should NOT get called for a passthough command.
> > 
> > So IIUC, for a non-passthrough command, scsi_io_completion_action() will
> > decide what to do depending on the sense data, but a passthrough command will
> > get finished with the sense data, leaving the user to decide what to do.
> >
> 
> Thank you for the detailed explanation, Niklas!
> I was looking at a related logic in ata_eh_link_report():
> https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-eh.c#L2359-L2360
>  
> Is my understanding correct that if we have ATA_QCFLAG_SENSE_VALID set and 
> qc->err_mask is zero then we don't want to report the error to user since
> SCSI might decide that it is not an error based on the sense data?

I'm assuming that that was the reasoning.

However, IIUC, passthrough commands should never be retried by SCSI,
it should always be reported back to the user.


> 
> > 
> > > 
> > > ata_scsi_qc_complete() should not use qc->err_mask for ATA PASS-THROUGH
> > > commands because qc->err_mask can be zero (i.e. "no error") even when
> > > the corresponding command has failed with ATA_ERR/ATA_DF bits set.
> > > 
> > > Additionally, the presence of valid sense data (ATA_QCFLAG_SENSE_VALID)
> > > should not prevent SATL from generating sense data for ATA PASS-THROUGH.
> > > 
> > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > > ---
> > >  drivers/ata/libata-scsi.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > > index 032cf11d0bcc..79e8103ef3a9 100644
> > > --- a/drivers/ata/libata-scsi.c
> > > +++ b/drivers/ata/libata-scsi.c
> > > @@ -1632,8 +1632,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> > >  		!(qc->flags & ATA_QCFLAG_SENSE_VALID);
> > >  
> > >  	/* For ATA pass thru (SAT) commands, generate a sense block if
> > > -	 * user mandated it or if there's an error.  Note that if we
> > > -	 * generate because the user forced us to [CK_COND =1], a check
> > > +	 * user mandated it or if ATA_ERR or ATA_DF bits are set. Note that
> > > +	 * if we generate because the user forced us to [CK_COND=1], a check
> > >  	 * condition is generated and the ATA register values are returned
> > >  	 * whether the command completed successfully or not. If there
> > >  	 * was no error, we use the following sense data:
> > > @@ -1641,7 +1641,7 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> > >  	 * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
> > >  	 */
> > >  	if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
> > > -	    ((cdb[2] & 0x20) || need_sense))
> > > +	    ((cdb[2] & 0x20) || (qc->result_tf.status & (ATA_ERR | ATA_DF))))
> > 
> > qc->result_tf can only be used if qc->flags has ATA_QCFLAG_RESULT_TF set,
> > otherwise it can contain bogus data.
> > You don't seem to check if ATA_QCFLAG_RESULT_TF is set.
> >
> > ata_scsi_pass_thru() does set ATA_QCFLAG_RESULT_TF.
> > 
> 
> Thanks for pointing this out! Looks like ATA PASS-TRHOUGH case is fine
> since the flag is always set by ata_scsi_pass_thru() as you pointed out.
> Do we still want to add the check even though we know that it is always
> set by ata_scsi_pass_thru()?
> 
> If the answer is "yes", I wonder if we should use the ATA_QCFLAG_RTF_FILLED
> flag instead? Currently it is used for ahci only but looks like it can be
> expanded to other drivers. inic_qc_fill_rtf() will benefit from this change
> because it is not always setting the status/error values:
> https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/sata_inic162x.c#L583-L586
> 
> For the non passthough case qc->result_tf in ata_gen_ata_sense() is also valid
> because fill_result_tf() is being called for failed commands regardless of
> the ATA_QCFLAG_RESULT_TF flag:
> https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-core.c#L4856-L4873
> 
> In this case using ATA_QCFLAG_RTF_FILLED will be more accurate because
> fill_result_tf() is being called even when ATA_QCFLAG_RESULT_TF is not set.
> 
> With that said I'm not sure if it makes sense to update all of the ATA
> error handling to start checking for the ATA_QCFLAG_RTF_FILLED flag.
> 
> What are your thoughts on this?

I see your point, we will fill the result if there is an error,
even if ATA_QCFLAG_RESULT_TF wasn't set.

Perhaps we should modify fill_result_tf() to set ATA_QCFLAG_RTF_FILLED,
after it has called ap->ops->qc_fill_rtf(qc);

Then this code can check if ATA_QCFLAG_RTF_FILLED is set, like you suggested.


Kind regards,
Niklas

Re: [PATCH v1 2/4] ata: libata-scsi: Generate ATA PT sense data when ATA ERR/DF are set

[Niklas Cassel]

On Tue, Jun 18, 2024 at 01:13:24AM +0000, Igor Pylypiv wrote:
> 
> Any failed ATA PASS-THROUGH command with CK_COND=1 has this issue. For example,
> Sending READ SECTOR(S) EXT via ATA PASS-THROUGH with CK_COND=1 to an lba
> that was previously corrupted by WRITE UNCORRECTABLE EXT is supposed to
> return "READ ERROR - LBA MARKED BAD BY APPLICATION CLIENT" but instead it
> returns generated "UNRECOVERED READ ERROR - AUTO REALLOCATE FAILED".

I assume that the drive generated correct sense data, but that
ata_gen_passthru_sense() overwrites/overwrote the sense data with
generated sense data based on taskfile registers?


Kind regards,
Niklas

Re: [PATCH v1 3/4] ata: libata-scsi: Report valid sense data for ATA PT if present

[Niklas Cassel]

On Mon, Jun 17, 2024 at 11:31:49PM +0000, Igor Pylypiv wrote:
> On Mon, Jun 17, 2024 at 12:49:47PM +0200, Niklas Cassel wrote:
> > On Fri, Jun 14, 2024 at 07:18:34PM +0000, Igor Pylypiv wrote:
> > > Do not generate sense data from ATA status/error registers
> > > if valid sense data is already present.
> > > 
> > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > > ---
> > >  drivers/ata/libata-scsi.c | 17 +++++++++++------
> > >  1 file changed, 11 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > > index 79e8103ef3a9..4bfe47e7d266 100644
> > > --- a/drivers/ata/libata-scsi.c
> > > +++ b/drivers/ata/libata-scsi.c
> > > @@ -858,12 +858,17 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> > >  	unsigned char *desc = sb + 8;
> > >  	u8 sense_key, asc, ascq;
> > 
> > Like I suggested in the earlier patch,
> > 
> > can't you do a:
> > 
> > if (qc->flags & ATA_QCFLAG_SENSE_VALID)
> > 	return;
> > 
> > here instead?
> > 
> 
> We need to populate the "ATA Status Return sense data descriptor" as per SAT-5
> "Table 209 — ATA command results". By returning early we are skipping the code
> that copies ATA output fields into descriptor/fixed sense data buffer.

We might get sense data from the drive.
If we use REQUEST SENSE DATA EXT, we will get SK/ASC/ASCQ,
we will then call scsi_build_sense_buffer() which will generate
sense data in either the descriptor format or the fixed format,
based on the D_SENSE bit, which is set in the control mode page.

The user can toggle this bit, see:
https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-scsi.c#L3691-L3694

But by default it is 0:
https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-scsi.c#L86
which means fixed format.

This all seems to be in accordance with
"Table 209 — Returned sense data with the CK_COND bit set to one"
in sat6r1.



When calling scsi_build_sense_buffer(), we supply:
scsi_build_sense_buffer(dev->flags & ATA_DFLAG_D_SENSE,
                        cmd->sense_buffer, tf.lbah,
                        tf.lbam, tf.lbal);

so we do not supply the STATUS and ERROR fields when building the sense data.

This seems fine, since SCSI has no knowledge of ATA status or ATA error.


However, for ATA-passthrough commands, ATA status and ATA error fields
are part of either the COMMAND-SPECIFIC information in the fixed format
case, or part for the descriptor format, in case of the descriptor type
being ATA Status Return sense data descriptor.


So what I think we should do:
1) Keep the sense data if it exists, and fill in
   the ATA status and ATA error at the correct offset (depending on if
   the existing sense data is in fixed format or descriptor format).
2) If there doesn't exist any sense data, generate it, including the
   ATA passthru command specific fields (ATA status and ATA error).
   This is basically what ata_gen_passthru_sense() does today.


So something like this:
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index bb4d30d377ae..a0687eb28ff7 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1645,9 +1645,17 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
         * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
         */
        if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
-           ((cdb[2] & 0x20) || need_sense))
-               ata_gen_passthru_sense(qc);
-       else if (need_sense)
+           ((cdb[2] & 0x20) || need_sense)) {
+               if ((qc->flags & ATA_QCFLAG_SENSE_VALID)) {
+                       /*
+                        * ATA PASS-THROUGH commands also need to fill in the
+                        * command specific ATA status and ATA error fields.
+                        */
+                       ata_fill_passthru_specific_sense_fields(qc);
+               } else {
+                       ata_gen_passthru_sense(qc);
+               }
+       } else if (need_sense)
                ata_gen_ata_sense(qc);
        else
                /* Keep the SCSI ML and status byte, clear host byte. */


Kind regards,
Niklas

Re: [PATCH v1 3/4] ata: libata-scsi: Report valid sense data for ATA PT if present

[Niklas Cassel]

On Thu, Jun 20, 2024 at 04:24:11PM +0200, Niklas Cassel wrote:
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index bb4d30d377ae..a0687eb28ff7 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1645,9 +1645,17 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
>          * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
>          */
>         if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
> -           ((cdb[2] & 0x20) || need_sense))

Hmm... I just noticed that we don't seem to have support for ATA_32 commands.

We should probably add support for that sometime in the future.

(I don't think that it makes sense to do it as part of this series.)


Kind regards,
Niklas

Re: [PATCH v1 1/4] ata: libata: Remove redundant sense_buffer memsets

[Igor Pylypiv]

On Thu, Jun 20, 2024 at 01:51:26PM +0200, Niklas Cassel wrote:
> On Tue, Jun 18, 2024 at 07:58:56PM +0000, Igor Pylypiv wrote:
> > 
> > I think we should explicitly memset buffers before passing them to
> > atapi_eh_request_sense() in atapi_eh_clear_ua() and zpready() so that
> > atapi_eh_request_sense() can have the same behavior as ata_eh_request_sense()
> > with regards to sense buffer expectations i.e. both functions will expect
> > buffers that are already memeset to zero.
> 
> Well, you could argue that:
> static bool ata_eh_request_sense(struct ata_queued_cmd *qc)
> doesn't take a sense_buffer, but:
> 
> unsigned int atapi_eh_request_sense(struct ata_device *dev,
>                                     u8 *sense_buf, u8 dfl_sense_key)
> 
> does, so it makes sense for atapi_eh_request_sense() to memset() the buffer.
> 
> 
> > I think that it is still benefitial to remove the redundant memset() from
> > the ata_eh_analyze_tf() -> atapi_eh_request_sense() path?
> 
> atapi_eh_request_sense() should only be called when ATA_SENSE bit is set,
> so this is only called in special circumstances, so it is not like the
> memset() is in the hot path.
> 
> If you ask me, I think that the current code is fine.
> 

I didn't think about the "takes a sense_buffer as an argument" vs "doesn't take
a sense_buffer as an argument" aspect. Yeah, keeping memset() makes sense in
this case. I'll drop the memset removal from atapi_eh_request_sense() in v2.

Thank you!
Igor

> 
> Kind regards,
> Niklas

Re: [PATCH v1 2/4] ata: libata-scsi: Generate ATA PT sense data when ATA ERR/DF are set

[Igor Pylypiv]

On Thu, Jun 20, 2024 at 03:12:37PM +0200, Niklas Cassel wrote:
> On Tue, Jun 18, 2024 at 01:13:24AM +0000, Igor Pylypiv wrote:
> > 
> > Any failed ATA PASS-THROUGH command with CK_COND=1 has this issue. For example,
> > Sending READ SECTOR(S) EXT via ATA PASS-THROUGH with CK_COND=1 to an lba
> > that was previously corrupted by WRITE UNCORRECTABLE EXT is supposed to
> > return "READ ERROR - LBA MARKED BAD BY APPLICATION CLIENT" but instead it
> > returns generated "UNRECOVERED READ ERROR - AUTO REALLOCATE FAILED".
> 
> I assume that the drive generated correct sense data, but that
> ata_gen_passthru_sense() overwrites/overwrote the sense data with
> generated sense data based on taskfile registers?
>

Yes, that is correct.

Thanks,
Igor
 
> 
> Kind regards,
> Niklas

Re: [PATCH v1 3/4] ata: libata-scsi: Report valid sense data for ATA PT if present

[Igor Pylypiv]

On Thu, Jun 20, 2024 at 04:24:11PM +0200, Niklas Cassel wrote:
> On Mon, Jun 17, 2024 at 11:31:49PM +0000, Igor Pylypiv wrote:
> > On Mon, Jun 17, 2024 at 12:49:47PM +0200, Niklas Cassel wrote:
> > > On Fri, Jun 14, 2024 at 07:18:34PM +0000, Igor Pylypiv wrote:
> > > > Do not generate sense data from ATA status/error registers
> > > > if valid sense data is already present.
> > > > 
> > > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > > > ---
> > > >  drivers/ata/libata-scsi.c | 17 +++++++++++------
> > > >  1 file changed, 11 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > > > index 79e8103ef3a9..4bfe47e7d266 100644
> > > > --- a/drivers/ata/libata-scsi.c
> > > > +++ b/drivers/ata/libata-scsi.c
> > > > @@ -858,12 +858,17 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> > > >  	unsigned char *desc = sb + 8;
> > > >  	u8 sense_key, asc, ascq;
> > > 
> > > Like I suggested in the earlier patch,
> > > 
> > > can't you do a:
> > > 
> > > if (qc->flags & ATA_QCFLAG_SENSE_VALID)
> > > 	return;
> > > 
> > > here instead?
> > > 
> > 
> > We need to populate the "ATA Status Return sense data descriptor" as per SAT-5
> > "Table 209 — ATA command results". By returning early we are skipping the code
> > that copies ATA output fields into descriptor/fixed sense data buffer.
> 
> We might get sense data from the drive.
> If we use REQUEST SENSE DATA EXT, we will get SK/ASC/ASCQ,
> we will then call scsi_build_sense_buffer() which will generate
> sense data in either the descriptor format or the fixed format,
> based on the D_SENSE bit, which is set in the control mode page.
> 
> The user can toggle this bit, see:
> https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-scsi.c#L3691-L3694
> 
> But by default it is 0:
> https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-scsi.c#L86
> which means fixed format.
> 
> This all seems to be in accordance with
> "Table 209 — Returned sense data with the CK_COND bit set to one"
> in sat6r1.
> 
> 
> 
> When calling scsi_build_sense_buffer(), we supply:
> scsi_build_sense_buffer(dev->flags & ATA_DFLAG_D_SENSE,
>                         cmd->sense_buffer, tf.lbah,
>                         tf.lbam, tf.lbal);
> 
> so we do not supply the STATUS and ERROR fields when building the sense data.
> 
> This seems fine, since SCSI has no knowledge of ATA status or ATA error.
> 
> 
> However, for ATA-passthrough commands, ATA status and ATA error fields
> are part of either the COMMAND-SPECIFIC information in the fixed format
> case, or part for the descriptor format, in case of the descriptor type
> being ATA Status Return sense data descriptor.
> 
> 
> So what I think we should do:
> 1) Keep the sense data if it exists, and fill in
>    the ATA status and ATA error at the correct offset (depending on if
>    the existing sense data is in fixed format or descriptor format).
> 2) If there doesn't exist any sense data, generate it, including the
>    ATA passthru command specific fields (ATA status and ATA error).
>    This is basically what ata_gen_passthru_sense() does today.
>

Yes! That's exactly what I was trying to do with the horrible empty if in
ata_gen_passthru_sense(). Factoring out ATA status and ATA error population
into a separate function makes the code more readable and less error-prone.

I'll make the change in v2.

Thank you!
Igor

> 
> So something like this:
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index bb4d30d377ae..a0687eb28ff7 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1645,9 +1645,17 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
>          * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
>          */
>         if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
> -           ((cdb[2] & 0x20) || need_sense))
> -               ata_gen_passthru_sense(qc);
> -       else if (need_sense)
> +           ((cdb[2] & 0x20) || need_sense)) {
> +               if ((qc->flags & ATA_QCFLAG_SENSE_VALID)) {
> +                       /*
> +                        * ATA PASS-THROUGH commands also need to fill in the
> +                        * command specific ATA status and ATA error fields.
> +                        */
> +                       ata_fill_passthru_specific_sense_fields(qc);
> +               } else {
> +                       ata_gen_passthru_sense(qc);
> +               }
> +       } else if (need_sense)
>                 ata_gen_ata_sense(qc);
>         else
>                 /* Keep the SCSI ML and status byte, clear host byte. */
> 
> 
> Kind regards,
> Niklas
> 
> 
> 
> 

Re: [PATCH v1 2/4] ata: libata-scsi: Generate ATA PT sense data when ATA ERR/DF are set

[Damien Le Moal]

On 6/20/24 21:55, Niklas Cassel wrote:
>> Thanks for pointing this out! Looks like ATA PASS-TRHOUGH case is fine
>> since the flag is always set by ata_scsi_pass_thru() as you pointed out.
>> Do we still want to add the check even though we know that it is always
>> set by ata_scsi_pass_thru()?
>>
>> If the answer is "yes", I wonder if we should use the ATA_QCFLAG_RTF_FILLED
>> flag instead? Currently it is used for ahci only but looks like it can be
>> expanded to other drivers. inic_qc_fill_rtf() will benefit from this change
>> because it is not always setting the status/error values:
>> https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/sata_inic162x.c#L583-L586
>>
>> For the non passthough case qc->result_tf in ata_gen_ata_sense() is also valid
>> because fill_result_tf() is being called for failed commands regardless of
>> the ATA_QCFLAG_RESULT_TF flag:
>> https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-core.c#L4856-L4873
>>
>> In this case using ATA_QCFLAG_RTF_FILLED will be more accurate because
>> fill_result_tf() is being called even when ATA_QCFLAG_RESULT_TF is not set.
>>
>> With that said I'm not sure if it makes sense to update all of the ATA
>> error handling to start checking for the ATA_QCFLAG_RTF_FILLED flag.
>>
>> What are your thoughts on this?
> 
> I see your point, we will fill the result if there is an error,
> even if ATA_QCFLAG_RESULT_TF wasn't set.
> 
> Perhaps we should modify fill_result_tf() to set ATA_QCFLAG_RTF_FILLED,
> after it has called ap->ops->qc_fill_rtf(qc);

Yes, let's do that.

> Then this code can check if ATA_QCFLAG_RTF_FILLED is set, like you suggested.

And I wonder if we should not just drop ATA_QCFLAG_RESULT_TF and *always* set
the result tf for all commands. I fail to see why this is conditional to that flag.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v1 2/4] ata: libata-scsi: Generate ATA PT sense data when ATA ERR/DF are set

[Niklas Cassel]

On Fri, Jun 21, 2024 at 09:05:33AM +0900, Damien Le Moal wrote:
> On 6/20/24 21:55, Niklas Cassel wrote:
> > 
> > Perhaps we should modify fill_result_tf() to set ATA_QCFLAG_RTF_FILLED,
> > after it has called ap->ops->qc_fill_rtf(qc);
> 
> Yes, let's do that.
> 
> > Then this code can check if ATA_QCFLAG_RTF_FILLED is set, like you suggested.
> 
> And I wonder if we should not just drop ATA_QCFLAG_RESULT_TF and *always* set
> the result tf for all commands. I fail to see why this is conditional to that flag.

I'm guessing that originally this was just an optimization, that you did
not read the taskfile register for a command that was completed successfully.
(Since they did not see a need for it.)

And a command that failed would have gotten an error IRQ anyway, so the
result TF would be populated for those.

I'm not sure how much time we save by not reading the TF register for non-NCQ
commands... Most likely it would be possible to read the TF register for all
drivers for non-NCQ commands on completion.

E.g. we set the ATA_QCFLAG_RESULT_TF flag for internal commands and for
ATA-passthru commands, however, both of these are non-NCQ commands.



I think it is NCQ commands that are the problem...

For AHCI it is possible to get ATA status and ATA error, for NCQ commands,
if we extract it from the FIS rather than reading the PxTFD register,
and this is what we do in ahci_qc_ncq_fill_rtf().

Probably, most other drivers could also extract this from the FIS,
if we spend the effort on implementing that for every driver.

But if we don't do that, the drivers will read the TF register,
which e.g. for:
https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/sata_nv.c#L1400-L1407
doesn't seem to work for NCQ commands.

So I'm not sure if we can remove ATA_QCFLAG_RESULT_TF, but we could
definitely set it unconditionally for non-NCQ commands if we want.


Kind regards,
Niklas