Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 03 Sep 2019 14:08:10 -0000
From:      Alexander Motin <mav@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r346491 - head/sys/cam/scsi
Message-ID:  <201904211907.x3LJ73vc010293@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: mav
Date: Sun Apr 21 19:07:03 2019
New Revision: 346491
URL: https://svnweb.freebsd.org/changeset/base/346491

Log:
  Polish SCSI sense data validity checks.
  
  According to specs and common sense, all sense data reported in descriptor
  format should be valid.  But practice shows different, some devices return
  descriptors with invalid data, resulting in error messages looking worse.
  
  Decouple block/stream commands sense data and information field printing.
  Looking on present specs, there are much more cases when those fields are
  not related, and incomplete old code was not printing valid sense data and
  leaving empty lines for invalid.
  
  MFC after:	2 weeks

Modified:
  head/sys/cam/scsi/scsi_all.c
  head/sys/cam/scsi/scsi_all.h

Modified: head/sys/cam/scsi/scsi_all.c
==============================================================================
--- head/sys/cam/scsi/scsi_all.c	Sun Apr 21 18:27:13 2019	(r346490)
+++ head/sys/cam/scsi/scsi_all.c	Sun Apr 21 19:07:03 2019	(r346491)
@@ -4061,6 +4061,10 @@ scsi_get_sense_info(struct scsi_sense_data *sense_data
 			struct scsi_sense_info *info_desc;
 
 			info_desc = (struct scsi_sense_info *)desc;
+
+			if ((info_desc->byte2 & SSD_INFO_VALID) == 0)
+				goto bailout;
+
 			*info = scsi_8btou64(info_desc->info);
 			if (signed_info != NULL)
 				*signed_info = *info;
@@ -4081,6 +4085,9 @@ scsi_get_sense_info(struct scsi_sense_data *sense_data
 
 			fru_desc = (struct scsi_sense_fru *)desc;
 
+			if (fru_desc->fru == 0)
+				goto bailout;
+
 			*info = fru_desc->fru;
 			if (signed_info != NULL)
 				*signed_info = (int8_t)fru_desc->fru;
@@ -4181,10 +4188,9 @@ scsi_get_sks(struct scsi_sense_data *sense_data, u_int
 		if (desc == NULL)
 			goto bailout;
 
-		/*
-		 * No need to check the SKS valid bit for descriptor sense.
-		 * If the descriptor is present, it is valid.
-		 */
+		if ((desc->sense_key_spec[0] & SSD_SKS_VALID) == 0)
+			goto bailout;
+
 		bcopy(desc->sense_key_spec, sks, sizeof(desc->sense_key_spec));
 		break;
 	}
@@ -4261,9 +4267,6 @@ scsi_get_block_info(struct scsi_sense_data *sense_data
 		if (SSD_FIXED_IS_PRESENT(sense, sense_len, flags) == 0)
 			goto bailout;
 
-		if ((sense->flags & SSD_ILI) == 0)
-			goto bailout;
-
 		*block_bits = sense->flags & SSD_ILI;
 		break;
 	}
@@ -4317,9 +4320,6 @@ scsi_get_stream_info(struct scsi_sense_data *sense_dat
 		if (SSD_FIXED_IS_PRESENT(sense, sense_len, flags) == 0)
 			goto bailout;
 
-		if ((sense->flags & (SSD_ILI|SSD_EOM|SSD_FILEMARK)) == 0)
-			goto bailout;
-
 		*stream_bits = sense->flags & (SSD_ILI|SSD_EOM|SSD_FILEMARK);
 		break;
 	}
@@ -4361,8 +4361,6 @@ scsi_progress_sbuf(struct sbuf *sb, uint16_t progress)
 int
 scsi_sks_sbuf(struct sbuf *sb, int sense_key, uint8_t *sks)
 {
-	if ((sks[0] & SSD_SKS_VALID) == 0)
-		return (1);
 
 	switch (sense_key) {
 	case SSD_KEY_ILLEGAL_REQUEST: {
@@ -4459,7 +4457,7 @@ scsi_fru_sbuf(struct sbuf *sb, uint64_t fru)
 }
 
 void
-scsi_stream_sbuf(struct sbuf *sb, uint8_t stream_bits, uint64_t info)
+scsi_stream_sbuf(struct sbuf *sb, uint8_t stream_bits)
 {
 	int need_comma;
 
@@ -4467,6 +4465,7 @@ scsi_stream_sbuf(struct sbuf *sb, uint8_t stream_bits,
 	/*
 	 * XXX KDM this needs more descriptive decoding.
 	 */
+	sbuf_printf(sb, "Stream Command Sense Data: ");
 	if (stream_bits & SSD_DESC_STREAM_FM) {
 		sbuf_printf(sb, "Filemark");
 		need_comma = 1;
@@ -4479,15 +4478,15 @@ scsi_stream_sbuf(struct sbuf *sb, uint8_t stream_bits,
 
 	if (stream_bits & SSD_DESC_STREAM_ILI)
 		sbuf_printf(sb, "%sILI", (need_comma) ? "," : "");
-
-	sbuf_printf(sb, ": Info: %#jx", (uintmax_t) info);
 }
 
 void
-scsi_block_sbuf(struct sbuf *sb, uint8_t block_bits, uint64_t info)
+scsi_block_sbuf(struct sbuf *sb, uint8_t block_bits)
 {
+
+	sbuf_printf(sb, "Block Command Sense Data: ");
 	if (block_bits & SSD_DESC_BLOCK_ILI)
-		sbuf_printf(sb, "ILI: residue %#jx", (uintmax_t) info);
+		sbuf_printf(sb, "ILI");
 }
 
 void
@@ -4500,6 +4499,9 @@ scsi_sense_info_sbuf(struct sbuf *sb, struct scsi_sens
 
 	info = (struct scsi_sense_info *)header;
 
+	if ((info->byte2 & SSD_INFO_VALID) == 0)
+		return;
+
 	scsi_info_sbuf(sb, cdb, cdb_len, inq_data, scsi_8btou64(info->info));
 }
 
@@ -4528,6 +4530,9 @@ scsi_sense_sks_sbuf(struct sbuf *sb, struct scsi_sense
 
 	sks = (struct scsi_sense_sks *)header;
 
+	if ((sks->sense_key_spec[0] & SSD_SKS_VALID) == 0)
+		return;
+
 	scsi_extract_sense_len(sense, sense_len, &error_code, &sense_key,
 			       &asc, &ascq, /*show_errors*/ 1);
 
@@ -4544,6 +4549,9 @@ scsi_sense_fru_sbuf(struct sbuf *sb, struct scsi_sense
 
 	fru = (struct scsi_sense_fru *)header;
 
+	if (fru->fru == 0)
+		return;
+
 	scsi_fru_sbuf(sb, (uint64_t)fru->fru);
 }
 
@@ -4554,14 +4562,9 @@ scsi_sense_stream_sbuf(struct sbuf *sb, struct scsi_se
 		       struct scsi_sense_desc_header *header)
 {
 	struct scsi_sense_stream *stream;
-	uint64_t info;
 
 	stream = (struct scsi_sense_stream *)header;
-	info = 0;
-
-	scsi_get_sense_info(sense, sense_len, SSD_DESC_INFO, &info, NULL);
-
-	scsi_stream_sbuf(sb, stream->byte3, info);
+	scsi_stream_sbuf(sb, stream->byte3);
 }
 
 void
@@ -4571,14 +4574,9 @@ scsi_sense_block_sbuf(struct sbuf *sb, struct scsi_sen
 		      struct scsi_sense_desc_header *header)
 {
 	struct scsi_sense_block *block;
-	uint64_t info;
 
 	block = (struct scsi_sense_block *)header;
-	info = 0;
-
-	scsi_get_sense_info(sense, sense_len, SSD_DESC_INFO, &info, NULL);
-
-	scsi_block_sbuf(sb, block->byte3, info);
+	scsi_block_sbuf(sb, block->byte3);
 }
 
 void
@@ -4863,7 +4861,7 @@ scsi_sense_only_sbuf(struct scsi_sense_data *sense, u_
 		const char *asc_desc;
 		uint8_t sks[3];
 		uint64_t val;
-		int info_valid;
+		uint8_t bits;
 
 		/*
 		 * Get descriptions for the sense key, ASC, and ASCQ.  If
@@ -4882,42 +4880,28 @@ scsi_sense_only_sbuf(struct scsi_sense_data *sense, u_
 		sbuf_printf(sb, " asc:%x,%x (%s)\n", asc, ascq, asc_desc);
 
 		/*
-		 * Get the info field if it is valid.
+		 * Print any block or stream device-specific information.
 		 */
-		if (scsi_get_sense_info(sense, sense_len, SSD_DESC_INFO,
-					&val, NULL) == 0)
-			info_valid = 1;
-		else
-			info_valid = 0;
+		if (scsi_get_block_info(sense, sense_len, inq_data,
+		    &bits) == 0 && bits != 0) {
+			sbuf_cat(sb, path_str);
+			scsi_block_sbuf(sb, bits);
+			sbuf_printf(sb, "\n");
+		} else if (scsi_get_stream_info(sense, sense_len, inq_data,
+		    &bits) == 0 && bits != 0) {
+			sbuf_cat(sb, path_str);
+			scsi_stream_sbuf(sb, bits);
+			sbuf_printf(sb, "\n");
+		}
 
-		if (info_valid != 0) {
-			uint8_t bits;
-
-			/*
-			 * Determine whether we have any block or stream
-			 * device-specific information.
-			 */
-			if (scsi_get_block_info(sense, sense_len, inq_data,
-						&bits) == 0) {
-				sbuf_cat(sb, path_str);
-				scsi_block_sbuf(sb, bits, val);
-				sbuf_printf(sb, "\n");
-			} else if (scsi_get_stream_info(sense, sense_len,
-							inq_data, &bits) == 0) {
-				sbuf_cat(sb, path_str);
-				scsi_stream_sbuf(sb, bits, val);
-				sbuf_printf(sb, "\n");
-			} else if (val != 0) {
-				/*
-				 * The information field can be valid but 0.
-				 * If the block or stream bits aren't set,
-				 * and this is 0, it isn't terribly useful
-				 * to print it out.
-				 */
-				sbuf_cat(sb, path_str);
-				scsi_info_sbuf(sb, cdb, cdb_len, inq_data, val);
-				sbuf_printf(sb, "\n");
-			}
+		/*
+		 * Print the info field.
+		 */
+		if (scsi_get_sense_info(sense, sense_len, SSD_DESC_INFO,
+					&val, NULL) == 0) {
+			sbuf_cat(sb, path_str);
+			scsi_info_sbuf(sb, cdb, cdb_len, inq_data, val);
+			sbuf_printf(sb, "\n");
 		}
 
 		/* 

Modified: head/sys/cam/scsi/scsi_all.h
==============================================================================
--- head/sys/cam/scsi/scsi_all.h	Sun Apr 21 18:27:13 2019	(r346490)
+++ head/sys/cam/scsi/scsi_all.h	Sun Apr 21 19:07:03 2019	(r346491)
@@ -3749,8 +3749,8 @@ void scsi_command_sbuf(struct sbuf *sb, uint8_t *cdb, 
 void scsi_progress_sbuf(struct sbuf *sb, uint16_t progress);
 int scsi_sks_sbuf(struct sbuf *sb, int sense_key, uint8_t *sks);
 void scsi_fru_sbuf(struct sbuf *sb, uint64_t fru);
-void scsi_stream_sbuf(struct sbuf *sb, uint8_t stream_bits, uint64_t info);
-void scsi_block_sbuf(struct sbuf *sb, uint8_t block_bits, uint64_t info);
+void scsi_stream_sbuf(struct sbuf *sb, uint8_t stream_bits);
+void scsi_block_sbuf(struct sbuf *sb, uint8_t block_bits);
 void scsi_sense_info_sbuf(struct sbuf *sb, struct scsi_sense_data *sense,
 			  u_int sense_len, uint8_t *cdb, int cdb_len,
 			  struct scsi_inquiry_data *inq_data,





Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201904211907.x3LJ73vc010293>