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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: midlayer byte wiped in atapi_qc_complete() breaks error translation
--

commit a67971887516df13f132f00266219f908c09e9aa
Author: Hengyu Liang <hengyul@cs.unc.edu>

ata: libata-scsi: do not lose CHECK CONDITION for failed ATAPI commands

This commit modifies atapi_qc_complete() to preserve cmd->result only if the
host byte is set. For other errors, it sets the status to
SAM_STAT_CHECK_CONDITION to avoid losing the CHECK CONDITION status for
failed ATAPI commands.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index d2160ee7ca7d4..436ddf5ed4748 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3020,7 +3020,14 @@ 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)
> +		/*
> +		 * Only preserve the result of a command that was terminated
> +		 * with a host byte set, e.g. DID_TIME_OUT. A failed command
> +		 * may also have the SCSI midlayer internal byte set by
> +		 * scsi_check_sense(), which must not prevent reporting the
> +		 * CHECK CONDITION status.
> +		 */
> +		if (get_host_byte(cmd) != DID_OK)
>  			ata_scsi_qc_done(qc, false, 0);
>  		else
>  			ata_scsi_qc_done(qc, true, SAM_STAT_CHECK_CONDITION);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this assignment destroy the upper 24 bits of cmd->result, completely
erasing the midlayer byte?

When an ATAPI command fails, ata_eh_analyze_tf() fetches sense data and
delegates to scsi_check_sense(), which translates sense keys (e.g.,
MEDIUM_ERROR) by setting the SCSI midlayer internal byte in cmd->result.

When atapi_qc_complete() calls ata_scsi_qc_done() with set_result = true:

drivers/ata/libata-scsi.c:ata_scsi_qc_done() {
    ...
	if (set_result)
		cmd->result = scmd_result;
    ...
}

This directly overwrites the 32-bit field and silently erases the midlayer
byte previously set by scsi_check_sense().

Consequently, scsi_result_to_blk_status() sees an empty midlayer byte and
translates legitimate ATAPI block errors to generic BLK_STS_IOERR (-EIO)
instead of the specific status (like BLK_STS_MEDIUM), breaking block layer
error translation.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924180414.2102946-1-hengyul@cs.unc.edu?part=1