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#L...
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