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

[PATCH v2 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.

Igor Pylypiv (6):
  ata: libata-scsi: Do not overwrite valid sense data when CK_COND=1
  ata: libata-scsi: Fix offsets for the fixed format sense data
  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 | 179 +++++++++++++++++++++-----------------
 3 files changed, 107 insertions(+), 90 deletions(-)

-- 
2.45.2.741.gdbec12cfda-goog

[PATCH v2 1/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>
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 bb4d30d377ae..54fed6a427b1 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;
+	unsigned char *desc = sb + 8;
+
+	if ((cmd->sense_buffer[0] & 0x7f) >= 0x72) {
+		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 */
+		desc[0] = tf->error;
+		desc[1] = tf->status;
+		desc[2] = tf->device;
+		desc[3] = tf->nsect;
+		desc[7] = 0;
+		if (tf->flags & ATA_TFLAG_LBA48)  {
+			desc[8] |= 0x80;
+			if (tf->hob_nsect)
+				desc[8] |= 0x40;
+			if (tf->hob_lbal || tf->hob_lbam || tf->hob_lbah)
+				desc[8] |= 0x20;
+		}
+		desc[9] = tf->lbal;
+		desc[10] = tf->lbam;
+		desc[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
@@ -855,7 +927,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);
@@ -876,62 +947,6 @@ 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) {
-		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 */
-		desc[0] = tf->error;
-		desc[1] = tf->status;
-		desc[2] = tf->device;
-		desc[3] = tf->nsect;
-		desc[7] = 0;
-		if (tf->flags & ATA_TFLAG_LBA48)  {
-			desc[8] |= 0x80;
-			if (tf->hob_nsect)
-				desc[8] |= 0x40;
-			if (tf->hob_lbal || tf->hob_lbam || tf->hob_lbah)
-				desc[8] |= 0x20;
-		}
-		desc[9] = tf->lbal;
-		desc[10] = tf->lbam;
-		desc[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.741.gdbec12cfda-goog

[PATCH v2 2/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
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 54fed6a427b1..26b1263f5c7c 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -245,9 +245,9 @@ 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;
-	unsigned char *desc = sb + 8;
 
-	if ((cmd->sense_buffer[0] & 0x7f) >= 0x72) {
+	if ((sb[0] & 0x7f) >= 0x72) {
+		unsigned char *desc;
 		u8 len;
 
 		/* descriptor format */
@@ -286,21 +286,21 @@ static void ata_scsi_set_passthru_sense_fields(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.741.gdbec12cfda-goog

[PATCH v2 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.

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

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 26b1263f5c7c..1aeab6e1c8b3 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -929,8 +929,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
 	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 +966,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.741.gdbec12cfda-goog

[PATCH v2 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().

Fixes: ff8072d589dc ("ata: libata: remove references to non-existing error_handler()")
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 1aeab6e1c8b3..e5669a296d81 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;
 
@@ -935,7 +934,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 {
@@ -977,7 +976,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.741.gdbec12cfda-goog

[PATCH v2 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.

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.741.gdbec12cfda-goog

[PATCH v2 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.

For ATA errors and ATA PASS-THROUGH commands the ATA_QCFLAG_RTF_FILLED
flag should be always set. Added WARN_ON_ONCE() checks to generate
a warning when ATA_QCFLAG_RTF_FILLED is not set and libata needs to
generate sense data.

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

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index e5669a296d81..7a8a08692ce9 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -246,6 +246,9 @@ static void ata_scsi_set_passthru_sense_fields(struct ata_queued_cmd *qc)
 	struct ata_taskfile *tf = &qc->result_tf;
 	unsigned char *sb = cmd->sense_buffer;
 
+	if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED)))
+		return;
+
 	if ((sb[0] & 0x7f) >= 0x72) {
 		unsigned char *desc;
 		u8 len;
@@ -928,6 +931,9 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
 	unsigned char *sb = cmd->sense_buffer;
 	u8 sense_key, asc, ascq;
 
+	if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED)))
+		return;
+
 	/*
 	 * Use ata_to_sense_error() to map status register bits
 	 * onto sense key, asc & ascq.
@@ -971,6 +977,10 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
 		ata_scsi_set_sense(dev, cmd, NOT_READY, 0x04, 0x21);
 		return;
 	}
+
+	if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED)))
+		return;
+
 	/* Use ata_to_sense_error() to map status register bits
 	 * onto sense key, asc & ascq.
 	 */
-- 
2.45.2.741.gdbec12cfda-goog

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

[Hannes Reinecke]

On 6/25/24 00:12, 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>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>   drivers/ata/libata-scsi.c | 158 +++++++++++++++++++++-----------------
>   1 file changed, 86 insertions(+), 72 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

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

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

[Hannes Reinecke]

On 6/25/24 00:12, Igor Pylypiv wrote:
> Correct the ATA PASS-THROUGH fixed format sense data offsets to conform
> to SPC-6 and SAT-5 specifications. Additionally, set the VALID bit to
> indicate that the INFORMATION field contains valid information.
> 
> INFORMATION
> ===========
> 
> SAT-5 Table 212 — "Fixed format sense data INFORMATION field for the ATA
> PASS-THROUGH commands" defines the following format:
> 
> +------+------------+
> | Byte |   Field    |
> +------+------------+
> |    0 | ERROR      |
> |    1 | STATUS     |
> |    2 | DEVICE     |
> |    3 | COUNT(7:0) |
> +------+------------+
> 
> SPC-6 Table 48 - "Fixed format sense data" specifies that the INFORMATION
> field starts at byte 3 in sense buffer resulting in the following offsets
> for the ATA PASS-THROUGH commands:
> 
> +------------+-------------------------+
> |   Field    |  Offset in sense buffer |
> +------------+-------------------------+
> | ERROR      |  3                      |
> | STATUS     |  4                      |
> | DEVICE     |  5                      |
> | COUNT(7:0) |  6                      |
> +------------+-------------------------+
> 
> COMMAND-SPECIFIC INFORMATION
> ============================
> 
> SAT-5 Table 213 - "Fixed format sense data COMMAND-SPECIFIC INFORMATION
> field for ATA PASS-THROUGH" defines the following format:
> 
> +------+-------------------+
> | Byte |        Field      |
> +------+-------------------+
> |    0 | FLAGS | LOG INDEX |
> |    1 | LBA (7:0)         |
> |    2 | LBA (15:8)        |
> |    3 | LBA (23:16)       |
> +------+-------------------+
> 
> SPC-6 Table 48 - "Fixed format sense data" specifies that
> the COMMAND-SPECIFIC-INFORMATION field starts at byte 8
> in sense buffer resulting in the following offsets for
> the ATA PASS-THROUGH commands:
> 
> Offsets of these fields in the fixed sense format are as follows:
> 
> +-------------------+-------------------------+
> |       Field       |  Offset in sense buffer |
> +-------------------+-------------------------+
> | FLAGS | LOG INDEX |  8                      |
> | LBA (7:0)         |  9                      |
> | LBA (15:8)        |  10                     |
> | LBA (23:16)       |  11                     |
> +-------------------+-------------------------+
> 
> Reported-by: Akshat Jain <akshatzen@google.com>
> Fixes: 11093cb1ef56 ("libata-scsi: generate correct ATA pass-through sense")
> Cc: stable@vger.kernel.org
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>   drivers/ata/libata-scsi.c | 26 +++++++++++++-------------
>   1 file changed, 13 insertions(+), 13 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

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

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

[Hannes Reinecke]

On 6/25/24 00:12, Igor Pylypiv wrote:
> SCSI layer clears sense_buffer in scsi_queue_rq() so there is no need for
> libata to clear it again.
> 
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>   drivers/ata/libata-scsi.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 26b1263f5c7c..1aeab6e1c8b3 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -929,8 +929,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
>   	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 +966,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 */

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

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

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

[Hannes Reinecke]

On 6/25/24 00:12, Igor Pylypiv wrote:
> ATA device id is not used in ata_to_sense_error().
> 
> Fixes: ff8072d589dc ("ata: libata: remove references to non-existing error_handler()")
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>   drivers/ata/libata-scsi.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

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

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

[Hannes Reinecke]

On 6/25/24 00:12, 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.
> 
> 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(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

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

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

[Hannes Reinecke]

On 6/25/24 00:12, 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.
> 
> For ATA errors and ATA PASS-THROUGH commands the ATA_QCFLAG_RTF_FILLED
> flag should be always set. Added WARN_ON_ONCE() checks to generate
> a warning when ATA_QCFLAG_RTF_FILLED is not set and libata needs to
> generate sense data.
> 
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>   drivers/ata/libata-scsi.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index e5669a296d81..7a8a08692ce9 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -246,6 +246,9 @@ static void ata_scsi_set_passthru_sense_fields(struct ata_queued_cmd *qc)
>   	struct ata_taskfile *tf = &qc->result_tf;
>   	unsigned char *sb = cmd->sense_buffer;
>   
> +	if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED)))
> +		return;
> +
>   	if ((sb[0] & 0x7f) >= 0x72) {
>   		unsigned char *desc;
>   		u8 len;
> @@ -928,6 +931,9 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
>   	unsigned char *sb = cmd->sense_buffer;
>   	u8 sense_key, asc, ascq;
>   
> +	if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED)))
> +		return;
> +
>   	/*
>   	 * Use ata_to_sense_error() to map status register bits
>   	 * onto sense key, asc & ascq.
> @@ -971,6 +977,10 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
>   		ata_scsi_set_sense(dev, cmd, NOT_READY, 0x04, 0x21);
>   		return;
>   	}
> +
> +	if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED)))
> +		return;
> +
>   	/* Use ata_to_sense_error() to map status register bits
>   	 * onto sense key, asc & ascq.
>   	 */

Hmm. Not sure if we really need the WARN_ON() here or whether a simple 
logging message wouldn't be sufficient; after all, we continue fine here.

Cheers,

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

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

[Igor Pylypiv]

On Tue, Jun 25, 2024 at 08:26:59AM +0200, Hannes Reinecke wrote:
> On 6/25/24 00:12, 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.
> > 
> > For ATA errors and ATA PASS-THROUGH commands the ATA_QCFLAG_RTF_FILLED
> > flag should be always set. Added WARN_ON_ONCE() checks to generate
> > a warning when ATA_QCFLAG_RTF_FILLED is not set and libata needs to
> > generate sense data.
> > 
> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > ---
> >   drivers/ata/libata-scsi.c | 10 ++++++++++
> >   1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index e5669a296d81..7a8a08692ce9 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -246,6 +246,9 @@ static void ata_scsi_set_passthru_sense_fields(struct ata_queued_cmd *qc)
> >   	struct ata_taskfile *tf = &qc->result_tf;
> >   	unsigned char *sb = cmd->sense_buffer;
> >
> > +	if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED)))
> > +		return;
> > +
> >   	if ((sb[0] & 0x7f) >= 0x72) {
> >   		unsigned char *desc;
> >   		u8 len;
> > @@ -928,6 +931,9 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> >   	unsigned char *sb = cmd->sense_buffer;
> >   	u8 sense_key, asc, ascq;
> > +	if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED)))
> > +		return;
> > +
> >   	/*
> >   	 * Use ata_to_sense_error() to map status register bits
> >   	 * onto sense key, asc & ascq.
> > @@ -971,6 +977,10 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
> >   		ata_scsi_set_sense(dev, cmd, NOT_READY, 0x04, 0x21);
> >   		return;
> >   	}
> > +
> > +	if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED)))
> > +		return;
> > +
> >   	/* Use ata_to_sense_error() to map status register bits
> >   	 * onto sense key, asc & ascq.
> >   	 */
> 
> Hmm. Not sure if we really need the WARN_ON() here or whether a simple
> logging message wouldn't be sufficient; after all, we continue fine here.
> 

