On Thu, Jul 11, 2013 at 14:53:32 -0600, Kenneth D. Merry wrote:
> On Wed, Jul 10, 2013 at 20:58:33 +0200, Andre Albsmeier wrote:
> > On Wed, 10-Jul-2013 at 20:49:15 +0200, Kenneth D. Merry wrote:
> > > On Wed, Jul 03, 2013 at 07:43:49 +0200, Andre Albsmeier wrote:
> > > > On Wed, 03-Jul-2013 at 07:22:36 +0200, Kenneth D. Merry wrote:
> > > > > On Tue, Jul 02, 2013 at 07:53:33 +0200, Andre Albsmeier wrote:
> > > > > > On Tue, 25-Jun-2013 at 23:43:50 +0200, Kenneth D. Merry wrote:
> > > > > > > Author: ken
> > > > > > > Date: Tue Jun 25 21:43:49 2013
> > > > > > > New Revision: 252214
> > > > > > > URL: http://svnweb.freebsd.org/changeset/base/252214
> > > > > > 
> > > > > > Hi Ken,
> > > > > > 
> > > > > > > Log:
> > > > > > >   MFC: 249658, 249701
> > > > > > > 
> > > > > > >   Update chio(1) and ch(4) to support reporting element 
> > > > > > > designators.
> > > > > > > 
> > > > > > >   This allows mapping a tape drive in a changer (as reported by
> > > > > > >   'chio status') to a sa(4) driver instance by comparing the
> > > > > > >   serial numbers.
> > > > > > > 
> > > > > > >   The designators can be ASCII (which is printed out directly), 
> > > > > > > binary
> > > > > > >   (which is printed in hex format) or UTF-8, which is printed in 
> > > > > > > either
> > > > > > >   native UTF-8 format if the terminal can support it, or in %XX 
> > > > > > > notation
> > > > > > >   for non-ASCII characters.  Thanks to Hiroki Sato <hrs@> for the
> > > > > > >   explanation and example UTF-8 printing code.
> > > > > > > 
> > > > > > >   chio.h:               Modify the changer_element_status 
> > > > > > > structure to add new
> > > > > > >                 fields and definitions from the SMC3r16 spec.
> > > > > > > 
> > > > > > >                 Rename the original CHIOGSTATUS ioctl to 
> > > > > > > OCHIOGTATUS and
> > > > > > >                 define a new CHIOGSTATUS ioctl.
> > > > > > > 
> > > > > > >                 Clean up some tab/space issues.
> > > > > > > 
> > > > > > >   chio.c:       For the 'status' subcommand, print the designator 
> > > > > > > field
> > > > > > >                 if it is supplied by a device.
> > > > > > > 
> > > > > > >   scsi_ch.h:    Add new flags for DVCID and CURDATA to the READ
> > > > > > >                 ELEMENT STATUS command structure.
> > > > > > > 
> > > > > > >                 Add a read_element_status_device_id structure
> > > > > > >                 for the data fields in the new standard. Add new
> > > > > > >                 unions, dt_or_obsolete and voltage_devid, to hold
> > > > > > >                 and address data from either SCSI-2 or newer 
> > > > > > > devices.
> > > > > > > 
> > > > > > >   scsi_ch.c:    Implement support for fetching device IDs with 
> > > > > > > READ
> > > > > > >                 ELEMENT STATUS data.
> > > > > > > 
> > > > > > >                 Add new arguments to scsi_read_element_status() to
> > > > > > >                 allow the user to request the DVCID and CURDATA 
> > > > > > > bits.
> > > > > > 
> > > > > > This broke "chio status" when talking to my QUALSTAR TLS-8211 
> > > > > > library:
> > > > > > 
> > > > > > Jul  2 07:08:22 <kern.crit> server kernel: (ch0:ahd3:0:1:0): READ 
> > > > > > ELEMENT STATUS. CDB: b8 10 fd e8 00 01 03 00 04 00 00 00
> > > > > > Jul  2 07:08:22 <kern.crit> server kernel: (ch0:ahd3:0:1:0): CAM 
> > > > > > status: SCSI Status Error
> > > > > > Jul  2 07:08:22 <kern.crit> server kernel: (ch0:ahd3:0:1:0): SCSI 
> > > > > > status: Check Condition
> > > > > > Jul  2 07:08:22 <kern.crit> server kernel: (ch0:ahd3:0:1:0): SCSI 
> > > > > > sense: ILLEGAL REQUEST asc:24,0 (Invalid field in CDB)
> > > > > > Jul  2 07:08:22 <kern.crit> server kernel: (ch0:ahd3:0:1:0): 
> > > > > > Command byte 6 is invalid
> > > > > > Jul  2 07:08:22 <kern.crit> server kernel: (ch0:ahd3:0:1:0): Error 
> > > > > > 22, Unretryable error
> > > > > > 
> > > > > > scsi_cmd->flags (Byte 6) must be zero for this library so I had to 
> > > > > > use
> > > > > > this workaround to make it work again until a better solution is 
> > > > > > available:
> > > > > > 
> > > > > > --- sys/cam/scsi/scsi_ch.c.ORI      2013-06-26 13:38:54.000000000 
> > > > > > +0200
> > > > > > +++ sys/cam/scsi/scsi_ch.c  2013-07-02 07:42:24.000000000 +0200
> > > > > > @@ -1245,8 +1245,8 @@
> > > > > >                              /* tag_action */ MSG_SIMPLE_Q_TAG,
> > > > > >                              /* voltag */ want_voltags,
> > > > > >                              /* sea */ softc->sc_firsts[chet],
> > > > > > -                            /* dvcid */ 1,
> > > > > > -                            /* curdata */ 1,
> > > > > > +                            /* dvcid */ 0,
> > > > > > +                            /* curdata */ 0,
> > > > > >                              /* count */ 1,
> > > > > >                              /* data_ptr */ data,
> > > > > >                              /* dxfer_len */ 1024,
> > > > > > @@ -1284,8 +1284,8 @@
> > > > > >                              /* voltag */ want_voltags,
> > > > > >                              /* sea */ softc->sc_firsts[chet]
> > > > > >                              + cesr->cesr_element_base,
> > > > > > -                            /* dvcid */ 1,
> > > > > > -                            /* curdata */ 1,
> > > > > > +                            /* dvcid */ 0,
> > > > > > +                            /* curdata */ 0,
> > > > > >                              /* count */ cesr->cesr_element_count,
> > > > > >                              /* data_ptr */ data,
> > > > > >                              /* dxfer_len */ size,
> > > > > > 
> > > > > >     -Andre
> > > > > 
> > > > > Oops, sorry.
> > > > > 
> > > > > We need to check the SCSI version to see whether to set those bits, 
> > > > > and
> > > > > also fall back in case it doesn't work.
> > > > > 
> > > > > I am on vacation and have very spotty net access.  I can't do anything
> > > > > about it until I get back next week.
> > > > > 
> > > > > Justin and Alan (CCed) can work on the fix, though.
> > > > 
> > > > Take your time, for me it's working right now
> > > > with the above patch...
> > > 
> > > Okay, I'm back and can take a look at this.
> > 
> > Welcome back ;-)
> > 
> > > 
> > > Can you send me:
> > 
> > Sure, I am happy to help.
> > 
> > > 
> > > camcontrol inquiry ch0 -v
> > 
> > pass17: <QUALSTAR TLS-8211 227d> Removable Changer SCSI-2 device 
> > pass17: Serial Number 0021009613
> > pass17: 3.300MB/s transfers
> > 
> > > camcontrol cmd ch0 -v -c "12 0 0 0 ff 0" -i 255 "s58 i2 i2 i2 i2 i2 i2 i2 
> > > i2"
> > 
> > 0 0 0 0 0 0 0 0 
> 
> Okay, that was helpful, thanks!
> 
> > > 
> > > I want to see what SCSI version this changer reports.  The second command
> > > will print out the SCSI version descriptors, if any, that the changer
> > > reports.
> > 
> > In case you need more info just drop me a note.
> > 
> > Thanks for looking into this.
> 
> Can you try the attached patch, and see how it works for you?
> 
> It defaults to not setting those bits for changers that claim to be SCSI-2
> or below.  And if a SCSI-3 or higher changer returns an Illegal Request
> type error, it will retry with the bits cleared to see whether that works.
> 
> I also bumped the read element status timeout after some testing with a
> Spectra T-380.
> 
> The patch is against head, but stable/9 shouldn't be much different.

