[nvmewin] patch review status

Luse, Paul E paul.e.luse at intel.com
Wed Aug 15 21:33:58 PDT 2012


FYI the linux guys were going this direction as well, it just hasn't made it out there yet....  Yes, I'm aware of the negative implications of waiting on RDY and don't like it.  I'll push the last agreed upon 'safest' patch then and I think we can all feel comfortable knowing that on the windows side this really is a reference implementation.  You can't take a pre-WHQL'd binary so if this doesn't work for some specific implementation then that IHV can change it.  And, if/when Msft does a native NVMe driver the HW that doesn't work with that interpretation of the spec will have challenges for sure J

Thanks again for the inputs

Paul

From: Karthik_Rajagopalan at Dell.com [mailto:Karthik_Rajagopalan at Dell.com]
Sent: Wednesday, August 15, 2012 5:49 PM
To: Luse, Paul E; Verma, Vishal L; Alex.Chang at idt.com; nvmewin at lists.openfabrics.org
Subject: RE: [nvmewin] patch review status

Correction of typo below:
I agree with your assessment of the spec for 0-->1 transition without correct admin-queue setup.


From: Rajagopalan, Karthik
Sent: Wednesday, August 15, 2012 7:45 PM
To: 'Luse, Paul E'; Verma, Vishal L; Alex.Chang at idt.com<mailto:Alex.Chang at idt.com>; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: [nvmewin] patch review status

I'd say the patch you sent is the safest, but can be optimized further to avoid the MMIO read (a non-posted transaction) to get CC.EN.

I agree with your assessment of the spec for 1-->0 transition without correct admin-queue setup.

-        But, on the other hand, any h/w that doesn't get enabled correctly on 0-->1 is broken... it cannot expect a 0-->1-->0-->1 to be enabled. The Linux driver certainly isn't doing it.

-        1-->0-->1 transition (when driver is loaded) is fair & will happen with both our patches (the Linux driver just writes 0 to CC - same as my patch)

Consider this too:

If we set CC.EN to 1, we need to wait at least 500 ms (CAP.TO) for STS.RDY - in NvmeInitialize, we're in DIRQL time & really should not linger unnecessarily (or give up earlier than what the spec says).


From: Luse, Paul E [mailto:paul.e.luse at intel.com]<mailto:[mailto:paul.e.luse at intel.com]>
Sent: Wednesday, August 15, 2012 7:11 PM
To: Rajagopalan, Karthik; Verma, Vishal L; Alex.Chang at idt.com<mailto:Alex.Chang at idt.com>; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: [nvmewin] patch review status


OK, now I understand what you are saying but I don't think the spec is clear enough either way to make a black and white decision (and I bet of the 3 IHV boards I have access to, there's at least 1 that behaves differently than the other 2 in this regard).



I have a plane to catch so will think about this overnight.



I'd welcome other comments/opinions as well and thanks everyone so far for the inpits!



Paul

From: Karthik_Rajagopalan at Dell.com<mailto:Karthik_Rajagopalan at Dell.com> [mailto:Karthik_Rajagopalan at Dell.com]<mailto:[mailto:Karthik_Rajagopalan at Dell.com]>
Sent: Wednesday, August 15, 2012 4:58 PM
To: Luse, Paul E; Verma, Vishal L; Alex.Chang at idt.com<mailto:Alex.Chang at idt.com>; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: [nvmewin] patch review status

Setting CC.EN to 0 will satisfy the 1-->0 transition.

>To your immediate email:  I don't know what you're trying to say
What I've removed in the patch is the 0-->1 transition... per the spec it can happen only after we've setup the AQA, ASQ, ACQ entries in MMIO (EnableAdapter).

-         Otherwise, the controller doesn't have to set STS.RDY - so we'll in fact be waiting forever for RDY

From: Luse, Paul E [mailto:paul.e.luse at intel.com]<mailto:[mailto:paul.e.luse at intel.com]>
Sent: Wednesday, August 15, 2012 6:47 PM
To: Rajagopalan, Karthik; Verma, Vishal L; Alex.Chang at idt.com<mailto:Alex.Chang at idt.com>; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: [nvmewin] patch review status

To your immediate email:  I don't know what you're trying to say

To your email before that:

You realize that's what's in the patch I sent out initially right?  We've come full circle here, Alex suggested that I add a wait for RDY just before setting to 0 as we don't know if its safe to set to 0 yet and then further suggested the RDY check instead of EN to avoid having to deal with what we do if RDY never comes up.  Additionally, this approach does not reset the card at startup.  If its 0 to being with, we simply move it to 1 which you would *assume* it OK but I have at least one IHV report that its not good enough, a 1-->0 transition is needed at init.

So, I still think this is the most generic and safest:

IF EN == 0 {

    /* Set EN to 1 (all others zero) */
    CC.EN = 1;
    StorPortWriteRegisterUlong(pAE,
                               (PULONG)(&pAE->pCtrlRegister->CC),
                               CC.AsUlong);

WAIT ON RDY
}

    /* Now reset */
    CC.EN = 0;
    StorPortWriteRegisterUlong(pAE,
                               (PULONG)(&pAE->pCtrlRegister->CC),
                               CC.AsUlong);


From: Karthik_Rajagopalan at Dell.com<mailto:Karthik_Rajagopalan at Dell.com> [mailto:Karthik_Rajagopalan at Dell.com]<mailto:[mailto:Karthik_Rajagopalan at Dell.com]>
Sent: Wednesday, August 15, 2012 4:43 PM
To: Luse, Paul E; Verma, Vishal L; Alex.Chang at idt.com<mailto:Alex.Chang at idt.com>; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: [nvmewin] patch review status

BTW, here's what the spec says for CC.EN
When this field is set to '1', the controller sets CSTS.RDY to '1' when it is ready to process commands.

I read it as "set CC.EN to 1 after setting up the admin-queues, otherwise RDY will *not* be 1"

From: Rajagopalan, Karthik
Sent: Wednesday, August 15, 2012 6:32 PM
To: 'Luse, Paul E'; Verma, Vishal L; Alex.Chang at idt.com<mailto:Alex.Chang at idt.com>; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: [nvmewin] patch review status

Yep :) but I think it doesn't have to be so - all we need to do is an unconditional (attempt) to reset.. we don't set CC.EN to 1 until we're ready to enable the controller.

Here are the possibilities when we try to reset:

1.      CC.EN is 0 to begin with => no need to reset; but setting it again to 0 should be a NOP

2.      CC.EN is 1 to begin with => need to reset; we didn't wiggle this yet, so STS.RDY should be 1; setting EN to 0 will actually reset

a.      The original issue you mention could still happen if STS.RDY stayed 0 (bad controller IMHO); still goodness in trying to reset

Here's a patch in case you want to try this approach...

From: Luse, Paul E [mailto:paul.e.luse at intel.com]<mailto:[mailto:paul.e.luse at intel.com]>
Sent: Wednesday, August 15, 2012 6:22 PM
To: Rajagopalan, Karthik; Verma, Vishal L; Alex.Chang at idt.com<mailto:Alex.Chang at idt.com>; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: [nvmewin] patch review status

0 doesn't reset unless it was previously 1... that's why the initial implementation set it to 1, then 0 to force reset then init happened.  The question came in about setting it to 0 right after setting it to 1, anytime you transition to 1 you need to wait for RDY so the code I proposed last will always reset but only for a 2nd wait on RDY (there's one after we enable as part of init) if the controller wasn't disabled when we started.  Hard to believe something so simple can be so convoluted isn't it?

From: Karthik_Rajagopalan at Dell.com<mailto:Karthik_Rajagopalan at Dell.com> [mailto:Karthik_Rajagopalan at Dell.com]<mailto:[mailto:Karthik_Rajagopalan at Dell.com]>
Sent: Wednesday, August 15, 2012 4:11 PM
To: Luse, Paul E; Verma, Vishal L; Alex.Chang at idt.com<mailto:Alex.Chang at idt.com>; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: [nvmewin] patch review status

Why not unconditionally set CC.EN to 0 as soon as the driver loads ?

-        There will be an issue with this iff someone before (opROM, UEFI ?) had set CC.EN to 1 & somehow STS.RDY never went to 1... but things still continued (unlikely)

i.e.,
    /* Disable / Reset */
    CC.EN = 0;
    StorPortWriteRegisterUlong(pAE,
                               (PULONG)(&pAE->pCtrlRegister->CC),
                               CC.AsUlong);

    /* Set EN to 1 (all others zero) */
    CC.EN = 1;
    StorPortWriteRegisterUlong(pAE,
                               (PULONG)(&pAE->pCtrlRegister->CC),
                               CC.AsUlong);

   /* Wait for RDY */

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 Luse, Paul E
Sent: Wednesday, August 15, 2012 5:59 PM
To: Verma, Vishal L; Chang, Alex; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: Re: [nvmewin] patch review status

So maybe a better proposal for this section of code... go back to what we had before (sorta) but include a wait on RDY after setting EN to 1 and only set EN to 1 if its zero to start.  This is the most generic soln and since the behavior of the HW at this detailed level isn't specified we can call this the least common denominator and if IHVs want to optimize for their implementations they can.  If this first RDY wait files, then we fail driver init.  This code assures we always reset and we never touch EN after setting it to 1 without waiting for RDY

IF EN == 0 {

    /* Set EN to 1 (all others zero) */
    CC.EN = 1;
    StorPortWriteRegisterUlong(pAE,
                               (PULONG)(&pAE->pCtrlRegister->CC),
                               CC.AsUlong);

WAIT ON RDY
}

    /* Now reset */
    CC.EN = 0;
    StorPortWriteRegisterUlong(pAE,
                               (PULONG)(&pAE->pCtrlRegister->CC),
                               CC.AsUlong);


From: Luse, Paul E
Sent: Wednesday, August 15, 2012 3:21 PM
To: Verma, Vishal L; Chang, Alex; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: [nvmewin] patch review status

I was thinking about that too... If EN was already 1 and we caught RDY before it was 0 then we would indeed set EN to 1 again (possibly before, during or after RDY is set).  The spec, of course, does not dictate behavior in this case but one would assume it would be treated as a NOP and our init state machine would properly wait for RDY.  I'm not a HW guy though, anyone else have a comment/concern on setting EN to 1 if tis already 1?

From: Verma, Vishal L
Sent: Wednesday, August 15, 2012 1:46 PM
To: Luse, Paul E; Chang, Alex; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: Re: [nvmewin] patch review status

Paul,

Is this correct though?
Making CC.EN 0 implies that RDY will  become 0, but RDY being 0 does not guarantee that CC.EN is 0.
If what I'm thinking is right, we could be in a state where CC.EN was 1 when the driver comes up, but the controller never became ready. In this case we will think everything is alright, and we will try to make CC.EN 1 again - Which is not something we'd want to do, right?

Vishal

From: <Luse>, Paul E <paul.e.luse at intel.com<mailto:paul.e.luse at intel.com>>
Date: Wednesday, August 15, 2012 11:43 AM
To: "Chang, Alex" <Alex.Chang at idt.com<mailto:Alex.Chang at idt.com>>, "nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>" <nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>>
Subject: Re: [nvmewin] patch review status

OK, I think I can get behind that.  I was thinking STS might not be valid when EN is 0 but looking at the spec I think this is fine

Ready (RDY): This field is set to '1' when the controller is ready to process commands after CC.EN is set to '1'. This field shall be cleared to '0' when CC.EN is cleared to '0'. Commands shall not be issued to the controller until this field is set to '1' after the CC.EN bit is set to '1'. Failure to follow this requirement produces undefined results. Host software shall wait a minimum of CAP.TO seconds for this field to be set to '1' after setting CC.EN to '1' from a previous value of '0'.

So I'll just chance the check below for EN == 1 to RDY == 1 then

Thanks!
Paul

From: Chang, Alex [mailto:Alex.Chang at idt.com]
Sent: Wednesday, August 15, 2012 10:21 AM
To: Luse, Paul E; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: patch review status

Absolutely. Now, the tricky part is when CC.EN=1 and STS.RDY never becomes 1. I'd say simply check STS.RDY only. If it's 1, then reset the device, otherwise, skip resetting it. What do you think?

Thanks,
Alex

________________________________
From: Luse, Paul E [mailto:paul.e.luse at intel.com]<mailto:[mailto:paul.e.luse at intel.com]>
Sent: Wednesday, August 15, 2012 9:56 AM
To: Chang, Alex; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: patch review status
OK, I see what you're saying I think.   We don't want to check for RDY without first looking at EN so does this make sense:


    if (CC.EN == 1) {

                -- now wait for RDY -

        /* Now reset */
        CC.EN = 0;
        StorPortWriteRegisterUlong(pAE,
                               (PULONG)(&pAE->pCtrlRegister->CC),
                               CC.AsUlong);
    }


From: Chang, Alex [mailto:Alex.Chang at idt.com]<mailto:[mailto:Alex.Chang at idt.com]>
Sent: Wednesday, August 15, 2012 9:52 AM
To: Luse, Paul E; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: patch review status

Hi Paul,

I agree with the new logic on #3. However, instead of checking CC.EN, should we check STS.RDY or both?

Thanks,
Alex

________________________________
From: Luse, Paul E [mailto:paul.e.luse at intel.com]<mailto:[mailto:paul.e.luse at intel.com]>
Sent: Wednesday, August 15, 2012 9:03 AM
To: Chang, Alex; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: patch review status
Thanks Alex, see below.

From: Chang, Alex [mailto:Alex.Chang at idt.com]<mailto:[mailto:Alex.Chang at idt.com]>
Sent: Wednesday, August 15, 2012 8:51 AM
To: Luse, Paul E; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: patch review status

Hi Paul,

Some questions after browsing the difference between current source and your changes:
1. Why you removed "CC.SHN  = 1;" in NVMeNormalShutdown routine?
PL>  Good eye, that was a mistake.  I'll add it back before pushing

2. In nvme.h, why you changed the opcode of NVM_DATASET_MANAGEMENT from 0x06 to 0x09?
PL>    6 was wrong, 9 is correct per spec

3. Is there any specific reason(s), you added the following in NVMeInitialize, and then immediately clear EN without checking RDY = 1?
    /* Set EN to 1 (all others zero) */
    CC.EN = 1;
    StorPortWriteRegisterUlong(pAE,
                               (PULONG)(&pAE->pCtrlRegister->CC),
                               CC.AsUlong);
PL>    I wrote this up in the patch submission and I can't find that email right now.  I didn't add code, I changed code.  Before, we use to set EN to 1, read it back, and then set it to 0 to kick off a reset and then later we wait for RDY in the init state machine.  We had an IHV complain that their implementation required waiting on RDY after setting EN 1 to even before clearing EN.  So, our old implementation was a problem.  So what I did now is to not force a reset unless EN is set to 1 when we come in.  If its set to 0 then either (a) we're coming from power on or (b) the pre_OS reset the card when it was done.  Either way if its already 0 then we're in a known state so we do nothing and then later we set to 1 and wait on RDY.  If its 1 when we enter here we set to 0 and then later set to 1 and wait on RDY.  Make sense?  This assures we never enable without waiting for RDY

***** WAS *******
    /* Set EN to 1 (all others zero) */
    CC.EN = 1;
    StorPortWriteRegisterUlong(pAE,
                               (PULONG)(&pAE->pCtrlRegister->CC),
                               CC.AsUlong);
    /* Read it back */
    CC.AsUlong =
        StorPortReadRegisterUlong(pAE, (PULONG)(&pAE->pCtrlRegister->CC));

    /* Now reset */
    CC.EN = 0;
    StorPortWriteRegisterUlong(pAE,
                               (PULONG)(&pAE->pCtrlRegister->CC),
                               CC.AsUlong);

    CC.AsUlong =
        StorPortReadRegisterUlong(pAE, (PULONG)(&pAE->pCtrlRegister->CC));

*** NOW IS *******
    if (CC.EN == 1) {
        /* Now reset */
        CC.EN = 0;
        StorPortWriteRegisterUlong(pAE,
                               (PULONG)(&pAE->pCtrlRegister->CC),
                               CC.AsUlong);
    }


Thanks,
Alex

________________________________
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 Luse, Paul E
Sent: Wednesday, August 15, 2012 8:12 AM
To: nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: [nvmewin] patch review status
Just checking in... any questions to date and how are folks tracking for completing their review by this Fri?

Thx
Paul

____________________________________
Paul Luse
Sr. Staff Engineer
PCG Server Software Engineering
Desk: 480.554.3688, Mobile: 480.334.4630

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


More information about the nvmewin mailing list