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>