[v3,2/6] 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.

Cc: stable@vger.kernel.org
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
 drivers/ata/libata-scsi.c | 158 +++++++++++++++++++++-----------------
 1 file changed, 86 insertions(+), 72 deletions(-)

On 6/27/24 08:04, Igor Pylypiv wrote:
> 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.
> 
> Cc: stable@vger.kernel.org
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>

I wonder if we can find the patch that introduced the bug in the first place so
that we can add a Fixes tag. I have not checked. This may have been wrong since
a long time ago...

> ---
>  drivers/ata/libata-scsi.c | 158 +++++++++++++++++++++-----------------
>  1 file changed, 86 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index a9e44ad4c2de..26b1263f5c7c 100644
> --- 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;
> -	}
>  }
>  
>  /**
> @@ -1634,6 +1649,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
>  	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);
>  
>  	/* For ATA pass thru (SAT) commands, generate a sense block if
>  	 * user mandated it or if there's an error.  Note that if we
> @@ -1645,13 +1662,16 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
>  	 * 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)
> +	    ((cdb[2] & 0x20) || need_passthru_sense)) {
> +		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID))
> +			ata_gen_passthru_sense(qc);
> +		ata_scsi_set_passthru_sense_fields(qc);
> +	} else if (need_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 +2610,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

On Wed, Jun 26, 2024 at 11:04:07PM +0000, Igor Pylypiv wrote:
> 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.
> 
> Cc: stable@vger.kernel.org
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>  drivers/ata/libata-scsi.c | 158 +++++++++++++++++++++-----------------
>  1 file changed, 86 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index a9e44ad4c2de..26b1263f5c7c 100644
> --- 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;
> -	}
>  }
>  
>  /**
> @@ -1634,6 +1649,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
>  	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);
>  
>  	/* For ATA pass thru (SAT) commands, generate a sense block if
>  	 * user mandated it or if there's an error.  Note that if we
> @@ -1645,13 +1662,16 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
>  	 * 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)
> +	    ((cdb[2] & 0x20) || need_passthru_sense)) {
> +		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID))
> +			ata_gen_passthru_sense(qc);
> +		ata_scsi_set_passthru_sense_fields(qc);

This whole logic looks too complicated to me.

Can't we do something to make it easier to read, e.g. something like:


{
	if (command_is_ata_passthru(cdb)) {
		handle_passthru_completion(qc);
		ata_qc_done();
		return;
	}

	if (need_sense)
		ata_gen_ata_sense(qc);
	else
		/* Keep the SCSI ML and status byte, clear host byte. */ 
		cmd->result &= 0x0000ffff;

	ata_qc_done();
}

And then put the complicated logic in handle_passthru_command() ?

/* CASES:
* a) IF error command (ERROR or DF set) and ATA_QCFLAG_SENSE_VALID (SK+ASC+ASCQ) set:
*    - don't touch SK/ASC/ASCQ in sense_buffer
*    - set ATA registers in fixed format or descriptor format (based on dev->flags ATA_DFLAG_D_SENSE)
* b) IF error command (ERROR or DF set) and ATA_QCFLAG_SENSE_VALID (SK+ASC+ASCQ) not set:
*    - generate the SK+ASC+ASCQ from ATA status and ATA error, and
*    - set CHECK_CONDITION in cmd->result (scsi_build_sense() does this)
*    - set ATA registers in fixed format or descriptor format (based on dev->flags ATA_DFLAG_D_SENSE)
* c) IF success command (ERROR and DF not set), and ATA_QCFLAG_SENSE_VALID set, CK_COND set:
*    - don't touch SK/ASC/ASCQ in sense_buffer
*    - set ATA registers in fixed format or descriptor format (based on dev->flags ATA_DFLAG_D_SENSE)
*    - we should probably set CHECK_CONDITION status byte in cmd->result here.... but not sure...
* d) IF success command (ERROR and DF not set), and ATA_QCFLAG_SENSE_VALID set, CK_COND not set:
*    - don't touch SK/ASC/ASCQ in sense_buffer
*    - don't fill ATA registers
*    - keep the SCSI ML and status byte, clear host byte, in cmd->result
* e) IF success command (ERROR and DF not set), and ATA_QCFLAG_SENSE_VALID not set, CK_COND set:
*    - set SK to "RECOVERED ERROR" ASCQ to "ATA PASS-THROUGH INFORMATION AVAILABLE", ASC to 0.
*    - set ATA registers in fixed format or descriptor format (based on dev->flags ATA_DFLAG_D_SENSE)
*    - set CHECK_CONDITION status byte in cmd->result
* f) IF success command (ERROR and DF not set), and ATA_QCFLAG_SENSE_VALID not set, CK_COND not set:
*    - keep the SCSI ML and status byte, clear host byte, in cmd->result
*/
static void ata_handle_passthru_completion(struct ata_queued_cmd *qc);

