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
