From owner-svn-src-stable@FreeBSD.ORG Thu Jul 11 22:02:13 2013 Return-Path: Delivered-To: svn-src-stable@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id E3A5F5E3; Thu, 11 Jul 2013 22:02:13 +0000 (UTC) (envelope-from ken@kdm.org) Received: from nargothrond.kdm.org (nargothrond.kdm.org [70.56.43.81]) by mx1.freebsd.org (Postfix) with ESMTP id 7CF851861; Thu, 11 Jul 2013 22:02:13 +0000 (UTC) Received: from nargothrond.kdm.org (localhost [127.0.0.1]) by nargothrond.kdm.org (8.14.2/8.14.2) with ESMTP id r6BM2C22030411; Thu, 11 Jul 2013 16:02:12 -0600 (MDT) (envelope-from ken@nargothrond.kdm.org) Received: (from ken@localhost) by nargothrond.kdm.org (8.14.2/8.14.2/Submit) id r6BM2B7W030410; Thu, 11 Jul 2013 16:02:11 -0600 (MDT) (envelope-from ken) Date: Thu, 11 Jul 2013 16:02:11 -0600 From: "Kenneth D. Merry" To: Andre Albsmeier Subject: Re: svn commit: r252214 - in stable/9: bin/chio sys/cam/scsi sys/sys Message-ID: <20130711220211.GA30392@nargothrond.kdm.org> References: <201306252143.r5PLhoM0064338@svn.freebsd.org> <20130702055333.GA2729@bali> <20130703052236.GA65730@nargothrond.kdm.org> <20130703054349.GA13656@bali> <20130710184915.GA28790@nargothrond.kdm.org> <20130710185833.GA83135@bali> <20130711205332.GA26100@nargothrond.kdm.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="9amGYk9869ThD9tj" Content-Disposition: inline In-Reply-To: <20130711205332.GA26100@nargothrond.kdm.org> User-Agent: Mutt/1.4.2i Cc: "src-committers@freebsd.org" , "asomers@freebsd.org" , "svn-src-stable@freebsd.org" , "svn-src-all@freebsd.org" , "svn-src-stable-9@freebsd.org" , "gibbs@freebsd.org" X-BeenThere: svn-src-stable@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for all the -stable branches of the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Jul 2013 22:02:14 -0000 --9amGYk9869ThD9tj Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 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 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 server kernel: (ch0:ahd3:0:1:0): CAM status: SCSI Status Error > > > > > > Jul 2 07:08:22 server kernel: (ch0:ahd3:0:1:0): SCSI status: Check Condition > > > > > > Jul 2 07:08:22 server kernel: (ch0:ahd3:0:1:0): SCSI sense: ILLEGAL REQUEST asc:24,0 (Invalid field in CDB) > > > > > > Jul 2 07:08:22 server kernel: (ch0:ahd3:0:1:0): Command byte 6 is invalid > > > > > > Jul 2 07:08:22 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: 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 ken@FreeBSD.ORG --9amGYk9869ThD9tj Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="scsi_ch.c.read_element_status.20130711.txt" ==== //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, --9amGYk9869ThD9tj--