From owner-svn-src-stable-11@freebsd.org Wed May 8 15:17:50 2019 Return-Path: Delivered-To: svn-src-stable-11@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 2F82C1589E03; Wed, 8 May 2019 15:17:50 +0000 (UTC) (envelope-from mav@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id C65CC7744F; Wed, 8 May 2019 15:17:49 +0000 (UTC) (envelope-from mav@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 95E33792C; Wed, 8 May 2019 15:17:46 +0000 (UTC) (envelope-from mav@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id x48FHkhR096976; Wed, 8 May 2019 15:17:46 GMT (envelope-from mav@FreeBSD.org) Received: (from mav@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id x48FHkkO096975; Wed, 8 May 2019 15:17:46 GMT (envelope-from mav@FreeBSD.org) Message-Id: <201905081517.x48FHkkO096975@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: mav set sender to mav@FreeBSD.org using -f From: Alexander Motin Date: Wed, 8 May 2019 15:17:46 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-11@freebsd.org Subject: svn commit: r347335 - stable/11/sys/cam/scsi X-SVN-Group: stable-11 X-SVN-Commit-Author: mav X-SVN-Commit-Paths: stable/11/sys/cam/scsi X-SVN-Commit-Revision: 347335 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: C65CC7744F X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-2.96 / 15.00]; local_wl_from(0.00)[FreeBSD.org]; NEURAL_HAM_MEDIUM(-1.00)[-0.999,0]; NEURAL_HAM_SHORT(-0.96)[-0.964,0]; ASN(0.00)[asn:11403, ipnet:2610:1c1:1::/48, country:US]; NEURAL_HAM_LONG(-1.00)[-1.000,0] X-BeenThere: svn-src-stable-11@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for only the 11-stable src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 May 2019 15:17:50 -0000 Author: mav Date: Wed May 8 15:17:45 2019 New Revision: 347335 URL: https://svnweb.freebsd.org/changeset/base/347335 Log: MFC r346491: 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. Modified: stable/11/sys/cam/scsi/scsi_all.c stable/11/sys/cam/scsi/scsi_all.h Directory Properties: stable/11/ (props changed) Modified: stable/11/sys/cam/scsi/scsi_all.c ============================================================================== --- stable/11/sys/cam/scsi/scsi_all.c Wed May 8 15:17:36 2019 (r347334) +++ stable/11/sys/cam/scsi/scsi_all.c Wed May 8 15:17:45 2019 (r347335) @@ -4059,6 +4059,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; @@ -4079,6 +4083,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; @@ -4179,10 +4186,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; } @@ -4259,9 +4265,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; } @@ -4315,9 +4318,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; } @@ -4359,8 +4359,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: { @@ -4457,7 +4455,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; @@ -4465,6 +4463,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; @@ -4477,15 +4476,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 @@ -4498,6 +4497,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)); } @@ -4526,6 +4528,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); @@ -4542,6 +4547,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); } @@ -4552,14 +4560,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 @@ -4569,14 +4572,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 @@ -4861,7 +4859,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 @@ -4880,42 +4878,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: stable/11/sys/cam/scsi/scsi_all.h ============================================================================== --- stable/11/sys/cam/scsi/scsi_all.h Wed May 8 15:17:36 2019 (r347334) +++ stable/11/sys/cam/scsi/scsi_all.h Wed May 8 15:17:45 2019 (r347335) @@ -3745,8 +3745,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,