My worry about adding a simple log statement is that it might cause a log
spam if things go wrong for some reason.

This code is more like a "this should never happen" comment and we always
expect ATA_QCFLAG_RTF_FILLED to be present when generating sense data
based on ATA registers.

If WARN_ON_ONCE() is too much for this case I guess we can just remove it
and silently return?

Damien, Niklas, what are your thoughts on this?

Thanks,
Igor

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

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

[Damien Le Moal]

On 6/26/24 09:30, Igor Pylypiv wrote:
> On Tue, Jun 25, 2024 at 08:26:59AM +0200, Hannes Reinecke wrote:
>> On 6/25/24 00:12, 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.
>>>
>>> For ATA errors and ATA PASS-THROUGH commands the ATA_QCFLAG_RTF_FILLED
>>> flag should be always set. Added WARN_ON_ONCE() checks to generate
>>> a warning when ATA_QCFLAG_RTF_FILLED is not set and libata needs to
>>> generate sense data.
>>>
>>> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
>>> ---
>>>   drivers/ata/libata-scsi.c | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>>> index e5669a296d81..7a8a08692ce9 100644
>>> --- a/drivers/ata/libata-scsi.c
>>> +++ b/drivers/ata/libata-scsi.c
>>> @@ -246,6 +246,9 @@ static void ata_scsi_set_passthru_sense_fields(struct ata_queued_cmd *qc)
>>>   	struct ata_taskfile *tf = &qc->result_tf;
>>>   	unsigned char *sb = cmd->sense_buffer;
>>>
>>> +	if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED)))
>>> +		return;
>>> +
>>>   	if ((sb[0] & 0x7f) >= 0x72) {
>>>   		unsigned char *desc;
>>>   		u8 len;
>>> @@ -928,6 +931,9 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
>>>   	unsigned char *sb = cmd->sense_buffer;
>>>   	u8 sense_key, asc, ascq;
>>> +	if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED)))
>>> +		return;
>>> +
>>>   	/*
>>>   	 * Use ata_to_sense_error() to map status register bits
>>>   	 * onto sense key, asc & ascq.
>>> @@ -971,6 +977,10 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
>>>   		ata_scsi_set_sense(dev, cmd, NOT_READY, 0x04, 0x21);
>>>   		return;
>>>   	}
>>> +
>>> +	if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED)))
>>> +		return;
>>> +
>>>   	/* Use ata_to_sense_error() to map status register bits
>>>   	 * onto sense key, asc & ascq.
>>>   	 */
>>
>> Hmm. Not sure if we really need the WARN_ON() here or whether a simple
>> logging message wouldn't be sufficient; after all, we continue fine here.
>>
> 
> My worry about adding a simple log statement is that it might cause a log
> spam if things go wrong for some reason.
> 
> This code is more like a "this should never happen" comment and we always
> expect ATA_QCFLAG_RTF_FILLED to be present when generating sense data
> based on ATA registers.
> 
> If WARN_ON_ONCE() is too much for this case I guess we can just remove it
> and silently return?

