[PATCH v3 0/6] ATA PASS-THROUGH sense data fixes

[PATCH v3 0/6] ATA PASS-THROUGH sense data fixes

[Igor Pylypiv]

This patch series is fixing a few ATA PASS-THROUGH issues:
1. Not reporting "ATA Status Return sense data descriptor" / "Fixed format
   sense data" when ATA_QCFLAG_SENSE_VALID is set.
2. Generating "fake" sk/asc/ascq based on ATA status/error registers when
   ATA_QCFLAG_SENSE_VALID is set and CK_COND=1.
3. Fixed format sense data was using incorrect field offsets for ATA
   PASS-THROUGH commands.
4. Using qc->result_tf in ATA sense data generation functions without
   checking if qc->result_tf contains a valid data.

Changes since v1:
Thanks Damien and Niklas for the reviews!

- Squashed two v1 patches 2/4 and 3/4 into one patch with a different
  implementation.
- Added 'Cc: stable@vger.kernel.org' tags to patches that are fixing bugs.
- Reordered patches with the 'Cc: stable@vger.kernel.org' tag to be applied
  first in order to simplify backports to stable releases.
- Restored the buffer memset in atapi_eh_request_sense().
- Updated declaration order in v1 patch 4/4.
- Added a patch to cleanup unused ATA device id in ata_to_sense_error().
- Updated fill_result_tf() to set ATA_QCFLAG_RTF_FILLED after populating
  the result taskfile. Removed now redundant flag sets/checks from ahci.
- Updated ATA sense data generation functions to return early if result_tf
  is not filled. Added WARN_ON_ONCE checks to generate a warning when
  ATA_QCFLAG_RTF_FILLED is not set and libata needs to generate sense data.

Changes since v2:
- Moved v2 2/6 patch (fixed ATA PT offsets) to be the first one in v3.
- Removed unused variable 'sb' from ata_gen_passthru_sense().
- Removed WARN_ON_ONCE checks and added ata_dev_dbg() logs instead.
- Removed the Fixes tag from v2 4/6 patch because the patch is doing
  a cleanup and is not fixing any bugs.

Igor Pylypiv (6):
  ata: libata-scsi: Fix offsets for the fixed format sense data
  ata: libata-scsi: Do not overwrite valid sense data when CK_COND=1
  ata: libata-scsi: Remove redundant sense_buffer memsets
  ata: libata-scsi: Do not pass ATA device id to ata_to_sense_error()
  ata: libata: Set ATA_QCFLAG_RTF_FILLED in fill_result_tf()
  ata: libata-scsi: Check ATA_QCFLAG_RTF_FILLED before using result_tf

 drivers/ata/libahci.c     |  10 --
 drivers/ata/libata-core.c |   8 ++
 drivers/ata/libata-scsi.c | 188 ++++++++++++++++++++++----------------
 3 files changed, 115 insertions(+), 91 deletions(-)

-- 
2.45.2.803.g4e1b14247a-goog

[PATCH v3 1/6] 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")
Cc: stable@vger.kernel.org
Reviewed-by: Hannes Reinecke <hare@suse.de>
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 bb4d30d377ae..a9e44ad4c2de 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;
 
 	memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
@@ -877,7 +876,8 @@ 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) {
+		unsigned char *desc;
 		u8 len;
 
 		/* descriptor format */
@@ -916,21 +916,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.803.g4e1b14247a-goog

[PATCH v3 2/6] ata: libata-scsi: Do not overwrite valid sense data when CK_COND=1

[Igor Pylypiv]

Current ata_gen_passthru_sense() code performs two actions:
1. Generates sense data based on the ATA 'status' and ATA 'error' fields.
2. Populates "ATA Status Return sense data descriptor" / "Fixed format
   sense data" with ATA taskfile fields.

The problem is that #1 generates sense data even when a valid sense data
is already present (ATA_QCFLAG_SENSE_VALID is set). Factoring out #2 into
a separate function allows us to generate sense data only when there is
no valid sense data (ATA_QCFLAG_SENSE_VALID is not set).

As a bonus, we can now delete a FIXME comment in atapi_qc_complete()
which states that we don't want to translate taskfile registers into
sense descriptors for ATAPI.

Cc: stable@vger.kernel.org
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
 drivers/ata/libata-scsi.c | 158 +++++++++++++++++++++-----------------
 1 file changed, 86 insertions(+), 72 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index a9e44ad4c2de..26b1263f5c7c 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -230,6 +230,80 @@ void ata_scsi_set_sense_information(struct ata_device *dev,
 				   SCSI_SENSE_BUFFERSIZE, information);
 }
 
+/**
+ *	ata_scsi_set_passthru_sense_fields - Set ATA fields in sense buffer
+ *	@qc: ATA PASS-THROUGH command.
+ *
+ *	Populates "ATA Status Return sense data descriptor" / "Fixed format
+ *	sense data" with ATA taskfile fields.
+ *
+ *	LOCKING:
+ *	None.
+ */
+static void ata_scsi_set_passthru_sense_fields(struct ata_queued_cmd *qc)
+{
+	struct scsi_cmnd *cmd = qc->scsicmd;
+	struct ata_taskfile *tf = &qc->result_tf;
+	unsigned char *sb = cmd->sense_buffer;
+
+	if ((sb[0] & 0x7f) >= 0x72) {
+		unsigned char *desc;
+		u8 len;
+
+		/* descriptor format */
+		len = sb[7];
+		desc = (char *)scsi_sense_desc_find(sb, len + 8, 9);
+		if (!desc) {
+			if (SCSI_SENSE_BUFFERSIZE < len + 14)
+				return;
+			sb[7] = len + 14;
+			desc = sb + 8 + len;
+		}
+		desc[0] = 9;
+		desc[1] = 12;
+		/*
+		 * Copy registers into sense buffer.
+		 */
+		desc[2] = 0x00;
+		desc[3] = tf->error;
+		desc[5] = tf->nsect;
+		desc[7] = tf->lbal;
+		desc[9] = tf->lbam;
+		desc[11] = tf->lbah;
+		desc[12] = tf->device;
+		desc[13] = tf->status;
+
+		/*
+		 * Fill in Extend bit, and the high order bytes
+		 * if applicable.
+		 */
+		if (tf->flags & ATA_TFLAG_LBA48) {
+			desc[2] |= 0x01;
+			desc[4] = tf->hob_nsect;
+			desc[6] = tf->hob_lbal;
+			desc[8] = tf->hob_lbam;
+			desc[10] = tf->hob_lbah;
+		}
+	} else {
+		/* Fixed sense format */
+		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)  {
+			sb[8] |= 0x80;
+			if (tf->hob_nsect)
+				sb[8] |= 0x40;
+			if (tf->hob_lbal || tf->hob_lbam || tf->hob_lbah)
+				sb[8] |= 0x20;
+		}
+		sb[9] = tf->lbal;
+		sb[10] = tf->lbam;
+		sb[11] = tf->lbah;
+	}
+}
+
 static void ata_scsi_set_invalid_field(struct ata_device *dev,
 				       struct scsi_cmnd *cmd, u16 field, u8 bit)
 {
@@ -837,10 +911,8 @@ static void ata_to_sense_error(unsigned id, u8 drv_stat, u8 drv_err, u8 *sk,
  *	ata_gen_passthru_sense - Generate check condition sense block.
  *	@qc: Command that completed.
  *
- *	This function is specific to the ATA descriptor format sense
- *	block specified for the ATA pass through commands.  Regardless
- *	of whether the command errored or not, return a sense
- *	block. Copy all controller registers into the sense
+ *	This function is specific to the ATA pass through commands.
+ *	Regardless of whether the command errored or not, return a sense
  *	block. If there was no error, we get the request from an ATA
  *	passthrough command, so we use the following sense data:
  *	sk = RECOVERED ERROR
@@ -875,63 +947,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
 		 */
 		scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
 	}
-
-	if ((sb[0] & 0x7f) >= 0x72) {
-		unsigned char *desc;
-		u8 len;
-
-		/* descriptor format */
-		len = sb[7];
-		desc = (char *)scsi_sense_desc_find(sb, len + 8, 9);
-		if (!desc) {
-			if (SCSI_SENSE_BUFFERSIZE < len + 14)
-				return;
-			sb[7] = len + 14;
-			desc = sb + 8 + len;
-		}
-		desc[0] = 9;
-		desc[1] = 12;
-		/*
-		 * Copy registers into sense buffer.
-		 */
-		desc[2] = 0x00;
-		desc[3] = tf->error;
-		desc[5] = tf->nsect;
-		desc[7] = tf->lbal;
-		desc[9] = tf->lbam;
-		desc[11] = tf->lbah;
-		desc[12] = tf->device;
-		desc[13] = tf->status;
-
-		/*
-		 * Fill in Extend bit, and the high order bytes
-		 * if applicable.
-		 */
-		if (tf->flags & ATA_TFLAG_LBA48) {
-			desc[2] |= 0x01;
-			desc[4] = tf->hob_nsect;
-			desc[6] = tf->hob_lbal;
-			desc[8] = tf->hob_lbam;
-			desc[10] = tf->hob_lbah;
-		}
-	} else {
-		/* Fixed sense format */
-		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)  {
-			sb[8] |= 0x80;
-			if (tf->hob_nsect)
-				sb[8] |= 0x40;
-			if (tf->hob_lbal || tf->hob_lbam || tf->hob_lbah)
-				sb[8] |= 0x20;
-		}
-		sb[9] = tf->lbal;
-		sb[10] = tf->lbam;
-		sb[11] = tf->lbah;
-	}
 }
 
 /**
@@ -1634,6 +1649,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 	u8 *cdb = cmd->cmnd;
 	int need_sense = (qc->err_mask != 0) &&
 		!(qc->flags & ATA_QCFLAG_SENSE_VALID);
+	int need_passthru_sense = (qc->err_mask != 0) ||
+		(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
@@ -1645,13 +1662,16 @@ 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_passthru_sense)) {
+		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID))
+			ata_gen_passthru_sense(qc);
+		ata_scsi_set_passthru_sense_fields(qc);
+	} else if (need_sense) {
 		ata_gen_ata_sense(qc);
-	else
+	} else {
 		/* Keep the SCSI ML and status byte, clear host byte. */
 		cmd->result &= 0x0000ffff;
+	}
 
 	ata_qc_done(qc);
 }
@@ -2590,14 +2610,8 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
 	/* handle completion from EH */
 	if (unlikely(err_mask || qc->flags & ATA_QCFLAG_SENSE_VALID)) {
 
-		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID)) {
-			/* FIXME: not quite right; we don't want the
-			 * translation of taskfile registers into a
-			 * sense descriptors, since that's only
-			 * correct for ATA, not ATAPI
-			 */
+		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID))
 			ata_gen_passthru_sense(qc);
-		}
 
 		/* SCSI EH automatically locks door if sdev->locked is
 		 * set.  Sometimes door lock request continues to
-- 
2.45.2.803.g4e1b14247a-goog

[PATCH v3 3/6] ata: libata-scsi: Remove redundant sense_buffer memsets

[Igor Pylypiv]

SCSI layer clears sense_buffer in scsi_queue_rq() so there is no need for
libata to clear it again.

Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
 drivers/ata/libata-scsi.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 26b1263f5c7c..6b6e35292620 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -926,11 +926,8 @@ 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;
 	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.
@@ -968,8 +965,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.803.g4e1b14247a-goog

[PATCH v3 4/6] ata: libata-scsi: Do not pass ATA device id to ata_to_sense_error()

[Igor Pylypiv]

ATA device id is not used in ata_to_sense_error().

Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
 drivers/ata/libata-scsi.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 6b6e35292620..02af4d5d5799 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -785,7 +785,6 @@ static void ata_qc_set_pc_nbytes(struct ata_queued_cmd *qc)
 
 /**
  *	ata_to_sense_error - convert ATA error to SCSI error
- *	@id: ATA device number
  *	@drv_stat: value contained in ATA status register
  *	@drv_err: value contained in ATA error register
  *	@sk: the sense key we'll fill out
@@ -799,8 +798,8 @@ static void ata_qc_set_pc_nbytes(struct ata_queued_cmd *qc)
  *	LOCKING:
  *	spin_lock_irqsave(host lock)
  */
