Re: [PATCH v4 8/8] ata: libata-scsi: Make ata_scsi_qc_complete() more readable

[Niklas Cassel <cassel@kernel.org>]

On Mon, Jul 01, 2024 at 07:57:58PM +0000, Igor Pylypiv wrote:
> 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.
> 
> 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.
> 
> Co-developed-by: Niklas Cassel <cassel@kernel.org>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>  drivers/ata/libata-scsi.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index a66c177b6087..8f21b3b0bc75 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1659,26 +1659,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. */
> -- 
> 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 2/8.

Sure, the changes in this patch will make it harder to backport...
but, even patch 2/8 will be a pain to backport...

And this patch will need to have CC: stable and be backported as well...
(such that we always set CHECK_CONDITION when CK_COND=1), so I strongly
suggest that we should squash these, since it will probably be way simpler
to backport the patch that is "patch 2/8 squashed with this patch",
compared to backporting patch 2/8, and then backporting this patch.
(That would just give two patches that will need manual backport, rather
than one patch that needs manual backport.)

Both of these are fixing incorrect sense data for ATA passthough commands
anyway.


Kind regards,
Niklas