what about ata_dev_dbg() or ata_port_dbg() ?
No message spamming by default and if problems are detected, they can be turned
on to figure out what is going on.

> 
> Damien, Niklas, what are your thoughts on this?
> 
> Thanks,
> Igor
> 
>> 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
>>

-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 6/25/24 07:12, 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>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>

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

-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 6/25/24 07:12, Igor Pylypiv wrote:
> Correct the ATA PASS-THROUGH fixed format sense data offsets to conform
> to SPC-6 and SAT-5 specifications. Additionally, set the VALID bit to
> indicate that the INFORMATION field contains valid information.

This needs to go before patch 1. Then patch 1 modification introducing the new
function ata_scsi_set_passthru_sense_fields() will be doing so using the
corrected code.

-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 6/25/24 07:12, Igor Pylypiv wrote:
> SCSI layer clears sense_buffer in scsi_queue_rq() so there is no need for
> libata to clear it again.
> 
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>

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

-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 6/25/24 07:12, Igor Pylypiv wrote:
> ATA device id is not used in ata_to_sense_error().
> 
> Fixes: ff8072d589dc ("ata: libata: remove references to non-existing error_handler()")

Not needed. This is only cleaning up the function interface and not fixing any
bug. So please remove this tag.

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

With that done:

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

-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 6/25/24 07:12, 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.
> 
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>

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