-static void ata_to_sense_error(unsigned id, u8 drv_stat, u8 drv_err, u8 *sk,
-			       u8 *asc, u8 *ascq)
+static void ata_to_sense_error(u8 drv_stat, u8 drv_err, u8 *sk, u8 *asc,
+			       u8 *ascq)
 {
 	int i;
 
@@ -934,7 +933,7 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
 	 */
 	if (qc->err_mask ||
 	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
-		ata_to_sense_error(qc->ap->print_id, tf->status, tf->error,
+		ata_to_sense_error(tf->status, tf->error,
 				   &sense_key, &asc, &ascq);
 		ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);
 	} else {
@@ -976,7 +975,7 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
 	 */
 	if (qc->err_mask ||
 	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
-		ata_to_sense_error(qc->ap->print_id, tf->status, tf->error,
+		ata_to_sense_error(tf->status, tf->error,
 				   &sense_key, &asc, &ascq);
 		ata_scsi_set_sense(dev, cmd, sense_key, asc, ascq);
 	} else {
-- 
2.45.2.803.g4e1b14247a-goog

[PATCH v3 5/6] ata: libata: Set ATA_QCFLAG_RTF_FILLED in fill_result_tf()

[Igor Pylypiv]

ATA_QCFLAG_RTF_FILLED is not specific to ahci and can be used generally
to check if qc->result_tf contains valid data.

Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
 drivers/ata/libahci.c     | 10 ----------
 drivers/ata/libata-core.c |  8 ++++++++
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 83431aae74d8..0728d445e531 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2075,13 +2075,6 @@ static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
 	struct ahci_port_priv *pp = qc->ap->private_data;
 	u8 *rx_fis = pp->rx_fis;
 
-	/*
-	 * rtf may already be filled (e.g. for successful NCQ commands).
-	 * If that is the case, we have nothing to do.
-	 */
-	if (qc->flags & ATA_QCFLAG_RTF_FILLED)
-		return;
-
 	if (pp->fbs_enabled)
 		rx_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
 
@@ -2095,7 +2088,6 @@ static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
 	    !(qc->flags & ATA_QCFLAG_EH)) {
 		ata_tf_from_fis(rx_fis + RX_FIS_PIO_SETUP, &qc->result_tf);
 		qc->result_tf.status = (rx_fis + RX_FIS_PIO_SETUP)[15];
-		qc->flags |= ATA_QCFLAG_RTF_FILLED;
 		return;
 	}
 
@@ -2118,12 +2110,10 @@ static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
 		 */
 		qc->result_tf.status = fis[2];
 		qc->result_tf.error = fis[3];
-		qc->flags |= ATA_QCFLAG_RTF_FILLED;
 		return;
 	}
 
 	ata_tf_from_fis(rx_fis + RX_FIS_D2H_REG, &qc->result_tf);
-	qc->flags |= ATA_QCFLAG_RTF_FILLED;
 }
 
 static void ahci_qc_ncq_fill_rtf(struct ata_port *ap, u64 done_mask)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index e1bf8a19b3c8..a9fc3ec9300f 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4801,8 +4801,16 @@ static void fill_result_tf(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
 
+	/*
+	 * rtf may already be filled (e.g. for successful NCQ commands).
+	 * If that is the case, we have nothing to do.
+	 */
+	if (qc->flags & ATA_QCFLAG_RTF_FILLED)
+		return;
+
 	qc->result_tf.flags = qc->tf.flags;
 	ap->ops->qc_fill_rtf(qc);
+	qc->flags |= ATA_QCFLAG_RTF_FILLED;
 }
 
 static void ata_verify_xfer(struct ata_queued_cmd *qc)
-- 
2.45.2.803.g4e1b14247a-goog

[PATCH v3 6/6] ata: libata-scsi: Check ATA_QCFLAG_RTF_FILLED before using result_tf

[Igor Pylypiv]

qc->result_tf contents are only valid when the ATA_QCFLAG_RTF_FILLED flag
is set. The ATA_QCFLAG_RTF_FILLED flag should be always set for commands
that failed or for commands that have the ATA_QCFLAG_RESULT_TF flag set.

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

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 02af4d5d5799..d5874d4b9253 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -242,10 +242,16 @@ void ata_scsi_set_sense_information(struct ata_device *dev,
  */
 static void ata_scsi_set_passthru_sense_fields(struct ata_queued_cmd *qc)
 {
+	struct ata_device *dev = qc->dev;
 	struct scsi_cmnd *cmd = qc->scsicmd;
 	struct ata_taskfile *tf = &qc->result_tf;
 	unsigned char *sb = cmd->sense_buffer;
 
+	if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
+		ata_dev_dbg(dev, "Missing RTF: cannot set ATA PT sense fields.\n");
+		return;
+	}
+
 	if ((sb[0] & 0x7f) >= 0x72) {
 		unsigned char *desc;
 		u8 len;
@@ -923,10 +929,16 @@ static void ata_to_sense_error(u8 drv_stat, u8 drv_err, u8 *sk, u8 *asc,
  */
 static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
 {
+	struct ata_device *dev = qc->dev;
 	struct scsi_cmnd *cmd = qc->scsicmd;
 	struct ata_taskfile *tf = &qc->result_tf;
 	u8 sense_key, asc, ascq;
 
+	if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
+		ata_dev_dbg(dev, "Missing RTF: cannot generate ATA PT sense data.\n");
+		return;
+	}
+
 	/*
 	 * Use ata_to_sense_error() to map status register bits
 	 * onto sense key, asc & ascq.
@@ -970,6 +982,12 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
 		ata_scsi_set_sense(dev, cmd, NOT_READY, 0x04, 0x21);
 		return;
 	}
+
+	if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
+		ata_dev_dbg(dev, "Missing RTF: cannot generate sense data.\n");
+		return;
+	}
+
 	/* Use ata_to_sense_error() to map status register bits
 	 * onto sense key, asc & ascq.
 	 */
-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH v3 2/6] ata: libata-scsi: Do not overwrite valid sense data when CK_COND=1

[Damien Le Moal]

On 6/27/24 08:04, Igor Pylypiv wrote:
> Current ata_gen_passthru_sense() code performs two actions:
> 1. Generates sense data based on the ATA 'status' and ATA 'error' fields.
> 2. Populates "ATA Status Return sense data descriptor" / "Fixed format
>    sense data" with ATA taskfile fields.
> 
> The problem is that #1 generates sense data even when a valid sense data
> is already present (ATA_QCFLAG_SENSE_VALID is set). Factoring out #2 into
> a separate function allows us to generate sense data only when there is
> no valid sense data (ATA_QCFLAG_SENSE_VALID is not set).
> 
> As a bonus, we can now delete a FIXME comment in atapi_qc_complete()
> which states that we don't want to translate taskfile registers into
> sense descriptors for ATAPI.
> 
> Cc: stable@vger.kernel.org
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>

I wonder if we can find the patch that introduced the bug in the first place so
that we can add a Fixes tag. I have not checked. This may have been wrong since
a long time ago...

> ---
>  drivers/ata/libata-scsi.c | 158 +++++++++++++++++++++-----------------
>  1 file changed, 86 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index a9e44ad4c2de..26b1263f5c7c 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -230,6 +230,80 @@ void ata_scsi_set_sense_information(struct ata_device *dev,
>  				   SCSI_SENSE_BUFFERSIZE, information);
>  }
>  
> +/**
> + *	ata_scsi_set_passthru_sense_fields - Set ATA fields in sense buffer
> + *	@qc: ATA PASS-THROUGH command.
> + *
> + *	Populates "ATA Status Return sense data descriptor" / "Fixed format
> + *	sense data" with ATA taskfile fields.
> + *
> + *	LOCKING:
> + *	None.
> + */
> +static void ata_scsi_set_passthru_sense_fields(struct ata_queued_cmd *qc)
> +{
> +	struct scsi_cmnd *cmd = qc->scsicmd;
> +	struct ata_taskfile *tf = &qc->result_tf;
> +	unsigned char *sb = cmd->sense_buffer;
> +
> +	if ((sb[0] & 0x7f) >= 0x72) {
> +		unsigned char *desc;
> +		u8 len;
> +
> +		/* descriptor format */
> +		len = sb[7];
> +		desc = (char *)scsi_sense_desc_find(sb, len + 8, 9);
> +		if (!desc) {
> +			if (SCSI_SENSE_BUFFERSIZE < len + 14)
> +				return;
> +			sb[7] = len + 14;
> +			desc = sb + 8 + len;
> +		}
> +		desc[0] = 9;
> +		desc[1] = 12;
> +		/*
> +		 * Copy registers into sense buffer.
> +		 */
> +		desc[2] = 0x00;
> +		desc[3] = tf->error;
> +		desc[5] = tf->nsect;
> +		desc[7] = tf->lbal;
> +		desc[9] = tf->lbam;
> +		desc[11] = tf->lbah;
> +		desc[12] = tf->device;
> +		desc[13] = tf->status;
> +
> +		/*
> +		 * Fill in Extend bit, and the high order bytes
> +		 * if applicable.
> +		 */
> +		if (tf->flags & ATA_TFLAG_LBA48) {
> +			desc[2] |= 0x01;
> +			desc[4] = tf->hob_nsect;
> +			desc[6] = tf->hob_lbal;
> +			desc[8] = tf->hob_lbam;
> +			desc[10] = tf->hob_lbah;
> +		}
> +	} else {
> +		/* Fixed sense format */
> +		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)  {
> +			sb[8] |= 0x80;
> +			if (tf->hob_nsect)
> +				sb[8] |= 0x40;
> +			if (tf->hob_lbal || tf->hob_lbam || tf->hob_lbah)
> +				sb[8] |= 0x20;
> +		}
> +		sb[9] = tf->lbal;
> +		sb[10] = tf->lbam;
> +		sb[11] = tf->lbah;
> +	}
> +}
> +
>  static void ata_scsi_set_invalid_field(struct ata_device *dev,
>  				       struct scsi_cmnd *cmd, u16 field, u8 bit)
>  {
> @@ -837,10 +911,8 @@ static void ata_to_sense_error(unsigned id, u8 drv_stat, u8 drv_err, u8 *sk,
>   *	ata_gen_passthru_sense - Generate check condition sense block.
>   *	@qc: Command that completed.
>   *
> - *	This function is specific to the ATA descriptor format sense
> - *	block specified for the ATA pass through commands.  Regardless
> - *	of whether the command errored or not, return a sense
> - *	block. Copy all controller registers into the sense
> + *	This function is specific to the ATA pass through commands.
> + *	Regardless of whether the command errored or not, return a sense
>   *	block. If there was no error, we get the request from an ATA
>   *	passthrough command, so we use the following sense data:
>   *	sk = RECOVERED ERROR
> @@ -875,63 +947,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
>  		 */
>  		scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
>  	}
> -
> -	if ((sb[0] & 0x7f) >= 0x72) {
> -		unsigned char *desc;
> -		u8 len;
> -
> -		/* descriptor format */
> -		len = sb[7];
> -		desc = (char *)scsi_sense_desc_find(sb, len + 8, 9);
> -		if (!desc) {
> -			if (SCSI_SENSE_BUFFERSIZE < len + 14)
> -				return;
> -			sb[7] = len + 14;
> -			desc = sb + 8 + len;
> -		}
> -		desc[0] = 9;
> -		desc[1] = 12;
> -		/*
> -		 * Copy registers into sense buffer.
> -		 */
> -		desc[2] = 0x00;
> -		desc[3] = tf->error;
> -		desc[5] = tf->nsect;
> -		desc[7] = tf->lbal;
> -		desc[9] = tf->lbam;
> -		desc[11] = tf->lbah;
> -		desc[12] = tf->device;
> -		desc[13] = tf->status;
> -
> -		/*
> -		 * Fill in Extend bit, and the high order bytes
> -		 * if applicable.
> -		 */
> -		if (tf->flags & ATA_TFLAG_LBA48) {
> -			desc[2] |= 0x01;
> -			desc[4] = tf->hob_nsect;
> -			desc[6] = tf->hob_lbal;
> -			desc[8] = tf->hob_lbam;
> -			desc[10] = tf->hob_lbah;
> -		}
> -	} else {
> -		/* Fixed sense format */
> -		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)  {
> -			sb[8] |= 0x80;
> -			if (tf->hob_nsect)
> -				sb[8] |= 0x40;
> -			if (tf->hob_lbal || tf->hob_lbam || tf->hob_lbah)
> -				sb[8] |= 0x20;
> -		}
> -		sb[9] = tf->lbal;
> -		sb[10] = tf->lbam;
> -		sb[11] = tf->lbah;
> -	}
>  }
>  
>  /**
> @@ -1634,6 +1649,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
>  	u8 *cdb = cmd->cmnd;
>  	int need_sense = (qc->err_mask != 0) &&
>  		!(qc->flags & ATA_QCFLAG_SENSE_VALID);
> +	int need_passthru_sense = (qc->err_mask != 0) ||
> +		(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
> @@ -1645,13 +1662,16 @@ 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_passthru_sense)) {
> +		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID))
> +			ata_gen_passthru_sense(qc);
> +		ata_scsi_set_passthru_sense_fields(qc);
> +	} else if (need_sense) {
>  		ata_gen_ata_sense(qc);
> -	else
> +	} else {
>  		/* Keep the SCSI ML and status byte, clear host byte. */
>  		cmd->result &= 0x0000ffff;
> +	}
>  
>  	ata_qc_done(qc);
>  }
> @@ -2590,14 +2610,8 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
>  	/* handle completion from EH */
>  	if (unlikely(err_mask || qc->flags & ATA_QCFLAG_SENSE_VALID)) {
>  
> -		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID)) {
> -			/* FIXME: not quite right; we don't want the
> -			 * translation of taskfile registers into a
> -			 * sense descriptors, since that's only
> -			 * correct for ATA, not ATAPI
> -			 */
> +		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID))
>  			ata_gen_passthru_sense(qc);
> -		}
>  
>  		/* SCSI EH automatically locks door if sdev->locked is
>  		 * set.  Sometimes door lock request continues to

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 6/6] ata: libata-scsi: Check ATA_QCFLAG_RTF_FILLED before using result_tf

[Damien Le Moal]

On 6/27/24 08:04, Igor Pylypiv wrote:
> qc->result_tf contents are only valid when the ATA_QCFLAG_RTF_FILLED flag
> is set. The ATA_QCFLAG_RTF_FILLED flag should be always set for commands
> that failed or for commands that have the ATA_QCFLAG_RESULT_TF flag set.
> 
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>

Please change "RTF" to "result TF" in the messages below, to be clear (RTF is
not). Feel free to split the ata_dev_dbg() lines after "dev," to avoid long lines.

With that:

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

> ---
>  drivers/ata/libata-scsi.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 02af4d5d5799..d5874d4b9253 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -242,10 +242,16 @@ void ata_scsi_set_sense_information(struct ata_device *dev,
>   */
>  static void ata_scsi_set_passthru_sense_fields(struct ata_queued_cmd *qc)
>  {
> +	struct ata_device *dev = qc->dev;
>  	struct scsi_cmnd *cmd = qc->scsicmd;
>  	struct ata_taskfile *tf = &qc->result_tf;
>  	unsigned char *sb = cmd->sense_buffer;
>  
> +	if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
> +		ata_dev_dbg(dev, "Missing RTF: cannot set ATA PT sense fields.\n");
> +		return;
> +	}
> +
>  	if ((sb[0] & 0x7f) >= 0x72) {
>  		unsigned char *desc;
>  		u8 len;
> @@ -923,10 +929,16 @@ static void ata_to_sense_error(u8 drv_stat, u8 drv_err, u8 *sk, u8 *asc,
>   */
>  static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
>  {
> +	struct ata_device *dev = qc->dev;
>  	struct scsi_cmnd *cmd = qc->scsicmd;
>  	struct ata_taskfile *tf = &qc->result_tf;
>  	u8 sense_key, asc, ascq;
>  
> +	if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
> +		ata_dev_dbg(dev, "Missing RTF: cannot generate ATA PT sense data.\n");
> +		return;
> +	}
> +
>  	/*
>  	 * Use ata_to_sense_error() to map status register bits
>  	 * onto sense key, asc & ascq.
> @@ -970,6 +982,12 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
>  		ata_scsi_set_sense(dev, cmd, NOT_READY, 0x04, 0x21);
>  		return;
>  	}
> +
> +	if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
> +		ata_dev_dbg(dev, "Missing RTF: cannot generate sense data.\n");
> +		return;
> +	}
> +
>  	/* Use ata_to_sense_error() to map status register bits
>  	 * onto sense key, asc & ascq.
>  	 */

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 6/6] ata: libata-scsi: Check ATA_QCFLAG_RTF_FILLED before using result_tf

[Hannes Reinecke]

On 6/27/24 01:04, Igor Pylypiv wrote:
> qc->result_tf contents are only valid when the ATA_QCFLAG_RTF_FILLED flag
> is set. The ATA_QCFLAG_RTF_FILLED flag should be always set for commands
> that failed or for commands that have the ATA_QCFLAG_RESULT_TF flag set.
> 
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>   drivers/ata/libata-scsi.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
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 v3 1/6] ata: libata-scsi: Fix offsets for the fixed format sense data

[Niklas Cassel]

Hello Igor, Hannes,

The changes in this patch looks good, however, there is still one thing that
bothers me:
https://github.com/torvalds/linux/blob/v6.10-rc5/drivers/ata/libata-scsi.c#L873-L877

Specifically the code in the else statement below:

	if (qc->err_mask ||
	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
		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);
	} else {
		/*
		 * ATA PASS-THROUGH INFORMATION AVAILABLE
		 * Always in descriptor format sense.
		 */
		scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
	}

Looking at sat6r01, I see that this is table:
Table 217 — ATA command results

And this text:
No error, successful completion or command in progress. The SATL
shall terminate the command with CHECK CONDITION status with
the sense key set to RECOVERED ERROR with the additional
sense code set to ATA PASS-THROUGH INFORMATION
AVAILABLE (see SPC-5). Descriptor format sense data shall include
the ATA Status Return sense data descriptor (see 12.2.2.7).

However, I don't see anything in this text that says that the
sense key should always be in descriptor format sense.

In fact, what will happen if the user has not set the D_SENSE bit
(libata will default not set it), is that:

The else statement above will be executed, filling in sense key in
descriptor format, after this if/else, we will continue checking
if the sense buffer is in descriptor format, or fixed format.

Since the scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
is called with (..., 1, ..., ..., ...) it will always generate
the sense data in descriptor format, regardless of
dev->flags ATA_DFLAG_D_SENSE being set or not.

Should perhaps the code in the else statement be:

} else {
	ata_scsi_set_sense(qc->dev, cmd, RECOVERED_ERROR, 0, 0x1D);
}

So that we actually respect the D_SENSE bit?

(We currently respect if when filling the sense data buffer with
sense data from REQUEST SENSE EXT, so I'm not sure why we shouldn't
respect it for successful ATA PASS-THROUGH commands.)


Kind regards,
Niklas

Re: [PATCH v3 2/6] ata: libata-scsi: Do not overwrite valid sense data when CK_COND=1

[Niklas Cassel]

On Wed, Jun 26, 2024 at 11:04:07PM +0000, Igor Pylypiv wrote:
> Current ata_gen_passthru_sense() code performs two actions:
> 1. Generates sense data based on the ATA 'status' and ATA 'error' fields.
> 2. Populates "ATA Status Return sense data descriptor" / "Fixed format
>    sense data" with ATA taskfile fields.
> 
> The problem is that #1 generates sense data even when a valid sense data
> is already present (ATA_QCFLAG_SENSE_VALID is set). Factoring out #2 into
> a separate function allows us to generate sense data only when there is
> no valid sense data (ATA_QCFLAG_SENSE_VALID is not set).
> 
> As a bonus, we can now delete a FIXME comment in atapi_qc_complete()
> which states that we don't want to translate taskfile registers into
> sense descriptors for ATAPI.
> 
> Cc: stable@vger.kernel.org
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>  drivers/ata/libata-scsi.c | 158 +++++++++++++++++++++-----------------
>  1 file changed, 86 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index a9e44ad4c2de..26b1263f5c7c 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -230,6 +230,80 @@ void ata_scsi_set_sense_information(struct ata_device *dev,
>  				   SCSI_SENSE_BUFFERSIZE, information);
>  }
>  
> +/**
> + *	ata_scsi_set_passthru_sense_fields - Set ATA fields in sense buffer
> + *	@qc: ATA PASS-THROUGH command.
> + *
> + *	Populates "ATA Status Return sense data descriptor" / "Fixed format
> + *	sense data" with ATA taskfile fields.
> + *
> + *	LOCKING:
> + *	None.
> + */
> +static void ata_scsi_set_passthru_sense_fields(struct ata_queued_cmd *qc)
> +{
> +	struct scsi_cmnd *cmd = qc->scsicmd;
> +	struct ata_taskfile *tf = &qc->result_tf;
> +	unsigned char *sb = cmd->sense_buffer;
> +
> +	if ((sb[0] & 0x7f) >= 0x72) {
> +		unsigned char *desc;
> +		u8 len;
> +
> +		/* descriptor format */
> +		len = sb[7];
> +		desc = (char *)scsi_sense_desc_find(sb, len + 8, 9);
> +		if (!desc) {
> +			if (SCSI_SENSE_BUFFERSIZE < len + 14)
> +				return;
> +			sb[7] = len + 14;
> +			desc = sb + 8 + len;
> +		}
> +		desc[0] = 9;
> +		desc[1] = 12;
> +		/*
> +		 * Copy registers into sense buffer.
> +		 */
> +		desc[2] = 0x00;
> +		desc[3] = tf->error;
> +		desc[5] = tf->nsect;
> +		desc[7] = tf->lbal;
> +		desc[9] = tf->lbam;
> +		desc[11] = tf->lbah;
> +		desc[12] = tf->device;
> +		desc[13] = tf->status;
> +
> +		/*
> +		 * Fill in Extend bit, and the high order bytes
> +		 * if applicable.
> +		 */
> +		if (tf->flags & ATA_TFLAG_LBA48) {
> +			desc[2] |= 0x01;
> +			desc[4] = tf->hob_nsect;
> +			desc[6] = tf->hob_lbal;
> +			desc[8] = tf->hob_lbam;
> +			desc[10] = tf->hob_lbah;
> +		}
> +	} else {
> +		/* Fixed sense format */
> +		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)  {
> +			sb[8] |= 0x80;
> +			if (tf->hob_nsect)
> +				sb[8] |= 0x40;
> +			if (tf->hob_lbal || tf->hob_lbam || tf->hob_lbah)
> +				sb[8] |= 0x20;
> +		}
> +		sb[9] = tf->lbal;
> +		sb[10] = tf->lbam;
> +		sb[11] = tf->lbah;
> +	}
> +}
> +
>  static void ata_scsi_set_invalid_field(struct ata_device *dev,
>  				       struct scsi_cmnd *cmd, u16 field, u8 bit)
>  {
> @@ -837,10 +911,8 @@ static void ata_to_sense_error(unsigned id, u8 drv_stat, u8 drv_err, u8 *sk,
>   *	ata_gen_passthru_sense - Generate check condition sense block.
>   *	@qc: Command that completed.
>   *
> - *	This function is specific to the ATA descriptor format sense
> - *	block specified for the ATA pass through commands.  Regardless
> - *	of whether the command errored or not, return a sense
> - *	block. Copy all controller registers into the sense
> + *	This function is specific to the ATA pass through commands.
> + *	Regardless of whether the command errored or not, return a sense
>   *	block. If there was no error, we get the request from an ATA
>   *	passthrough command, so we use the following sense data:
>   *	sk = RECOVERED ERROR
> @@ -875,63 +947,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
>  		 */
>  		scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
>  	}
> -
> -	if ((sb[0] & 0x7f) >= 0x72) {
> -		unsigned char *desc;
> -		u8 len;
> -
> -		/* descriptor format */
> -		len = sb[7];
> -		desc = (char *)scsi_sense_desc_find(sb, len + 8, 9);
> -		if (!desc) {
> -			if (SCSI_SENSE_BUFFERSIZE < len + 14)
> -				return;
> -			sb[7] = len + 14;
> -			desc = sb + 8 + len;
> -		}
> -		desc[0] = 9;
> -		desc[1] = 12;
> -		/*
> -		 * Copy registers into sense buffer.
> -		 */
> -		desc[2] = 0x00;
> -		desc[3] = tf->error;
> -		desc[5] = tf->nsect;
> -		desc[7] = tf->lbal;
> -		desc[9] = tf->lbam;
> -		desc[11] = tf->lbah;
> -		desc[12] = tf->device;
> -		desc[13] = tf->status;
> -
> -		/*
> -		 * Fill in Extend bit, and the high order bytes
> -		 * if applicable.
> -		 */
> -		if (tf->flags & ATA_TFLAG_LBA48) {
> -			desc[2] |= 0x01;
> -			desc[4] = tf->hob_nsect;
> -			desc[6] = tf->hob_lbal;
> -			desc[8] = tf->hob_lbam;
> -			desc[10] = tf->hob_lbah;
> -		}
> -	} else {
> -		/* Fixed sense format */
> -		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)  {
> -			sb[8] |= 0x80;
> -			if (tf->hob_nsect)
> -				sb[8] |= 0x40;
> -			if (tf->hob_lbal || tf->hob_lbam || tf->hob_lbah)
> -				sb[8] |= 0x20;
> -		}
> -		sb[9] = tf->lbal;
> -		sb[10] = tf->lbam;
> -		sb[11] = tf->lbah;
> -	}
>  }
>  
>  /**
> @@ -1634,6 +1649,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
>  	u8 *cdb = cmd->cmnd;
>  	int need_sense = (qc->err_mask != 0) &&
>  		!(qc->flags & ATA_QCFLAG_SENSE_VALID);
> +	int need_passthru_sense = (qc->err_mask != 0) ||
> +		(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
> @@ -1645,13 +1662,16 @@ 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_passthru_sense)) {
> +		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID))
> +			ata_gen_passthru_sense(qc);
> +		ata_scsi_set_passthru_sense_fields(qc);

This whole logic looks too complicated to me.

Can't we do something to make it easier to read, e.g. something like:


{
	if (command_is_ata_passthru(cdb)) {
		handle_passthru_completion(qc);
		ata_qc_done();
		return;
	}

	if (need_sense)
		ata_gen_ata_sense(qc);
	else
		/* Keep the SCSI ML and status byte, clear host byte. */ 
		cmd->result &= 0x0000ffff;

	ata_qc_done();
}

And then put the complicated logic in handle_passthru_command() ?

/* CASES:
* a) IF error command (ERROR or DF set) and ATA_QCFLAG_SENSE_VALID (SK+ASC+ASCQ) set:
*    - don't touch SK/ASC/ASCQ in sense_buffer
*    - set ATA registers in fixed format or descriptor format (based on dev->flags ATA_DFLAG_D_SENSE)
* b) IF error command (ERROR or DF set) and ATA_QCFLAG_SENSE_VALID (SK+ASC+ASCQ) not set:
*    - generate the SK+ASC+ASCQ from ATA status and ATA error, and
*    - set CHECK_CONDITION in cmd->result (scsi_build_sense() does this)
*    - set ATA registers in fixed format or descriptor format (based on dev->flags ATA_DFLAG_D_SENSE)
* c) IF success command (ERROR and DF not set), and ATA_QCFLAG_SENSE_VALID set, CK_COND set:
*    - don't touch SK/ASC/ASCQ in sense_buffer
*    - set ATA registers in fixed format or descriptor format (based on dev->flags ATA_DFLAG_D_SENSE)
*    - we should probably set CHECK_CONDITION status byte in cmd->result here.... but not sure...
* d) IF success command (ERROR and DF not set), and ATA_QCFLAG_SENSE_VALID set, CK_COND not set:
*    - don't touch SK/ASC/ASCQ in sense_buffer
*    - don't fill ATA registers
*    - keep the SCSI ML and status byte, clear host byte, in cmd->result
* e) IF success command (ERROR and DF not set), and ATA_QCFLAG_SENSE_VALID not set, CK_COND set:
*    - set SK to "RECOVERED ERROR" ASCQ to "ATA PASS-THROUGH INFORMATION AVAILABLE", ASC to 0.
*    - set ATA registers in fixed format or descriptor format (based on dev->flags ATA_DFLAG_D_SENSE)
*    - set CHECK_CONDITION status byte in cmd->result
* f) IF success command (ERROR and DF not set), and ATA_QCFLAG_SENSE_VALID not set, CK_COND not set:
*    - keep the SCSI ML and status byte, clear host byte, in cmd->result
*/
static void ata_handle_passthru_completion(struct ata_queued_cmd *qc);

So we should only copy the ATA registers when CK_COND is set, or if ERROR bit
or DF bit was set. CK_COND being set in the cdb (input command) basically means
that the user requested that the ATA registers should be copied into the sense
buffer (in the result).

The only tricky case is if we should set CHECK_CONDITION in case c) or not.
All other cases seems quite clear by looking at the SAT spec.


> +	} else if (need_sense) {
>  		ata_gen_ata_sense(qc);
> -	else
> +	} else {
>  		/* Keep the SCSI ML and status byte, clear host byte. */
>  		cmd->result &= 0x0000ffff;
> +	}
>  
>  	ata_qc_done(qc);
>  }
> @@ -2590,14 +2610,8 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
>  	/* handle completion from EH */
>  	if (unlikely(err_mask || qc->flags & ATA_QCFLAG_SENSE_VALID)) {
>  
> -		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID)) {
> -			/* FIXME: not quite right; we don't want the
> -			 * translation of taskfile registers into a
> -			 * sense descriptors, since that's only
> -			 * correct for ATA, not ATAPI
> -			 */
> +		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID))
>  			ata_gen_passthru_sense(qc);
> -		}
>  
>  		/* SCSI EH automatically locks door if sdev->locked is
>  		 * set.  Sometimes door lock request continues to
> -- 
> 2.45.2.803.g4e1b14247a-goog
> 

Re: [PATCH v3 2/6] ata: libata-scsi: Do not overwrite valid sense data when CK_COND=1

[Niklas Cassel]

On Thu, Jun 27, 2024 at 04:14:09PM +0200, Niklas Cassel wrote:
> 
> The only tricky case is if we should set CHECK_CONDITION in case c) or not.
> All other cases seems quite clear by looking at the SAT spec.

I think we should set CHECK_CONDITION in case c),
even if SK+ASCQ+ASC is not "ATA PASS-THROUGH INFORMATION AVAILABLE".

That way we at least align CK_COND with CHECK_CONDITION, which is
most likely what the user (and spec writers) expect.


Kind regards,
Niklas

Re: [PATCH v3 2/6] ata: libata-scsi: Do not overwrite valid sense data when CK_COND=1

[Igor Pylypiv]

On Thu, Jun 27, 2024 at 09:16:09AM +0900, Damien Le Moal wrote:
> On 6/27/24 08:04, Igor Pylypiv wrote:
> > Current ata_gen_passthru_sense() code performs two actions:
> > 1. Generates sense data based on the ATA 'status' and ATA 'error' fields.
> > 2. Populates "ATA Status Return sense data descriptor" / "Fixed format
> >    sense data" with ATA taskfile fields.
> > 
> > The problem is that #1 generates sense data even when a valid sense data
> > is already present (ATA_QCFLAG_SENSE_VALID is set). Factoring out #2 into
> > a separate function allows us to generate sense data only when there is
> > no valid sense data (ATA_QCFLAG_SENSE_VALID is not set).
> > 
> > As a bonus, we can now delete a FIXME comment in atapi_qc_complete()
> > which states that we don't want to translate taskfile registers into
> > sense descriptors for ATAPI.
> > 
> > Cc: stable@vger.kernel.org
> > Reviewed-by: Hannes Reinecke <hare@suse.de>
> > Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> 
> I wonder if we can find the patch that introduced the bug in the first place so
> that we can add a Fixes tag. I have not checked. This may have been wrong since
> a long time ago...

This code was first introduced in 2005 in commit b095518ef51c3 ("[libata]
ATA passthru (arbitrary ATA command execution)").

ATA_QCFLAG_SENSE_VALID was introduced a year later in commit 9ec957f2002b
("[PATCH] libata-eh-fw: add flags and operations for new EH").

IIUC, ATA_QCFLAG_SENSE_VALID has not been set for ATA drives until 2016
when the support for fetching the sense data was added in 5b01e4b9efa0
("libata: Implement NCQ autosense") and commit e87fd28cf9a2d ("libata:
Implement support for sense data reporting").

To me none of the commits looks like a good candidate for the Fixes tag.
What are your thoughts on this?

> 
> > ---
> >  drivers/ata/libata-scsi.c | 158 +++++++++++++++++++++-----------------
> >  1 file changed, 86 insertions(+), 72 deletions(-)
> > 
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index a9e44ad4c2de..26b1263f5c7c 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -230,6 +230,80 @@ void ata_scsi_set_sense_information(struct ata_device *dev,
> >  				   SCSI_SENSE_BUFFERSIZE, information);
> >  }
> >  
> > +/**
> > + *	ata_scsi_set_passthru_sense_fields - Set ATA fields in sense buffer
> > + *	@qc: ATA PASS-THROUGH command.
> > + *
> > + *	Populates "ATA Status Return sense data descriptor" / "Fixed format
> > + *	sense data" with ATA taskfile fields.
> > + *
> > + *	LOCKING:
> > + *	None.
> > + */
> > +static void ata_scsi_set_passthru_sense_fields(struct ata_queued_cmd *qc)
> > +{
> > +	struct scsi_cmnd *cmd = qc->scsicmd;
> > +	struct ata_taskfile *tf = &qc->result_tf;
> > +	unsigned char *sb = cmd->sense_buffer;
> > +
> > +	if ((sb[0] & 0x7f) >= 0x72) {
> > +		unsigned char *desc;
> > +		u8 len;
> > +
> > +		/* descriptor format */
> > +		len = sb[7];
> > +		desc = (char *)scsi_sense_desc_find(sb, len + 8, 9);
> > +		if (!desc) {
> > +			if (SCSI_SENSE_BUFFERSIZE < len + 14)
> > +				return;
> > +			sb[7] = len + 14;
> > +			desc = sb + 8 + len;
> > +		}
> > +		desc[0] = 9;
> > +		desc[1] = 12;
> > +		/*
> > +		 * Copy registers into sense buffer.
> > +		 */
> > +		desc[2] = 0x00;
> > +		desc[3] = tf->error;
> > +		desc[5] = tf->nsect;
> > +		desc[7] = tf->lbal;
> > +		desc[9] = tf->lbam;
> > +		desc[11] = tf->lbah;
> > +		desc[12] = tf->device;
> > +		desc[13] = tf->status;
> > +
> > +		/*
> > +		 * Fill in Extend bit, and the high order bytes
> > +		 * if applicable.
> > +		 */
> > +		if (tf->flags & ATA_TFLAG_LBA48) {
> > +			desc[2] |= 0x01;
> > +			desc[4] = tf->hob_nsect;
> > +			desc[6] = tf->hob_lbal;
> > +			desc[8] = tf->hob_lbam;
> > +			desc[10] = tf->hob_lbah;
> > +		}
> > +	} else {
> > +		/* Fixed sense format */
> > +		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)  {
> > +			sb[8] |= 0x80;
> > +			if (tf->hob_nsect)
> > +				sb[8] |= 0x40;
> > +			if (tf->hob_lbal || tf->hob_lbam || tf->hob_lbah)
> > +				sb[8] |= 0x20;
> > +		}
> > +		sb[9] = tf->lbal;
> > +		sb[10] = tf->lbam;
> > +		sb[11] = tf->lbah;
> > +	}
> > +}
> > +
> >  static void ata_scsi_set_invalid_field(struct ata_device *dev,
> >  				       struct scsi_cmnd *cmd, u16 field, u8 bit)
> >  {
> > @@ -837,10 +911,8 @@ static void ata_to_sense_error(unsigned id, u8 drv_stat, u8 drv_err, u8 *sk,
> >   *	ata_gen_passthru_sense - Generate check condition sense block.
> >   *	@qc: Command that completed.
> >   *
> > - *	This function is specific to the ATA descriptor format sense
> > - *	block specified for the ATA pass through commands.  Regardless
> > - *	of whether the command errored or not, return a sense
> > - *	block. Copy all controller registers into the sense
> > + *	This function is specific to the ATA pass through commands.
> > + *	Regardless of whether the command errored or not, return a sense
> >   *	block. If there was no error, we get the request from an ATA
> >   *	passthrough command, so we use the following sense data:
> >   *	sk = RECOVERED ERROR
> > @@ -875,63 +947,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> >  		 */
> >  		scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
> >  	}
> > -
> > -	if ((sb[0] & 0x7f) >= 0x72) {
> > -		unsigned char *desc;
> > -		u8 len;
> > -
> > -		/* descriptor format */
> > -		len = sb[7];
> > -		desc = (char *)scsi_sense_desc_find(sb, len + 8, 9);
> > -		if (!desc) {
> > -			if (SCSI_SENSE_BUFFERSIZE < len + 14)
> > -				return;
> > -			sb[7] = len + 14;
> > -			desc = sb + 8 + len;
> > -		}
> > -		desc[0] = 9;
> > -		desc[1] = 12;
> > -		/*
> > -		 * Copy registers into sense buffer.
> > -		 */
> > -		desc[2] = 0x00;
> > -		desc[3] = tf->error;
> > -		desc[5] = tf->nsect;
> > -		desc[7] = tf->lbal;
> > -		desc[9] = tf->lbam;
> > -		desc[11] = tf->lbah;
> > -		desc[12] = tf->device;
> > -		desc[13] = tf->status;
> > -
> > -		/*
> > -		 * Fill in Extend bit, and the high order bytes
> > -		 * if applicable.
> > -		 */
> > -		if (tf->flags & ATA_TFLAG_LBA48) {
> > -			desc[2] |= 0x01;
> > -			desc[4] = tf->hob_nsect;
> > -			desc[6] = tf->hob_lbal;
> > -			desc[8] = tf->hob_lbam;
> > -			desc[10] = tf->hob_lbah;
> > -		}
> > -	} else {
> > -		/* Fixed sense format */
> > -		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)  {
> > -			sb[8] |= 0x80;
> > -			if (tf->hob_nsect)
> > -				sb[8] |= 0x40;
> > -			if (tf->hob_lbal || tf->hob_lbam || tf->hob_lbah)
> > -				sb[8] |= 0x20;
> > -		}
> > -		sb[9] = tf->lbal;
> > -		sb[10] = tf->lbam;
> > -		sb[11] = tf->lbah;
> > -	}
> >  }
> >  
> >  /**
> > @@ -1634,6 +1649,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> >  	u8 *cdb = cmd->cmnd;
> >  	int need_sense = (qc->err_mask != 0) &&
> >  		!(qc->flags & ATA_QCFLAG_SENSE_VALID);
> > +	int need_passthru_sense = (qc->err_mask != 0) ||
> > +		(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
> > @@ -1645,13 +1662,16 @@ 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_passthru_sense)) {
> > +		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID))
> > +			ata_gen_passthru_sense(qc);
> > +		ata_scsi_set_passthru_sense_fields(qc);
> > +	} else if (need_sense) {
> >  		ata_gen_ata_sense(qc);
> > -	else
> > +	} else {
> >  		/* Keep the SCSI ML and status byte, clear host byte. */
> >  		cmd->result &= 0x0000ffff;
> > +	}
> >  
> >  	ata_qc_done(qc);
> >  }
> > @@ -2590,14 +2610,8 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
> >  	/* handle completion from EH */
> >  	if (unlikely(err_mask || qc->flags & ATA_QCFLAG_SENSE_VALID)) {
> >  
> > -		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID)) {
> > -			/* FIXME: not quite right; we don't want the
> > -			 * translation of taskfile registers into a
> > -			 * sense descriptors, since that's only
> > -			 * correct for ATA, not ATAPI
> > -			 */
> > +		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID))
> >  			ata_gen_passthru_sense(qc);
> > -		}
> >  
> >  		/* SCSI EH automatically locks door if sdev->locked is
> >  		 * set.  Sometimes door lock request continues to
> 
> -- 
> Damien Le Moal
> Western Digital Research
> 
Thanks,
Igor

Re: [PATCH v3 1/6] ata: libata-scsi: Fix offsets for the fixed format sense data

[Igor Pylypiv]

On Thu, Jun 27, 2024 at 02:08:50PM +0200, Niklas Cassel wrote:
> Hello Igor, Hannes,
> 
> The changes in this patch looks good, however, there is still one thing that
> bothers me:
> https://github.com/torvalds/linux/blob/v6.10-rc5/drivers/ata/libata-scsi.c#L873-L877
> 
> Specifically the code in the else statement below:
> 
> 	if (qc->err_mask ||
> 	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
> 		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);
> 	} else {
> 		/*
> 		 * ATA PASS-THROUGH INFORMATION AVAILABLE
> 		 * Always in descriptor format sense.
> 		 */
> 		scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
> 	}
> 
> Looking at sat6r01, I see that this is table:
> Table 217 — ATA command results
> 
> And this text:
> No error, successful completion or command in progress. The SATL
> shall terminate the command with CHECK CONDITION status with
> the sense key set to RECOVERED ERROR with the additional
> sense code set to ATA PASS-THROUGH INFORMATION
> AVAILABLE (see SPC-5). Descriptor format sense data shall include
> the ATA Status Return sense data descriptor (see 12.2.2.7).
> 
> However, I don't see anything in this text that says that the
> sense key should always be in descriptor format sense.
> 
> In fact, what will happen if the user has not set the D_SENSE bit
> (libata will default not set it), is that:
> 
> The else statement above will be executed, filling in sense key in
> descriptor format, after this if/else, we will continue checking
> if the sense buffer is in descriptor format, or fixed format.
> 
> Since the scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
> is called with (..., 1, ..., ..., ...) it will always generate
> the sense data in descriptor format, regardless of
> dev->flags ATA_DFLAG_D_SENSE being set or not.
> 
> Should perhaps the code in the else statement be:
> 
> } else {
> 	ata_scsi_set_sense(qc->dev, cmd, RECOVERED_ERROR, 0, 0x1D);
> }
> 
> So that we actually respect the D_SENSE bit?
> 
> (We currently respect if when filling the sense data buffer with
> sense data from REQUEST SENSE EXT, so I'm not sure why we shouldn't
> respect it for successful ATA PASS-THROUGH commands.)
> 

Thanks for pointing this out, Niklas! I agree, it seems like there is no
reason to ignore the D_SENSE bit.

