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.albsme...@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!


Cheers,
UlI
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Reply via email to