* [PATCH v4 1/8] ata: libata-scsi: Fix offsets for the fixed format sense data
[not found] <20240701195758.1045917-1-ipylypiv@google.com>
@ 2024-07-01 19:57 ` Igor Pylypiv
2024-07-01 21:09 ` Niklas Cassel
2024-07-01 19:57 ` [PATCH v4 2/8] ata: libata-scsi: Do not overwrite valid sense data when CK_COND=1 Igor Pylypiv
2024-07-01 19:57 ` [PATCH v4 3/8] ata: libata-scsi: Honour the D_SENSE bit for CK_COND=1 and no error Igor Pylypiv
2 siblings, 1 reply; 6+ messages in thread
From: Igor Pylypiv @ 2024-07-01 19:57 UTC (permalink / raw)
To: Damien Le Moal, Niklas Cassel
Cc: Tejun Heo, Hannes Reinecke, linux-ide, linux-kernel, Igor Pylypiv,
Akshat Jain, stable
Correct the ATA PASS-THROUGH fixed format sense data offsets to conform
to SPC-6 and SAT-5 specifications. Additionally, set the VALID bit to
indicate that the INFORMATION field contains valid information.
INFORMATION
===========
SAT-5 Table 212 — "Fixed format sense data INFORMATION field for the ATA
PASS-THROUGH commands" defines the following format:
+------+------------+
| Byte | Field |
+------+------------+
| 0 | ERROR |
| 1 | STATUS |
| 2 | DEVICE |
| 3 | COUNT(7:0) |
+------+------------+
SPC-6 Table 48 - "Fixed format sense data" specifies that the INFORMATION
field starts at byte 3 in sense buffer resulting in the following offsets
for the ATA PASS-THROUGH commands:
+------------+-------------------------+
| Field | Offset in sense buffer |
+------------+-------------------------+
| ERROR | 3 |
| STATUS | 4 |
| DEVICE | 5 |
| COUNT(7:0) | 6 |
+------------+-------------------------+
COMMAND-SPECIFIC INFORMATION
============================
SAT-5 Table 213 - "Fixed format sense data COMMAND-SPECIFIC INFORMATION
field for ATA PASS-THROUGH" defines the following format:
+------+-------------------+
| Byte | Field |
+------+-------------------+
| 0 | FLAGS | LOG INDEX |
| 1 | LBA (7:0) |
| 2 | LBA (15:8) |
| 3 | LBA (23:16) |
+------+-------------------+
SPC-6 Table 48 - "Fixed format sense data" specifies that
the COMMAND-SPECIFIC-INFORMATION field starts at byte 8
in sense buffer resulting in the following offsets for
the ATA PASS-THROUGH commands:
Offsets of these fields in the fixed sense format are as follows:
+-------------------+-------------------------+
| Field | Offset in sense buffer |
+-------------------+-------------------------+
| FLAGS | LOG INDEX | 8 |
| LBA (7:0) | 9 |
| LBA (15:8) | 10 |
| LBA (23:16) | 11 |
+-------------------+-------------------------+
Reported-by: Akshat Jain <akshatzen@google.com>
Fixes: 11093cb1ef56 ("libata-scsi: generate correct ATA pass-through sense")
Cc: stable@vger.kernel.org
Reviewed-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
drivers/ata/libata-scsi.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index bb4d30d377ae..a9e44ad4c2de 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -855,7 +855,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
struct scsi_cmnd *cmd = qc->scsicmd;
struct ata_taskfile *tf = &qc->result_tf;
unsigned char *sb = cmd->sense_buffer;
- unsigned char *desc = sb + 8;
u8 sense_key, asc, ascq;
memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
@@ -877,7 +876,8 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
}
- if ((cmd->sense_buffer[0] & 0x7f) >= 0x72) {
+ if ((sb[0] & 0x7f) >= 0x72) {
+ unsigned char *desc;
u8 len;
/* descriptor format */
@@ -916,21 +916,21 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
}
} else {
/* Fixed sense format */
- desc[0] = tf->error;
- desc[1] = tf->status;
- desc[2] = tf->device;
- desc[3] = tf->nsect;
- desc[7] = 0;
+ 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) {
- desc[8] |= 0x80;
+ sb[8] |= 0x80;
if (tf->hob_nsect)
- desc[8] |= 0x40;
+ sb[8] |= 0x40;
if (tf->hob_lbal || tf->hob_lbam || tf->hob_lbah)
- desc[8] |= 0x20;
+ sb[8] |= 0x20;
}
- desc[9] = tf->lbal;
- desc[10] = tf->lbam;
- desc[11] = tf->lbah;
+ sb[9] = tf->lbal;
+ sb[10] = tf->lbam;
+ sb[11] = tf->lbah;
}
}
--
2.45.2.803.g4e1b14247a-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v4 2/8] ata: libata-scsi: Do not overwrite valid sense data when CK_COND=1
[not found] <20240701195758.1045917-1-ipylypiv@google.com>
2024-07-01 19:57 ` [PATCH v4 1/8] ata: libata-scsi: Fix offsets for the fixed format sense data Igor Pylypiv
@ 2024-07-01 19:57 ` Igor Pylypiv
2024-07-01 21:15 ` Niklas Cassel
2024-07-01 19:57 ` [PATCH v4 3/8] ata: libata-scsi: Honour the D_SENSE bit for CK_COND=1 and no error Igor Pylypiv
2 siblings, 1 reply; 6+ messages in thread
From: Igor Pylypiv @ 2024-07-01 19:57 UTC (permalink / raw)
To: Damien Le Moal, Niklas Cassel
Cc: Tejun Heo, Hannes Reinecke, linux-ide, linux-kernel, Igor Pylypiv,
stable
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 # 4.19+
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);
+ } 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v4 3/8] ata: libata-scsi: Honour the D_SENSE bit for CK_COND=1 and no error
[not found] <20240701195758.1045917-1-ipylypiv@google.com>
2024-07-01 19:57 ` [PATCH v4 1/8] ata: libata-scsi: Fix offsets for the fixed format sense data Igor Pylypiv
2024-07-01 19:57 ` [PATCH v4 2/8] ata: libata-scsi: Do not overwrite valid sense data when CK_COND=1 Igor Pylypiv
@ 2024-07-01 19:57 ` Igor Pylypiv
2024-07-01 21:12 ` Niklas Cassel
2 siblings, 1 reply; 6+ messages in thread
From: Igor Pylypiv @ 2024-07-01 19:57 UTC (permalink / raw)
To: Damien Le Moal, Niklas Cassel
Cc: Tejun Heo, Hannes Reinecke, linux-ide, linux-kernel, Igor Pylypiv,
stable
SAT-5 revision 8 specification removed the text about the ANSI INCITS
431-2007 compliance which was requiring SCSI/ATA Translation (SAT) to
return descriptor format sense data for the ATA PASS-THROUGH commands
regardless of the setting of the D_SENSE bit.
Let's honour the D_SENSE bit for CK_COND=1 commands that had no error.
Kernel already honours the D_SENSE bit when creating the sense buffer
for commands that had an error.
SAT-5 revision 7
================
12.2.2.8 Fixed format sense data
Table 212 shows the fields returned in the fixed format sense data
(see SPC-5) for ATA PASS-THROUGH commands. SATLs compliant with ANSI
INCITS 431-2007, SCSI/ATA Translation (SAT) return descriptor format
sense data for the ATA PASS-THROUGH commands regardless of the setting
of the D_SENSE bit.
SAT-5 revision 8
================
12.2.2.8 Fixed format sense data
Table 211 shows the fields returned in the fixed format sense data
(see SPC-5) for ATA PASS-THROUGH commands.
Cc: stable@vger.kernel.org # 4.19+
Reported-by: Niklas Cassel <cassel@kernel.org>
Closes: https://lore.kernel.org/linux-ide/Zn1WUhmLglM4iais@ryzen.lan
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
drivers/ata/libata-scsi.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 26b1263f5c7c..ace6b009e7ff 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -941,11 +941,8 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
&sense_key, &asc, &ascq);
ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);
} else {
- /*
- * ATA PASS-THROUGH INFORMATION AVAILABLE
- * Always in descriptor format sense.
- */
- scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
+ /* ATA PASS-THROUGH INFORMATION AVAILABLE */
+ ata_scsi_set_sense(qc->dev, cmd, RECOVERED_ERROR, 0, 0x1D);
}
}
--
2.45.2.803.g4e1b14247a-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/8] ata: libata-scsi: Fix offsets for the fixed format sense data
2024-07-01 19:57 ` [PATCH v4 1/8] ata: libata-scsi: Fix offsets for the fixed format sense data Igor Pylypiv
@ 2024-07-01 21:09 ` Niklas Cassel
0 siblings, 0 replies; 6+ messages in thread
From: Niklas Cassel @ 2024-07-01 21:09 UTC (permalink / raw)
To: Igor Pylypiv
Cc: Damien Le Moal, Tejun Heo, Hannes Reinecke, linux-ide,
linux-kernel, Akshat Jain, stable
On Mon, Jul 01, 2024 at 07:57:51PM +0000, Igor Pylypiv wrote:
> Correct the ATA PASS-THROUGH fixed format sense data offsets to conform
> to SPC-6 and SAT-5 specifications. Additionally, set the VALID bit to
> indicate that the INFORMATION field contains valid information.
>
> INFORMATION
> ===========
>
> SAT-5 Table 212 — "Fixed format sense data INFORMATION field for the ATA
> PASS-THROUGH commands" defines the following format:
>
> +------+------------+
> | Byte | Field |
> +------+------------+
> | 0 | ERROR |
> | 1 | STATUS |
> | 2 | DEVICE |
> | 3 | COUNT(7:0) |
> +------+------------+
>
> SPC-6 Table 48 - "Fixed format sense data" specifies that the INFORMATION
> field starts at byte 3 in sense buffer resulting in the following offsets
> for the ATA PASS-THROUGH commands:
>
> +------------+-------------------------+
> | Field | Offset in sense buffer |
> +------------+-------------------------+
> | ERROR | 3 |
> | STATUS | 4 |
> | DEVICE | 5 |
> | COUNT(7:0) | 6 |
> +------------+-------------------------+
>
> COMMAND-SPECIFIC INFORMATION
> ============================
>
> SAT-5 Table 213 - "Fixed format sense data COMMAND-SPECIFIC INFORMATION
> field for ATA PASS-THROUGH" defines the following format:
>
> +------+-------------------+
> | Byte | Field |
> +------+-------------------+
> | 0 | FLAGS | LOG INDEX |
> | 1 | LBA (7:0) |
> | 2 | LBA (15:8) |
> | 3 | LBA (23:16) |
> +------+-------------------+
>
> SPC-6 Table 48 - "Fixed format sense data" specifies that
> the COMMAND-SPECIFIC-INFORMATION field starts at byte 8
> in sense buffer resulting in the following offsets for
> the ATA PASS-THROUGH commands:
>
> Offsets of these fields in the fixed sense format are as follows:
>
> +-------------------+-------------------------+
> | Field | Offset in sense buffer |
> +-------------------+-------------------------+
> | FLAGS | LOG INDEX | 8 |
> | LBA (7:0) | 9 |
> | LBA (15:8) | 10 |
> | LBA (23:16) | 11 |
> +-------------------+-------------------------+
>
> Reported-by: Akshat Jain <akshatzen@google.com>
> Fixes: 11093cb1ef56 ("libata-scsi: generate correct ATA pass-through sense")
> Cc: stable@vger.kernel.org
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
> drivers/ata/libata-scsi.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index bb4d30d377ae..a9e44ad4c2de 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -855,7 +855,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> struct scsi_cmnd *cmd = qc->scsicmd;
> struct ata_taskfile *tf = &qc->result_tf;
> unsigned char *sb = cmd->sense_buffer;
> - unsigned char *desc = sb + 8;
> u8 sense_key, asc, ascq;
>
> memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
> @@ -877,7 +876,8 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
> }
>
> - if ((cmd->sense_buffer[0] & 0x7f) >= 0x72) {
> + if ((sb[0] & 0x7f) >= 0x72) {
> + unsigned char *desc;
> u8 len;
>
> /* descriptor format */
> @@ -916,21 +916,21 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> }
> } else {
> /* Fixed sense format */
> - desc[0] = tf->error;
> - desc[1] = tf->status;
> - desc[2] = tf->device;
> - desc[3] = tf->nsect;
> - desc[7] = 0;
> + 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) {
> - desc[8] |= 0x80;
> + sb[8] |= 0x80;
> if (tf->hob_nsect)
> - desc[8] |= 0x40;
> + sb[8] |= 0x40;
> if (tf->hob_lbal || tf->hob_lbam || tf->hob_lbah)
> - desc[8] |= 0x20;
> + sb[8] |= 0x20;
> }
> - desc[9] = tf->lbal;
> - desc[10] = tf->lbam;
> - desc[11] = tf->lbah;
> + sb[9] = tf->lbal;
> + sb[10] = tf->lbam;
> + sb[11] = tf->lbah;
> }
> }
>
> --
> 2.45.2.803.g4e1b14247a-goog
>
Reviewed-by: Niklas Cassel <cassel@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 3/8] ata: libata-scsi: Honour the D_SENSE bit for CK_COND=1 and no error
2024-07-01 19:57 ` [PATCH v4 3/8] ata: libata-scsi: Honour the D_SENSE bit for CK_COND=1 and no error Igor Pylypiv
@ 2024-07-01 21:12 ` Niklas Cassel
0 siblings, 0 replies; 6+ messages in thread
From: Niklas Cassel @ 2024-07-01 21:12 UTC (permalink / raw)
To: Igor Pylypiv
Cc: Damien Le Moal, Tejun Heo, Hannes Reinecke, linux-ide,
linux-kernel, stable
On Mon, Jul 01, 2024 at 07:57:53PM +0000, Igor Pylypiv wrote:
> SAT-5 revision 8 specification removed the text about the ANSI INCITS
> 431-2007 compliance which was requiring SCSI/ATA Translation (SAT) to
> return descriptor format sense data for the ATA PASS-THROUGH commands
> regardless of the setting of the D_SENSE bit.
>
> Let's honour the D_SENSE bit for CK_COND=1 commands that had no error.
> Kernel already honours the D_SENSE bit when creating the sense buffer
> for commands that had an error.
Nit: we also honor it when creating the sense buffer for successful NCQ
commands (e.g. CDL policy 0xD).
>
> SAT-5 revision 7
> ================
>
> 12.2.2.8 Fixed format sense data
>
> Table 212 shows the fields returned in the fixed format sense data
> (see SPC-5) for ATA PASS-THROUGH commands. SATLs compliant with ANSI
> INCITS 431-2007, SCSI/ATA Translation (SAT) return descriptor format
> sense data for the ATA PASS-THROUGH commands regardless of the setting
> of the D_SENSE bit.
>
> SAT-5 revision 8
> ================
>
> 12.2.2.8 Fixed format sense data
>
> Table 211 shows the fields returned in the fixed format sense data
> (see SPC-5) for ATA PASS-THROUGH commands.
>
> Cc: stable@vger.kernel.org # 4.19+
> Reported-by: Niklas Cassel <cassel@kernel.org>
> Closes: https://lore.kernel.org/linux-ide/Zn1WUhmLglM4iais@ryzen.lan
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
> drivers/ata/libata-scsi.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 26b1263f5c7c..ace6b009e7ff 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -941,11 +941,8 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> &sense_key, &asc, &ascq);
> ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);
> } else {
> - /*
> - * ATA PASS-THROUGH INFORMATION AVAILABLE
> - * Always in descriptor format sense.
> - */
> - scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
> + /* ATA PASS-THROUGH INFORMATION AVAILABLE */
> + ata_scsi_set_sense(qc->dev, cmd, RECOVERED_ERROR, 0, 0x1D);
> }
> }
>
> --
> 2.45.2.803.g4e1b14247a-goog
>
With or without nit fixed:
Reviewed-by: Niklas Cassel <cassel@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 2/8] ata: libata-scsi: Do not overwrite valid sense data when CK_COND=1
2024-07-01 19:57 ` [PATCH v4 2/8] ata: libata-scsi: Do not overwrite valid sense data when CK_COND=1 Igor Pylypiv
@ 2024-07-01 21:15 ` Niklas Cassel
0 siblings, 0 replies; 6+ messages in thread
From: Niklas Cassel @ 2024-07-01 21:15 UTC (permalink / raw)
To: Igor Pylypiv
Cc: Damien Le Moal, Tejun Heo, Hannes Reinecke, linux-ide,
linux-kernel, stable
On Mon, Jul 01, 2024 at 07:57:52PM +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 # 4.19+
> 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);
> + } 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
>
Reviewed-by: Niklas Cassel <cassel@kernel.org>
However: I really think that this patch should be squashed with patch 8/8.
For more details, see my reply to patch 8/8.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-07-01 21:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20240701195758.1045917-1-ipylypiv@google.com>
2024-07-01 19:57 ` [PATCH v4 1/8] ata: libata-scsi: Fix offsets for the fixed format sense data Igor Pylypiv
2024-07-01 21:09 ` Niklas Cassel
2024-07-01 19:57 ` [PATCH v4 2/8] ata: libata-scsi: Do not overwrite valid sense data when CK_COND=1 Igor Pylypiv
2024-07-01 21:15 ` Niklas Cassel
2024-07-01 19:57 ` [PATCH v4 3/8] ata: libata-scsi: Honour the D_SENSE bit for CK_COND=1 and no error Igor Pylypiv
2024-07-01 21:12 ` Niklas Cassel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).