ATA PASS-THROUGH sense data fixes

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

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.)

Quoted 53 lines · 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);
}

Quoted 6 lines

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.

Quoted 1 line · Hannes Reinecke

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).

Quoted 13 lines · Niklas Cassel

Quoted 1 line · Hannes Reinecke

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.

Quoted 2 lines

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:

drivers/ata/libata-scsi.c +2 -5

index d5874d4b9253..5b211551ac10 100644
@@ -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

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 [email protected]

Reviewed-by Hannes Reinecke <[email protected]>

Reviewed-by Damien Le Moal <[email protected]>

Signed-off-by Igor Pylypiv <[email protected]>

---
 drivers/ata/libata-scsi.c | 158 +++++++++++++++++++++-----------------
 1 file changed, 86 insertions(+), 72 deletions(-)

drivers/ata/libata-scsi.c +86 -72

index a9e44ad4c2de..26b1263f5c7c 100644
@@ -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

Quoted 14 lines · 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.

Quoted 4 lines

Cc [email protected]

Reviewed-by Hannes Reinecke <[email protected]>

Reviewed-by Damien Le Moal <[email protected]>

Signed-off-by Igor Pylypiv <[email protected]>

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...

Quoted 211 lines

---
 drivers/ata/libata-scsi.c | 158 +++++++++++++++++++++-----------------
 1 file changed, 86 insertions(+), 72 deletions(-)

drivers/ata/libata-scsi.c +86 -72

index a9e44ad4c2de..26b1263f5c7c 100644
@@ -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);
-		}
 

Quoted 2 lines

drivers/ata/libata-scsi.c +0 -0

 		/* SCSI EH automatically locks door if sdev->locked is
 		 * set.  Sometimes door lock request continues to

Signature · 2 lines

Damien Le Moal
Western Digital Research

Quoted 15 lines · Damien Le Moal

Quoted 14 lines · 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.

Quoted 8 lines

Quoted 4 lines

Cc [email protected]

Reviewed-by Hannes Reinecke <[email protected]>

Reviewed-by Damien Le Moal <[email protected]>

Signed-off-by Igor Pylypiv <[email protected]>

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?

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 [email protected]

Reviewed-by Hannes Reinecke <[email protected]>

Reviewed-by Damien Le Moal <[email protected]>

Signed-off-by Igor Pylypiv <[email protected]>

---
 drivers/ata/libata-scsi.c | 158 +++++++++++++++++++++-----------------
 1 file changed, 86 insertions(+), 72 deletions(-)

drivers/ata/libata-scsi.c +78 -61

index a9e44ad4c2de..26b1263f5c7c 100644
@@ -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

drivers/ata/libata-scsi.c +4 -8

+	} 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

Signature · 1 line

2.45.2.803.g4e1b14247a-goog

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.

Quoted 194 lines · 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 [email protected]

Reviewed-by Hannes Reinecke <[email protected]>

Reviewed-by Damien Le Moal <[email protected]>

Signed-off-by Igor Pylypiv <[email protected]>

---
 drivers/ata/libata-scsi.c | 158 +++++++++++++++++++++-----------------
 1 file changed, 86 insertions(+), 72 deletions(-)

drivers/ata/libata-scsi.c +78 -61

index a9e44ad4c2de..26b1263f5c7c 100644
@@ -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

drivers/ata/libata-scsi.c static void ata_scsi_qc_complete(struct ata_queued_cmd *qc) · line 1668

Quoted 10 lines

drivers/ata/libata-scsi.c +4 -3

@@ -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.

Quoted 30 lines

drivers/ata/libata-scsi.c +4 -8

+	} 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

Signature · 1 line

2.45.2.803.g4e1b14247a-goog

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?

Quoted 8 lines · Igor Pylypiv

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:

drivers/ata/libata-scsi.c +6 -6

index d5874d4b9253..5b211551ac10 100644
@@ -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
         */

Quoted 23 lines · Niklas Cassel

Quoted 8 lines · Igor Pylypiv

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. """

Quoted 7 lines

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?

Quoted 34 lines

drivers/ata/libata-scsi.c +8 -9

index d5874d4b9253..5b211551ac10 100644
@@ -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)

drivers/ata/libata-scsi.c static void ata_scsi_qc_complete(struct ata_queued_cmd *qc) · line 1678

Quoted 5 lines

drivers/ata/libata-scsi.c +2 -1

                        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?

Quoted 35 lines · Igor Pylypiv

Quoted 23 lines · Niklas Cassel

Quoted 8 lines · Igor Pylypiv

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. """