-- 
Damien Le Moal
Western Digital Research

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

[kernel test robot]

Hi Igor,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.10-rc5 next-20240625]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Igor-Pylypiv/ata-libata-scsi-Do-not-overwrite-valid-sense-data-when-CK_COND-1/20240625-215527
base:   linus/master
patch link:    https://lore.kernel.org/r/20240624221211.2593736-4-ipylypiv%40google.com
patch subject: [PATCH v2 3/6] ata: libata-scsi: Remove redundant sense_buffer memsets
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20240626/202406261836.Q3sEjY8b-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240626/202406261836.Q3sEjY8b-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406261836.Q3sEjY8b-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/ata/libata-scsi.c: In function 'ata_gen_passthru_sense':
>> drivers/ata/libata-scsi.c:929:24: warning: unused variable 'sb' [-Wunused-variable]
     929 |         unsigned char *sb = cmd->sense_buffer;
         |                        ^~


vim +/sb +929 drivers/ata/libata-scsi.c

^1da177e4c3f41 drivers/scsi/libata-scsi.c Linus Torvalds  2005-04-16  909  
b095518ef51c37 drivers/scsi/libata-scsi.c Jeff Garzik     2005-05-12  910  /*
750426aa1ad1dd drivers/ata/libata-scsi.c  Tejun Heo       2006-11-14  911   *	ata_gen_passthru_sense - Generate check condition sense block.
b095518ef51c37 drivers/scsi/libata-scsi.c Jeff Garzik     2005-05-12  912   *	@qc: Command that completed.
b095518ef51c37 drivers/scsi/libata-scsi.c Jeff Garzik     2005-05-12  913   *
2dc8b0ba527a4a drivers/ata/libata-scsi.c  Igor Pylypiv    2024-06-24  914   *	This function is specific to the ATA pass through commands.
2dc8b0ba527a4a drivers/ata/libata-scsi.c  Igor Pylypiv    2024-06-24  915   *	Regardless of whether the command errored or not, return a sense
84a9a8cd9d0aa9 drivers/ata/libata-scsi.c  Gwendal Grignou 2013-01-18  916   *	block. If there was no error, we get the request from an ATA
84a9a8cd9d0aa9 drivers/ata/libata-scsi.c  Gwendal Grignou 2013-01-18  917   *	passthrough command, so we use the following sense data:
84a9a8cd9d0aa9 drivers/ata/libata-scsi.c  Gwendal Grignou 2013-01-18  918   *	sk = RECOVERED ERROR
84a9a8cd9d0aa9 drivers/ata/libata-scsi.c  Gwendal Grignou 2013-01-18  919   *	asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
84a9a8cd9d0aa9 drivers/ata/libata-scsi.c  Gwendal Grignou 2013-01-18  920   *      
b095518ef51c37 drivers/scsi/libata-scsi.c Jeff Garzik     2005-05-12  921   *
b095518ef51c37 drivers/scsi/libata-scsi.c Jeff Garzik     2005-05-12  922   *	LOCKING:
750426aa1ad1dd drivers/ata/libata-scsi.c  Tejun Heo       2006-11-14  923   *	None.
b095518ef51c37 drivers/scsi/libata-scsi.c Jeff Garzik     2005-05-12  924   */
750426aa1ad1dd drivers/ata/libata-scsi.c  Tejun Heo       2006-11-14  925  static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
b095518ef51c37 drivers/scsi/libata-scsi.c Jeff Garzik     2005-05-12  926  {
b095518ef51c37 drivers/scsi/libata-scsi.c Jeff Garzik     2005-05-12  927  	struct scsi_cmnd *cmd = qc->scsicmd;
e61e067227bc76 drivers/scsi/libata-scsi.c Tejun Heo       2006-05-15  928  	struct ata_taskfile *tf = &qc->result_tf;
b095518ef51c37 drivers/scsi/libata-scsi.c Jeff Garzik     2005-05-12 @929  	unsigned char *sb = cmd->sense_buffer;
b525e7731b90eb drivers/ata/libata-scsi.c  Hannes Reinecke 2016-04-04  930  	u8 sense_key, asc, ascq;
b095518ef51c37 drivers/scsi/libata-scsi.c Jeff Garzik     2005-05-12  931  
b095518ef51c37 drivers/scsi/libata-scsi.c Jeff Garzik     2005-05-12  932  	/*
b095518ef51c37 drivers/scsi/libata-scsi.c Jeff Garzik     2005-05-12  933  	 * Use ata_to_sense_error() to map status register bits
b095518ef51c37 drivers/scsi/libata-scsi.c Jeff Garzik     2005-05-12  934  	 * onto sense key, asc & ascq.
b095518ef51c37 drivers/scsi/libata-scsi.c Jeff Garzik     2005-05-12  935  	 */
058e55e120ca59 drivers/scsi/libata-scsi.c Tejun Heo       2006-04-02  936  	if (qc->err_mask ||
efcef265fd83d9 drivers/ata/libata-scsi.c  Sergey Shtylyov 2022-02-15  937  	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
efcef265fd83d9 drivers/ata/libata-scsi.c  Sergey Shtylyov 2022-02-15  938  		ata_to_sense_error(qc->ap->print_id, tf->status, tf->error,
ff8072d589dcff drivers/ata/libata-scsi.c  Hannes Reinecke 2023-07-31  939  				   &sense_key, &asc, &ascq);
06dbde5f3a4424 drivers/ata/libata-scsi.c  Hannes Reinecke 2016-04-04  940  		ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);
84a9a8cd9d0aa9 drivers/ata/libata-scsi.c  Gwendal Grignou 2013-01-18  941  	} else {
b095518ef51c37 drivers/scsi/libata-scsi.c Jeff Garzik     2005-05-12  942  		/*
11093cb1ef5614 drivers/ata/libata-scsi.c  Hannes Reinecke 2016-04-04  943  		 * ATA PASS-THROUGH INFORMATION AVAILABLE
11093cb1ef5614 drivers/ata/libata-scsi.c  Hannes Reinecke 2016-04-04  944  		 * Always in descriptor format sense.
b095518ef51c37 drivers/scsi/libata-scsi.c Jeff Garzik     2005-05-12  945  		 */
f2b1e9c6f867ec drivers/ata/libata-scsi.c  Hannes Reinecke 2021-04-27  946  		scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
11093cb1ef5614 drivers/ata/libata-scsi.c  Hannes Reinecke 2016-04-04  947  	}
^1da177e4c3f41 drivers/scsi/libata-scsi.c Linus Torvalds  2005-04-16  948  }
^1da177e4c3f41 drivers/scsi/libata-scsi.c Linus Torvalds  2005-04-16  949  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Igor Pylypiv]