Oops, here is the patch.

Ken
-- 
Kenneth Merry
k...@freebsd.org
==== //depot/users/kenm/FreeBSD-test4/sys/cam/scsi/scsi_ch.c#6 - 
/usr/home/kenm/perforce4/kenm/FreeBSD-test4/sys/cam/scsi/scsi_ch.c ====
*** /tmp/tmp.49464.18   Thu Jul 11 14:45:15 2013
--- /usr/home/kenm/perforce4/kenm/FreeBSD-test4/sys/cam/scsi/scsi_ch.c  Thu Jul 
11 14:43:14 2013
***************
*** 102,108 ****
  static const u_int32_t        CH_TIMEOUT_MOVE_MEDIUM               = 100000;
  static const u_int32_t        CH_TIMEOUT_EXCHANGE_MEDIUM           = 100000;
  static const u_int32_t        CH_TIMEOUT_POSITION_TO_ELEMENT       = 100000;
! static const u_int32_t        CH_TIMEOUT_READ_ELEMENT_STATUS       = 10000;
  static const u_int32_t        CH_TIMEOUT_SEND_VOLTAG               = 10000;
  static const u_int32_t        CH_TIMEOUT_INITIALIZE_ELEMENT_STATUS = 500000;
  
--- 102,108 ----
  static const u_int32_t        CH_TIMEOUT_MOVE_MEDIUM               = 100000;
  static const u_int32_t        CH_TIMEOUT_EXCHANGE_MEDIUM           = 100000;
  static const u_int32_t        CH_TIMEOUT_POSITION_TO_ELEMENT       = 100000;