Quoted 7 lines

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.

Quoted 2 lines

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

Quoted 35 lines

Quoted 35 lines

drivers/ata/libata-scsi.c +8 -9

index d5874d4b9253..5b211551ac10 100644
@@ -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)

Quoted 8 lines

drivers/ata/libata-scsi.c static void ata_scsi_qc_complete(struct ata_queued_cmd *qc) · line 1678

Quoted 5 lines

drivers/ata/libata-scsi.c +2 -1

                        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

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 <[email protected]>

Reviewed-by Damien Le Moal <[email protected]>

Signed-off-by Igor Pylypiv <[email protected]>

---
 drivers/ata/libahci.c     | 10 ----------
 drivers/ata/libata-core.c |  8 ++++++++
 2 files changed, 8 insertions(+), 10 deletions(-)

drivers/ata/libahci.c +0 -10

index 83431aae74d8..0728d445e531 100644
@@ -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)

drivers/ata/libata-core.c +0 -0

index e1bf8a19b3c8..a9fc3ec9300f 100644
@@ -4801,8 +4801,16 @@ static void fill_result_tf(struct ata_queued_cmd *qc)
 {

drivers/ata/libata-core.c +1 -0

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

Signature · 1 line

2.45.2.803.g4e1b14247a-goog

Quoted 58 lines · 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 <[email protected]>

Reviewed-by Damien Le Moal <[email protected]>

Signed-off-by Igor Pylypiv <[email protected]>

---
 drivers/ata/libahci.c     | 10 ----------
 drivers/ata/libata-core.c |  8 ++++++++
 2 files changed, 8 insertions(+), 10 deletions(-)

drivers/ata/libahci.c +0 -10

index 83431aae74d8..0728d445e531 100644
@@ -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)

drivers/ata/libata-core.c +0 -0

index e1bf8a19b3c8..a9fc3ec9300f 100644
@@ -4801,8 +4801,16 @@ static void fill_result_tf(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
 

drivers/ata/libata-core.c static void fill_result_tf(struct ata_queued_cmd *qc) · line 4811

Quoted 8 lines

drivers/ata/libata-core.c +7 -0

+	/*
+	 * 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:

drivers/ata/libahci.c +2 -0

index 0728d445e531..fdfa7b266218 100644
@@ -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);

Quoted 11 lines

drivers/ata/libata-core.c +1 -0

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

Signature · 1 line

2.45.2.803.g4e1b14247a-goog

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 <[email protected]>

---
 drivers/ata/libata-scsi.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

drivers/ata/libata-scsi.c +18 -0

index 02af4d5d5799..d5874d4b9253 100644
@@ -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.
 	 */

Quoted 5 lines · 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 <[email protected]>

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 <[email protected]>

Quoted 47 lines

---
 drivers/ata/libata-scsi.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

drivers/ata/libata-scsi.c +13 -0

index 02af4d5d5799..d5874d4b9253 100644
@@ -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;
 	}
+

Quoted 8 lines

drivers/ata/libata-scsi.c +5 -0

+	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.
 	 */

Signature · 2 lines

Damien Le Moal
Western Digital Research

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 <[email protected]>

---
  drivers/ata/libata-scsi.c | 18 ++++++++++++++++++
  1 file changed, 18 insertions(+)

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 <[email protected]>

---
 drivers/ata/libata-scsi.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

drivers/ata/libata-scsi.c +18 -0

index 02af4d5d5799..d5874d4b9253 100644
@@ -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.
 	 */

Signature · 1 line

2.45.2.803.g4e1b14247a-goog

Quoted 58 lines · 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 <[email protected]>

---
 drivers/ata/libata-scsi.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

drivers/ata/libata-scsi.c +18 -0

index 02af4d5d5799..d5874d4b9253 100644
@@ -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

Quoted 5 lines

drivers/ata/libata-scsi.c +0 -0

 	 * onto sense key, asc & ascq.
 	 */

Signature · 1 line

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