On Mon, Jul 01, 2024 at 07:57:52PM +0000, Igor Pylypiv wrote:
Current ata_gen_passthru_sense() code performs two actions:
- Generates sense data based on the ATA 'status' and ATA 'error' fields.
- 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 # 4.19+ Reviewed-by: Hannes Reinecke hare@suse.de Reviewed-by: Damien Le Moal dlemoal@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 a9e44ad4c2de..26b1263f5c7c 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;
- 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 -- 2.45.2.803.g4e1b14247a-goog
Reviewed-by: Niklas Cassel cassel@kernel.org
However: I really think that this patch should be squashed with patch 8/8.
For more details, see my reply to patch 8/8.