drivers/ata/libata-sata.c | 2 -- drivers/ata/libata-scsi.c | 31 ++++++++++++++----------------- drivers/ata/libata.h | 3 --- 3 files changed, 14 insertions(+), 22 deletions(-)
The INFORMATION field is not set when sense data is obtained using
ata_eh_request_sense(). Move the ata_scsi_set_sense_information() call
to ata_scsi_qc_complete() to consistently set the INFORMATION field
regardless of the way how the sense data is obtained.
This call should be limited to regular commands only, as the INFORMATION
field is populated with different data for ATA PASS-THROUGH commands.
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
Changes in v2:
- Rephrased commit message to make it clearer.
- Dropped kernel-doc comment for the ata_scsi_set_sense_information().
drivers/ata/libata-sata.c | 2 --
drivers/ata/libata-scsi.c | 31 ++++++++++++++-----------------
drivers/ata/libata.h | 3 ---
3 files changed, 14 insertions(+), 22 deletions(-)
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index ba300cc0a3a3..b01b52e95352 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1644,8 +1644,6 @@ void ata_eh_analyze_ncq_error(struct ata_link *link)
if (ata_scsi_sense_is_valid(sense_key, asc, ascq)) {
ata_scsi_set_sense(dev, qc->scsicmd, sense_key, asc,
ascq);
- ata_scsi_set_sense_information(dev, qc->scsicmd,
- &qc->result_tf);
qc->flags |= ATA_QCFLAG_SENSE_VALID;
}
}
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 2796c0da8257..ef117a0bc248 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -216,17 +216,21 @@ void ata_scsi_set_sense(struct ata_device *dev, struct scsi_cmnd *cmd,
scsi_build_sense(cmd, d_sense, sk, asc, ascq);
}
-void ata_scsi_set_sense_information(struct ata_device *dev,
- struct scsi_cmnd *cmd,
- const struct ata_taskfile *tf)
+static void ata_scsi_set_sense_information(struct ata_queued_cmd *qc)
{
u64 information;
- information = ata_tf_read_block(tf, dev);
+ if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
+ ata_dev_dbg(qc->dev,
+ "missing result TF: can't set INFORMATION sense field\n");
+ return;
+ }
+
+ information = ata_tf_read_block(&qc->result_tf, qc->dev);
if (information == U64_MAX)
return;
- scsi_set_sense_information(cmd->sense_buffer,
+ scsi_set_sense_information(qc->scsicmd->sense_buffer,
SCSI_SENSE_BUFFERSIZE, information);
}
@@ -971,8 +975,7 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
* ata_gen_ata_sense - generate a SCSI fixed sense block
* @qc: Command that we are erroring out
*
- * Generate sense block for a failed ATA command @qc. Descriptor
- * format is used to accommodate LBA48 block address.
+ * Generate sense block for a failed ATA command @qc.
*
* LOCKING:
* None.
@@ -982,8 +985,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
struct ata_device *dev = qc->dev;
struct scsi_cmnd *cmd = qc->scsicmd;
struct ata_taskfile *tf = &qc->result_tf;
- unsigned char *sb = cmd->sense_buffer;
- u64 block;
u8 sense_key, asc, ascq;
if (ata_dev_disabled(dev)) {
@@ -1014,12 +1015,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
ata_scsi_set_sense(dev, cmd, ABORTED_COMMAND, 0, 0);
return;
}
-
- block = ata_tf_read_block(&qc->result_tf, dev);
- if (block == U64_MAX)
- return;
-
- scsi_set_sense_information(sb, SCSI_SENSE_BUFFERSIZE, block);
}
void ata_scsi_sdev_config(struct scsi_device *sdev)
@@ -1679,8 +1674,10 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
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 if (is_error) {
+ if (!have_sense)
+ ata_gen_ata_sense(qc);
+ ata_scsi_set_sense_information(qc);
}
ata_qc_done(qc);
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 0337be4faec7..ce5c628fa6fd 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -141,9 +141,6 @@ extern int ata_scsi_offline_dev(struct ata_device *dev);
extern bool ata_scsi_sense_is_valid(u8 sk, u8 asc, u8 ascq);
extern void ata_scsi_set_sense(struct ata_device *dev,
struct scsi_cmnd *cmd, u8 sk, u8 asc, u8 ascq);
-extern void ata_scsi_set_sense_information(struct ata_device *dev,
- struct scsi_cmnd *cmd,
- const struct ata_taskfile *tf);
extern void ata_scsi_media_change_notify(struct ata_device *dev);
extern void ata_scsi_hotplug(struct work_struct *work);
extern void ata_scsi_dev_rescan(struct work_struct *work);
--
2.49.0.504.g3bcea36a83-googHello Igor, I'm missing the bigger picture here. Are we violating the spec? If so, please reference a specific section in the specs. From SPC-7: """ The contents of the INFORMATION field are device type or command specific and are defined in a command standard. See 4.4.4 for device server requirements regarding how values are returned in the INFORMATION field. """ Looking at SBC-5, "4.18.1 Error reporting overview": """ If a command attempts to access or reference an invalid LBA, then the device server shall report the first invalid LBA (e.g., lowest numbered LBA) in the INFORMATION field of the sense data (see SPC-6). If a recovered read error is reported, then the device server shall report the last LBA (e.g., highest numbered LBA) on which a recovered read error occurred for the command in the INFORMATION field of the sense data. """ Since we are generating this, it makes me thing that perhaps we should not set the INFORMATION field unconditionally? I guess it makes sense for e.g. REQ_OP_READ/READ_OP_WRITE commands, but probably does not make sense for e.g. REQ_OP_FLUSH commands? On Thu, Apr 03, 2025 at 02:29:24PM -0700, Igor Pylypiv wrote: > The INFORMATION field is not set when sense data is obtained using > ata_eh_request_sense(). Move the ata_scsi_set_sense_information() call > to ata_scsi_qc_complete() to consistently set the INFORMATION field > regardless of the way how the sense data is obtained. As you know, we also have successful commands with sense data (CDL policy 0xD), see ata_eh_get_success_sense(). These commands will either fetch sense data using ata_eh_get_ncq_success_sense() or using ata_eh_get_non_ncq_success_sense() (the latter function will fetch sense data using ata_eh_request_sense()). Regardless of the path taken, these commands will also end up in ata_scsi_qc_complete(), so perhaps it is not enough for your patch to modify ata_scsi_qc_complete() to simply set the INFORMATION field for commands with ATA_ERR bit set (is_error) ? Perhaps you should also consider commands with sense data (have_sense), but without is_error set? > > This call should be limited to regular commands only, as the INFORMATION > field is populated with different data for ATA PASS-THROUGH commands. I do agree that for ATA PASS-THROUGH commands with fixed format sense, the INFORMATION field is already defined by SAT. However, what about ATA PASS-THROUGH commands with descriptor format sense? ATA Status Return sense data descriptor, which is used by ATA PASS-THROUGH commands has descriptor type 09h. Information sense data descriptor has descriptor type 00h. (See 4.4.2.2 Information sense data descriptor in SPC-7.) Is it perhaps possible for a command to have both descriptors? After reading SPC-7, "Table 30 – DESCRIPTOR TYPE field" I would say that is appears that you usually just have one descriptor, so I would say let's continue only having the ATA Status Return sense data descriptor for ATA PASS-THOUGH commands. Kind regards, Niklas
On Fri, Apr 04, 2025 at 01:57:07PM +0200, Niklas Cassel wrote:
> Hello Igor,
>
>
> I'm missing the bigger picture here.
>
> Are we violating the spec? If so, please reference a specific
> section in the specs.
Hi Niklas,
Thank you for the thorough review!
I'm using the SAT-6 (revision 2) spec:
11 Translation of ATA errors to SCSI errors
11.7 INFORMATION field
Table 201 — Contents of the INFORMATION field
+---------------------------+------------------------------------------+
| ATA command | INFORMATION field |
+---------------------------+------------------------------------------+
| FLUSH CACHE | |
| FLUSH CACHE EXT | |
| READ DMA | |
| READ DMA EXT | |
| READ FPDMA QUEUED | |
| READ SECTORS | |
| READ SECTORS EXT | |
| READ VERIFY SECTOR(S) | ATA LBA field ᵃ |
| READ VERIFY SECTOR(S) EXT | |
| WRITE DMA | |
| WRITE DMA EXT | |
| WRITE DMA FUA EXT | |
| WRITE FPDMA QUEUED | |
| WRITE SECTOR(S) | |
| WRITE SECTOR(S) EXT | |
+---------------------------+------------------------------------------+
| All others | Unspecified |
+---------------------------+------------------------------------------+
| ᵃ From ATA error outputs (non-NCQ) or ATA NCQ Command Error log |
+----------------------------------------------------------------------+
>
> From SPC-7:
> """
> The contents of the INFORMATION field are device type or command specific
> and are defined in a command standard. See 4.4.4 for device server
> requirements regarding how values are returned in the INFORMATION field.
> """
>
> Looking at SBC-5, "4.18.1 Error reporting overview":
>
> """
> If a command attempts to access or reference an invalid LBA, then the device
> server shall report the first invalid LBA (e.g., lowest numbered LBA) in the
> INFORMATION field of the sense data (see SPC-6). If a recovered read error is
> reported, then the device server shall report the last LBA (e.g., highest
> numbered LBA) on which a recovered read error occurred for the command in the
> INFORMATION field of the sense data.
> """
>
> Since we are generating this, it makes me thing that perhaps we should not
> set the INFORMATION field unconditionally? I guess it makes sense for e.g.
> REQ_OP_READ/READ_OP_WRITE commands, but probably does not make sense for e.g.
> REQ_OP_FLUSH commands?
>
SAT-6 specifies that we should set ATA LBA for FLUSH CACHE [EXT] as well.
For "All others" commands (not explicitly listed in Table 201), the value
in the INFORMATION field is "Unspecified". I think it should be fine to
set ATA LBA for other commands as well.
>
> On Thu, Apr 03, 2025 at 02:29:24PM -0700, Igor Pylypiv wrote:
> > The INFORMATION field is not set when sense data is obtained using
> > ata_eh_request_sense(). Move the ata_scsi_set_sense_information() call
> > to ata_scsi_qc_complete() to consistently set the INFORMATION field
> > regardless of the way how the sense data is obtained.
>
> As you know, we also have successful commands with sense data
> (CDL policy 0xD), see ata_eh_get_success_sense().
>
> These commands will either fetch sense data using
> ata_eh_get_ncq_success_sense() or using ata_eh_get_non_ncq_success_sense()
> (the latter function will fetch sense data using ata_eh_request_sense()).
>
> Regardless of the path taken, these commands will also end up in
> ata_scsi_qc_complete(), so perhaps it is not enough for your patch to
> modify ata_scsi_qc_complete() to simply set the INFORMATION field for
> commands with ATA_ERR bit set (is_error) ? Perhaps you should also
> consider commands with sense data (have_sense), but without is_error set?
>
SAT-6 "11.7 INFORMATION field" has a footnote for the "ATA LBA field" as
follows: "From ATA error outputs (non-NCQ) or ATA NCQ Command Error log".
I limited the change to commands with ATA_ERR bit set (is_error) because
the spec explicitly mentions errors and the whole section 11 is dedicated
to the translation of ATA errors.
>
> >
> > This call should be limited to regular commands only, as the INFORMATION
> > field is populated with different data for ATA PASS-THROUGH commands.
>
> I do agree that for ATA PASS-THROUGH commands with fixed format sense,
> the INFORMATION field is already defined by SAT.
>
> However, what about ATA PASS-THROUGH commands with descriptor format sense?
>
> ATA Status Return sense data descriptor, which is used by ATA PASS-THROUGH
> commands has descriptor type 09h.
>
> Information sense data descriptor has descriptor type 00h.
> (See 4.4.2.2 Information sense data descriptor in SPC-7.)
>
> Is it perhaps possible for a command to have both descriptors?
>
> After reading SPC-7, "Table 30 – DESCRIPTOR TYPE field"
>
> I would say that is appears that you usually just have one descriptor,
> so I would say let's continue only having the ATA Status Return sense
> data descriptor for ATA PASS-THOUGH commands.
>
Agree. ATA Status Return sense data descriptor for ATA PASS-THOUGH commands
already contains the ATA LBA in bytes [6..11] so it seems redundant to
also include the same in the Information sense data descriptor.
Thank you,
Igor
>
> Kind regards,
> Niklas
On Fri, Apr 04, 2025 at 12:18:16PM -0700, Igor Pylypiv wrote: > > Agree. ATA Status Return sense data descriptor for ATA PASS-THOUGH commands > already contains the ATA LBA in bytes [6..11] so it seems redundant to > also include the same in the Information sense data descriptor. Damien and I talked about this. Since this patch only affects non-PT commands, what it this patch actually solving? Since for non-PT commands, sense data is not returned to the user. So unless SCSI core does some specific handling based on the INFORMATION field (and AFAICT, SCSI core only looks at SK/ASC/ASCQ), I can't see how this patch can actually solve anything. Kind regards, Niklas
On Mon, Apr 07, 2025 at 10:52:19AM +0200, Niklas Cassel wrote: > On Fri, Apr 04, 2025 at 12:18:16PM -0700, Igor Pylypiv wrote: > > > > Agree. ATA Status Return sense data descriptor for ATA PASS-THOUGH commands > > already contains the ATA LBA in bytes [6..11] so it seems redundant to > > also include the same in the Information sense data descriptor. > > Damien and I talked about this. > > Since this patch only affects non-PT commands, what it this patch actually > solving? For ATA PASS-THROUGH + fixed format sense data + NCQ autosense, this patch eliminates reduntant writes to set the INFORMATION field and the VALID bit. First write: scsi_set_sense_information() sets the INFORMATION field to ATA LBA (which is incorrect for ATA PASS-THROUGH). Second write: ata_scsi_set_passthru_sense_fields() sets the INFORMATION field to ATA ERROR/STATUS/DEVICE/COUNT(7:0) as per SAT spec. User space should not see an issue because second write overwrites the incorrect data from the first write. I think we should still fix this in case someone updates the code to remove the second write in the future. How I found this issue? In commit 38dab832c3f4 (ata: libata-scsi: Fix offsets for the fixed format sense data) the offsets of fixed format sense data were updated to match the SAT spec. To simplify the migration from old incorrect offsets to the new correct offsets I was considering using the VALID bit to determine what offsets to use in user space during the migration. The problem is that for ATA PASS-THROUGH + fixed format sense data + NCQ autosense the VALID bit is also being set by the scsi_set_sense_information() so we cannot use the VALID bit to reliably determine if kernel uses the correct fixed sense data offsets or not. > > Since for non-PT commands, sense data is not returned to the user. > > So unless SCSI core does some specific handling based on the INFORMATION > field (and AFAICT, SCSI core only looks at SK/ASC/ASCQ), I can't see how > this patch can actually solve anything. +1 the patch does not seem to solve any issues for non-PT commands besides a spec compliance which is not visible to a user. Deleting ata_scsi_set_sense_information() should work as well. If SCSI core does not use the INFORMATION field perhaps there is no need to set it at all? This will eliminate the reduntant writes for ATA PASS-THROUGH as well. Thanks, Igor > > > Kind regards, > Niklas
On 2025/04/08 3:18, Igor Pylypiv wrote: > On Mon, Apr 07, 2025 at 10:52:19AM +0200, Niklas Cassel wrote: >> On Fri, Apr 04, 2025 at 12:18:16PM -0700, Igor Pylypiv wrote: >>> >>> Agree. ATA Status Return sense data descriptor for ATA PASS-THOUGH commands >>> already contains the ATA LBA in bytes [6..11] so it seems redundant to >>> also include the same in the Information sense data descriptor. >> >> Damien and I talked about this. >> >> Since this patch only affects non-PT commands, what it this patch actually >> solving? > > For ATA PASS-THROUGH + fixed format sense data + NCQ autosense, this patch > eliminates reduntant writes to set the INFORMATION field and the VALID bit. > > First write: scsi_set_sense_information() sets the INFORMATION field > to ATA LBA (which is incorrect for ATA PASS-THROUGH). > > Second write: ata_scsi_set_passthru_sense_fields() sets the INFORMATION > field to ATA ERROR/STATUS/DEVICE/COUNT(7:0) as per SAT spec. > > User space should not see an issue because second write overwrites > the incorrect data from the first write. I think we should still fix > this in case someone updates the code to remove the second write in > the future. OK. This now makes more sense. Please add all this description to the commit message to clarify WHAT you are fixing, and clearly explain how the first (useless) INFORMATION field write is removed. >> So unless SCSI core does some specific handling based on the INFORMATION >> field (and AFAICT, SCSI core only looks at SK/ASC/ASCQ), I can't see how >> this patch can actually solve anything. > > +1 the patch does not seem to solve any issues for non-PT commands besides > a spec compliance which is not visible to a user. If the above sequence applies to both passthrugh and regular IOs, then there is no spec violation since the second write does set the INFORMATION filed to a correct value. > Deleting ata_scsi_set_sense_information() should work as well. If SCSI core > does not use the INFORMATION field perhaps there is no need to set it at all? > This will eliminate the reduntant writes for ATA PASS-THROUGH as well. I do not think the scsi core use that field at all. But libata should still behave correctly and act as a spec compliant SAT. So if SAT says that this field should be set, then let's initialize it. If SAT says otherwise, let's do that. I have not checked SAT specs. Have you ? -- Damien Le Moal Western Digital Research
Hello Igor, On Fri, Apr 04, 2025 at 12:18:16PM -0700, Igor Pylypiv wrote: > > > > I'm missing the bigger picture here. > > > > Are we violating the spec? If so, please reference a specific > > section in the specs. > > Hi Niklas, > > Thank you for the thorough review! > > I'm using the SAT-6 (revision 2) spec: > > 11 Translation of ATA errors to SCSI errors > 11.7 INFORMATION field > > Table 201 — Contents of the INFORMATION field > +---------------------------+------------------------------------------+ > | ATA command | INFORMATION field | > +---------------------------+------------------------------------------+ > | FLUSH CACHE | | > | FLUSH CACHE EXT | | > | READ DMA | | > | READ DMA EXT | | > | READ FPDMA QUEUED | | > | READ SECTORS | | > | READ SECTORS EXT | | > | READ VERIFY SECTOR(S) | ATA LBA field ᵃ | > | READ VERIFY SECTOR(S) EXT | | > | WRITE DMA | | > | WRITE DMA EXT | | > | WRITE DMA FUA EXT | | > | WRITE FPDMA QUEUED | | > | WRITE SECTOR(S) | | > | WRITE SECTOR(S) EXT | | > +---------------------------+------------------------------------------+ > | All others | Unspecified | > +---------------------------+------------------------------------------+ > | ᵃ From ATA error outputs (non-NCQ) or ATA NCQ Command Error log | > +----------------------------------------------------------------------+ > Please send a V3 where you include a reference to SAT-6 (revision 2), "11 Translation of ATA errors to SCSI errors" in the commit message. > > If a command attempts to access or reference an invalid LBA, then the device > > server shall report the first invalid LBA (e.g., lowest numbered LBA) in the > > INFORMATION field of the sense data (see SPC-6). If a recovered read error is > > reported, then the device server shall report the last LBA (e.g., highest > > numbered LBA) on which a recovered read error occurred for the command in the > > INFORMATION field of the sense data. > > """ > > > > Since we are generating this, it makes me thing that perhaps we should not > > set the INFORMATION field unconditionally? I guess it makes sense for e.g. > > REQ_OP_READ/READ_OP_WRITE commands, but probably does not make sense for e.g. > > REQ_OP_FLUSH commands? > > > > SAT-6 specifies that we should set ATA LBA for FLUSH CACHE [EXT] as well. > For "All others" commands (not explicitly listed in Table 201), the value > in the INFORMATION field is "Unspecified". I think it should be fine to > set ATA LBA for other commands as well. Agreed, let's just set it unconditionally for now, and if there ever comes a command that wants to use the INFORMATION field for something else (which seems unlikely), we can simply special case that command. However, please mention this in the commit message as well. > > As you know, we also have successful commands with sense data > > (CDL policy 0xD), see ata_eh_get_success_sense(). > > > > These commands will either fetch sense data using > > ata_eh_get_ncq_success_sense() or using ata_eh_get_non_ncq_success_sense() > > (the latter function will fetch sense data using ata_eh_request_sense()). > > > > Regardless of the path taken, these commands will also end up in > > ata_scsi_qc_complete(), so perhaps it is not enough for your patch to > > modify ata_scsi_qc_complete() to simply set the INFORMATION field for > > commands with ATA_ERR bit set (is_error) ? Perhaps you should also > > consider commands with sense data (have_sense), but without is_error set? > > > > SAT-6 "11.7 INFORMATION field" has a footnote for the "ATA LBA field" as > follows: "From ATA error outputs (non-NCQ) or ATA NCQ Command Error log". > > I limited the change to commands with ATA_ERR bit set (is_error) because > the spec explicitly mentions errors and the whole section 11 is dedicated > to the translation of ATA errors. We can have sense data for both SCSI status codes GOOD and CHECK CONDITION. I'm not really a big fan that the sense data is not the same (does not include the INFORMATION field) for these cases. The logical thing would be, either we have sense data or not, and if we do, the sense data has the same amount of information. You do mention the footnote that the SCSI INFORMATION field should be set to the ATA LBA field "From ATA error outputs (non-NCQ) or ATA NCQ Command Error log" If we look at the ATA NCQ Command Error log, it does have LBA field, but no INFORMATION field. If we look at the ATA Sense Data for Successful NCQ Commands log, it has a bunch of Sense Data descriptors. ACS-6, 9.29.3 Successful Sense Data descriptor, does have the LBA field and an INFORMATION field. The definition of the INFORMATION field in the Successful Sense Data descriptor: """ 9.29.3.5 INFORMATION field If definition of the sense data to be returned when a command completes without an error includes an information value, then the INFORMATION field contains the defined value. Otherwise, the INFORMATION field is reserved. """ Thus, I do think that the most correct thing, for Successful NCQ commands with sense data, is to fill in the SCSI INFORMATION field to the INFORMATION field from the Successful Sense Data descriptor. Not sure how to deal with non-NCQ Successful commands... The spec writers do make our lives interesting by providing different amount of information depending on the method used to get the sense data :) Kind regards, Niklas
On Mon, Apr 07, 2025 at 10:03:54AM +0200, Niklas Cassel wrote: > > If we look at the ATA Sense Data for Successful NCQ Commands log, > it has a bunch of Sense Data descriptors. > > ACS-6, 9.29.3 Successful Sense Data descriptor, > does have the LBA field and an INFORMATION field. > > The definition of the INFORMATION field in the Successful Sense Data > descriptor: > > """ > 9.29.3.5 INFORMATION field > If definition of the sense data to be returned when a command completes > without an error includes an information value, then the INFORMATION field > contains the defined value. Otherwise, the INFORMATION field is reserved. > """ Side note: Considering that this is just two bytes, and the INFORMATION field in SCSI is four bytes, I honestly have no idea what this field actually contains. Anyway, the biggest question is what actual problem this fixes, since the sense data is not returned to the user for non-passthrough commands. Kind regards, Niklas
© 2016 - 2025 Red Hat, Inc.