So we should only copy the ATA registers when CK_COND is set, or if ERROR bit
or DF bit was set. CK_COND being set in the cdb (input command) basically means
that the user requested that the ATA registers should be copied into the sense
buffer (in the result).

The only tricky case is if we should set CHECK_CONDITION in case c) or not.
All other cases seems quite clear by looking at the SAT spec.


> +	} else if (need_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 +2610,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
> -- 
> 2.45.2.803.g4e1b14247a-goog
>

On Thu, Jun 27, 2024 at 04:14:09PM +0200, Niklas Cassel wrote:
> 
> The only tricky case is if we should set CHECK_CONDITION in case c) or not.
> All other cases seems quite clear by looking at the SAT spec.

I think we should set CHECK_CONDITION in case c),
even if SK+ASCQ+ASC is not "ATA PASS-THROUGH INFORMATION AVAILABLE".

That way we at least align CK_COND with CHECK_CONDITION, which is
most likely what the user (and spec writers) expect.


Kind regards,
Niklas

On Thu, Jun 27, 2024 at 09:16:09AM +0900, Damien Le Moal wrote:
> On 6/27/24 08:04, Igor Pylypiv wrote:
> > 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.
> > 
> > Cc: stable@vger.kernel.org
> > Reviewed-by: Hannes Reinecke <hare@suse.de>
> > Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> 
> I wonder if we can find the patch that introduced the bug in the first place so
> that we can add a Fixes tag. I have not checked. This may have been wrong since
> a long time ago...

