Re: [PATCH] ata: libata-scsi: do not lose CHECK CONDITION for failed ATAPI commands

[Niklas Cassel <cassel@kernel.org>]

Hello Hengyu,

On Thu, Sep 24, 2026 at 02:04:14PM -0400, hengyul@cs.unc.edu wrote:
> From: Hengyu Liang <hengyul@cs.unc.edu>
> 
> Since commit 2e1d2e65e773 ("ata: libata-scsi: terminate deferred commands
> on time out"), atapi_qc_complete() sets SAM_STAT_CHECK_CONDITION for a
> failed command only if cmd->result is zero, so that the DID_TIME_OUT or
> DID_REQUEUE host byte of a terminated command is preserved.
> 
> However, cmd->result can also be non-zero for a regular failed ATAPI
> command completed through libata EH: ata_eh_analyze_tf() calls
> ata_eh_decide_disposition(), that is, scsi_check_sense(), on the sense
> data obtained with REQUEST SENSE, and scsi_check_sense() sets the SCSI
> midlayer internal byte of the result for some sense codes, e.g.
> SCSIML_STAT_TGT_FAILURE for ILLEGAL REQUEST with ASC 0x20 (INVALID
> COMMAND OPERATION CODE) or 0x24 (INVALID FIELD IN CDB), or
> SCSIML_STAT_MED_ERROR for MEDIUM ERROR with ASC 0x11 (UNRECOVERED READ
> ERROR). For such commands, atapi_qc_complete() keeps the result as is and
> the command completes with a GOOD status byte, even though it failed and
> valid sense data is available.
> 
> As a result, passthrough users see these failed ATAPI commands as
> successful. For example, with the emulated IDE CD-ROM drive of QEMU and
> no medium loaded, a PLAY AUDIO MSF command issued with SG_IO completes
> with status 0x00 (and no DRIVER_SENSE) instead of 0x02 (CHECK CONDITION)
> with sense key ILLEGAL REQUEST, ASC/ASCQ 0x20/0x00, and the CDROM ioctls
> CDROMPLAYTRKIND, CDROMSUBCHNL, CDROMPAUSE, CDROMRESUME, CDROMPLAYMSF and
> CDROM_GET_MCN return 0 instead of -ENOMEDIUM or -EOPNOTSUPP.
> 
> Fix this by preserving the result only if its host byte is set, and by
> otherwise completing the command with SAM_STAT_CHECK_CONDITION, as was
> done before that commit.
> 
> Fixes: 2e1d2e65e773 ("ata: libata-scsi: terminate deferred commands on time out")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hengyu Liang <hengyul@cs.unc.edu>
> ---

I think it is better if you set only the status byte with set_status_byte()
instead of overwriting cmd->result, so that the SCSI midlayer byte set by
scsi_check_sense() is kept.

Something like:

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 8f9aa97a519d..fb5517d51095 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3059,10 +3059,15 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
                if (qc->cdb[0] == ALLOW_MEDIUM_REMOVAL && qc->dev->sdev)
                        qc->dev->sdev->locked = 0;
 
-               if (cmd->result)
-                       ata_scsi_qc_done(qc, false, 0);
-               else
-                       ata_scsi_qc_done(qc, true, SAM_STAT_CHECK_CONDITION);
+               /*
+                * Report CHECK CONDITION unless the command was terminated
+                * with a host byte set. Only set the status byte, so that the
+                * SCSI midlayer internal byte, which scsi_check_sense() may
+                * have set from the sense data, is preserved.
+                */
+               if (get_host_byte(cmd) == DID_OK)
+                       set_status_byte(cmd, SAM_STAT_CHECK_CONDITION);
+               ata_scsi_qc_done(qc, false, 0);
                goto schedule_deferred;
        }