[RFC] expand information in the scsi_sense_hdr

[RFC] expand information in the scsi_sense_hdr

[James Bottomley]

Since the scsi_execute_req() call is trying to make this the standard
way of passing back sense (mainly because scsi_execute_req() is designed
for internal consumption, which has more simple sense parsing
requirements) there needs to be enough information in the header for all
sense uses within the kernel.

This patch adds the info field, the flags field, the sense key specific
field and the command info field to the sense header.  The disadvantage
is that it adds nineteen bytes to the sense header.

James

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1919,7 +1919,14 @@ int scsi_normalize_sense(const u8 *sense
 	if (!scsi_sense_valid(sshdr))
 		return 0;
 
-	if (sshdr->response_code >= 0x72) {
+	if (sshdr->response_code == 0x72 || sshdr->response_code == 0x73)
+		sshdr->descriptor = 1;
+	if (sshdr->response_code < 0x70 || sshdr->response_code > 0x73)
+		return 0;
+	sshdr->deferred = (sshdr->response_code & 0x01) ? 1 : 0;
+
+	if (sshdr->descriptor) {
+		const u8 *desc;
 		/*
 		 * descriptor format
 		 */
@@ -1931,19 +1938,86 @@ int scsi_normalize_sense(const u8 *sense
 			sshdr->ascq = sense_buffer[3];
 		if (sb_len > 7)
 			sshdr->additional_length = sense_buffer[7];
+
+		/* now get descriptor flags data */
+		desc = scsi_sense_desc_find(sense_buffer, sb_len,
+					    SCSI_SENSE_DESCRIPTOR_SSC);
+		if (desc)
+			sshdr->flags = desc[3] & 0xe0;
+		desc = scsi_sense_desc_find(sense_buffer, sb_len,
+					    SCSI_SENSE_DESCRIPTOR_SBC);
+		if (desc)
+			sshdr->flags = desc[3] & 0x20; /* ILI */
+		desc = scsi_sense_desc_find(sense_buffer, sb_len,
+					    SCSI_SENSE_DESCRIPTOR_INFO);
+		if (desc) {
+			sshdr->valid = (desc[2] & 0x80) ? 1 : 0;
+			sshdr->info = 
+				((u64)desc[4]  << 56) |
+				((u64)desc[5]  << 48) |
+				((u64)desc[6]  << 40) |
+				((u64)desc[7]  << 32) |
+				((u64)desc[8]  << 24) |
+				((u64)desc[9]  << 16) |
+				((u64)desc[10] <<  8) |
+				(u64)desc[11];
+		}
+		desc = scsi_sense_desc_find(sense_buffer, sb_len,
+					    SCSI_SENSE_DESCRIPTOR_CMD);
+		if (desc)
+			sshdr->cmd_info =
+				((u64)desc[4]  << 56) |
+				((u64)desc[5]  << 48) |
+				((u64)desc[6]  << 40) |
+				((u64)desc[7]  << 32) |
+				((u64)desc[8]  << 24) |
+				((u64)desc[9]  << 16) |
+				((u64)desc[10] <<  8) |
+				(u64)desc[11];
+		desc = scsi_sense_desc_find(sense_buffer, sb_len,
+					    SCSI_SENSE_DESCRIPTOR_CMD);
+		if (desc) {
+			sshdr->sksv = (desc[2] & 0x80) ? 1 : 0;
+			sshdr->extra_sense_specific = desc[2] & 0x7f;
+			sshdr->sense_specific =
+				((u16)desc[3] << 8) |
+				(u16)desc[4];
+		}
 	} else {
 		/* 
 		 * fixed format
 		 */
+		sshdr->valid = (sense_buffer[0] & 0x80) ? 1 : 0;
 		if (sb_len > 2)
 			sshdr->sense_key = (sense_buffer[2] & 0xf);
-		if (sb_len > 7) {
+		if (sb_len > 3)
+			sshdr->flags = sense_buffer[2] & 0xe0;
+		if (sb_len > 6 && sshdr->valid)
+			sshdr->info = 
+				((u32)sense_buffer[3] << 24) |
+				((u32)sense_buffer[4] << 16) |
+				((u32)sense_buffer[5] <<  8) |
+				(u32)sense_buffer[6];
+		if (sb_len > 7)
 			sb_len = (sb_len < (sense_buffer[7] + 8)) ?
 					 sb_len : (sense_buffer[7] + 8);
-			if (sb_len > 12)
-				sshdr->asc = sense_buffer[12];
-			if (sb_len > 13)
-				sshdr->ascq = sense_buffer[13];
+
+		if (sb_len > 11)
+			sshdr->cmd_info = 
+				((u32)sense_buffer[8] << 24) |
+				((u32)sense_buffer[9] << 16) |
+				((u32)sense_buffer[10] << 8) |
+				(u32)sense_buffer[11];
+		if (sb_len > 12)
+			sshdr->asc = sense_buffer[12];
+		if (sb_len > 13)
+			sshdr->ascq = sense_buffer[13];
+		if (sb_len > 17) {
+			sshdr->sksv = (sense_buffer[15] & 0x80) ? 1 : 0;
+			if (sshdr->sksv)
+				sshdr->sense_specific =
+					((u16)sense_buffer[15] & 0x7f) << 8 |
+					(u16)sense_buffer[16];
 		}
 	}
 
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -16,8 +16,17 @@ struct Scsi_Host;
  * in which more information is required (e.g. the LBA of a MEDIUM ERROR).
  */
 struct scsi_sense_hdr {		/* See SPC-3 section 4.5 */
-	u8 response_code;	/* permit: 0x0, 0x70, 0x71, 0x72, 0x73 */
-	u8 sense_key;
+	u64 info;
+	u64 cmd_info;
+	u16 sense_specific;
+	u8 extra_sense_specific; /* only valid for descriptor format */
+	u8 valid:1,
+	   response_code:7;	/* permit: 0x0, 0x70, 0x71, 0x72, 0x73 */
+	u8 sksv:1,
+	   deferred:1,
+	   descriptor:1,
+	   sense_key:4;
+	u8 flags;		/* FILEMARK, EOM or ILI */
 	u8 asc;
 	u8 ascq;
 	u8 byte4;
@@ -26,6 +35,13 @@ struct scsi_sense_hdr {		/* See SPC-3 se
 	u8 additional_length;	/* always 0 for fixed sense format */
 };
 
+/* Descriptor types */
+#define SCSI_SENSE_DESCRIPTOR_INFO	0x00
+#define SCSI_SENSE_DESCRIPTOR_CMD	0x01
+#define SCSI_SENSE_DESCRIPTOR_SKS	0x02
+#define SCSI_SENSE_DESCRIPTOR_SSC	0x04
+#define SCSI_SENSE_DESCRIPTOR_SBC	0x05
+
 static inline int scsi_sense_valid(struct scsi_sense_hdr *sshdr)
 {
 	if (!sshdr)

Re: [RFC] expand information in the scsi_sense_hdr

[Douglas Gilbert]

James Bottomley wrote:
> Since the scsi_execute_req() call is trying to make this the standard
> way of passing back sense (mainly because scsi_execute_req() is designed
> for internal consumption, which has more simple sense parsing
> requirements) there needs to be enough information in the header for all
> sense uses within the kernel.
> 
> This patch adds the info field, the flags field, the sense key specific
> field and the command info field to the sense header.  The disadvantage
> is that it adds nineteen bytes to the sense header.

Another disadvantage is that it doesn't cope with
the newer descriptor fields that don't have equivalents
in the older fixed format. OSD and SAT have their own
sense descriptors. Vendors can (and do) have fixed sense
data (defined beyond offset 18 and field replaceable unit)
and can add their own sense descriptors [0x80 to 0xff].
Mapping extensible descriptor format (still limited to 252
bytes overall) back to a (different) fixed format could
be self defeating.

My intention with struct scsi_sense_hdr was to give
a small fixed size view of sense data (from either
fixed or descriptor format) sufficient for drivers
to do error processing. It is sufficient for many but
not all cases in drivers that I converted. I was not
trying to replace the sense data. Is the actual sense
data still going to be available to the SG_IO ioctl
and the sg driver?

BTW If you implement this change, I don't think
field member "u8 byte4;" and friends are any longer
relevant as that was to maintain the mapping between
"struct scsi_sense_data" and the descriptor format
sense data header (i.e. both 8 bytes long).

Doug Gilbert

Re: [RFC] expand information in the scsi_sense_hdr

[James Bottomley]

On Wed, 2005-06-15 at 00:25 -0400, Douglas Gilbert wrote:
> Another disadvantage is that it doesn't cope with
> the newer descriptor fields that don't have equivalents
> in the older fixed format. OSD and SAT have their own
> sense descriptors. Vendors can (and do) have fixed sense
> data (defined beyond offset 18 and field replaceable unit)
> and can add their own sense descriptors [0x80 to 0xff].
> Mapping extensible descriptor format (still limited to 252
> bytes overall) back to a (different) fixed format could
> be self defeating.

Remember that the only use is for internal, so if no-one's interested in
the information it doesn't matter if we don't use it.  If someone
becomes interested in OSD or SAT descriptors, they can add them
(currently OSD is rather expensive and rare and SAT is the province of
SAS which is still under discussion).  However, the only use currently
is Filemark, ILI and EOM which are really only a property of SSC and
SBC.

> My intention with struct scsi_sense_hdr was to give
> a small fixed size view of sense data (from either
> fixed or descriptor format) sufficient for drivers
> to do error processing. It is sufficient for many but
> not all cases in drivers that I converted. I was not
> trying to replace the sense data. Is the actual sense
> data still going to be available to the SG_IO ioctl
> and the sg driver?

All commands that report sense data back to the user still send the raw
sense data.  Like I said, the sense header is for internal kernel use.

> BTW If you implement this change, I don't think
> field member "u8 byte4;" and friends are any longer
> relevant as that was to maintain the mapping between
> "struct scsi_sense_data" and the descriptor format
> sense data header (i.e. both 8 bytes long).

OK, I'll junk those, thanks.

James