This code was first introduced in 2005 in commit b095518ef51c3 ("[libata]
ATA passthru (arbitrary ATA command execution)").

ATA_QCFLAG_SENSE_VALID was introduced a year later in commit 9ec957f2002b
("[PATCH] libata-eh-fw: add flags and operations for new EH").

IIUC, ATA_QCFLAG_SENSE_VALID has not been set for ATA drives until 2016
when the support for fetching the sense data was added in 5b01e4b9efa0
("libata: Implement NCQ autosense") and commit e87fd28cf9a2d ("libata:
Implement support for sense data reporting").

To me none of the commits looks like a good candidate for the Fixes tag.
What are your thoughts on this?

> 
> > ---
> >  drivers/ata/libata-scsi.c | 158 +++++++++++++++++++++-----------------
> >  1 file changed, 86 insertions(+), 72 deletions(-)
> > 
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index a9e44ad4c2de..26b1263f5c7c 100644
> > --- 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;
> > -	}
> >  }
> >  
> >  /**
> > @@ -1634,6 +1649,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> >  	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);
> >  
> >  	/* For ATA pass thru (SAT) commands, generate a sense block if
> >  	 * user mandated it or if there's an error.  Note that if we
> > @@ -1645,13 +1662,16 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> >  	 * 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)
> > +	    ((cdb[2] & 0x20) || need_passthru_sense)) {
> > +		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID))
> > +			ata_gen_passthru_sense(qc);
> > +		ata_scsi_set_passthru_sense_fields(qc);
> > +	} else if (need_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 +2610,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
> 
> -- 
> Damien Le Moal
> Western Digital Research
> 
Thanks,
Igor

On Thu, Jun 27, 2024 at 04:14:09PM +0200, Niklas Cassel wrote:
> On Wed, Jun 26, 2024 at 11:04:07PM +0000, Igor Pylypiv wrote:
> > 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.
> > 
> > Cc: stable@vger.kernel.org
> > Reviewed-by: Hannes Reinecke <hare@suse.de>
> > Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > ---
> >  drivers/ata/libata-scsi.c | 158 +++++++++++++++++++++-----------------
> >  1 file changed, 86 insertions(+), 72 deletions(-)
> > 
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index a9e44ad4c2de..26b1263f5c7c 100644
> > --- 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;
> > -	}
> >  }
> >  
> >  /**
> > @@ -1634,6 +1649,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> >  	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);
> >  
> >  	/* For ATA pass thru (SAT) commands, generate a sense block if
> >  	 * user mandated it or if there's an error.  Note that if we
> > @@ -1645,13 +1662,16 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> >  	 * 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)
> > +	    ((cdb[2] & 0x20) || need_passthru_sense)) {
> > +		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID))
> > +			ata_gen_passthru_sense(qc);
> > +		ata_scsi_set_passthru_sense_fields(qc);
> 
> This whole logic looks too complicated to me.
> 
> Can't we do something to make it easier to read, e.g. something like:
> 
> 
> {
> 	if (command_is_ata_passthru(cdb)) {
> 		handle_passthru_completion(qc);
> 		ata_qc_done();
> 		return;
> 	}
> 
> 	if (need_sense)
> 		ata_gen_ata_sense(qc);
> 	else
> 		/* Keep the SCSI ML and status byte, clear host byte. */ 
> 		cmd->result &= 0x0000ffff;
> 
> 	ata_qc_done();
> }
> 
> And then put the complicated logic in handle_passthru_command() ?
> 
> /* CASES:
> * a) IF error command (ERROR or DF set) and ATA_QCFLAG_SENSE_VALID (SK+ASC+ASCQ) set:
> *    - don't touch SK/ASC/ASCQ in sense_buffer
> *    - set ATA registers in fixed format or descriptor format (based on dev->flags ATA_DFLAG_D_SENSE)
> * b) IF error command (ERROR or DF set) and ATA_QCFLAG_SENSE_VALID (SK+ASC+ASCQ) not set:
> *    - generate the SK+ASC+ASCQ from ATA status and ATA error, and
> *    - set CHECK_CONDITION in cmd->result (scsi_build_sense() does this)
> *    - set ATA registers in fixed format or descriptor format (based on dev->flags ATA_DFLAG_D_SENSE)
> * c) IF success command (ERROR and DF not set), and ATA_QCFLAG_SENSE_VALID set, CK_COND set:
> *    - don't touch SK/ASC/ASCQ in sense_buffer
> *    - set ATA registers in fixed format or descriptor format (based on dev->flags ATA_DFLAG_D_SENSE)
> *    - we should probably set CHECK_CONDITION status byte in cmd->result here.... but not sure...
> * d) IF success command (ERROR and DF not set), and ATA_QCFLAG_SENSE_VALID set, CK_COND not set:
> *    - don't touch SK/ASC/ASCQ in sense_buffer
> *    - don't fill ATA registers
> *    - keep the SCSI ML and status byte, clear host byte, in cmd->result
> * e) IF success command (ERROR and DF not set), and ATA_QCFLAG_SENSE_VALID not set, CK_COND set:
> *    - set SK to "RECOVERED ERROR" ASCQ to "ATA PASS-THROUGH INFORMATION AVAILABLE", ASC to 0.
> *    - set ATA registers in fixed format or descriptor format (based on dev->flags ATA_DFLAG_D_SENSE)
> *    - set CHECK_CONDITION status byte in cmd->result
> * f) IF success command (ERROR and DF not set), and ATA_QCFLAG_SENSE_VALID not set, CK_COND not set:
> *    - keep the SCSI ML and status byte, clear host byte, in cmd->result
> */
> static void ata_handle_passthru_completion(struct ata_queued_cmd *qc);
>
> So we should only copy the ATA registers when CK_COND is set, or if ERROR bit
> or DF bit was set. CK_COND being set in the cdb (input command) basically means
> that the user requested that the ATA registers should be copied into the sense
> buffer (in the result).
> 
> The only tricky case is if we should set CHECK_CONDITION in case c) or not.
> All other cases seems quite clear by looking at the SAT spec.
> 

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 think there are some additional simplifications that can be made,
e.g. both ata_gen_passthru_sense() and ata_gen_ata_sense() contain
the same exact code:

        /*                                                                      
         * Use ata_to_sense_error() to map status register bits                 
         * onto sense key, asc & ascq.                                          
         */                                                                     
        if (qc->err_mask ||                                                     
            tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {             
                ata_to_sense_error(tf->status, tf->error,                       
                                   &sense_key, &asc, &ascq);                    
                ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);         
        } else {

Perhaps it can be moved into a separate function, etc.

Thanks,
Igor

> 
> > +	} else if (need_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 +2610,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
> > -- 
> > 2.45.2.803.g4e1b14247a-goog
> >

On 6/28/24 5:55 AM, Igor Pylypiv wrote:
> On Thu, Jun 27, 2024 at 09:16:09AM +0900, Damien Le Moal wrote:
>> On 6/27/24 08:04, Igor Pylypiv wrote:
>>> 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.
>>>
>>> Cc: stable@vger.kernel.org
>>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>>> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
>>> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
>>
>> I wonder if we can find the patch that introduced the bug in the first place so
>> that we can add a Fixes tag. I have not checked. This may have been wrong since
>> a long time ago...
> 
> This code was first introduced in 2005 in commit b095518ef51c3 ("[libata]
> ATA passthru (arbitrary ATA command execution)").
> 
> ATA_QCFLAG_SENSE_VALID was introduced a year later in commit 9ec957f2002b
> ("[PATCH] libata-eh-fw: add flags and operations for new EH").
> 
> IIUC, ATA_QCFLAG_SENSE_VALID has not been set for ATA drives until 2016
> when the support for fetching the sense data was added in 5b01e4b9efa0
> ("libata: Implement NCQ autosense") and commit e87fd28cf9a2d ("libata:
> Implement support for sense data reporting").
> 
> To me none of the commits looks like a good candidate for the Fixes tag.
> What are your thoughts on this?

Then let's just mark which LTS version need the patch.
E.g. Cc: stable@vger.kernel.org # X.Y +

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. */

On Fri, Jun 28, 2024 at 08:44:41PM +0200, Niklas Cassel wrote:
> 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:
> 

Agree, having self-explanatory variable names makes the code much more
readable. I'll add the patch in v4.

Do you mind if I set you as the author of the patch with the corresponding
Signed-off-by tag?

> 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);

SAM_STAT_CHECK_CONDITION will be set by ata_gen_passthru_sense(). Perhaps we
can move the SAM_STAT_CHECK_CONDITION setting into else if?

        if (is_ata_passthru && (is_ck_cond_request || is_error || have_sense)) {
               if (!have_sense)
                       ata_gen_passthru_sense(qc);
               else if (is_ck_cond_request)
                       set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION);
               ata_scsi_set_passthru_sense_fields(qc);
        } else if (is_error && !have_sense) {


> +       } else if (is_error && !have_sense) {
>                 ata_gen_ata_sense(qc);
>         } else {
>                 /* Keep the SCSI ML and status byte, clear host byte. */

Thanks,
Igor

On Fri, Jun 28, 2024 at 11:31:35PM +0000, Igor Pylypiv wrote:
> On Fri, Jun 28, 2024 at 08:44:41PM +0200, Niklas Cassel wrote:
> > 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:
> > 
> 
> Agree, having self-explanatory variable names makes the code much more
> readable. I'll add the patch in v4.
> 
> Do you mind if I set you as the author of the patch with the corresponding
> Signed-off-by tag?

I still think that you are the author.

But if you want, feel free to add me as: Co-developed-by
(which would also require you to add my Signed-off-by), see:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by


> 
> > 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);
> 
> SAM_STAT_CHECK_CONDITION will be set by ata_gen_passthru_sense(). Perhaps we
> can move the SAM_STAT_CHECK_CONDITION setting into else if?

I think it is fine that:
if (is_ck_cond_request)
	set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION);