Interestingly, the code was using ata_scsi_set_sense() before.
Commit 11093cb1ef56 ("libata-scsi: generate correct ATA pass-through sense)"
changed it to always be in the descriptor format.

> 
> Kind regards,
> Niklas

Thanks,
Igor

Re: [PATCH v3 2/6] ata: libata-scsi: Do not overwrite valid sense data when CK_COND=1

[Igor Pylypiv]

On Thu, Jun 27, 2024 at 04:14:09PM +0200, Niklas Cassel wrote:
> On Wed, Jun 26, 2024 at 11:04:07PM +0000, Igor Pylypiv wrote:
> > Current ata_gen_passthru_sense() code performs two actions:
> > 1. Generates sense data based on the ATA 'status' and ATA 'error' fields.
> > 2. Populates "ATA Status Return sense data descriptor" / "Fixed format
> >    sense data" with ATA taskfile fields.
> > 
> > The problem is that #1 generates sense data even when a valid sense data
> > is already present (ATA_QCFLAG_SENSE_VALID is set). Factoring out #2 into
> > a separate function allows us to generate sense data only when there is
> > no valid sense data (ATA_QCFLAG_SENSE_VALID is not set).
> > 
> > As a bonus, we can now delete a FIXME comment in atapi_qc_complete()
> > which states that we don't want to translate taskfile registers into
> > sense descriptors for ATAPI.
> > 
> > Cc: stable@vger.kernel.org
> > Reviewed-by: Hannes Reinecke <hare@suse.de>
> > Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > ---
> >  drivers/ata/libata-scsi.c | 158 +++++++++++++++++++++-----------------
> >  1 file changed, 86 insertions(+), 72 deletions(-)
> > 
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index a9e44ad4c2de..26b1263f5c7c 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -230,6 +230,80 @@ void ata_scsi_set_sense_information(struct ata_device *dev,
> >  				   SCSI_SENSE_BUFFERSIZE, information);
> >  }
> >  
> > +/**
> > + *	ata_scsi_set_passthru_sense_fields - Set ATA fields in sense buffer
> > + *	@qc: ATA PASS-THROUGH command.
> > + *
> > + *	Populates "ATA Status Return sense data descriptor" / "Fixed format
> > + *	sense data" with ATA taskfile fields.
> > + *
> > + *	LOCKING:
> > + *	None.
> > + */
> > +static void ata_scsi_set_passthru_sense_fields(struct ata_queued_cmd *qc)
> > +{
> > +	struct scsi_cmnd *cmd = qc->scsicmd;
> > +	struct ata_taskfile *tf = &qc->result_tf;
> > +	unsigned char *sb = cmd->sense_buffer;
> > +
> > +	if ((sb[0] & 0x7f) >= 0x72) {
> > +		unsigned char *desc;
> > +		u8 len;
> > +
> > +		/* descriptor format */
> > +		len = sb[7];
> > +		desc = (char *)scsi_sense_desc_find(sb, len + 8, 9);
> > +		if (!desc) {
> > +			if (SCSI_SENSE_BUFFERSIZE < len + 14)
> > +				return;
> > +			sb[7] = len + 14;
> > +			desc = sb + 8 + len;
> > +		}
> > +		desc[0] = 9;
> > +		desc[1] = 12;
> > +		/*
> > +		 * Copy registers into sense buffer.
> > +		 */
> > +		desc[2] = 0x00;
> > +		desc[3] = tf->error;
> > +		desc[5] = tf->nsect;
> > +		desc[7] = tf->lbal;
> > +		desc[9] = tf->lbam;
> > +		desc[11] = tf->lbah;
> > +		desc[12] = tf->device;
> > +		desc[13] = tf->status;
> > +
> > +		/*
> > +		 * Fill in Extend bit, and the high order bytes
> > +		 * if applicable.
> > +		 */
> > +		if (tf->flags & ATA_TFLAG_LBA48) {
> > +			desc[2] |= 0x01;
> > +			desc[4] = tf->hob_nsect;
> > +			desc[6] = tf->hob_lbal;
> > +			desc[8] = tf->hob_lbam;
> > +			desc[10] = tf->hob_lbah;
> > +		}
> > +	} else {
> > +		/* Fixed sense format */
> > +		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)  {
> > +			sb[8] |= 0x80;
> > +			if (tf->hob_nsect)
> > +				sb[8] |= 0x40;
> > +			if (tf->hob_lbal || tf->hob_lbam || tf->hob_lbah)
> > +				sb[8] |= 0x20;
> > +		}
> > +		sb[9] = tf->lbal;
> > +		sb[10] = tf->lbam;
> > +		sb[11] = tf->lbah;
> > +	}
> > +}
> > +
> >  static void ata_scsi_set_invalid_field(struct ata_device *dev,
> >  				       struct scsi_cmnd *cmd, u16 field, u8 bit)
> >  {
> > @@ -837,10 +911,8 @@ static void ata_to_sense_error(unsigned id, u8 drv_stat, u8 drv_err, u8 *sk,
> >   *	ata_gen_passthru_sense - Generate check condition sense block.
> >   *	@qc: Command that completed.
> >   *
> > - *	This function is specific to the ATA descriptor format sense
> > - *	block specified for the ATA pass through commands.  Regardless
> > - *	of whether the command errored or not, return a sense
> > - *	block. Copy all controller registers into the sense
> > + *	This function is specific to the ATA pass through commands.
> > + *	Regardless of whether the command errored or not, return a sense
> >   *	block. If there was no error, we get the request from an ATA
> >   *	passthrough command, so we use the following sense data:
> >   *	sk = RECOVERED ERROR
> > @@ -875,63 +947,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> >  		 */
> >  		scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
> >  	}
> > -
> > -	if ((sb[0] & 0x7f) >= 0x72) {
> > -		unsigned char *desc;
> > -		u8 len;
> > -
> > -		/* descriptor format */
> > -		len = sb[7];
> > -		desc = (char *)scsi_sense_desc_find(sb, len + 8, 9);
> > -		if (!desc) {
> > -			if (SCSI_SENSE_BUFFERSIZE < len + 14)
> > -				return;
> > -			sb[7] = len + 14;
> > -			desc = sb + 8 + len;
> > -		}
> > -		desc[0] = 9;
> > -		desc[1] = 12;
> > -		/*
> > -		 * Copy registers into sense buffer.
> > -		 */
> > -		desc[2] = 0x00;
> > -		desc[3] = tf->error;
> > -		desc[5] = tf->nsect;
> > -		desc[7] = tf->lbal;
> > -		desc[9] = tf->lbam;
> > -		desc[11] = tf->lbah;
> > -		desc[12] = tf->device;
> > -		desc[13] = tf->status;
> > -
> > -		/*
> > -		 * Fill in Extend bit, and the high order bytes
> > -		 * if applicable.
> > -		 */
> > -		if (tf->flags & ATA_TFLAG_LBA48) {
> > -			desc[2] |= 0x01;
> > -			desc[4] = tf->hob_nsect;
> > -			desc[6] = tf->hob_lbal;
> > -			desc[8] = tf->hob_lbam;
> > -			desc[10] = tf->hob_lbah;
> > -		}
> > -	} else {
> > -		/* Fixed sense format */
> > -		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)  {
> > -			sb[8] |= 0x80;
> > -			if (tf->hob_nsect)
> > -				sb[8] |= 0x40;
> > -			if (tf->hob_lbal || tf->hob_lbam || tf->hob_lbah)
> > -				sb[8] |= 0x20;
> > -		}
> > -		sb[9] = tf->lbal;
> > -		sb[10] = tf->lbam;
> > -		sb[11] = tf->lbah;
> > -	}
> >  }
> >  
> >  /**
> > @@ -1634,6 +1649,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> >  	u8 *cdb = cmd->cmnd;
> >  	int need_sense = (qc->err_mask != 0) &&
> >  		!(qc->flags & ATA_QCFLAG_SENSE_VALID);
> > +	int need_passthru_sense = (qc->err_mask != 0) ||
> > +		(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
> > @@ -1645,13 +1662,16 @@ 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_passthru_sense)) {
> > +		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID))
> > +			ata_gen_passthru_sense(qc);
> > +		ata_scsi_set_passthru_sense_fields(qc);
> 
> This whole logic looks too complicated to me.
> 
> Can't we do something to make it easier to read, e.g. something like:
> 
> 
> {
> 	if (command_is_ata_passthru(cdb)) {
> 		handle_passthru_completion(qc);
> 		ata_qc_done();
> 		return;
> 	}
> 
> 	if (need_sense)
> 		ata_gen_ata_sense(qc);
> 	else
> 		/* Keep the SCSI ML and status byte, clear host byte. */ 
> 		cmd->result &= 0x0000ffff;
> 
> 	ata_qc_done();
> }
> 
> And then put the complicated logic in handle_passthru_command() ?
> 
> /* CASES:
> * a) IF error command (ERROR or DF set) and ATA_QCFLAG_SENSE_VALID (SK+ASC+ASCQ) set:
> *    - don't touch SK/ASC/ASCQ in sense_buffer
> *    - set ATA registers in fixed format or descriptor format (based on dev->flags ATA_DFLAG_D_SENSE)
> * b) IF error command (ERROR or DF set) and ATA_QCFLAG_SENSE_VALID (SK+ASC+ASCQ) not set:
> *    - generate the SK+ASC+ASCQ from ATA status and ATA error, and
> *    - set CHECK_CONDITION in cmd->result (scsi_build_sense() does this)
> *    - set ATA registers in fixed format or descriptor format (based on dev->flags ATA_DFLAG_D_SENSE)
> * c) IF success command (ERROR and DF not set), and ATA_QCFLAG_SENSE_VALID set, CK_COND set:
> *    - don't touch SK/ASC/ASCQ in sense_buffer
> *    - set ATA registers in fixed format or descriptor format (based on dev->flags ATA_DFLAG_D_SENSE)
> *    - we should probably set CHECK_CONDITION status byte in cmd->result here.... but not sure...
> * d) IF success command (ERROR and DF not set), and ATA_QCFLAG_SENSE_VALID set, CK_COND not set:
> *    - don't touch SK/ASC/ASCQ in sense_buffer
> *    - don't fill ATA registers
> *    - keep the SCSI ML and status byte, clear host byte, in cmd->result
> * e) IF success command (ERROR and DF not set), and ATA_QCFLAG_SENSE_VALID not set, CK_COND set:
> *    - set SK to "RECOVERED ERROR" ASCQ to "ATA PASS-THROUGH INFORMATION AVAILABLE", ASC to 0.
> *    - set ATA registers in fixed format or descriptor format (based on dev->flags ATA_DFLAG_D_SENSE)
> *    - set CHECK_CONDITION status byte in cmd->result
> * f) IF success command (ERROR and DF not set), and ATA_QCFLAG_SENSE_VALID not set, CK_COND not set:
> *    - keep the SCSI ML and status byte, clear host byte, in cmd->result
> */
> static void ata_handle_passthru_completion(struct ata_queued_cmd *qc);
>
> So we should only copy the ATA registers when CK_COND is set, or if ERROR bit
> or DF bit was set. CK_COND being set in the cdb (input command) basically means
> that the user requested that the ATA registers should be copied into the sense
> buffer (in the result).
> 
> The only tricky case is if we should set CHECK_CONDITION in case c) or not.
> All other cases seems quite clear by looking at the SAT spec.
> 

Thank you, Niklas! I agree that this code is too complicated and should be
simplified. I don't think we should change the code too much in this patch
since it is going to be backported to stable releases.

Would you mind sending a patch for the proposed simplifications following
this patch series?

I think there are some additional simplifications that can be made,
e.g. both ata_gen_passthru_sense() and ata_gen_ata_sense() contain
the same exact code:

        /*                                                                      
         * 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)) {             
                ata_to_sense_error(tf->status, tf->error,                       
                                   &sense_key, &asc, &ascq);                    
                ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);         
        } else {

Perhaps it can be moved into a separate function, etc.

Thanks,
Igor

> 
> > +	} else if (need_sense) {
> >  		ata_gen_ata_sense(qc);
> > -	else
> > +	} else {
> >  		/* Keep the SCSI ML and status byte, clear host byte. */
> >  		cmd->result &= 0x0000ffff;
> > +	}
> >  
> >  	ata_qc_done(qc);
> >  }
> > @@ -2590,14 +2610,8 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
> >  	/* handle completion from EH */
> >  	if (unlikely(err_mask || qc->flags & ATA_QCFLAG_SENSE_VALID)) {
> >  
> > -		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID)) {
> > -			/* FIXME: not quite right; we don't want the
> > -			 * translation of taskfile registers into a
> > -			 * sense descriptors, since that's only
> > -			 * correct for ATA, not ATAPI
> > -			 */
> > +		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID))
> >  			ata_gen_passthru_sense(qc);
> > -		}
> >  
> >  		/* SCSI EH automatically locks door if sdev->locked is
> >  		 * set.  Sometimes door lock request continues to
> > -- 
> > 2.45.2.803.g4e1b14247a-goog
> > 

Re: [PATCH v3 6/6] ata: libata-scsi: Check ATA_QCFLAG_RTF_FILLED before using result_tf

[Igor Pylypiv]

On Thu, Jun 27, 2024 at 09:19:17AM +0900, Damien Le Moal wrote:
> On 6/27/24 08:04, Igor Pylypiv wrote:
> > qc->result_tf contents are only valid when the ATA_QCFLAG_RTF_FILLED flag
> > is set. The ATA_QCFLAG_RTF_FILLED flag should be always set for commands
> > that failed or for commands that have the ATA_QCFLAG_RESULT_TF flag set.
> > 
> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> 
> Please change "RTF" to "result TF" in the messages below, to be clear (RTF is
> not). Feel free to split the ata_dev_dbg() lines after "dev," to avoid long lines.

Thanks Damien! I wrote "result taskfile" first, then changed it to "result TF",
and then to "RTF" to shorten the lines :)

I'll change it to "result TF" with the line split in v3.

> 
> With that:
> 
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> 
> > ---
> >  drivers/ata/libata-scsi.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index 02af4d5d5799..d5874d4b9253 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -242,10 +242,16 @@ void ata_scsi_set_sense_information(struct ata_device *dev,
> >   */
> >  static void ata_scsi_set_passthru_sense_fields(struct ata_queued_cmd *qc)
> >  {
> > +	struct ata_device *dev = qc->dev;
> >  	struct scsi_cmnd *cmd = qc->scsicmd;
> >  	struct ata_taskfile *tf = &qc->result_tf;
> >  	unsigned char *sb = cmd->sense_buffer;
> >  
> > +	if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
> > +		ata_dev_dbg(dev, "Missing RTF: cannot set ATA PT sense fields.\n");
> > +		return;
> > +	}
> > +
> >  	if ((sb[0] & 0x7f) >= 0x72) {
> >  		unsigned char *desc;
> >  		u8 len;
> > @@ -923,10 +929,16 @@ static void ata_to_sense_error(u8 drv_stat, u8 drv_err, u8 *sk, u8 *asc,
> >   */
> >  static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> >  {
> > +	struct ata_device *dev = qc->dev;
> >  	struct scsi_cmnd *cmd = qc->scsicmd;
> >  	struct ata_taskfile *tf = &qc->result_tf;
> >  	u8 sense_key, asc, ascq;
> >  
> > +	if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
> > +		ata_dev_dbg(dev, "Missing RTF: cannot generate ATA PT sense data.\n");
> > +		return;
> > +	}
> > +
> >  	/*
> >  	 * Use ata_to_sense_error() to map status register bits
> >  	 * onto sense key, asc & ascq.
> > @@ -970,6 +982,12 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
> >  		ata_scsi_set_sense(dev, cmd, NOT_READY, 0x04, 0x21);
> >  		return;
> >  	}
> > +
> > +	if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
> > +		ata_dev_dbg(dev, "Missing RTF: cannot generate sense data.\n");
> > +		return;
> > +	}
> > +
> >  	/* Use ata_to_sense_error() to map status register bits
> >  	 * onto sense key, asc & ascq.
> >  	 */
> 
> -- 
> Damien Le Moal
> Western Digital Research
> 
Thanks,
Igor

Re: [PATCH v3 2/6] ata: libata-scsi: Do not overwrite valid sense data when CK_COND=1

[Damien Le Moal]

On 6/28/24 5:55 AM, Igor Pylypiv wrote:
> On Thu, Jun 27, 2024 at 09:16:09AM +0900, Damien Le Moal wrote:
>> On 6/27/24 08:04, Igor Pylypiv wrote:
>>> Current ata_gen_passthru_sense() code performs two actions:
>>> 1. Generates sense data based on the ATA 'status' and ATA 'error' fields.
>>> 2. Populates "ATA Status Return sense data descriptor" / "Fixed format
>>>    sense data" with ATA taskfile fields.
>>>
>>> The problem is that #1 generates sense data even when a valid sense data
>>> is already present (ATA_QCFLAG_SENSE_VALID is set). Factoring out #2 into
>>> a separate function allows us to generate sense data only when there is
>>> no valid sense data (ATA_QCFLAG_SENSE_VALID is not set).
>>>
>>> As a bonus, we can now delete a FIXME comment in atapi_qc_complete()
>>> which states that we don't want to translate taskfile registers into
>>> sense descriptors for ATAPI.
>>>
>>> Cc: stable@vger.kernel.org
>>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>>> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
>>> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
>>
>> I wonder if we can find the patch that introduced the bug in the first place so
>> that we can add a Fixes tag. I have not checked. This may have been wrong since
>> a long time ago...
> 
> This code was first introduced in 2005 in commit b095518ef51c3 ("[libata]
> ATA passthru (arbitrary ATA command execution)").
> 
> ATA_QCFLAG_SENSE_VALID was introduced a year later in commit 9ec957f2002b
> ("[PATCH] libata-eh-fw: add flags and operations for new EH").
> 
> IIUC, ATA_QCFLAG_SENSE_VALID has not been set for ATA drives until 2016
> when the support for fetching the sense data was added in 5b01e4b9efa0
> ("libata: Implement NCQ autosense") and commit e87fd28cf9a2d ("libata:
> Implement support for sense data reporting").
> 
> To me none of the commits looks like a good candidate for the Fixes tag.
> What are your thoughts on this?

Then let's just mark which LTS version need the patch.
E.g. Cc: stable@vger.kernel.org # X.Y +


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 1/6] ata: libata-scsi: Fix offsets for the fixed format sense data

[Hannes Reinecke]

On 6/27/24 14:08, Niklas Cassel wrote:
> Hello Igor, Hannes,
> 
> The changes in this patch looks good, however, there is still one thing that
> bothers me:
> https://github.com/torvalds/linux/blob/v6.10-rc5/drivers/ata/libata-scsi.c#L873-L877
> 
> Specifically the code in the else statement below:
> 
> 	if (qc->err_mask ||
> 	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
> 		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);
> 	} else {
> 		/*
> 		 * ATA PASS-THROUGH INFORMATION AVAILABLE
> 		 * Always in descriptor format sense.
> 		 */
> 		scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
> 	}
> 
> Looking at sat6r01, I see that this is table:
> Table 217 — ATA command results
> 
> And this text:
> No error, successful completion or command in progress. The SATL
> shall terminate the command with CHECK CONDITION status with
> the sense key set to RECOVERED ERROR with the additional
> sense code set to ATA PASS-THROUGH INFORMATION
> AVAILABLE (see SPC-5). Descriptor format sense data shall include
> the ATA Status Return sense data descriptor (see 12.2.2.7).
> 
> However, I don't see anything in this text that says that the
> sense key should always be in descriptor format sense.
> 
> In fact, what will happen if the user has not set the D_SENSE bit
> (libata will default not set it), is that:
> 
> The else statement above will be executed, filling in sense key in
> descriptor format, after this if/else, we will continue checking
> if the sense buffer is in descriptor format, or fixed format.
> 
> Since the scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
> is called with (..., 1, ..., ..., ...) it will always generate
> the sense data in descriptor format, regardless of
> dev->flags ATA_DFLAG_D_SENSE being set or not.
> 
> Should perhaps the code in the else statement be:
> 
> } else {
> 	ata_scsi_set_sense(qc->dev, cmd, RECOVERED_ERROR, 0, 0x1D);
> }
> 
> So that we actually respect the D_SENSE bit?
> 
> (We currently respect if when filling the sense data buffer with
> sense data from REQUEST SENSE EXT, so I'm not sure why we shouldn't
> respect it for successful ATA PASS-THROUGH commands.)
> 
I guess that we've misread the spec.

The sentence:

"Descriptor format sense data shall include the ATA Status Return
  Descriptor"

should be interpreted as:

  _If_ the sense code is formatted in descriptor format _then_ the
  ATA Status Return Descriptor should be included.

IE if the sense code is not in descriptor format then the ATA Status
Return Descriptor shouldn't be included (kinda obvious, really).
But of course the D_SENSE bit should be honoured.

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 v3 1/6] ata: libata-scsi: Fix offsets for the fixed format sense data

