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. The sentence: "Descriptor format sense data shall include the ATA Status Return Descriptor" should be interpreted as: _If_ the sense code is formatted in descriptor format _then_ the ATA Status Return Descriptor should be included. IE if the sense code is not in descriptor format then the ATA Status Return Descriptor shouldn't be included (kinda obvious, really). But of course the D_SENSE bit should be honoured. Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@xxxxxxx +49 911 74053 688 SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
