Re: [PATCH] ata: libata-eh: Clear scsicmd->result when setting SAM_STAT_CHECK_CONDITION

[Niklas Cassel]

On Thu, Sep 05, 2024 at 10:33:38AM +0200, Niklas Cassel wrote:
> There are many different paths a QC can take via EH, e.g. policy 0xD NCQ
> commands will not fetch sense data via ata_eh_request_sense(), so clearing
> the host byte in ata_scsi_qc_complete() should be fine, otherwise we need
> to do a similar change to yours in all the different code paths that sets
> sense data ...which might actually be something that makes sense, but then
> I would expect a patch series that changes all the locations where we set
> sense data, not just in ata_eh_analyze_tf(), and then drops the clearing in
> ata_scsi_qc_complete() (which was introduced in commit 7574a8377c7a ("ata:
> libata-scsi: do not overwrite SCSI ML and status bytes")).

I tried to implement the suggestion above, it looks like this:

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index e4023fc288ac..ff4945a8f647 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4824,6 +4824,14 @@ void ata_qc_free(struct ata_queued_cmd *qc)
 		qc->tag = ATA_TAG_POISON;
 }
 
+void ata_qc_set_sense_valid_flag(struct ata_queued_cmd *qc)
+{
+	qc->flags |= ATA_QCFLAG_SENSE_VALID;
+
+	/* Keep the SCSI ML and status byte, clear host byte. */
+	qc->scsicmd->result &= 0x0000ffff;
+}
+
 void __ata_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap;
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 7de97ee8e78b..df83251601dc 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1482,7 +1482,8 @@ static bool ata_eh_request_sense(struct ata_queued_cmd *qc)
 			scsi_build_sense_buffer(dev->flags & ATA_DFLAG_D_SENSE,
 						cmd->sense_buffer, tf.lbah,
 						tf.lbam, tf.lbal);
-			qc->flags |= ATA_QCFLAG_SENSE_VALID;
+			ata_qc_set_sense_valid_flag(qc);
+
 			return true;
 		}
 	} else {
@@ -1657,7 +1658,7 @@ static unsigned int ata_eh_analyze_tf(struct ata_queued_cmd *qc)
 						qc->scsicmd->sense_buffer,
 						qc->result_tf.error >> 4);
 			if (!tmp)
-				qc->flags |= ATA_QCFLAG_SENSE_VALID;
+				ata_qc_set_sense_valid_flag(qc);
 			else
 				qc->err_mask |= tmp;
 		}
@@ -2049,7 +2050,7 @@ static void ata_eh_get_success_sense(struct ata_link *link)
 
 		/* This success command had sense data, but we failed to get. */
 		ata_scsi_set_sense(dev, qc->scsicmd, ABORTED_COMMAND, 0, 0);
-		qc->flags |= ATA_QCFLAG_SENSE_VALID;
+		ata_qc_set_sense_valid_flag(qc);
 	}
 	ata_eh_done(link, dev, ATA_EH_GET_SUCCESS_SENSE);
 }
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index ea6126c139af..c3bbe8877376 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1452,7 +1452,7 @@ int ata_eh_read_sense_success_ncq_log(struct ata_link *link)
 		scsi_build_sense_buffer(dev->flags & ATA_DFLAG_D_SENSE,
 					qc->scsicmd->sense_buffer, sk,
 					asc, ascq);
-		qc->flags |= ATA_QCFLAG_SENSE_VALID;
+		ata_qc_set_sense_valid_flag(qc);
 
 		/*
 		 * No point in checking the return value, since the command has
@@ -1539,7 +1539,7 @@ void ata_eh_analyze_ncq_error(struct ata_link *link)
 					   ascq);
 			ata_scsi_set_sense_information(dev, qc->scsicmd,
 						       &qc->result_tf);
-			qc->flags |= ATA_QCFLAG_SENSE_VALID;
+			ata_qc_set_sense_valid_flag(qc);
 		}
 	}
 
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 3a442f564b0d..6a90062c8b55 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1680,9 +1680,6 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 			set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION);
 	} else if (is_error && !have_sense) {
 		ata_gen_ata_sense(qc);
-	} else {
-		/* Keep the SCSI ML and status byte, clear host byte. */
-		cmd->result &= 0x0000ffff;
 	}
 
 	ata_qc_done(qc);
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index ab4bd44ba17c..85d19d6edcea 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -70,6 +70,7 @@ extern int ata_down_xfermask_limit(struct ata_device *dev, unsigned int sel);
 extern unsigned int ata_dev_set_feature(struct ata_device *dev,
 					u8 subcmd, u8 action);
 extern void ata_qc_free(struct ata_queued_cmd *qc);
+extern void ata_qc_set_sense_valid_flag(struct ata_queued_cmd *qc);
 extern void ata_qc_issue(struct ata_queued_cmd *qc);
 extern void __ata_qc_complete(struct ata_queued_cmd *qc);
 extern int atapi_check_dma(struct ata_queued_cmd *qc);



I guess the obvious advantage that I can see is that we would do the
right thing regardless of qc->complete_fn.

qc->complete_fn can be any of:
ata_qc_complete_internal()
ata_scsi_qc_complete()
atapi_qc_complete()
ata_scsi_report_zones_complete()

Instead of only doing the right thing if:
qc->complete_fn == ata_scsi_qc_complete().

Thoughts?


Kind regards,
Niklas