From owner-svn-src-head@FreeBSD.ORG Mon Jul 15 15:14:02 2013 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 82BB3859; Mon, 15 Jul 2013 15:14:02 +0000 (UTC) (envelope-from uqs@FreeBSD.org) Received: from acme.spoerlein.net (acme.spoerlein.net [IPv6:2a01:4f8:131:23c2::1]) by mx1.freebsd.org (Postfix) with ESMTP id DEEA224A; Mon, 15 Jul 2013 15:14:01 +0000 (UTC) Received: from localhost (acme.spoerlein.net [IPv6:2a01:4f8:131:23c2::1]) by acme.spoerlein.net (8.14.7/8.14.7) with ESMTP id r6FFE0dx014498 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 15 Jul 2013 17:14:00 +0200 (CEST) (envelope-from uqs@FreeBSD.org) Date: Mon, 15 Jul 2013 17:14:00 +0200 From: Ulrich =?utf-8?B?U3DDtnJsZWlu?= To: "Kenneth D. Merry" Subject: Re: svn commit: r253274 - head/sys/cam/scsi Message-ID: <20130715151400.GB9092@acme.spoerlein.net> Mail-Followup-To: Ulrich =?utf-8?B?U3DDtnJsZWlu?= , "Kenneth D. Merry" , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org References: <201307121709.r6CH9oZr078053@svn.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201307121709.r6CH9oZr078053@svn.freebsd.org> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 15 Jul 2013 15:14:02 -0000 On Fri, 2013-07-12 at 17:09:50 +0000, Kenneth D. Merry wrote: > Author: ken > Date: Fri Jul 12 17:09:50 2013 > New Revision: 253274 > URL: http://svnweb.freebsd.org/changeset/base/253274 > > Log: > Fix a problem with READ ELEMENT STATUS that occurs on some > changers that don't support the DVCID and CURDATA bits that were > introduced in the SMC spec. > > These changers will return an Illegal Request type error if the > bits are set. This causes "chio status" to fail. > > The fix is two-fold. First, for changers that claim to be SCSI-2 > or older, don't set the DVCID and CURDATA bits for READ ELEMENT > STATUS. For newer changers (SCSI-3 and newer), we default to > setting the new bits, but back off and try the READ ELEMENT STATUS > without the bits if we get an Illegal Request type error. > > This has been tested on a Qualstar TLS-8211, which is a SCSI-2 > changer that does not support the new bits, and a Spectra T-380, > which is a SCSI-3 changer that does support the new bits. In the > absence of a SCSI-3 changer that does not support the bits, I > tested that with some error injection code. (The SMC spec says > that support for CURDATA is mandatory, and DVCID is optional.) > > scsi_ch.c: Add a new quirk, CH_Q_NO_DVCID that gets set for > SCSI-2 and older libraries, or newer libraries that > report errors when the DVCID/CURDATA bits are set. > > In chgetelemstatus(), use the new quirk to > determine whether or not to set DVCID and CURDATA. > If we get an error with the bits set, back off and > try without the bits. Set the quirk flag if the > read element status succeeds without the bits set. > > Increase the READ ELEMENT STATUS timeout to 60 > seconds after testing with a Spectra T-380. The > previous value was 10 seconds, and too short for > the T-380. This may be decreased later after > some additional testing and investigation. > > Tested by: Andre Albsmeier > Sponsored by: Spectra Logic > MFC after: 3 days > > Modified: > head/sys/cam/scsi/scsi_ch.c > > Modified: head/sys/cam/scsi/scsi_ch.c > ============================================================================== > --- head/sys/cam/scsi/scsi_ch.c Fri Jul 12 16:41:58 2013 (r253273) > +++ head/sys/cam/scsi/scsi_ch.c Fri Jul 12 17:09:50 2013 (r253274) > @@ -1284,8 +1342,8 @@ chgetelemstatus(struct cam_periph *perip > /* voltag */ want_voltags, > /* sea */ softc->sc_firsts[chet] > + cesr->cesr_element_base, > - /* dvcid */ 1, > - /* curdata */ 1, > + /* dvcid */ dvcid, > + /* curdata */ curdata, > /* count */ cesr->cesr_element_count, > /* data_ptr */ data, > /* dxfer_len */ size, Are you sure? Coverity flags this as being in the wrong argument order (there's no CID for this yet). CID null (#2 of 2): Arguments in wrong order (SWAPPED_ARGUMENTS) swapped_arguments: The positions of arguments curdata and dvcid are inconsistent with the positions of the corresponding parameters for "scsi_read_element_status(struct ccb_scsiio *, u_int32_t, void (*)(struct cam_periph *, union ccb *), u_int8_t, int, u_int32_t, int, int, u_int32_t, u_int8_t *, u_int32_t, u_int8_t, u_int32_t)". [show details] 1338 scsi_read_element_status(&ccb->csio, 1339 /* retries */ 1, 1340 /* cbfcnp */ chdone, 1341 /* tag_action */ MSG_SIMPLE_Q_TAG, 1342 /* voltag */ want_voltags, 1343 /* sea */ softc->sc_firsts[chet] 1344 + cesr->cesr_element_base, 1345 /* dvcid */ dvcid, 1346 /* curdata */ curdata, 1347 /* count */ cesr->cesr_element_count, 1348 /* data_ptr */ data, 1349 /* dxfer_len */ size, 1350 /* sense_len */ SSD_FULL_SIZE, 1351 /* timeout */ CH_TIMEOUT_READ_ELEMENT_STATUS); And this is the definition: 1860void 1861scsi_read_element_status(struct ccb_scsiio *csio, u_int32_t retries, 1862 void (*cbfcnp)(struct cam_periph *, union ccb *), 1863 u_int8_t tag_action, int voltag, u_int32_t sea, 1864 int curdata, int dvcid, 1865 u_int32_t count, u_int8_t *data_ptr, 1866 u_int32_t dxfer_len, u_int8_t sense_len, 1867 u_int32_t timeout) Looks like you want to swap those two lines above, thanks! Cheers, UlI