redundant ata APIs

[ThunderEX]

Please describe the feature/improvement you would like to see added.

from ata_helper_func.h I could see these definitions:

    //These functions below are commands that can be sent in PIO or DMA Mode.
    //They will automatically try DMA if it is supported, then retry with PIO mode if the Translator or Driver doesn't support issuing DMA mode commands.
    OPENSEA_TRANSPORT_API int send_ATA_Read_Log_Ext_Cmd(tDevice *device, uint8_t logAddress, uint16_t pageNumber, uint8_t *ptrData, uint32_t dataSize, uint16_t featureRegister);
    OPENSEA_TRANSPORT_API int send_ATA_Write_Log_Ext_Cmd(tDevice *device, uint8_t logAddress, uint16_t pageNumber, uint8_t *ptrData, uint32_t dataSize, bool forceRTFRs);
    OPENSEA_TRANSPORT_API int send_ATA_Download_Microcode_Cmd(tDevice *device, eDownloadMicrocodeFeatures subCommand, uint16_t blockCount, uint16_t bufferOffset, uint8_t *pData, uint32_t dataLen, bool firstSegment, bool lastSegment, uint32_t timeoutSeconds);
    OPENSEA_TRANSPORT_API int send_ATA_Trusted_Send_Cmd(tDevice *device, uint8_t securityProtocol, uint16_t securityProtocolSpecific, uint8_t *ptrData, uint32_t dataSize);
    OPENSEA_TRANSPORT_API int send_ATA_Trusted_Receive_Cmd(tDevice *device, uint8_t securityProtocol, uint16_t securityProtocolSpecific, uint8_t *ptrData, uint32_t dataSize);
    OPENSEA_TRANSPORT_API int send_ATA_Read_Buffer_Cmd(tDevice *device, uint8_t *ptrData);
    OPENSEA_TRANSPORT_API int send_ATA_Write_Buffer_Cmd(tDevice *device, uint8_t *ptrData);
    OPENSEA_TRANSPORT_API int send_ATA_Read_Stream_Cmd(tDevice *device, uint8_t streamID, bool notSequential, bool readContinuous, uint8_t commandCCTL, uint64_t LBA, uint8_t *ptrData, uint32_t dataSize);
    OPENSEA_TRANSPORT_API int send_ATA_Write_Stream_Cmd(tDevice *device, uint8_t streamID, bool flush, bool writeContinuous, uint8_t commandCCTL, uint64_t LBA, uint8_t *ptrData, uint32_t dataSize);

    //Similar to above, but for SCT stuff. This will automatically retry from DMA to PIO mode. Also removes GPL flag. Now depends on if device supports GPL or not internally (can be flipped in device->drive_info.ata_Options.generalPurposeLoggingSupported if you want to force a SMART command)
    OPENSEA_TRANSPORT_API int send_ATA_SCT(tDevice *device, eDataTransferDirection direction, uint8_t logAddress, uint8_t *dataBuf, uint32_t dataSize, bool forceRTFRs);
    OPENSEA_TRANSPORT_API int send_ATA_SCT_Status(tDevice *device, uint8_t *dataBuf, uint32_t dataSize);
    OPENSEA_TRANSPORT_API int send_ATA_SCT_Command(tDevice *device, uint8_t *dataBuf, uint32_t dataSize, bool forceRTFRs);
    OPENSEA_TRANSPORT_API int send_ATA_SCT_Data_Transfer(tDevice *device, eDataTransferDirection direction, uint8_t *dataBuf, uint32_t dataSize);
    OPENSEA_TRANSPORT_API int send_ATA_SCT_Read_Write_Long(tDevice *device, eSCTRWLMode mode, uint64_t lba, uint8_t *dataBuf, uint32_t dataSize, uint16_t *numberOfECCCRCBytes, uint16_t *numberOfBlocksRequested);
    OPENSEA_TRANSPORT_API int send_ATA_SCT_Write_Same(tDevice *device, eSCTWriteSameFunctions functionCode, uint64_t startLBA, uint64_t fillCount, uint8_t *pattern, uint64_t patternLength);
    OPENSEA_TRANSPORT_API int send_ATA_SCT_Error_Recovery_Control(tDevice *device, uint16_t functionCode, uint16_t selectionCode, uint16_t *currentValue, uint16_t recoveryTimeLimit);
    OPENSEA_TRANSPORT_API int send_ATA_SCT_Feature_Control(tDevice *device, uint16_t functionCode, uint16_t featureCode, uint16_t *state, uint16_t *optionFlags);
    OPENSEA_TRANSPORT_API int send_ATA_SCT_Data_Table(tDevice *device, uint16_t functionCode, uint16_t tableID, uint8_t *dataBuf, uint32_t dataSize);