might set SAM_STAT_CHECK_CONDITION even if it is already set.

Personally, I think that my suggestion is slightly clearer when it comes
to highlight the behavior of CK_COND. (CK_COND will set CHECK_CONDITION,
regardless if successful command or error command, and regardless if
we already had sense or not.)

And considering that we finally make this hard to read code slightly more
readable than it was to start off with, I would prefer my alternative.


Kind regards,
Niklas

On Sat, Jun 29, 2024 at 05:09:54AM +0200, Niklas Cassel wrote:
> On Fri, Jun 28, 2024 at 11:31:35PM +0000, Igor Pylypiv wrote:
> > On Fri, Jun 28, 2024 at 08:44:41PM +0200, Niklas Cassel wrote:
> > > 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:
> > > 
> > 
> > Agree, having self-explanatory variable names makes the code much more
> > readable. I'll add the patch in v4.
> > 
> > Do you mind if I set you as the author of the patch with the corresponding
> > Signed-off-by tag?
> 
> I still think that you are the author.
> 
> But if you want, feel free to add me as: Co-developed-by
> (which would also require you to add my Signed-off-by), see:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
> 
Sounds good! Added the Co-developed-by abd Signed-off-by tags in v4. Thanks!
> 
> > 
> > > 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);
> > 
> > SAM_STAT_CHECK_CONDITION will be set by ata_gen_passthru_sense(). Perhaps we
> > can move the SAM_STAT_CHECK_CONDITION setting into else if?
> 
> I think it is fine that:
> if (is_ck_cond_request)
> 	set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION);
> 
> might set SAM_STAT_CHECK_CONDITION even if it is already set.
> 
> Personally, I think that my suggestion is slightly clearer when it comes
> to highlight the behavior of CK_COND. (CK_COND will set CHECK_CONDITION,
> regardless if successful command or error command, and regardless if
> we already had sense or not.)
> 
> And considering that we finally make this hard to read code slightly more
> readable than it was to start off with, I would prefer my alternative.
>
It makes senes. Added the patch in v4. Thank you!
> 
> Kind regards,
> Niklas