[Niklas Cassel]

On Fri, Jun 28, 2024 at 08:47:03AM +0200, Hannes Reinecke wrote:
> On 6/27/24 14:08, Niklas Cassel wrote:
> > Hello Igor, Hannes,
> > 
> > The changes in this patch looks good, however, there is still one thing that
> > bothers me:
> > https://github.com/torvalds/linux/blob/v6.10-rc5/drivers/ata/libata-scsi.c#L873-L877
> > 
> > Specifically the code in the else statement below:
> > 
> > 	if (qc->err_mask ||
> > 	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
> > 		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);
> > 	} else {
> > 		/*
> > 		 * ATA PASS-THROUGH INFORMATION AVAILABLE
> > 		 * Always in descriptor format sense.
> > 		 */
> > 		scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
> > 	}
> > 
> > Looking at sat6r01, I see that this is table:
> > Table 217 — ATA command results
> > 
> > And this text:
> > No error, successful completion or command in progress. The SATL
> > shall terminate the command with CHECK CONDITION status with
> > the sense key set to RECOVERED ERROR with the additional
> > sense code set to ATA PASS-THROUGH INFORMATION
> > AVAILABLE (see SPC-5). Descriptor format sense data shall include
> > the ATA Status Return sense data descriptor (see 12.2.2.7).
> > 
> > However, I don't see anything in this text that says that the
> > sense key should always be in descriptor format sense.
> > 
> > In fact, what will happen if the user has not set the D_SENSE bit
> > (libata will default not set it), is that:
> > 
> > The else statement above will be executed, filling in sense key in
> > descriptor format, after this if/else, we will continue checking
> > if the sense buffer is in descriptor format, or fixed format.
> > 
> > Since the scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
> > is called with (..., 1, ..., ..., ...) it will always generate
> > the sense data in descriptor format, regardless of
> > dev->flags ATA_DFLAG_D_SENSE being set or not.
> > 
> > Should perhaps the code in the else statement be:
> > 
> > } else {
> > 	ata_scsi_set_sense(qc->dev, cmd, RECOVERED_ERROR, 0, 0x1D);
> > }
> > 
> > So that we actually respect the D_SENSE bit?
> > 
> > (We currently respect if when filling the sense data buffer with
> > sense data from REQUEST SENSE EXT, so I'm not sure why we shouldn't
> > respect it for successful ATA PASS-THROUGH commands.)
> > 
> I guess that we've misread the spec.

I think I might have an idea where you got this from:

In sat5r06.pdf
"""
12.2.2.8 Fixed format sense data

Table 212 shows the fields returned in the fixed format sense data (see SPC-5) for ATA PASS-THROUGH
commands. SATLs compliant with ANSI INCITS 431-2007, SCSI/ATA Translation (SAT) return descriptor
format sense data for the ATA PASS-THROUGH commands regardless of the setting of the D_SENSE bit.
"""

In sat6r01.pdf:
"""
12.2.2.8 Fixed format sense data

Table 219 shows the fields returned in the fixed format sense data (see SPC-5)
for ATA PASS-THROUGH
commands.
"""

In SAT-6 there is no mention of compliance with ANSI INCITS 431-2007 should
ignore D_SENSE bit and unconditionally return sense data in descriptor format.

Anyway, considering that:
1) I'm not sure how a SAT would expose that it is compliant with ANSI INCITS
   431-2007.
2) This text has been removed from SAT-6.
3) We currently honour the D_SENSE bit when creating the sense buffer with the
   SK/ASC/ASCQ that we get from the device.

I think that it makes sense to honour the D_SENSE bit also when generating
sense data for successful ATA PASS-THROUGH commands (from ATA registers).


Kind regards,
Niklas

Re: [PATCH v3 1/6] ata: libata-scsi: Fix offsets for the fixed format sense data

[Niklas Cassel]

On Fri, Jun 28, 2024 at 05:49:22PM +0200, Niklas Cassel wrote:
> On Fri, Jun 28, 2024 at 08:47:03AM +0200, Hannes Reinecke wrote:
> > On 6/27/24 14:08, Niklas Cassel wrote:
> 
> In SAT-6 there is no mention of compliance with ANSI INCITS 431-2007 should
> ignore D_SENSE bit and unconditionally return sense data in descriptor format.
> 
> Anyway, considering that:
> 1) I'm not sure how a SAT would expose that it is compliant with ANSI INCITS
>    431-2007.
> 2) This text has been removed from SAT-6.
> 3) We currently honour the D_SENSE bit when creating the sense buffer with the
>    SK/ASC/ASCQ that we get from the device.
> 
> I think that it makes sense to honour the D_SENSE bit also when generating
> sense data for successful ATA PASS-THROUGH commands (from ATA registers).

Igor, I think you should add a new patch in your series that does:

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index d5874d4b9253..5b211551ac10 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -949,11 +949,8 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
                                   &sense_key, &asc, &ascq);
                ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);
        } else {
-               /*
-                * ATA PASS-THROUGH INFORMATION AVAILABLE
-                * Always in descriptor format sense.
-                */
-               scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
+               /* ATA PASS-THROUGH INFORMATION AVAILABLE */
+               ata_scsi_set_sense(qc->dev, cmd, RECOVERED_ERROR, 0, 0x1D);
        }
 }


Feel free to copy my arguments above.

I also checked VPD page 89h (ATA Information VPD page), and there are
no bits there either to claim certain SAT version compliance.

And since this text is not in SAT-6, I can only imagine that they decided
that is was not a good idea to not always honor D_SENSE...

(It does seem simpler to just always honor it...)


Kind regards,
Niklas

Re: [PATCH v3 2/6] ata: libata-scsi: Do not overwrite valid sense data when CK_COND=1

[Niklas Cassel]

On Thu, Jun 27, 2024 at 09:54:06PM +0000, Igor Pylypiv wrote:
> 
> Thank you, Niklas! I agree that this code is too complicated and should be
> simplified. I don't think we should change the code too much in this patch
> since it is going to be backported to stable releases.
> 
> Would you mind sending a patch for the proposed simplifications following
> this patch series?
> 

I would prefer if we changed it as part of this commit to be honest.


I also re-read the SAT spec, and found that it says that:
"""
If the CK_COND bit is set to:
a) one, then the SATL shall return a status of CHECK CONDITION upon ATA command completion,
without interpreting the contents of the STATUS field and returning the ATA fields from the request
completion in the sense data as specified in table 209; and
b) zero, then the SATL shall terminate the command with CHECK CONDITION status only if an error
occurs in processing the command. See clause 11 for a description of ATA error conditions.
"""

So it seems quite clear that if CK_COND == 1, we should set CHECK CONDITION,
so that answers the question/uncertainty I asked/expressed in earlier emails.


