ata: libata-scsi: Do not overwrite valid sense data when CK_COND=1

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.

Additionally, always set SAM_STAT_CHECK_CONDITION when CK_COND=1 because
SAT specification mandates that SATL shall return CHECK CONDITION if
the CK_COND bit is set.

The ATA PASS-THROUGH handling logic in ata_scsi_qc_complete() is hard
to read/understand. Improve the readability of the code by moving checks
into self-explanatory boolean variables.

Cc: stable@vger.kernel.org # 4.19+
Co-developed-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
Link: https://lore.kernel.org/r/20240702024735.1152293-3-ipylypiv@google.com
Signed-off-by: Niklas Cassel <cassel@kernel.org>

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index a0c68b0a00c410c006897ff355754bb124dc6d57..a9d2a1cf4c3d08b7bf950c2efe301608ac07dc73 100644 (file)
--- 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;
-       }
 }
 
 /**
@@ -1632,26 +1647,32 @@ 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);
+       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_sense))
-               ata_gen_passthru_sense(qc);
-       else if (need_sense)
+       if (is_ata_passthru && (is_ck_cond_request || is_error || have_sense)) {
+               if (!have_sense)
+                       ata_gen_passthru_sense(qc);
+               ata_scsi_set_passthru_sense_fields(qc);
+               if (is_ck_cond_request)
+                       set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION);
+       } else if (is_error && !have_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 +2611,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