I could see that these functions has their alternatives, such as ata_SCT in ata_cmds.c is similiar to send_ATA_SCT in ata_helper.c.

Please explain why you think adding this feature/improvement is important.

Is there some reason to provide these redundant functions? Is there any plan to remove some of them?

[vonericsen]

Hi @ThunderEX,

These functions were added as a version that will retry as needed between DMA and PIO mode for these commands. They call the versions in ata_cmds setting the useDMA parameter to true or false. They were added like this because not every HBA, driver, or other adapter will support DMA mode passthrough commands. When the fill_ATA_Drive_Info function is called, it uses various bits from the identify data to figure out if the drive supports the DMA mode commands and if the host has it running in DMA mode (which should basically always be the case today unless you find a drive old enough to not support DMA...I have one just to test this obscure case). These flags for each of these DMA command modes is generally used as the default way of communicating with the drive for better performance and better compatibility (multi-sector PIO commands can be very broken on USB adapters).

After a lot of troubleshooting issues over time, we first added the --forceATAPIO, --forceATADMA, and --forceATAUDMA flags to the utilities as the first way to work around these various adapter issues. Adaptec HBAs will accept SAT commands with the protocol set to DMA, but not UDMA In/Out which was the first major case we found. I eventually implemented a retry in the send_SAT_Passthrough_Command in sat_helper.c that handled this problem and after the first retry changes a flag so that any additional DMA mode commands no longer needed a retry. LSI HBAs will accept the commands in either DMA or UDMA In/out, however we have not observed this to be the case with USB adapters. Our previous codebase before this was written had some odd comments about why USB should always use UDMA mode in the SAT CDB, so we preserved that functionality here.

As we've been improving the USB support, we started noticing a lot of variance in what USB adapters would accept for transfer sizes, PIO, DMA, UDMA modes among other issues. While we've been slowly creating a lookup table for each Vendor/Product ID, this only works on Windows and Linux right now and wanted to have some better support for anything in the future so that the table is not needed in order to get proper functionality. I came up with the idea to implement these functions that will first attempt the DMA mode command, then a PIO mode command in case the adapter responds with "invalid field in CDB". This seemed like a better option than implementing retries everywhere else and could handle it in one specific location.

This library is also used by another team inside Seagate for drive development, diagnostics, and failure analysis. Most of the time, they want the same kind of functionality of "we just want the output from the command, we do not care about the protocol", but there are sometimes cases where a specific mode is needed to run, which is why the original functions in ata_cmds.c are left as-is so that a specific mode could be used. This is getting less and less common though.

So right now, we do not intend to remove any of these functions. If you think it would make sense to add some comments or rename these functions to help document this, let me know and I will add some to clarify these reasons.

[ThunderEX]

Hi @vonericsen Thanks for the explanation.

Actually I'm going to write myself a tool to read temperature with SCT status. I used to struggle with which command to use. Now I could see that I should pick the ones with send_ preffix.

I still have some questions with ata_SCT* stuffs. It's clear to me that functions like send_ATA_Read_Log_Ext_Cmd wraps ata_Read_Log_Ext from ata_cmds.c, but send_ATA_SCT is different, it does not wrap ata_SCT from ata_cmds.c. Instead, it directly calls high level send_ATA_Read_Log_Ext_Cmd and send_ATA_Write_Log_Ext_Cmd. So when I open definition of ata_SCT and send_ATA_SCT side by side, their codes look quiet redundant. And it looks ataPTHacks.smartCommandTransportWithSMARTLogCommandsOnly is not used due to ata_SCT is not called. https://github.com/Seagate/opensea-transport/blob/836385c83593f5869e86c9c5989a5aa61ad61218/src/ata_cmds.c#L1417

Have I got that right?

[vonericsen]

Have I got that right?

This is definitely confusing and can be cleaned up a bit.

The SCT functions in ata_cmds.c came about first as their own kind of helper functions (and probably should have been in ata_helper), then when adding the retry functions they were not cleaned up, probably because they were still used in some of our other software. I think the reason send_ATA_SCT calls the send_ATA_Read_Log_Ext_Cmd and send_ATA_Write_Log_Ext_Cmd was to pull in the built-in retries of those functions. I do think it is an bug to miss checking for ataPTHacks.smartCommandTransportWithSMARTLogCommandsOnly. I am only aware of one USB device where this was necessary to prevent the device from hanging.

Let me review it a bit and figure out where it makes the most sense to do some cleanup. I will need to check what functions some of our other tools are using first to figure out the best way to clean this up.

[ThunderEX]

Thanks @vonericsen !