[nvmewin] ***UNCHECKED*** Aug 28 - Patch (NVMe 1.0c Compliance and others) --- Intel Feedback

Knoblaugh, Rick Rick.Knoblaugh at lsi.com
Thu Sep 6 18:00:15 PDT 2012


Hi Alex,
                We were also curious about the structure change Ray mentioned. No worries on the NvmeCheckPendingCpl, as I see you are checking completion queue for newly completed entries -- our routine is for a different purpose, as it checks submission queue for commands that are pending.

                Thanks,

                             -Rick






From: nvmewin-bounces at lists.openfabrics.org [mailto:nvmewin-bounces at lists.openfabrics.org] On Behalf Of Robles, Raymond C
Sent: Thursday, September 06, 2012 5:25 PM
To: nvmewin at lists.openfabrics.org; 'Chang, Alex (Alex.Chang at idt.com)'
Subject: [nvmewin] ***UNCHECKED*** Aug 28 - Patch (NVMe 1.0c Compliance and others) --- Intel Feedback

Alex,

Here is Intel's feedback on your patch.  Let us know if you need any more info on our comments/questions.


-          nvme.h:

o    ADMIN_SET_FEATURES_LBA_COMMAND_RANGE_TYPE_ENTRY Structure: GUID field (changed from ULONGLONG to UCHAR [16]) - what was the reason for this change?

o    General Comment: With the 1.0c changes, will the driver be backward compatible with 1.0b? If not, do we need a mechanism to do so or have you thought about what we should be doing in this case?  Did you attempt any testing of this?


-          nvmeInit.c:

o    NVMeResetAdapter:

§  What is the use case for having a check for RDY already being 0 (we can never have nested resets so it would seem this would never be the case but not totally sure)?

o    NVMeNormalShutdown:

§  Same comment as above for reset adapter (same check is performed here).

§  The comment on line 2469 states that the code is waiting for all queues to be deleted, but really you are just checking to see that the RDY bit has been set to 0 indicating the transition of the EN bit from 1 to 0.

o    NvmeCheckPendingCpl:

§  "unsigned int" is used for a variable declaration. We always use typedef types... should be ULONG.

§  General Comment: There is already a function to detect if commands are pending... NVMeDetectPendingCmds in nvmeIo.c. This was done as part of the S3/S4 work that Rick/Arpit (LSI) did. Did you take a look at this function and see if it was similar to your new function? Was there something specific that you needed differently than what was already coded in the existing function?


-          nvmeStd.c:

o    Line 1657: Paul removed all support for CHATHAM in a previous patch (but left in CHATHAM2 support). Please remove the CHATHAM check from the code.

Thanks,
Ray

[cid:image001.png at 01CD8C59.42BDDED0]
Raymond C. Robles
Attached Platform Storage Software
Datacenter Software Division
Intel Corporation
Desk: 480.554.2600
Mobile: 480.399.0645

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20120906/99fc9af6/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image001.png
Type: image/png
Size: 1756 bytes
Desc: image001.png
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20120906/99fc9af6/attachment.png>


More information about the nvmewin mailing list