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