! static const u_int32_t        CH_TIMEOUT_READ_ELEMENT_STATUS       = 60000;
  static const u_int32_t        CH_TIMEOUT_SEND_VOLTAG               = 10000;
  static const u_int32_t        CH_TIMEOUT_INITIALIZE_ELEMENT_STATUS = 500000;
  
***************
*** 122,133 ****
  
  typedef enum {
        CH_Q_NONE       = 0x00,
!       CH_Q_NO_DBD     = 0x01
  } ch_quirks;
  
  #define CH_Q_BIT_STRING       \
        "\020"          \
!       "\001NO_DBD"
  
  #define ccb_state     ppriv_field0
  #define ccb_bp                ppriv_ptr1
--- 122,135 ----
  
  typedef enum {
        CH_Q_NONE       = 0x00,
!       CH_Q_NO_DBD     = 0x01,
!       CH_Q_NO_DVCID   = 0x02
  } ch_quirks;
  
  #define CH_Q_BIT_STRING       \
        "\020"          \
!       "\001NO_DBD"    \
!       "\002NO_DVCID"
  
  #define ccb_state     ppriv_field0
  #define ccb_bp                ppriv_ptr1
***************
*** 396,401 ****
--- 398,411 ----
        periph->softc = softc;
        softc->quirks = CH_Q_NONE;
  
+       /*
+        * The DVCID and CURDATA bits were not introduced until the SMC
+        * spec.  If this device claims SCSI-2 or earlier support, then it
+        * very likely does not support these bits.
+        */
+       if (cgd->inq_data.version <= SCSI_REV_2)
+               softc->quirks |= CH_Q_NO_DVCID;
+ 
        bzero(&cpi, sizeof(cpi));
        xpt_setup_ccb(&cpi.ccb_h, periph->path, CAM_PRIORITY_NORMAL);
        cpi.ccb_h.func_code = XPT_PATH_INQ;
***************
*** 1208,1213 ****
--- 1218,1225 ----
        caddr_t data = NULL;
        size_t size, desclen;
        int avail, i, error = 0;
+       int curdata, dvcid, sense_flags;
+       int try_no_dvcid = 0;
        struct changer_element_status *user_data = NULL;
        struct ch_softc *softc;
        union ccb *ccb;
