Date: Wed, 2 Mar 2016 21:34:05 -0700 From: Scott Long <scott4long@yahoo.com> To: "Kenneth D. Merry" <ken@FreeBSD.ORG> Cc: scsi@freebsd.org, current@freebsd.org Subject: Re: CAM Shingled Disk support patches available Message-ID: <D4C36E25-8017-42EF-96B9-EB1500D397B7@yahoo.com> In-Reply-To: <20160302212525.GA5920@mithlond.kdm.org> References: <20151118171309.GA3564@mithlond.kdm.org> <20160118223704.GA87506@mithlond.kdm.org> <20160301224758.GA86834@mithlond.kdm.org> <CF8C42C1-9024-429C-91F5-10A7175C1089@yahoo.com> <20160302212525.GA5920@mithlond.kdm.org>
next in thread | previous in thread | raw e-mail | index | archive | help
> On Mar 2, 2016, at 2:25 PM, Kenneth D. Merry <ken@FreeBSD.ORG> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?D4C36E25-8017-42EF-96B9-EB1500D397B7>