On Wed, Jun 26, 2024 at 03:21:58PM +0900, Damien Le Moal wrote:
> On 6/26/24 09:30, Igor Pylypiv wrote:
> > On Tue, Jun 25, 2024 at 08:26:59AM +0200, Hannes Reinecke wrote:
> >> On 6/25/24 00:12, 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.
> >>>
> >>> For ATA errors and ATA PASS-THROUGH commands the ATA_QCFLAG_RTF_FILLED
> >>> flag should be always set. Added WARN_ON_ONCE() checks to generate
> >>> a warning when ATA_QCFLAG_RTF_FILLED is not set and libata needs to
> >>> generate sense data.
> >>>
> >>> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> >>> ---
> >>>   drivers/ata/libata-scsi.c | 10 ++++++++++
> >>>   1 file changed, 10 insertions(+)
> >>>
> >>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> >>> index e5669a296d81..7a8a08692ce9 100644
> >>> --- a/drivers/ata/libata-scsi.c
> >>> +++ b/drivers/ata/libata-scsi.c
> >>> @@ -246,6 +246,9 @@ static void ata_scsi_set_passthru_sense_fields(struct ata_queued_cmd *qc)
> >>>   	struct ata_taskfile *tf = &qc->result_tf;
> >>>   	unsigned char *sb = cmd->sense_buffer;
> >>>
> >>> +	if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED)))
> >>> +		return;
> >>> +
> >>>   	if ((sb[0] & 0x7f) >= 0x72) {
> >>>   		unsigned char *desc;
> >>>   		u8 len;
> >>> @@ -928,6 +931,9 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> >>>   	unsigned char *sb = cmd->sense_buffer;
> >>>   	u8 sense_key, asc, ascq;
> >>> +	if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED)))
> >>> +		return;
> >>> +
> >>>   	/*
> >>>   	 * Use ata_to_sense_error() to map status register bits
> >>>   	 * onto sense key, asc & ascq.
> >>> @@ -971,6 +977,10 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
> >>>   		ata_scsi_set_sense(dev, cmd, NOT_READY, 0x04, 0x21);
> >>>   		return;
> >>>   	}
> >>> +
> >>> +	if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_RTF_FILLED)))
> >>> +		return;
> >>> +
> >>>   	/* Use ata_to_sense_error() to map status register bits
> >>>   	 * onto sense key, asc & ascq.
> >>>   	 */
> >>
> >> Hmm. Not sure if we really need the WARN_ON() here or whether a simple
> >> logging message wouldn't be sufficient; after all, we continue fine here.
> >>
> > 
> > My worry about adding a simple log statement is that it might cause a log
> > spam if things go wrong for some reason.
> > 
> > This code is more like a "this should never happen" comment and we always
> > expect ATA_QCFLAG_RTF_FILLED to be present when generating sense data
> > based on ATA registers.
> > 
> > If WARN_ON_ONCE() is too much for this case I guess we can just remove it
> > and silently return?
> 
> what about ata_dev_dbg() or ata_port_dbg() ?
> No message spamming by default and if problems are detected, they can be turned
> on to figure out what is going on.

ata_dev_dbg() should work. Updated the patch in v3.

Thank you!
Igor
> 
> > 
> > Damien, Niklas, what are your thoughts on this?
> > 
> > Thanks,
> > Igor
> > 
> >> 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
> >>
> 
> -- 
> Damien Le Moal
> Western Digital Research
>