Re: [PATCH v2] ata: libata-scsi: Set INFORMATION sense data field consistently

[Igor Pylypiv <ipylypiv@google.com>]

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