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

Igor Pylypiv
2024-06-27T21:22:19Z

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