CAM Shingled Disk support patches available

[Scott Long]

> On Mar 2, 2016, at 3:04 AM, Steven Hartland <killing at multiplay.co.uk> wrote:
> 
> scsi_ata_pass_16 was added to support TRIM and implements 12.2.3 (not 12.2.2) providing access to all features as described by Table 104. I'll admit I didn't take into account callers which make non LBA use of the LBA fields, but I'm sure you'll agree that's an experience thing i.e. knowing about about the existence of features which abuse structure fields for other purposes ;-)
> 

I’m going from SAT-3r5, which unfortunately is the only rev I have since T10 makes it hard to get SAT-3r7.  Maybe the numbers changed?  I meant 12.2.2.x, which covers 12.2.2.1, 12.2.2.2, and 12.2.2.3 in the draft that I have.  There is no 12.2.3 in my draft.

> Its use of u_int16_t dxfer_len is a bug, surprised the compiler didn't flag this, not sure this can be fixed without breaking backwards compatibility though?
> 
> Not sure I understand the 12 vs 13 registers comment, I'm guessing you mean the 12 bytes of 12.2.2, however it actually exposes all 16 bytes of 12.2.3.
> 

scsi_ata_pass_16 doesn’t accept a device argument and assumes it to be ATA_DEV_LBA, 0x40.  There are two commands in ACS-3, READ_FPDMA_QUEUED and WRITE_FPDMA_QUEUED, that could use a different value for bit 7.  Maybe that’s an unlikely case for userland passthrough, but not impossible.  However bit 4 is also a wildcard that is transport-dependent for many commands.  SATA is pretty clear about the bit always being zero.  Technically in PATA you might want to control bit 4 to specify the master vs slave device, but I’m guessing that FreeBSD would be more likely to override that than support it.  Still, I haven’t done a comprehensive review of all command sets and transports, and I think it would be prudent to leave the device register exposed to the API rather than hard-coded.

> For reference it has two callers in the kernel scsi_ata_identify used to probe TRIM capability and scsi_ata_trim used to actually perform TRIM operations.
> 

> In addition to this its also used by camcontrol to support passthrough from ata_do_cmd and ata_do_28bit_cmd again supporting TRIM, in this case the ATA security feature set as well as identify.
> 
> I would be very surprised if its used elsewhere but in order to maintain compatibility I'd definitely agree with Scott on creating an scsi_ata_pass which can be used to implement and scsi_ata_pass_16 which can then be deprecated.
> 

It’s exposed in libcam.so, and it sounds like Ken is also expanding its use, so it’s fair to say that it might have a wider audience than just those TRIM uses.

> With regards to providing 12.2.2 and 12.2.3 by looking at AP_EXTEND I don't believe that's possible as you need this to tell the difference between 28bit and 48bit ATA within 12.2.3.
> 

12.2.2.3 states the following:

If the EXTEND bit is set to zero, then the FEATURES (15:8) field, the SECTOR_COUNT (15:8) field, the LBA_LOW (15:8) field, then the LBA_MID (15:8) field, and the LBA_HIGH (15:8) field shall be ignored by the SATL, and the SATL shall process this command as specified in 12.2.2.2.

My proposal isn’t completely correct, it would be abusing the EXTEND bit to force the use of opcode A1 vs 85, and really the spec says that 85 can be used with the bit being zero.  One might want to send an 0x85 opcode with EXTEND set to zero to test that the receiver will do the right thing with ignoring the 5 extended fields, or you might have a receiver that only accepts 0x85 but you need to send a 28 bit command.  So yeah, I concede it’s not great.  Might need to either add an extra flag, or split the function into two.

Scott