Re: [PATCH v3 2/6] ata: libata-scsi: Do not overwrite valid sense data when CK_COND=1

[Niklas Cassel <cassel@kernel.org>]

On Thu, Jun 27, 2024 at 09:54:06PM +0000, Igor Pylypiv wrote:
> 
> Thank you, Niklas! I agree that this code is too complicated and should be
> simplified. I don't think we should change the code too much in this patch
> since it is going to be backported to stable releases.
> 
> Would you mind sending a patch for the proposed simplifications following
> this patch series?
> 

I would prefer if we changed it as part of this commit to be honest.


I also re-read the SAT spec, and found that it says that:
"""
If the CK_COND bit is set to:
a) one, then the SATL shall return a status of CHECK CONDITION upon ATA command completion,
without interpreting the contents of the STATUS field and returning the ATA fields from the request
completion in the sense data as specified in table 209; and
b) zero, then the SATL shall terminate the command with CHECK CONDITION status only if an error
occurs in processing the command. See clause 11 for a description of ATA error conditions.
"""

So it seems quite clear that if CK_COND == 1, we should set CHECK CONDITION,
so that answers the question/uncertainty I asked/expressed in earlier emails.


I think this patch (which should be applied on top of your v3 series),
makes the code way easier to read/understand:

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index d5874d4b9253..5b211551ac10 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1659,26 +1656,27 @@ 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);
-       int need_passthru_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_passthru_sense)) {
-               if (!(qc->flags & ATA_QCFLAG_SENSE_VALID))
+       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);
-       } else if (need_sense) {
+               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 {
                /* Keep the SCSI ML and status byte, clear host byte. */