Re: CAM Shingled Disk support patches available

On Tue, Mar 01, 2016 at 20:07:19 -0700, Scott Long wrote:
> Hi Ken,
> 
> I???m against changing the function signature of scsi_ata_pass_16().  Even
> if you manage to get things right with symbol versioning, it still leads to
> problems of code compatibility.  Maybe pre-existing binaries will work, but
> source code will forever have to include an #if __FreeBSD_version <
> xxxxxx bit of nonsense.

Good point, that would be annoying.

> I agree that it was incorrect for dxferlen to be declared as a uint16_t.
> However, the function already contains a sector count argument pair.  In
> theory the sector count multiplied by the sector length, both of which the
> application should know in order to arrive at a sensible dxferlen, can
> substitute for the dxferlen argument.  If so, then we can just ignore that
> argument and declare that sector_count has logical priority.

Okay.  That will probably work for the most part.

> Really though, I think that scsi_ata_pass_16() is a crummy function.  If its
> purpose is to implement SAT-3 12.2.2, it does an incredibly poor job at it:
> 
> - By my count, it only covers 12 of the available 13 registers.
> 
> - It has no 12 byte, opcode 0xa1 variant.
> 
> - It doesn???t make any allowance for providing the response registers to the
> caller on completion.  Well, maybe it kinda does through a sense descriptor,
> but???. it???s kinda open to vague interpretation.
> 
> - Its use of the registers is clunky, assuming for example that you???ll only want
> to fill the six LBA registers with a host-ordered 64-bit number.  There are
> plenty of commands that re-use sub-parts of the LBA, features, and/or sector
> count registers for different things.  
> 
> I know you stated that you didn???t want to do this, but I think it???s better to start
> over with a better function that has a better signature and a new name.  In fact,
> I think it???s better to use the existing ata_cmd and ata_res structures from 
> sys/cam/ata/ata_all.h, provide accessors for the multi-byte registers if needed,
> provide a 12-byte compatibility, and simply the signature.  Something like this:
> 
> 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,

I assume you only intended one line there, not two. :)

>                       u_int8_t sense_len, u_int32_t timeout);
> 
> 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.
> 
> 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.

In general I think that's a reasonable idea, but we should probably go
further.

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.

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.

Ken
-- 
Kenneth Merry
ken_at_FreeBSD.ORG