@@ -1401,6 +1401,43 @@ unsigned int atapi_eh_tur(struct ata_device *dev, u8 *r_sense_key)
return err_mask;
}
+/**
+ * ata_eh_decide_disposition - Disposition a qc based on sense data
+ * @qc: qc to examine
+ *
+ * For a regular SCSI command, the SCSI completion callback (scsi_done())
+ * will call scsi_complete(), which will call scsi_decide_disposition(),
+ * which will call scsi_check_sense(). scsi_complete() finally calls
+ * scsi_finish_command(). This is fine for SCSI, since any eventual sense
+ * data is usually returned in the completion itself (without invoking SCSI
+ * EH). However, for a QC, we always need to fetch the sense data
+ * explicitly using SCSI EH.
+ *
+ * A command that is completed via SCSI EH will instead be completed using
+ * scsi_eh_flush_done_q(), which will call scsi_finish_command() directly
+ * (without ever calling scsi_check_sense()).
+ *
+ * For a command that went through SCSI EH, it is the responsibility of the
+ * SCSI EH strategy handler to call scsi_decide_disposition(), see e.g. how
+ * scsi_eh_get_sense() calls scsi_decide_disposition() for SCSI LLDDs that
+ * do not get the sense data as part of the completion.
+ *
+ * Thus, for QC commands that went via SCSI EH, we need to call
+ * scsi_check_sense() ourselves, similar to how scsi_eh_get_sense() calls
+ * scsi_decide_disposition(), which calls scsi_check_sense(), in order to
+ * set the correct SCSI ML byte (if any).
+ *
+ * LOCKING:
+ * EH context.
+ *
+ * RETURNS:
+ * SUCCESS or FAILED or NEEDS_RETRY or ADD_TO_MLQUEUE
+ */
+enum scsi_disposition ata_eh_decide_disposition(struct ata_queued_cmd *qc)
+{
+ return scsi_check_sense(qc->scsicmd);
+}
+
/**
* ata_eh_request_sense - perform REQUEST_SENSE_DATA_EXT
* @qc: qc to perform REQUEST_SENSE_SENSE_DATA_EXT to
@@ -1627,7 +1664,8 @@ static unsigned int ata_eh_analyze_tf(struct ata_queued_cmd *qc)
}
if (qc->flags & ATA_QCFLAG_SENSE_VALID) {
- enum scsi_disposition ret = scsi_check_sense(qc->scsicmd);
+ enum scsi_disposition ret = ata_eh_decide_disposition(qc);
+
/*
* SUCCESS here means that the sense code could be
* evaluated and should be passed to the upper layers
@@ -1942,11 +1980,10 @@ static int ata_eh_read_sense_success_non_ncq(struct ata_link *link)
return -EIO;
/*
- * If we have sense data, call scsi_check_sense() in order to set the
- * correct SCSI ML byte (if any). No point in checking the return value,
- * since the command has already completed successfully.
+ * No point in checking the return value, since the command has already
+ * completed successfully.
*/
- scsi_check_sense(qc->scsicmd);
+ ata_eh_decide_disposition(qc);
return 0;
}
@@ -1455,12 +1455,10 @@ int ata_eh_read_sense_success_ncq_log(struct ata_link *link)
qc->flags |= ATA_QCFLAG_SENSE_VALID;
/*
- * If we have sense data, call scsi_check_sense() in order to
- * set the correct SCSI ML byte (if any). No point in checking
- * the return value, since the command has already completed
- * successfully.
+ * No point in checking the return value, since the command has
+ * already completed successfully.
*/
- scsi_check_sense(qc->scsicmd);
+ ata_eh_decide_disposition(qc);
}
return ret;
@@ -162,6 +162,7 @@ extern void ata_eh_finish(struct ata_port *ap);
extern int ata_ering_map(struct ata_ering *ering,
int (*map_fn)(struct ata_ering_entry *, void *),
void *arg);
+enum scsi_disposition ata_eh_decide_disposition(struct ata_queued_cmd *qc);
extern unsigned int atapi_eh_tur(struct ata_device *dev, u8 *r_sense_key);
extern unsigned int atapi_eh_request_sense(struct ata_device *dev,
u8 *sense_buf, u8 dfl_sense_key);
Every time I see libata code calling scsi_check_sense(), I get confused why the code path that is working fine for SCSI code, is not sufficient for libata code. The reason is that SCSI usually gets the sense data as part of the completion, and will thus automatically call scsi_check_sense(), which will set the SCSI ML byte (if any). However, for libata queued commands, we always need to fetch the sense data via SCSI EH, and thus do not get the luxury of having scsi_check_sense() called automatically. Add a new helper, ata_eh_decide_disposition(), that has a ata_eh_ prefix to more clearly highlight that this is only needed for code called by EH, while also having a similar name to scsi_decide_disposition(), such that it is easier to compare the libata code with the equivalent SCSI code. Also add a big kdoc comment explaining why this helper is called/needed in the first place. Signed-off-by: Niklas Cassel <cassel@kernel.org> --- Changes since v1: -Add an empty line after the preexisting variable declaration in ata_eh_analyze_tf(), as suggested by Damien. drivers/ata/libata-eh.c | 47 ++++++++++++++++++++++++++++++++++----- drivers/ata/libata-sata.c | 8 +++---- drivers/ata/libata.h | 1 + 3 files changed, 46 insertions(+), 10 deletions(-)