[nvmewin] UNCHECKED Sep 7 - Patch (NVMe 1.0c Compliance and others)

Tue Sep 11 15:51:49 PDT 2012

Hi Ray,

Hope it would go thru this time...

Thanks,
Alex

________________________________
From: Robles, Raymond C [mailto:raymond.c.robles at intel.com]
Sent: Tuesday, September 11, 2012 3:44 PM
To: Chang, Alex
Subject: RE: [nvmewin] ***UNCHECKED*** Sep 7 - Patch (NVMe 1.0c Compliance and others)

Hi Alex,
No I did not receive anything from the nvmwin mailing list from you. The last zip file I have from you came on the 7th last week.
Can you resend the zip to the distribution list?
Thanks,
Ray
From: Chang, Alex [mailto:Alex.Chang at idt.com]
Sent: Tuesday, September 11, 2012 3:42 PM
To: Robles, Raymond C
Subject: RE: [nvmewin] ***UNCHECKED*** Sep 7 - Patch (NVMe 1.0c Compliance and others)
Hi Ray,
I just sent out another zip file earlier today. Did you receive it? It includes the changes you suggested.
Thanks,
Alex
________________________________
From: Robles, Raymond C [mailto:raymond.c.robles at intel.com]<mailto:[mailto:raymond.c.robles at intel.com]>
Sent: Tuesday, September 11, 2012 3:31 PM
To: Chang, Alex; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: [nvmewin] ***UNCHECKED*** Sep 7 - Patch (NVMe 1.0c Compliance and others)
Thank you Alex.
If nobody has any more feedback on Alex's changes (IDT) by EOD tomorrow (Wed. 9/12), then I'll push the patch.
Rick and Arpit - are you both ok with the last revision of Alex's changes?
Thanks,
Ray
From: nvmewin-bounces at lists.openfabrics.org<mailto:nvmewin-bounces at lists.openfabrics.org> [mailto:nvmewin-bounces at lists.openfabrics.org]<mailto:[mailto:nvmewin-bounces at lists.openfabrics.org]> On Behalf Of Chang, Alex
Sent: Friday, September 07, 2012 5:07 PM
To: nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: [nvmewin] ***UNCHECKED*** Sep 7 - Patch (NVMe 1.0c Compliance and others)
Hi all,
I don't receive any more new feedbacks and assume everyone agrees the patch is good to go. Here comes the new sources after removing the RDY bit checking in NVMeResetAdapter. Password is idt123. Thanks again.
Regards,
Alex
________________________________
From: Luse, Paul E [mailto:paul.e.luse at intel.com]<mailto:[mailto:paul.e.luse at intel.com]>
Sent: Friday, September 07, 2012 2:10 PM
To: Chang, Alex; Kong, Kwok; Robles, Raymond C; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: [nvmewin] ***UNCHECKED*** Aug 28 - Patch (NVMe 1.0c Compliance and others) --- Intel Feedback
Good stuff Alex, thanks for contributing!
From: Chang, Alex [mailto:Alex.Chang at idt.com]<mailto:[mailto:Alex.Chang at idt.com]>
Sent: Friday, September 07, 2012 2:10 PM
To: Luse, Paul E; Kong, Kwok; Robles, Raymond C; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: [nvmewin] ***UNCHECKED*** Aug 28 - Patch (NVMe 1.0c Compliance and others) --- Intel Feedback
Paul,
I agree that "for error recovery we can't rely on any register values to be correct;" I will remove the checking and send out another zip file by the end of today if no more feedbacks or comments received. I appreciate all the inputs.
Regards,
Alex
________________________________
From: Luse, Paul E [mailto:paul.e.luse at intel.com]<mailto:[mailto:paul.e.luse at intel.com]>
Sent: Friday, September 07, 2012 1:48 PM
To: Chang, Alex; Kong, Kwok; Robles, Raymond C; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: [nvmewin] ***UNCHECKED*** Aug 28 - Patch (NVMe 1.0c Compliance and others) --- Intel Feedback
Alex-
Wrt checking RDY before letting the reset through, I'd like to remove that check.  We already check in the DPC code that issues error recovery resets to make sure we aren't sending more than 1 outstanding and for error recovery we can't rely on any register values to be correct; we don't know the condition is that we're wanting to reset the card for.
Thx
Paul
From: nvmewin-bounces at lists.openfabrics.org<mailto:nvmewin-bounces at lists.openfabrics.org> [mailto:nvmewin-bounces at lists.openfabrics.org]<mailto:[mailto:nvmewin-bounces at lists.openfabrics.org]> On Behalf Of Chang, Alex
Sent: Friday, September 07, 2012 8:59 AM
To: Kong, Kwok; Robles, Raymond C; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: Re: [nvmewin] ***UNCHECKED*** Aug 28 - Patch (NVMe 1.0c Compliance and others) --- Intel Feedback
Thanks a lot, Kwok, for addressing the issue in the specification. For the other changes in 1.0c are new features, such as ECN 23/29. Some size of fields got changed, I kept the same naming to avoid problems. I think we are fine.
Thanks,
Alex
________________________________
From: Kong, Kwok
Sent: Thursday, September 06, 2012 6:28 PM
To: Chang, Alex; Robles, Raymond C; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: [nvmewin] ***UNCHECKED*** Aug 28 - Patch (NVMe 1.0c Compliance and others) --- Intel Feedback
Alex,
Please see my embedded comment ...
From: nvmewin-bounces at lists.openfabrics.org<mailto:nvmewin-bounces at lists.openfabrics.org> [mailto:nvmewin-bounces at lists.openfabrics.org]<mailto:[mailto:nvmewin-bounces at lists.openfabrics.org]> On Behalf Of Chang, Alex
Sent: Thursday, September 06, 2012 6:07 PM
To: Robles, Raymond C; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: Re: [nvmewin] ***UNCHECKED*** Aug 28 - Patch (NVMe 1.0c Compliance and others) --- Intel Feedback
Hi Raymond,
Please see my comments in red...
Thanks,
Alex
________________________________
From: Robles, Raymond C [mailto:raymond.c.robles at intel.com]
Sent: Thursday, September 06, 2012 5:25 PM
To: nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>; Chang, Alex
Subject: ***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?

To match the size of GUID defined in NVMe spec, which is 16 bytes in length. If I understand it right that ULONGLONG is only 8-byte long.

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?

No, I don't think it's backward compatible with 1.0b. The only thing I can think of as compatibility issue is the 0's based NUMD of Firmware Image download and Get Log Page command. In 1.0b, the spec did not indicate it clearly. Now, 1.0c clarifies it. I don't mind to add an ifdef to differenciate them.
<Kwok> I think you meant it is backward compatible with 1.0b. The 0's based NUMD was not clearly indicated in 1.0b. We may have misinterpreted it but it was a bug in the driver if we mis-interpreted it.   It was a bug fix then and not a compatibility problem with 1.0b.

-          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)?

The code is checking the RDY bit to find out if the controller had already been reset. If so, there is no point to write 0 to EN bit of CC register again.

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.

Per NVMe specification, when RDY bit becomes 0 due to a reset, it indicates the created queues have been deleted.

o    NvmeCheckPendingCpl:

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

Will change it.

*  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?

I think they are for different purposes. NVMeDetectPendingCmds is called to ensure there is no pending IO before enterring power saving modes. NVMeCheckPendingCpl is called to see if we have any pending completed entries in any one of the created completion queues to determine if we do own the INTx interrupt. In other words, pending IOs don't mean they had just been completed when INTx interrupt happens.

-          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.

Will do it.
Thanks,
Ray
[Description: cid:image001.png at 01CB3870.4BB88E70]
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/20120911/10e3f4d6/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image001.jpg
Type: image/jpeg
Size: 1476 bytes
Desc: image001.jpg
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20120911/10e3f4d6/attachment.jpg>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sources_10c_new.zip
Type: application/x-zip-compressed
Size: 165635 bytes
Desc: sources_10c_new.zip
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20120911/10e3f4d6/attachment.bin>