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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux