[nvmewin] Samsung patch for Hot plug fixes

[Parag Sheth]

Hi Alex,

These changes look good. we approve the patch.

Thanks
Parag Sheth

On Mon, Nov 17, 2014 at 8:21 AM, Alex Chang <Alex.Chang at pmcs.com> wrote:

>   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>
> wrote:
>
> Hi Parag,
>
>
>
> Thanks for your feedback. Please find my comments inline.
>
>
>
> Thanks,
>
> Suman
>
>
>
> ------- *Original Message* -------
>
> *Sender* : Parag Sheth<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
>
>
>
>    [image: Image removed by sender.]
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20141117/10b134ff/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image001.gif
Type: image/gif
Size: 13168 bytes
Desc: not available
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20141117/10b134ff/attachment.gif>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ~WRD359.jpg
Type: image/jpeg
Size: 823 bytes
Desc: not available
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20141117/10b134ff/attachment.jpg>