***************
*** 1239,1252 ****
        cam_periph_lock(periph);
        ccb = cam_periph_getccb(periph, CAM_PRIORITY_NORMAL);
  
        scsi_read_element_status(&ccb->csio,
                                 /* retries */ 1,
                                 /* cbfcnp */ chdone,
                                 /* tag_action */ MSG_SIMPLE_Q_TAG,
                                 /* voltag */ want_voltags,
                                 /* sea */ softc->sc_firsts[chet],
!                                /* dvcid */ 1,
!                                /* curdata */ 1,
                                 /* count */ 1,
                                 /* data_ptr */ data,
                                 /* dxfer_len */ 1024,
--- 1251,1281 ----
        cam_periph_lock(periph);
        ccb = cam_periph_getccb(periph, CAM_PRIORITY_NORMAL);
  
+       sense_flags = SF_RETRY_UA;
+       if (softc->quirks & CH_Q_NO_DVCID) {
+               dvcid = 0;
+               curdata = 0;
+       } else {
+               dvcid = 1;
+               curdata = 1;
+               /*
+                * Don't print anything for an Illegal Request, because
+                * these flags can cause some changers to complain.  We'll
+                * retry without them if we get an error.
+                */
+               sense_flags |= SF_QUIET_IR;
+       }
+ 
+ retry_einval:
+ 
        scsi_read_element_status(&ccb->csio,
                                 /* retries */ 1,
                                 /* cbfcnp */ chdone,
                                 /* tag_action */ MSG_SIMPLE_Q_TAG,
                                 /* voltag */ want_voltags,
                                 /* sea */ softc->sc_firsts[chet],
!                                /* dvcid */ dvcid,
!                                /* curdata */ curdata,
                                 /* count */ 1,
                                 /* data_ptr */ data,
                                 /* dxfer_len */ 1024,
***************
*** 1254,1262 ****
                                 /* timeout */ CH_TIMEOUT_READ_ELEMENT_STATUS);
  
        error = cam_periph_runccb(ccb, cherror, /*cam_flags*/ CAM_RETRY_SELTO,
!                                 /*sense_flags*/ SF_RETRY_UA,
                                  softc->device_stats);
  
        if (error)
                goto done;
        cam_periph_unlock(periph);
--- 1283,1320 ----
                                 /* timeout */ CH_TIMEOUT_READ_ELEMENT_STATUS);
  
        error = cam_periph_runccb(ccb, cherror, /*cam_flags*/ CAM_RETRY_SELTO,
!                                 /*sense_flags*/ sense_flags,
                                  softc->device_stats);
  
+       /*
+        * An Illegal Request sense key (only used if there is no asc/ascq)
+        * or 0x24,0x00 for an ASC/ASCQ both map to EINVAL.  If dvcid or
+        * curdata are set (we set both or neither), try turning them off
+        * and see if the command is successful.
+        */
+       if ((error == EINVAL)
+        && (dvcid || curdata))  {
+               dvcid = 0;
+               curdata = 0;
+               error = 0;
+               /* At this point we want to report any Illegal Request */
+               sense_flags &= ~SF_QUIET_IR;
+               try_no_dvcid = 1;
+               goto retry_einval;
+       }
+ 
+       /*
+        * In this case, we tried a read element status with dvcid and
+        * curdata set, and it failed.  We retried without those bits, and
+        * it succeeded.  Suggest to the user that he set a quirk, so we
+        * don't go through the retry process the first time in the future.
+        * This should only happen on changers that claim SCSI-3 or higher,
+        * but don't support these bits.
+        */
+       if ((try_no_dvcid != 0)
+        && (error == 0))
+               softc->quirks |= CH_Q_NO_DVCID;
+ 
        if (error)
                goto done;
        cam_periph_unlock(periph);
***************
*** 1284,1291 ****
                                 /* voltag */ want_voltags,
                                 /* sea */ softc->sc_firsts[chet]
                                 + cesr->cesr_element_base,
!                                /* dvcid */ 1,
!                                /* curdata */ 1,
                                 /* count */ cesr->cesr_element_count,
                                 /* data_ptr */ data,
                                 /* dxfer_len */ size,
--- 1342,1349 ----
                                 /* voltag */ want_voltags,
                                 /* sea */ softc->sc_firsts[chet]
                                 + cesr->cesr_element_base,
!                                /* dvcid */ dvcid,
!                                /* curdata */ curdata,
                                 /* count */ cesr->cesr_element_count,
                                 /* data_ptr */ data,
                                 /* dxfer_len */ size,
_______________________________________________
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