svn commit: r253274 - head/sys/cam/scsi

[Kenneth D. Merry]

On Mon, Jul 15, 2013 at 17:14:00 +0200, Ulrich Sp??rlein wrote:
> 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 <Andre.Albsmeier at siemens.com>
> >   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!

Oops, you're right!  Thanks for pointing it out!  I just committed a fix.

How does Coverity detect something like that?  Using the comment, or the
variable name?

Ken
-- 
Kenneth Merry
ken at FreeBSD.ORG