[nvmewin] Samsung patch for Hot plug fixes
Parag Sheth
parag.sheth at seagate.com
Fri Nov 14 13:55:53 PST 2014
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
>
> On Thu, Nov 13, 2014 at 3:43 PM, Alex Chang <Alex.Chang at pmcs.com> wrote:
>
>> Hi Carolyn and Parag/Rick,
>>
>> If you approve the patch, please let me know.
>>
>> Thanks,
>>
>> Alex
>>
>>
>>
>> *From:* SUMAN PRAKASH B [mailto:suman.p at samsung.com]
>> *Sent:* Tuesday, November 04, 2014 2:50 AM
>> *To:* nvmewin at lists.openfabrics.org; Alex Chang;
>> judy.brock at ssi.samsung.com
>> *Subject:* 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/ <https://urldefense.proofpoint.com/v2/url?u=https-3A__puremessage.pmc-2Dsierra.bc.ca-3A28443_&d=AwMG-g&c=IGDlg0lD0b-nebmJJ0Kp8A&r=QOwFo5M7MYyQeT06CcSuSQHSUdSO20xC9GZe6-T9Svk&m=qj66b9kl-ER4H8nTAJ3KIV9MLGCDkT6FDCOvn_X6ROU&s=yvICFga94mkJ5RY9NX4IRvJ0jKJAN_uF6MVwVIpOIBY&e=>
>>
>>
>>
>> For more information on PMC's Anti-Spam system:
>>
>> http://pmc-intranet/wiki/index.php/Outlook:Anti-Spam_FAQ <https://urldefense.proofpoint.com/v2/url?u=http-3A__pmc-2Dintranet_wiki_index.php_Outlook-3AAnti-2DSpam-5FFAQ&d=AwMG-g&c=IGDlg0lD0b-nebmJJ0Kp8A&r=QOwFo5M7MYyQeT06CcSuSQHSUdSO20xC9GZe6-T9Svk&m=qj66b9kl-ER4H8nTAJ3KIV9MLGCDkT6FDCOvn_X6ROU&s=gmCkRcCg1YYL8cnkf3gGRcjW9WfsvumxQIJ8X-mxEKM&e=>
>>
>>
>>
>> IT Services
>>
>> PureMessage Admin
>>
>>
>>
>> Hi Everyone,
>>
>>
>> We have a patch for the Hot plug fixes.
>>
>> Please find attached the source code. The password is samsung123
>>
>>
>>
>> *Please find the change description below - *
>>
>>
>>
>> *1) Surprise removal while IOs are in progress.*
>>
>> *To reproduce this scenario -*
>> Connect the disk and execute IOmeter on the disk volume. When IOs are in
>> progress, surprise remove the device. User expects that the device should
>> be removed from device manager immediately and iometer should increase the
>> error count field. This does not happen since we don't handle this scenario
>> in OFA driver.
>>
>> *Resolution -*
>> a. Added a new function IsdeviceRemoved(). This is a recursive function.
>> Compares the values of Version Register values with old value and incase of
>> mismatch complete the outstanding commands with SRB_STATUS_ERROR.
>> (nvmeStd.c/IsDeviceRemoved)
>> b. Start the Timer for IsDeviceRemoved() when the NextDriverState is set
>> to StartComplete.(nvmeStat.c/NVMeRunning)
>> c. Stop the timer for IsDeviceRemoved() incase of ScsiStopAdapter.
>> (nvmeStat.c/NVMeAdapterControl)
>> d. Restart the timer for IsDeviceRemoved() incase of ScsiRestartAdapter.
>> (nvmeStat.c/NVMeAdapterControl)
>> e. Stop the timer for IsDeviceRemoved() incase of SRB_FUNCTION_SHUTDOWN.
>> (nvmeStd.c/NVMeBuildIo)
>> f. If DeviceRemovedDuringIO flag is set to TRUE, complete the SRBs with
>> SRB_STATUS_ERROR for the IOs. This case is to handle the IOs received once
>> the device has been surprise removed. (nvmeStdc/NVMeBuildIo)
>> g. Modified the prototype of NVMeDetectPendingCmds function. When device
>> is surprise removed when IOs are pending, the outstanding IOs has to be
>> completed with SRB_STATUS_ERROR. (nvmeIo.c/NVMeDetectPendingCmds)
>> h. Call the NVMeDetectPendingCmds function with SRB_STATUS_BUS_RESET.
>> (nvmeInit.c/NVMeNormalShutdown, nvmePwrMgmt.c/NVMeAdapterControlPowerDown,
>> nvmeStd.c/RecoveryDpcRoutine)
>>
>>
>>
>> *2) Memory leak issues.*
>>
>> *To reproduce this scenario -*
>> a. Memory leak observed during hot removal in Resource monitor->Non-paged
>> pool. (On Server2012R2 -> Task Manager -> Performance -> Non-paged pool)
>> b. Memory leak observed during disable/enable the NVMe controller in
>> device manager.
>>
>> *Resolution -*
>> To fix memory leak, in NVMeBuildIo()->SRB_FUNCTION_PNP, when PnPAction is
>> StorRemoveDevice(disable controller) and StorSurpriseRemoval(hot remove
>> device), NVMeAdapterControlPowerDown() is invoked to stop the adapter and
>> then NVMeFreeBuffers is invoked to free the memory. At this point, since
>> the ShutdownInProgress is set in NVMeAdapterControlPowerDown(), nothing is
>> done during NVMeAdapterControl() - ScsiStopAdapter ->
>> NVMeAdapterControlPowerDown().
>>
>>
>>
>> *3) Surprise Removal during Disk Initialization*
>>
>> *To reproduce this scenario - *
>> Hot insert the device and hot remove the device immediately. At this
>> point, our driver might be executing the initialization state machine in
>> NVMePassiveInitialize. The device will not be immediately removed from the
>> device manger. The while loop will be active till passiveTimeout happens,
>> then system BSOD.
>>
>> *Resolution -*
>> a. Read the Version register. This is used to compare against the value
>> in version register after a surprise removal. (nvmeStd.c/NVMeFindAdapter)
>> b. Read the Version Register and compare with old Version Register
>> value(i.e. value read in NVMeFindAdapter). Mismatch in these values means
>> surprise removal. (nvmeStd.c/NVMePassiveInitialize)
>> c. Set the NextDriverState to NVMeStartComplete and DeviceRemovedDuringIO
>> to TRUE and return TRUE from NVMePassiveInitialize.
>> d. Driver may get commands in NVMeBuildIo, where driver returns
>> SRB_STATUS_ERROR when DeviceRemovedDuringIO is set to TRUE.
>> e. Then NVMeAdapterControl() - ScsiStopAdapter is executed.
>>
>>
>>
>> *4) Delay in removing the device from device manager after hot removal of
>> device. *When device is hot removed, the NVMeAdapterControlPowerDown()
>> -> NVMeResetAdapter() -> NVMeWaitForCtrlRDY() is invoked which sets
>>
>> the EN bit to 0 and waits for RDY bit to become 0. Since the device is
>> physically removed, the memory mapped registers will be come all 1's and
>> the RDY bit will never become 0. Hence the while loop in
>> NVMeWaitForCtrlRDY() is active for some time even after device removal and
>> hence device is not removed from device manager immediately.
>>
>> *Resolution -*
>> Check for the value of CSTS. If its 0xFFFFFFFF, then device has been
>> surprise removed and return TRUE. (nvmeStd.c/NVMeWaitForCtrlRDY)
>>
>>
>>
>> *5) Avoid redundant call of NVMeResetAdapter()*
>> a. File/Function: nvmeInit.c/NVMeEnableAdapter - Removed the
>> NVMeResetAdapter() function call from NVMeEnableAdapter() as this is
>> redundant. The NVMeResetAdapter() is being invoked in the
>> RecoveryDpcRouitne() and then again its being invoked in the
>> NVMeEnableAdapter.
>> b. In the NVMeInitialize() function the EN and RDY bit are set to 0
>> before the NVMeEnableAdapter() is being invoked. But NVMeResetAdapter()
>> does again the same functionality.
>>
>>
>>
>> *6) *When testing hot insertion with different devices, we observed some
>> devices returned NAMESPACE_NOT_READY for IO commands during learning cores
>> and disk initialization(report luns, inquiry, etc). To address this issue
>> and provide support for these devices in the driver, we have done the
>> following changes.
>> a. During learning cores, driver sends read commands on all the queues to
>> get the core to MSI-x mapping. When the read commands are interrupted, in
>> the NVMeInitCallback(), if the SC and SCT values are not 0, then the
>> learning cores is not completed. This check is not required as driver wants
>> only the core to MSI-x mapping. Since this is not a fatal error, we can
>> skip reading the SC and SCT values, as this will impact the performance.
>> (nvmeInit.c/NVMeInitCallback).
>> b. Following the above, when the initialization state machine is complete
>> and kernel starts sending SCSI commands for disk initialization, and when
>> device returns NAMESPACE_NOT_READY, this has to be translated to the
>> corresponding SCSI sense data so that the commands will be re-tried after
>> some time. (nvmeSnti.c/genericCommandStatusTable[]).
>>
>>
>> *Tested the following.*
>>
>> - WHCK on Win7 and 2012R2
>> - Install/Uninstall, Enable/Disable, FS Format
>> - Hibernation/Resume, Sleep/Resume
>> - IOmeter
>> - Hot removal which iometer is running.
>> - Hot removal immediately after hot insertion.
>> - Continous hot insert and remove operations.
>> - Check for device removal after following sequence - Hot insert, system
>> hibernation, Hot remove, system resume.
>> - Check for device presense after following sequence - System
>> hibernation, hot insert, system resume.
>> - Memory leaks during hot plug operations and disable/enable.
>>
>>
>>
>> Thanks,
>> Suman
>>
>>
>>
>> [image: Image removed by sender.]
>>
>> _______________________________________________
>> nvmewin mailing list
>> nvmewin at lists.openfabrics.org
>>
>> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.openfabrics.org_mailman_listinfo_nvmewin&d=AwICAg&c=IGDlg0lD0b-nebmJJ0Kp8A&r=QOwFo5M7MYyQeT06CcSuSQHSUdSO20xC9GZe6-T9Svk&m=rnmski2jYd1H6IRrh39Hr9NnmnJ4uxPihLMFtc2S26w&s=qG6N7pG14tIF4qKcGWFXM52doegl5fefT0BHkh4qUO0&e=
>>
>>
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20141114/db46ffe7/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 201411141410434_9220TQUP.gif
Type: image/gif
Size: 13168 bytes
Desc: not available
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20141114/db46ffe7/attachment.gif>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 201411141410423_QZUWXYH6.jpg
Type: image/jpeg
Size: 823 bytes
Desc: not available
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20141114/db46ffe7/attachment.jpg>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 201411141410413_Y5W7Z1SF.gif
Type: image/gif
Size: 13168 bytes
Desc: not available
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20141114/db46ffe7/attachment-0001.gif>
More information about the nvmewin
mailing list