Re: [PATCH v3 1/6] ata: libata-scsi: Fix offsets for the fixed format sense data

[Niklas Cassel <cassel@kernel.org>]

On Fri, Jun 28, 2024 at 08:47:03AM +0200, Hannes Reinecke wrote:
> On 6/27/24 14:08, Niklas Cassel wrote:
> > Hello Igor, Hannes,
> > 
> > The changes in this patch looks good, however, there is still one thing that
> > bothers me:
> > https://github.com/torvalds/linux/blob/v6.10-rc5/drivers/ata/libata-scsi.c#L873-L877
> > 
> > Specifically the code in the else statement below:
> > 
> > 	if (qc->err_mask ||
> > 	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
> > 		ata_to_sense_error(qc->ap->print_id, tf->status, tf->error,
> > 				   &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);
> > 	}
> > 
> > Looking at sat6r01, I see that this is table:
> > Table 217 — ATA command results
> > 
> > And this text:
> > No error, successful completion or command in progress. The SATL
> > shall terminate the command with CHECK CONDITION status with
> > the sense key set to RECOVERED ERROR with the additional
> > sense code set to ATA PASS-THROUGH INFORMATION
> > AVAILABLE (see SPC-5). Descriptor format sense data shall include
> > the ATA Status Return sense data descriptor (see 12.2.2.7).
> > 
> > However, I don't see anything in this text that says that the
> > sense key should always be in descriptor format sense.
> > 
> > In fact, what will happen if the user has not set the D_SENSE bit
> > (libata will default not set it), is that:
> > 
> > The else statement above will be executed, filling in sense key in
> > descriptor format, after this if/else, we will continue checking
> > if the sense buffer is in descriptor format, or fixed format.
> > 
> > Since the scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
> > is called with (..., 1, ..., ..., ...) it will always generate
> > the sense data in descriptor format, regardless of
> > dev->flags ATA_DFLAG_D_SENSE being set or not.
> > 
> > Should perhaps the code in the else statement be:
> > 
> > } else {
> > 	ata_scsi_set_sense(qc->dev, cmd, RECOVERED_ERROR, 0, 0x1D);
> > }
> > 
> > So that we actually respect the D_SENSE bit?
> > 
> > (We currently respect if when filling the sense data buffer with
> > sense data from REQUEST SENSE EXT, so I'm not sure why we shouldn't
> > respect it for successful ATA PASS-THROUGH commands.)
> > 
> I guess that we've misread the spec.

I think I might have an idea where you got this from:

In sat5r06.pdf
"""
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.
"""

In sat6r01.pdf:
"""
12.2.2.8 Fixed format sense data

Table 219 shows the fields returned in the fixed format sense data (see SPC-5)
for ATA PASS-THROUGH
commands.
"""

In SAT-6 there is no mention of compliance with ANSI INCITS 431-2007 should
ignore D_SENSE bit and unconditionally return sense data in descriptor format.

Anyway, considering that:
1) I'm not sure how a SAT would expose that it is compliant with ANSI INCITS
   431-2007.
2) This text has been removed from SAT-6.
3) We currently honour the D_SENSE bit when creating the sense buffer with the
   SK/ASC/ASCQ that we get from the device.

I think that it makes sense to honour the D_SENSE bit also when generating
sense data for successful ATA PASS-THROUGH commands (from ATA registers).


Kind regards,
Niklas