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
