[nvmewin] Samsung patch for Hot plug fixes

[Alex Chang]

Dear all,

Please review/test the revised patch and provide your feedback. I will start to collect approvals on Thursday.

Thanks,
Alex

From: SUMAN PRAKASH B [mailto:suman.p at samsung.com]
Sent: Monday, November 17, 2014 1:17 AM
To: nvmewin at lists.openfabrics.org; Alex Chang; parag.sheth at seagate.com; judy.brock at ssi.samsung.com
Subject: RE: Re: [nvmewin] Samsung patch for Hot plug fixes


Content-Type: text/plain; charset=UTF-8

Content-Transfer-Encoding: 8bit

Date: %%SENT_DATE%%

Subject: Suspect Message Quarantined







WARNING: The virus scanner was unable to scan an attachment in an email message sent to you.  This attachment could possibly contain viruses or other malicious programs.  The attachment could not be scanned for the following reasons:



%%DESC%%



The full message and the attachment have been stored in the quarantine.



The identifier for this message is '%%QID%%'.



Access the quarantine at:

https://puremessage.pmc-sierra.bc.ca:28443/



For more information on PMC's Anti-Spam system:

http://pmc-intranet/wiki/index.php/Outlook:Anti-Spam_FAQ



IT Services

PureMessage Admin



Hi Alex,



Please find attached the revised code with the following review comments incorporated. Password is samsung123.



1. In function NVMeWaitForCtrlRDY(), you return TRUE when device is removed. And as per your explanation, this is to avoid delay in removing device from device manager. But returning TRUE is not intuitive. This function actually failed and hence we should be returning FALSE. And if this breaks your flow than the max delay would be 500 milliseconds. I would say that is negligible from user display point of view.

a. Moved the pAE->ShutdownInProgress inside the else part in NVMeAdapterControlPowerDown().
b. Returned FALSE from NVMeWaitForCtrlRDY() when device is removed.



2. Function brief needs to be updated for NVMeDetectPendingCmds() for the 3rd parameter.
Function brief updated for NVMeDetectPendingCmds() for the 3rd parameter.



Thanks,

Suman




------- Original Message -------

Sender : Alex Chang<Alex.Chang at pmcs.com>

Date : Nov 15, 2014 03:36 (GMT+05:00)

Title : RE: Re: [nvmewin] Samsung patch for Hot plug fixes

Hi Suman,



Could you please revise the codes, test and send it out at your earliest convenience?



Thank you!

Alex



From: Parag Sheth [mailto:parag.sheth at seagate.com]
Sent: Friday, November 14, 2014 1:56 PM
To: suman.p at samsung.com
Cc: Alex Chang; nvmewin at lists.openfabrics.org; judy.brock at ssi.samsung.com
Subject: Re: Re: [nvmewin] Samsung patch for Hot plug fixes



Hi Suman,



As long as this change passes all your test cases - I am ok with that.



Thanks

Parag Sheth



On Fri, Nov 14, 2014 at 12:40 AM, SUMAN PRAKASH B <suman.p at samsung.com<mailto:suman.p at samsung.com>> wrote:

Hi Parag,



Thanks for your feedback. Please find my comments inline.



Thanks,

Suman



------- Original Message -------

Sender : Parag Sheth<parag.sheth at seagate.com<mailto:parag.sheth at seagate.com>>

Date : Nov 14, 2014 06:31 (GMT+05:00)

Title : Re: [nvmewin] Samsung patch for Hot plug fixes



Hi Suman,



Here are my observations



1. In function NVMeWaitForCtrlRDY(), you return TRUE when device is removed. And as per your explanation, this is to avoid delay in removing device from device manager. But returning TRUE is not intuitive. This function actually failed and hence we should be returning FALSE. And if this breaks your flow than the max delay would be 500 milliseconds. I would say that is negligible from user display point of view.

[Suman] Agreed. We initially returned FALSE from NVMeWaitForCtrlRDY() when device is removed. We changed it to TRUE for the following reason.

When device is removed, the NVMeBuildIo() -> SRB_FUNCTION_PNP -> NVMeAdapterControlPowerDown() -> NVMeResetAdapter() -> NVMeWaitForCtrlRDY() was returning FALSE because of which the pAE->ShutdownInProgess was not set to TRUE in NVMeAdaptercontrolPowerDown.
After this when NVMeAdapterControl() -> ScsiStopAdapter -> NVMeAdapterControlPowerDown() is invoked, since the pAE->ShutdownInProgess was not set to TRUE in NVMeBuildIo(), the NVMeDetectPendingCmds() and NVMeResetAdapter() is executed again. This should not be executed second time.
To avoid this we retured TRUE from NVMeWaitForCtrlRDY() so that pAE->ShutdownInProgress is set to TRUE.



To resolve this, in NVMeAdapterControlPowerDown(), we can move setting the pAE->ShutdownInProgress inside the else part and return FALSE when device is removed from NVMeWaitForCtrlRDY() as follows.

NVMeAdapterControlPowerDown()
{
...
    if (pAE->ShutdownInProgress == TRUE) {
        /* Shutdown */
        status = TRUE;
    } else {
        pAE->ShutdownInProgress = TRUE;

        /* Hibernate or Sleep - sanity check that there is no cmd pending */
        if (NVMeDetectPendingCmds(pAE, FALSE, SRB_STATUS_BUS_RESET) == TRUE)
            return status;

        /* Stop the controller, but do not free the resources */
        if (NVMeResetAdapter(pAE) != TRUE) {
            return (FALSE);
        }
    }
...
}

Kindly let us know your opinion.



2. Function brief needs to be updated for NVMeDetectPendingCmds() for the 3rd parameter.

[Suman] Yes. We will change this.



Other than these 2, your changes look good.



Thanks

Parag Sheth



[cid:image001.gif at 01D0023F.70A856A0]

[Image removed by sender.]
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20141117/645737c8/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ~WRD359.jpg
Type: image/jpeg
Size: 823 bytes
Desc: ~WRD359.jpg
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20141117/645737c8/attachment.jpg>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image001.gif
Type: image/gif
Size: 13168 bytes
Desc: image001.gif
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20141117/645737c8/attachment.gif>