I think this patch (which should be applied on top of your v3 series),
makes the code way easier to read/understand:

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index d5874d4b9253..5b211551ac10 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1659,26 +1656,27 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 {
        struct scsi_cmnd *cmd = qc->scsicmd;
        u8 *cdb = cmd->cmnd;
-       int need_sense = (qc->err_mask != 0) &&
-               !(qc->flags & ATA_QCFLAG_SENSE_VALID);
-       int need_passthru_sense = (qc->err_mask != 0) ||
-               (qc->flags & ATA_QCFLAG_SENSE_VALID);
+       bool have_sense = qc->flags & ATA_QCFLAG_SENSE_VALID;
+       bool is_ata_passthru = cdb[0] == ATA_16 || cdb[0] == ATA_12;
+       bool is_ck_cond_request = cdb[2] & 0x20;
+       bool is_error = qc->err_mask != 0;
 
        /* 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
+        * 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:
+        * was no error, and CK_COND=1, we use the following sense data:
         * sk = RECOVERED ERROR
         * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
         */
-       if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
-           ((cdb[2] & 0x20) || need_passthru_sense)) {
-               if (!(qc->flags & ATA_QCFLAG_SENSE_VALID))
+       if (is_ata_passthru && (is_ck_cond_request || is_error || have_sense)) {
+               if (!have_sense)
                        ata_gen_passthru_sense(qc);
                ata_scsi_set_passthru_sense_fields(qc);
-       } else if (need_sense) {
+               if (is_ck_cond_request)
+                       set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION);
+       } else if (is_error && !have_sense) {
                ata_gen_ata_sense(qc);
        } else {
                /* Keep the SCSI ML and status byte, clear host byte. */

Re: [PATCH v3 6/6] ata: libata-scsi: Check ATA_QCFLAG_RTF_FILLED before using result_tf

[Niklas Cassel]

On Wed, Jun 26, 2024 at 11:04:11PM +0000, Igor Pylypiv wrote:
> qc->result_tf contents are only valid when the ATA_QCFLAG_RTF_FILLED flag
> is set. The ATA_QCFLAG_RTF_FILLED flag should be always set for commands
> that failed or for commands that have the ATA_QCFLAG_RESULT_TF flag set.
> 
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>  drivers/ata/libata-scsi.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 02af4d5d5799..d5874d4b9253 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -242,10 +242,16 @@ void ata_scsi_set_sense_information(struct ata_device *dev,
>   */
>  static void ata_scsi_set_passthru_sense_fields(struct ata_queued_cmd *qc)
>  {
> +	struct ata_device *dev = qc->dev;
>  	struct scsi_cmnd *cmd = qc->scsicmd;
>  	struct ata_taskfile *tf = &qc->result_tf;
>  	unsigned char *sb = cmd->sense_buffer;
>  
> +	if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
> +		ata_dev_dbg(dev, "Missing RTF: cannot set ATA PT sense fields.\n");
> +		return;
> +	}
> +
>  	if ((sb[0] & 0x7f) >= 0x72) {
>  		unsigned char *desc;
>  		u8 len;
> @@ -923,10 +929,16 @@ static void ata_to_sense_error(u8 drv_stat, u8 drv_err, u8 *sk, u8 *asc,
>   */
>  static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
>  {
> +	struct ata_device *dev = qc->dev;
>  	struct scsi_cmnd *cmd = qc->scsicmd;
>  	struct ata_taskfile *tf = &qc->result_tf;
>  	u8 sense_key, asc, ascq;
>  
> +	if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
> +		ata_dev_dbg(dev, "Missing RTF: cannot generate ATA PT sense data.\n");
> +		return;
> +	}
> +
>  	/*
>  	 * Use ata_to_sense_error() to map status register bits
>  	 * onto sense key, asc & ascq.
> @@ -970,6 +982,12 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
>  		ata_scsi_set_sense(dev, cmd, NOT_READY, 0x04, 0x21);
>  		return;
>  	}
> +
> +	if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
> +		ata_dev_dbg(dev, "Missing RTF: cannot generate sense data.\n");
> +		return;
> +	}
> +
>  	/* Use ata_to_sense_error() to map status register bits
>  	 * onto sense key, asc & ascq.
>  	 */
> -- 
> 2.45.2.803.g4e1b14247a-goog
> 

In order to be more consistent with existing prints in this file,
please do not capitalize the first letter, and remove the punctuation.


Kind regards,
Niklas

Re: [PATCH v3 5/6] ata: libata: Set ATA_QCFLAG_RTF_FILLED in fill_result_tf()

[Niklas Cassel]

On Wed, Jun 26, 2024 at 11:04:10PM +0000, Igor Pylypiv wrote:
> ATA_QCFLAG_RTF_FILLED is not specific to ahci and can be used generally
> to check if qc->result_tf contains valid data.
> 
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>  drivers/ata/libahci.c     | 10 ----------
>  drivers/ata/libata-core.c |  8 ++++++++
>  2 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 83431aae74d8..0728d445e531 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -2075,13 +2075,6 @@ static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
>  	struct ahci_port_priv *pp = qc->ap->private_data;
>  	u8 *rx_fis = pp->rx_fis;
>  
> -	/*
> -	 * rtf may already be filled (e.g. for successful NCQ commands).
> -	 * If that is the case, we have nothing to do.
> -	 */
> -	if (qc->flags & ATA_QCFLAG_RTF_FILLED)
> -		return;
> -
>  	if (pp->fbs_enabled)
>  		rx_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
>  
> @@ -2095,7 +2088,6 @@ static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
>  	    !(qc->flags & ATA_QCFLAG_EH)) {
>  		ata_tf_from_fis(rx_fis + RX_FIS_PIO_SETUP, &qc->result_tf);
>  		qc->result_tf.status = (rx_fis + RX_FIS_PIO_SETUP)[15];
> -		qc->flags |= ATA_QCFLAG_RTF_FILLED;
>  		return;
>  	}
>  
> @@ -2118,12 +2110,10 @@ static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
>  		 */
>  		qc->result_tf.status = fis[2];
>  		qc->result_tf.error = fis[3];
> -		qc->flags |= ATA_QCFLAG_RTF_FILLED;
>  		return;
>  	}
>  
>  	ata_tf_from_fis(rx_fis + RX_FIS_D2H_REG, &qc->result_tf);
> -	qc->flags |= ATA_QCFLAG_RTF_FILLED;
>  }
>  
>  static void ahci_qc_ncq_fill_rtf(struct ata_port *ap, u64 done_mask)
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index e1bf8a19b3c8..a9fc3ec9300f 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4801,8 +4801,16 @@ static void fill_result_tf(struct ata_queued_cmd *qc)
>  {
>  	struct ata_port *ap = qc->ap;
>  
> +	/*
> +	 * rtf may already be filled (e.g. for successful NCQ commands).
> +	 * If that is the case, we have nothing to do.
> +	 */
> +	if (qc->flags & ATA_QCFLAG_RTF_FILLED)
> +		return;
> +
>  	qc->result_tf.flags = qc->tf.flags;

One functional change that I can see from this is that after this commit,
we will no longer do: qc->result_tf.flags = qc->tf.flags;
if ATA_QCFLAG_RTF_FILLED was set.

e.g. ata_scsi_set_passthru_sense_fields() and ata_gen_ata_sense()
makes use of result_tf->flags, so we probably still want to do this.


Perhaps keep this function as you have it and simply do:

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 0728d445e531..fdfa7b266218 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2148,6 +2148,7 @@ static void ahci_qc_ncq_fill_rtf(struct ata_port *ap, u64 done_mask)
                        if (qc && ata_is_ncq(qc->tf.protocol)) {
                                qc->result_tf.status = status;
                                qc->result_tf.error = error;
+                               qc->result_tf.flags = qc->tf.flags;
                                qc->flags |= ATA_QCFLAG_RTF_FILLED;
                        }
                        done_mask &= ~(1ULL << tag);
@@ -2172,6 +2173,7 @@ static void ahci_qc_ncq_fill_rtf(struct ata_port *ap, u64 done_mask)
                        fis += RX_FIS_SDB;
                        qc->result_tf.status = fis[2];
                        qc->result_tf.error = fis[3];
+                       qc->result_tf.flags = qc->tf.flags;
                        qc->flags |= ATA_QCFLAG_RTF_FILLED;
                }
                done_mask &= ~(1ULL << tag);



>  	ap->ops->qc_fill_rtf(qc);
> +	qc->flags |= ATA_QCFLAG_RTF_FILLED;
>  }
>  
>  static void ata_verify_xfer(struct ata_queued_cmd *qc)
> -- 
> 2.45.2.803.g4e1b14247a-goog
> 

Re: [PATCH v3 5/6] ata: libata: Set ATA_QCFLAG_RTF_FILLED in fill_result_tf()

[Igor Pylypiv]

On Fri, Jun 28, 2024 at 10:12:19PM +0200, Niklas Cassel wrote:
> On Wed, Jun 26, 2024 at 11:04:10PM +0000, Igor Pylypiv wrote:
> > ATA_QCFLAG_RTF_FILLED is not specific to ahci and can be used generally
> > to check if qc->result_tf contains valid data.
> > 
> > Reviewed-by: Hannes Reinecke <hare@suse.de>
> > Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > ---
> >  drivers/ata/libahci.c     | 10 ----------
> >  drivers/ata/libata-core.c |  8 ++++++++
> >  2 files changed, 8 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> > index 83431aae74d8..0728d445e531 100644
> > --- a/drivers/ata/libahci.c
> > +++ b/drivers/ata/libahci.c
> > @@ -2075,13 +2075,6 @@ static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
> >  	struct ahci_port_priv *pp = qc->ap->private_data;
> >  	u8 *rx_fis = pp->rx_fis;
> >  
> > -	/*
> > -	 * rtf may already be filled (e.g. for successful NCQ commands).
> > -	 * If that is the case, we have nothing to do.
> > -	 */
> > -	if (qc->flags & ATA_QCFLAG_RTF_FILLED)
> > -		return;
> > -
> >  	if (pp->fbs_enabled)
> >  		rx_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
> >  
> > @@ -2095,7 +2088,6 @@ static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
> >  	    !(qc->flags & ATA_QCFLAG_EH)) {
> >  		ata_tf_from_fis(rx_fis + RX_FIS_PIO_SETUP, &qc->result_tf);
> >  		qc->result_tf.status = (rx_fis + RX_FIS_PIO_SETUP)[15];
> > -		qc->flags |= ATA_QCFLAG_RTF_FILLED;
> >  		return;
> >  	}
> >  
> > @@ -2118,12 +2110,10 @@ static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
> >  		 */
> >  		qc->result_tf.status = fis[2];
> >  		qc->result_tf.error = fis[3];
> > -		qc->flags |= ATA_QCFLAG_RTF_FILLED;
> >  		return;
> >  	}
> >  
> >  	ata_tf_from_fis(rx_fis + RX_FIS_D2H_REG, &qc->result_tf);
> > -	qc->flags |= ATA_QCFLAG_RTF_FILLED;
> >  }
> >  
> >  static void ahci_qc_ncq_fill_rtf(struct ata_port *ap, u64 done_mask)
> > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> > index e1bf8a19b3c8..a9fc3ec9300f 100644
> > --- a/drivers/ata/libata-core.c
> > +++ b/drivers/ata/libata-core.c
> > @@ -4801,8 +4801,16 @@ static void fill_result_tf(struct ata_queued_cmd *qc)
> >  {
> >  	struct ata_port *ap = qc->ap;
> >  
> > +	/*
> > +	 * rtf may already be filled (e.g. for successful NCQ commands).
> > +	 * If that is the case, we have nothing to do.
> > +	 */
> > +	if (qc->flags & ATA_QCFLAG_RTF_FILLED)
> > +		return;
> > +
> >  	qc->result_tf.flags = qc->tf.flags;
> 
> One functional change that I can see from this is that after this commit,
> we will no longer do: qc->result_tf.flags = qc->tf.flags;
> if ATA_QCFLAG_RTF_FILLED was set.

Nice catch, Niklas! I'll fix it in v4. Thank you!

> 
> e.g. ata_scsi_set_passthru_sense_fields() and ata_gen_ata_sense()
> makes use of result_tf->flags, so we probably still want to do this.
> 
> 
> Perhaps keep this function as you have it and simply do:
> 
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 0728d445e531..fdfa7b266218 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -2148,6 +2148,7 @@ static void ahci_qc_ncq_fill_rtf(struct ata_port *ap, u64 done_mask)
>                         if (qc && ata_is_ncq(qc->tf.protocol)) {
>                                 qc->result_tf.status = status;
>                                 qc->result_tf.error = error;
> +                               qc->result_tf.flags = qc->tf.flags;
>                                 qc->flags |= ATA_QCFLAG_RTF_FILLED;
>                         }
>                         done_mask &= ~(1ULL << tag);
> @@ -2172,6 +2173,7 @@ static void ahci_qc_ncq_fill_rtf(struct ata_port *ap, u64 done_mask)
>                         fis += RX_FIS_SDB;
>                         qc->result_tf.status = fis[2];
>                         qc->result_tf.error = fis[3];
> +                       qc->result_tf.flags = qc->tf.flags;
>                         qc->flags |= ATA_QCFLAG_RTF_FILLED;
>                 }
>                 done_mask &= ~(1ULL << tag);
> 
> 
> 
> >  	ap->ops->qc_fill_rtf(qc);
> > +	qc->flags |= ATA_QCFLAG_RTF_FILLED;
> >  }
> >  
> >  static void ata_verify_xfer(struct ata_queued_cmd *qc)
> > -- 
> > 2.45.2.803.g4e1b14247a-goog
> > 

Re: [PATCH v3 6/6] ata: libata-scsi: Check ATA_QCFLAG_RTF_FILLED before using result_tf

[Igor Pylypiv]

On Fri, Jun 28, 2024 at 09:42:53PM +0200, Niklas Cassel wrote:
> On Wed, Jun 26, 2024 at 11:04:11PM +0000, Igor Pylypiv wrote:
> > qc->result_tf contents are only valid when the ATA_QCFLAG_RTF_FILLED flag
> > is set. The ATA_QCFLAG_RTF_FILLED flag should be always set for commands
> > that failed or for commands that have the ATA_QCFLAG_RESULT_TF flag set.
> > 
> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > ---
> >  drivers/ata/libata-scsi.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index 02af4d5d5799..d5874d4b9253 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -242,10 +242,16 @@ void ata_scsi_set_sense_information(struct ata_device *dev,
> >   */
> >  static void ata_scsi_set_passthru_sense_fields(struct ata_queued_cmd *qc)
> >  {
> > +	struct ata_device *dev = qc->dev;
> >  	struct scsi_cmnd *cmd = qc->scsicmd;
> >  	struct ata_taskfile *tf = &qc->result_tf;
> >  	unsigned char *sb = cmd->sense_buffer;
> >  
> > +	if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
> > +		ata_dev_dbg(dev, "Missing RTF: cannot set ATA PT sense fields.\n");
> > +		return;
> > +	}
> > +
> >  	if ((sb[0] & 0x7f) >= 0x72) {
> >  		unsigned char *desc;
> >  		u8 len;
> > @@ -923,10 +929,16 @@ static void ata_to_sense_error(u8 drv_stat, u8 drv_err, u8 *sk, u8 *asc,
> >   */
> >  static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> >  {
> > +	struct ata_device *dev = qc->dev;
> >  	struct scsi_cmnd *cmd = qc->scsicmd;
> >  	struct ata_taskfile *tf = &qc->result_tf;
> >  	u8 sense_key, asc, ascq;
> >  
> > +	if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
> > +		ata_dev_dbg(dev, "Missing RTF: cannot generate ATA PT sense data.\n");
> > +		return;
> > +	}
> > +
> >  	/*
> >  	 * Use ata_to_sense_error() to map status register bits
> >  	 * onto sense key, asc & ascq.
> > @@ -970,6 +982,12 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
> >  		ata_scsi_set_sense(dev, cmd, NOT_READY, 0x04, 0x21);
> >  		return;
> >  	}
> > +
> > +	if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
> > +		ata_dev_dbg(dev, "Missing RTF: cannot generate sense data.\n");
> > +		return;
> > +	}
> > +
> >  	/* Use ata_to_sense_error() to map status register bits
> >  	 * onto sense key, asc & ascq.
> >  	 */
> > -- 
> > 2.45.2.803.g4e1b14247a-goog
> > 
> 
> In order to be more consistent with existing prints in this file,
> please do not capitalize the first letter, and remove the punctuation.

Thanks! I'll remove periods and will keep the colons.

> 
> 
> Kind regards,
> Niklas

Re: [PATCH v3 1/6] ata: libata-scsi: Fix offsets for the fixed format sense data

[Igor Pylypiv]

On Fri, Jun 28, 2024 at 08:25:40PM +0200, Niklas Cassel wrote:
> On Fri, Jun 28, 2024 at 05:49:22PM +0200, Niklas Cassel wrote:
> > On Fri, Jun 28, 2024 at 08:47:03AM +0200, Hannes Reinecke wrote:
> > > On 6/27/24 14:08, Niklas Cassel wrote:
> > 
> > In SAT-6 there is no mention of compliance with ANSI INCITS 431-2007 should
> > ignore D_SENSE bit and unconditionally return sense data in descriptor format.
> > 
> > Anyway, considering that:
> > 1) I'm not sure how a SAT would expose that it is compliant with ANSI INCITS
> >    431-2007.
> > 2) This text has been removed from SAT-6.
> > 3) We currently honour the D_SENSE bit when creating the sense buffer with the
> >    SK/ASC/ASCQ that we get from the device.
> > 
> > I think that it makes sense to honour the D_SENSE bit also when generating
> > sense data for successful ATA PASS-THROUGH commands (from ATA registers).
> 
> Igor, I think you should add a new patch in your series that does:

