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

[Niklas Cassel <cassel@kernel.org>]

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.)


Kind regards,
Niklas