[nvmewin] OFA Windows NVMe driver question/ feedback

[Robles, Raymond C]

Hi Judy,

Sorry for the late response on this... I've been out on medical leave and am currently catching up on all email.  See below for my comments in BOLD.

Thanks,
Ray

From: nvmewin-bounces at lists.openfabrics.org [mailto:nvmewin-bounces at lists.openfabrics.org] On Behalf Of Judy Brock-SSI
Sent: Tuesday, January 15, 2013 8:20 AM
To: nvmewin at lists.openfabrics.org
Subject: [nvmewin] OFA Windows NVMe driver question/ feedback

Hi,

I have a question and a little feedback:

1.       Currently, SntiBuildFirmwareImageDownloadCmd () always returns without generating valid PRPs due to the following code (numDwords is always zero):

    #define DEBUG_CHECK
    #ifdef DEBUG_CHECK
    if ((pSgl->NumberOfElements * sizeof(STOR_SCATTER_GATHER_ELEMENT)) >=
        (numDwords/sizeof(UINT32))) {
        /* In this case, must fail the command or create a temp buffer... */

        ASSERT(FALSE);
        return;
    }
                              #endif /* DEBUG_CHECK */

               It looks like the intention was for numDwords to have a valid value that would have allowed the function to continue as long as a temp buffer wasn't needed but it was never set...is that the correct? [Robles, Raymond C] This is correct. I'm not sure when this was added (was added after the original implementation) but you are correct in your assertion. The check should be to see that DWORD 10 (which does contain the correct value for nunDwords) was used in order to check if a temp buffer is needed. At the time of the development of this code, this was a command that was not easily testable on the QEMU VM. This is a bug that needs to be fixed and is a good candidate for contribution to the open source driver.

2.  SntiBuildFirmwareImageDownloadCmd () also has no return value and the caller is  assuming success , resulting in a malformed NVMe command being generated (PRP1 and PRP2 set all zeros) and sent out on behalf of SCSI write buffer. I believe this function should be changed to return SNTI_TRANSLATION_STATUS and SntiTranslateWriteBuffer() should be modified to check for success/failure rather than forging blindly ahead.[Robles, Raymond C]  Many of the SntiBuildXxx() functions do not have return values. This was by design when I first coded these. The reason being that if the code made it to this point (where all sanity checks had passed and we just needed to build the SQE), these functions would just build the SQE in the temp SQE unit within the scratch memory space in the SRB extension. There was never any intention to perform any types of checks within these "build" functions. They were merely helper functions... hence no return values. But as the driver evolved and the robustness grew, some of these functions had checks added to them (just like what you are pointing out) and they really should be returning values now. So, yes, I do agree this function should return a value. However, it should return SNTI_STATUS. My original intention was that all internal return values for helper functions should try to use SNTI_STATUS and all the first level major interface functions would return SNTI_TRANSLATION_STATUS. Of course there are always exceptions, but this was the guideline that I tried to implement and follow. This would be a good candidate for a  patch from a company using the driver.

3.       Seems like a few other routines which don't have any return value should also be changed to return SNTI_TRANSLATION_STATUS value( and all functions which call these routines be modified to check for success/failure):[Robles, Raymond C]  Agreed, with the exception of SntiTranslateSglToPrp(). See above response for all other functions. SntiTranslateSglToPrp() is a special case and does not return a value by design. I believe this still holds true today. The sole purpose of this function is to translate a Windows SGL to PRPs (entries, list, or combo of both). There are no scenarios in this function that would require a negative return value. When this function is invoked, a good SGL has been obtained and the needed PRP memory has already been allocated at init time (we wouldn't be at this point if either of these points were not true). Therefore, the translation process will not fail with the previous being true. A good SGL will always translate to correct PRP entries/list.

                VOID SntiTranslateSglToPrp()
                 VOID SntiBuildFirmwareActivateCmd()
                 VOID SntiBuildSecuritySendReceiveCmd()

Thanks,
Judy

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20130121/86dd33a5/attachment.html>