From owner-freebsd-current@freebsd.org Thu Mar 3 04:40:58 2016 Return-Path: Delivered-To: freebsd-current@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 33B7CAC25F8 for ; Thu, 3 Mar 2016 04:40:58 +0000 (UTC) (envelope-from scott4long@yahoo.com) Received: from mailman.ysv.freebsd.org (mailman.ysv.freebsd.org [IPv6:2001:1900:2254:206a::50:5]) by mx1.freebsd.org (Postfix) with ESMTP id 168721016 for ; Thu, 3 Mar 2016 04:40:58 +0000 (UTC) (envelope-from scott4long@yahoo.com) Received: by mailman.ysv.freebsd.org (Postfix) id 1581BAC25F3; Thu, 3 Mar 2016 04:40:58 +0000 (UTC) Delivered-To: current@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 150A2AC25F2 for ; Thu, 3 Mar 2016 04:40:58 +0000 (UTC) (envelope-from scott4long@yahoo.com) Received: from nm13-vm4.bullet.mail.gq1.yahoo.com (nm13-vm4.bullet.mail.gq1.yahoo.com [98.136.218.223]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id E58201014 for ; Thu, 3 Mar 2016 04:40:57 +0000 (UTC) (envelope-from scott4long@yahoo.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1456979648; bh=WrFp9V/33FP1bsmF71UyyvBcSlbjuUb+r1vGM1zpmGg=; h=Subject:From:In-Reply-To:Date:Cc:References:To:From:Subject; b=cd+ODG2dIsA2LujYbbnOzsgVcdaFx9ZjkTeUBts+Ulhw+9lzosrlWTmVejlJdsZXvMreHZLMDh1vewyOmtWXHk3oh7EU8Zn+QMH4pT6qm3ES5/4IRAT51NaKJFa5W6OFIAEXUVN3u4mHOwWdV4w0uRAYXaY1H48rtIN0wRqBkmutADUaa5CEl14vudh8ASP7DXpMDtxmusZfFZcYgBj1gOhAJ+QE9TSgehyo9qLfU5sNDlYPmVg1xG45j/HjD3rF6u489ltGEtrSBoMPUPAgotO6YCS4EqZLRhQn6lCH/1FEyv//bYapoHrG542Ae/vZVny/JCqSqp/HVgNUw4ZzXA== Received: from [98.137.12.62] by nm13.bullet.mail.gq1.yahoo.com with NNFMP; 03 Mar 2016 04:34:08 -0000 Received: from [208.71.42.193] by tm7.bullet.mail.gq1.yahoo.com with NNFMP; 03 Mar 2016 04:34:08 -0000 Received: from [127.0.0.1] by smtp204.mail.gq1.yahoo.com with NNFMP; 03 Mar 2016 04:34:08 -0000 X-Yahoo-Newman-Id: 214424.84781.bm@smtp204.mail.gq1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: WOvY3psVM1mdxXpbsNSZCHMK3IcoNYxTKiQRIHglkua2SqL wlr_D694MaVtAl6ZSrinNnQmMm_w.QtgxThnea095M_Tx3unjMUh1ufPRGlh xtJCHG5czo_LweuXHT98B4gBiBVAD4U72VcVHpNOWlIP9B1QGtq7ta9EAppj .G0iIXjZx4mme00VNJ7GDkhvlwdm9u8qH5spv28_b_1F8qch.jgWPCeRQP.G BWf3DGJtaeRdRIEkmsYUc4iQawi5GMpZgACeWIB802a9XRONTipRkgPKLdZT TBlji5a80R0N2Vx.Lie.W3h9rO6eUWhKKPFx0jW9zACbUeAOYtgnXdsk5BV0 il14i0m8Y3wSfKygKvIs4TZT3uASeWRZMs33B2Y5VOXyyE3g47NWPXCyYVBJ 0nAqsn3fJjC54yATeQHC4viI7bJzVvuiKvZqoeA.PExJSHxtUnzY_gqu0ZKa 1s48yWzK_PvLDUokEIBqoqoy8TavkTspu8G58jw_2R6.u3tUc3mpxubIKWze G6JI3P2OZA9DzaS0zxwYKT5e36m7N.uokWRD3ayGR X-Yahoo-SMTP: clhABp.swBB7fs.LwIJpv3jkWgo2NU8- Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 9.2 \(3112\)) Subject: Re: CAM Shingled Disk support patches available From: Scott Long In-Reply-To: <20160302212525.GA5920@mithlond.kdm.org> Date: Wed, 2 Mar 2016 21:34:05 -0700 Cc: scsi@freebsd.org, current@freebsd.org Content-Transfer-Encoding: quoted-printable Message-Id: References: <20151118171309.GA3564@mithlond.kdm.org> <20160118223704.GA87506@mithlond.kdm.org> <20160301224758.GA86834@mithlond.kdm.org> <20160302212525.GA5920@mithlond.kdm.org> To: "Kenneth D. Merry" X-Mailer: Apple Mail (2.3112) X-Mailman-Approved-At: Thu, 03 Mar 2016 04:55:34 +0000 X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 03 Mar 2016 04:40:58 -0000 > On Mar 2, 2016, at 2:25 PM, Kenneth D. Merry wrote: >=20 >>=20 >>=20 >> void scsi_ata_pass(struct ccb_scsiio *csio, u_int32_t retries, >> void (*cbfcnp)(struct cam_periph *, union ccb = *), >> u_int32_t flags, u_int8_t tag_action, >> struct ata_cmd *cmd, struct ata_res *res, >> u_int8_t *data_ptr, u_int32_t dxfer_len, >> u_int8_t *data_ptr, u_int16_t dxfer_len, >=20 > I assume you only intended one line there, not two. :) >=20 Indeed =3D-) >> u_int8_t sense_len, u_int32_t timeout); >>=20 >> To differentiate between the 12 and 16 byte variants, you???d look at = the >> AP_EXTEND flag in the protocol field. Btw, the handling of that flag = is >> inconsistent in the implementation of the existing = scsi_ata_pass_16(). If >> the caller providse an ata_res pointer then it gets filled on = completion, >> otherwise the caller does its best to look at 12.2.2.6 and extract = what it >> can from the sense descriptor. >>=20 >> So my proposal is to create a new scsi_ata_pass and deprecate but not = remove >> scsi_ata_pass_16. Tell people that if they need to use it, dxfer_len = is going to >> have lower priority than sector_count/sector_count_exp if the latter = multiply to >> more than 65535. >=20 > In general I think that's a reasonable idea, but we should probably go > further. >=20 > While we're at it, we should figure out what we need to do to add the > Auxiliary register to struct ata_cmd. We'll need that to do the NCQ > versions of the various SMR commands, as well as TRIM. >=20 Warner and I talked about this, and I thought that at one point we had a = patch that defined a =E2=80=98struct ata_cmd_aux=E2=80=99. As with function = signatures, I=E2=80=99m very much against redefining structures, and I think it=E2=80=99s reasonable = to create a new structure for what=E2=80=99s basically a late addition to the specs. = I need to read more of the draft ACS and SATA specs to see if there=E2=80=99s anything = else on the horizon that should also be included. However, and I talk a little bit = about this below, I think the situation is a bit more complicated than just = adding a field to the structure. The ata_cmd structure mostly models what=E2=80=99= s in an ATA taskfile, and the taskfile definition, whether from ACS or APT, has = never been expanded to include the aux (and aux_exp) registers. They exist = only in SATA as part of the Device-to-Host (D2H) FIS. However, I=E2=80=99m = kinda back and forth on this; maybe my interpretation of ata_cmd is too strict, and = we can stick in whatever is needed and let the SIM deal with it. At one point I had some patches that defined the various FIS packets and allowed the periphs to send in an XPT_SATA_FIS instead of an XPT_ATA_IO, but it seemed to not provide much value since most drivers (and hardware) still operate in terms of legacy ATA = taskfiles. It also wasn=E2=80=99t clear in the scheme of driving I/O via a FIS = where the responsibility of the periph stopped and the SIM started; If the periph sends a H2D FIS to initiate an I/O, does it need to then drive the PIO and/or DMA FIS=E2=80=99s as well? The FIS is really in the realm of the = transport, not the protocol, and it=E2=80=99s a huge shame that ATA is starting to = blur the lines by having the FIS Aux registers be part of the protocol. > The obvious challenge is that probably means changing the existing = struct > ccb_ataio CCB and bumping the CAM version. At least that will be = source > compatible, but will require ifdefs if people want to compile on older > versions of FreeBSD. But in that case, they'll also be faced with no > support for sending the NCQ versions of the commands, anyway. No way > around that, though, since we have to follow the changing specs. >=20 Again, not a fan of redefining the structures. A couple of other thoughts here. Steve Hartland was right about not = abusing the AP_EXTEND flag. What are your thoughts on having a new flag in the function to signal 12 vs 16 byte variants? Also do you have any = thoughts on the existing arguments? I=E2=80=99m not sure that tag_action has = ever made any sense for the ATA transport, there=E2=80=99s no such a thing as ordered = tags in ATA and we don=E2=80=99t expose the NCQ tag number outside of the SIM. MSG_SIMPLE_Q_TAG definitely has no meaning in ATA. I think the argument could go away. Second, I=E2=80=99m not sure that cam_ata_pass (or cam_ata_pass_16, for = that matter) is the right place to include the aux register. My reading is that = it=E2=80=99s implementing the 12.2.2.x chapter of SAT-3, and that doesn=E2=80=99t have any = allowance for the aux register. SAT-4 r.04a doesn=E2=80=99t seem to mention this register = either. The register seems to only be exposed when you have access to creating a H2D = FIS, and SAT is pretty much silent on that. IIRC, when Warner implemented NCQ TRIM, which required the Aux register, it could only be used on = devices attached via AHCI; LSI controllers had no way of expressing the = register. Still, we could add this ad-hoc to cam_ata_pass(). Maybe instead of it taking = an ata_cmd struct, it takes a union of ata_cmd and ata_cmd_aux, and we add = an argument to the function that tells it what is in the union. Maybe the = union could also include a H2D FIS? Anyways, here=E2=80=99s a possibility: union ata_cmds { struct ata_cmd cmd; struct ata_cmd_aux cmd_aux; /* Don=E2=80=99t = forget that this includes both aux registers! */ struct ata_fis_h2d fis; }; #define ATA_FORMAT_CMD 0x01 #define ATA_FORMAT_CMD_AUX 0x02 #define ATA_FORMAT_FIS_H2D 0x03 void scsi_ata_pass(struct ccb_scsiio *csio, u_int32_t retries, void (*cbfcnp)(struct cam_periph *, union ccb *), u_int32_t flags, u_int format, union ata_cmds *cmd, struct ata_res *res, u_int8_t *data_ptr, u_int32_t dxfer_len, u_int8_t *data_ptr, u_int16_t dxfer_len, u_int8_t sense_len, u_int32_t timeout); Scott > Ken > --=20 > Kenneth Merry > ken@FreeBSD.ORG