Thanks Niklas, I'll add the patch in v4.

> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index d5874d4b9253..5b211551ac10 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -949,11 +949,8 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
>                                    &sense_key, &asc, &ascq);
>                 ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);
>         } else {
> -               /*
> -                * ATA PASS-THROUGH INFORMATION AVAILABLE
> -                * Always in descriptor format sense.
> -                */
> -               scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
> +               /* ATA PASS-THROUGH INFORMATION AVAILABLE */
> +               ata_scsi_set_sense(qc->dev, cmd, RECOVERED_ERROR, 0, 0x1D);
>         }
>  }
> 
> 
> Feel free to copy my arguments above.
> 
> I also checked VPD page 89h (ATA Information VPD page), and there are
> no bits there either to claim certain SAT version compliance.
> 
> And since this text is not in SAT-6, I can only imagine that they decided
> that is was not a good idea to not always honor D_SENSE...
> 
> (It does seem simpler to just always honor it...)
> 
> 
> Kind regards,
> Niklas

Re: [PATCH v3 2/6] ata: libata-scsi: Do not overwrite valid sense data when CK_COND=1

[Igor Pylypiv]

On Fri, Jun 28, 2024 at 08:44:41PM +0200, Niklas Cassel wrote:
> On Thu, Jun 27, 2024 at 09:54:06PM +0000, Igor Pylypiv wrote:
> > 
> > Thank you, Niklas! I agree that this code is too complicated and should be
> > simplified. I don't think we should change the code too much in this patch
> > since it is going to be backported to stable releases.
> > 
> > Would you mind sending a patch for the proposed simplifications following
> > this patch series?
> > 
> 
> I would prefer if we changed it as part of this commit to be honest.
> 
> 
> I also re-read the SAT spec, and found that it says that:
> """
> If the CK_COND bit is set to:
> a) one, then the SATL shall return a status of CHECK CONDITION upon ATA command completion,
> without interpreting the contents of the STATUS field and returning the ATA fields from the request
> completion in the sense data as specified in table 209; and
> b) zero, then the SATL shall terminate the command with CHECK CONDITION status only if an error
> occurs in processing the command. See clause 11 for a description of ATA error conditions.
> """
> 
> So it seems quite clear that if CK_COND == 1, we should set CHECK CONDITION,
> so that answers the question/uncertainty I asked/expressed in earlier emails.
> 
> 
> I think this patch (which should be applied on top of your v3 series),
> makes the code way easier to read/understand:
> 

Agree, having self-explanatory variable names makes the code much more
readable. I'll add the patch in v4.

Do you mind if I set you as the author of the patch with the corresponding
Signed-off-by tag?

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index d5874d4b9253..5b211551ac10 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1659,26 +1656,27 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
>  {
>         struct scsi_cmnd *cmd = qc->scsicmd;
>         u8 *cdb = cmd->cmnd;
> -       int need_sense = (qc->err_mask != 0) &&
> -               !(qc->flags & ATA_QCFLAG_SENSE_VALID);
> -       int need_passthru_sense = (qc->err_mask != 0) ||
> -               (qc->flags & ATA_QCFLAG_SENSE_VALID);
> +       bool have_sense = qc->flags & ATA_QCFLAG_SENSE_VALID;
> +       bool is_ata_passthru = cdb[0] == ATA_16 || cdb[0] == ATA_12;
> +       bool is_ck_cond_request = cdb[2] & 0x20;
> +       bool is_error = qc->err_mask != 0;
>  
>         /* 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
> +        * 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:
> +        * was no error, and CK_COND=1, we use the following sense data:
>          * sk = RECOVERED ERROR
>          * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
>          */
> -       if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
> -           ((cdb[2] & 0x20) || need_passthru_sense)) {
> -               if (!(qc->flags & ATA_QCFLAG_SENSE_VALID))
> +       if (is_ata_passthru && (is_ck_cond_request || is_error || have_sense)) {
> +               if (!have_sense)
>                         ata_gen_passthru_sense(qc);
>                 ata_scsi_set_passthru_sense_fields(qc);
> -       } else if (need_sense) {
> +               if (is_ck_cond_request)
> +                       set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION);

SAM_STAT_CHECK_CONDITION will be set by ata_gen_passthru_sense(). Perhaps we
can move the SAM_STAT_CHECK_CONDITION setting into else if?

        if (is_ata_passthru && (is_ck_cond_request || is_error || have_sense)) {
               if (!have_sense)
                       ata_gen_passthru_sense(qc);
               else if (is_ck_cond_request)
                       set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION);
               ata_scsi_set_passthru_sense_fields(qc);
        } else if (is_error && !have_sense) {


> +       } else if (is_error && !have_sense) {
>                 ata_gen_ata_sense(qc);
>         } else {
>                 /* Keep the SCSI ML and status byte, clear host byte. */

Thanks,
Igor

Re: [PATCH v3 2/6] ata: libata-scsi: Do not overwrite valid sense data when CK_COND=1

[Niklas Cassel]

On Fri, Jun 28, 2024 at 11:31:35PM +0000, Igor Pylypiv wrote:
> On Fri, Jun 28, 2024 at 08:44:41PM +0200, Niklas Cassel wrote:
> > On Thu, Jun 27, 2024 at 09:54:06PM +0000, Igor Pylypiv wrote:
> > > 
> > > Thank you, Niklas! I agree that this code is too complicated and should be
> > > simplified. I don't think we should change the code too much in this patch
> > > since it is going to be backported to stable releases.
> > > 
> > > Would you mind sending a patch for the proposed simplifications following
> > > this patch series?
> > > 
> > 
> > I would prefer if we changed it as part of this commit to be honest.
> > 
> > 
> > I also re-read the SAT spec, and found that it says that:
> > """
> > If the CK_COND bit is set to:
> > a) one, then the SATL shall return a status of CHECK CONDITION upon ATA command completion,
> > without interpreting the contents of the STATUS field and returning the ATA fields from the request
> > completion in the sense data as specified in table 209; and
> > b) zero, then the SATL shall terminate the command with CHECK CONDITION status only if an error
> > occurs in processing the command. See clause 11 for a description of ATA error conditions.
> > """
> > 
> > So it seems quite clear that if CK_COND == 1, we should set CHECK CONDITION,
> > so that answers the question/uncertainty I asked/expressed in earlier emails.
> > 
> > 
> > I think this patch (which should be applied on top of your v3 series),
> > makes the code way easier to read/understand:
> > 
> 
> Agree, having self-explanatory variable names makes the code much more
> readable. I'll add the patch in v4.
> 
> Do you mind if I set you as the author of the patch with the corresponding
> Signed-off-by tag?

I still think that you are the author.

But if you want, feel free to add me as: Co-developed-by
(which would also require you to add my Signed-off-by), see:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by


> 
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index d5874d4b9253..5b211551ac10 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -1659,26 +1656,27 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> >  {
> >         struct scsi_cmnd *cmd = qc->scsicmd;
> >         u8 *cdb = cmd->cmnd;
> > -       int need_sense = (qc->err_mask != 0) &&
> > -               !(qc->flags & ATA_QCFLAG_SENSE_VALID);
> > -       int need_passthru_sense = (qc->err_mask != 0) ||
> > -               (qc->flags & ATA_QCFLAG_SENSE_VALID);
> > +       bool have_sense = qc->flags & ATA_QCFLAG_SENSE_VALID;
> > +       bool is_ata_passthru = cdb[0] == ATA_16 || cdb[0] == ATA_12;
> > +       bool is_ck_cond_request = cdb[2] & 0x20;
> > +       bool is_error = qc->err_mask != 0;
> >  
> >         /* 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
> > +        * 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:
> > +        * was no error, and CK_COND=1, we use the following sense data:
> >          * sk = RECOVERED ERROR
> >          * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
> >          */
> > -       if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
> > -           ((cdb[2] & 0x20) || need_passthru_sense)) {
> > -               if (!(qc->flags & ATA_QCFLAG_SENSE_VALID))
> > +       if (is_ata_passthru && (is_ck_cond_request || is_error || have_sense)) {
> > +               if (!have_sense)
> >                         ata_gen_passthru_sense(qc);
> >                 ata_scsi_set_passthru_sense_fields(qc);
> > -       } else if (need_sense) {
> > +               if (is_ck_cond_request)
> > +                       set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION);
> 
> SAM_STAT_CHECK_CONDITION will be set by ata_gen_passthru_sense(). Perhaps we
> can move the SAM_STAT_CHECK_CONDITION setting into else if?

I think it is fine that:
if (is_ck_cond_request)
	set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION);

might set SAM_STAT_CHECK_CONDITION even if it is already set.

Personally, I think that my suggestion is slightly clearer when it comes
to highlight the behavior of CK_COND. (CK_COND will set CHECK_CONDITION,
regardless if successful command or error command, and regardless if
we already had sense or not.)

And considering that we finally make this hard to read code slightly more
readable than it was to start off with, I would prefer my alternative.


Kind regards,
Niklas

Re: [PATCH v3 2/6] ata: libata-scsi: Do not overwrite valid sense data when CK_COND=1

[Igor Pylypiv]

On Sat, Jun 29, 2024 at 05:09:54AM +0200, Niklas Cassel wrote:
> On Fri, Jun 28, 2024 at 11:31:35PM +0000, Igor Pylypiv wrote:
> > On Fri, Jun 28, 2024 at 08:44:41PM +0200, Niklas Cassel wrote:
> > > On Thu, Jun 27, 2024 at 09:54:06PM +0000, Igor Pylypiv wrote:
> > > > 
> > > > Thank you, Niklas! I agree that this code is too complicated and should be
> > > > simplified. I don't think we should change the code too much in this patch
> > > > since it is going to be backported to stable releases.
> > > > 
> > > > Would you mind sending a patch for the proposed simplifications following
> > > > this patch series?
> > > > 
> > > 
> > > I would prefer if we changed it as part of this commit to be honest.
> > > 
> > > 
> > > I also re-read the SAT spec, and found that it says that:
> > > """
> > > If the CK_COND bit is set to:
> > > a) one, then the SATL shall return a status of CHECK CONDITION upon ATA command completion,
> > > without interpreting the contents of the STATUS field and returning the ATA fields from the request
> > > completion in the sense data as specified in table 209; and
> > > b) zero, then the SATL shall terminate the command with CHECK CONDITION status only if an error
> > > occurs in processing the command. See clause 11 for a description of ATA error conditions.
> > > """
> > > 
> > > So it seems quite clear that if CK_COND == 1, we should set CHECK CONDITION,
> > > so that answers the question/uncertainty I asked/expressed in earlier emails.
> > > 
> > > 
> > > I think this patch (which should be applied on top of your v3 series),
> > > makes the code way easier to read/understand:
> > > 
> > 
> > Agree, having self-explanatory variable names makes the code much more
> > readable. I'll add the patch in v4.
> > 
> > Do you mind if I set you as the author of the patch with the corresponding
> > Signed-off-by tag?
> 
> I still think that you are the author.
> 
> But if you want, feel free to add me as: Co-developed-by
> (which would also require you to add my Signed-off-by), see:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
> 
Sounds good! Added the Co-developed-by abd Signed-off-by tags in v4. Thanks!
> 
> > 
> > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > > index d5874d4b9253..5b211551ac10 100644
> > > --- a/drivers/ata/libata-scsi.c
> > > +++ b/drivers/ata/libata-scsi.c
> > > @@ -1659,26 +1656,27 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> > >  {
> > >         struct scsi_cmnd *cmd = qc->scsicmd;
> > >         u8 *cdb = cmd->cmnd;
> > > -       int need_sense = (qc->err_mask != 0) &&
> > > -               !(qc->flags & ATA_QCFLAG_SENSE_VALID);
> > > -       int need_passthru_sense = (qc->err_mask != 0) ||
> > > -               (qc->flags & ATA_QCFLAG_SENSE_VALID);
> > > +       bool have_sense = qc->flags & ATA_QCFLAG_SENSE_VALID;
> > > +       bool is_ata_passthru = cdb[0] == ATA_16 || cdb[0] == ATA_12;
> > > +       bool is_ck_cond_request = cdb[2] & 0x20;
> > > +       bool is_error = qc->err_mask != 0;
> > >  
> > >         /* 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
> > > +        * 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:
> > > +        * was no error, and CK_COND=1, we use the following sense data:
> > >          * sk = RECOVERED ERROR
> > >          * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
> > >          */
> > > -       if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
> > > -           ((cdb[2] & 0x20) || need_passthru_sense)) {
> > > -               if (!(qc->flags & ATA_QCFLAG_SENSE_VALID))
> > > +       if (is_ata_passthru && (is_ck_cond_request || is_error || have_sense)) {
> > > +               if (!have_sense)
> > >                         ata_gen_passthru_sense(qc);
> > >                 ata_scsi_set_passthru_sense_fields(qc);
> > > -       } else if (need_sense) {
> > > +               if (is_ck_cond_request)
> > > +                       set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION);
> > 
> > SAM_STAT_CHECK_CONDITION will be set by ata_gen_passthru_sense(). Perhaps we
> > can move the SAM_STAT_CHECK_CONDITION setting into else if?
> 
> I think it is fine that:
> if (is_ck_cond_request)
> 	set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION);
> 
> might set SAM_STAT_CHECK_CONDITION even if it is already set.
> 
> Personally, I think that my suggestion is slightly clearer when it comes
> to highlight the behavior of CK_COND. (CK_COND will set CHECK_CONDITION,
> regardless if successful command or error command, and regardless if
> we already had sense or not.)
> 
> And considering that we finally make this hard to read code slightly more
> readable than it was to start off with, I would prefer my alternative.
>
It makes senes. Added the patch in v4. Thank you!
> 
> Kind regards,
> Niklas