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@xxxxxxxxxxxxxxx
> Reviewed-by: Hannes Reinecke <hare@xxxxxxx>
> Reviewed-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
> Signed-off-by: Igor Pylypiv <ipylypiv@xxxxxxxxxx>
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
--
Damien Le Moal
Western Digital Research
