Commit 97981926 authored by Igor Pylypiv's avatar Igor Pylypiv Committed by Niklas Cassel
Browse files

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.

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.

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.

Cc: stable@vger.kernel.org # 4.19+
Co-developed-by: default avatarNiklas Cassel <cassel@kernel.org>
Reviewed-by: default avatarHannes Reinecke <hare@suse.de>
Reviewed-by: default avatarDamien Le Moal <dlemoal@kernel.org>
Reviewed-by: default avatarNiklas Cassel <cassel@kernel.org>
Signed-off-by: default avatarIgor Pylypiv <ipylypiv@google.com>
Link: https://lore.kernel.org/r/20240702024735.1152293-3-ipylypiv@google.com


Signed-off-by: default avatarNiklas Cassel <cassel@kernel.org>
parent 38dab832

drivers/ata/libata-scsi.c

+92 −77
Original line number Diff line number Diff line
@@ -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;

	}

}



/**
@@ -1632,26 +1647,32 @@ 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);

	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

	 * 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_sense))

	if (is_ata_passthru && (is_ck_cond_request || is_error || have_sense)) {

		if (!have_sense)

			ata_gen_passthru_sense(qc);

	else if (need_sense)

		ata_scsi_set_passthru_sense_fields(qc);

		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

	} else {

		/* Keep the SCSI ML and status byte, clear host byte. */

		cmd->result &= 0x0000ffff;

	}



	ata_qc_done(qc);

}
@@ -2590,14 +2611,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