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

[Niklas Cassel <cassel@kernel.org>]

Hello 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