On Thu, Jun 27, 2024 at 02:08:50PM +0200, 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.) > Thanks for pointing this out, Niklas! I agree, it seems like there is no reason to ignore the D_SENSE bit. Interestingly, the code was using ata_scsi_set_sense() before. Commit 11093cb1ef56 ("libata-scsi: generate correct ATA pass-through sense)" changed it to always be in the descriptor format. > > Kind regards, > Niklas Thanks, Igor
