[nvmewin] Samsung Patch for Bus Reset Enhancements

Alex Chang Alex.Chang at pmcs.com
Fri Oct 24 08:56:58 PDT 2014 

Sounds good to me. 
Thank you very much, Judy, for addressing the concerns.

Alex

-----Original Message-----
From: Judy Brock-SSI [mailto:judy.brock at ssi.samsung.com] 
Sent: Friday, October 24, 2014 2:19 AM
To: Alex Chang; Foster, Carolyn D; suman.p at samsung.com; nvmewin at lists.openfabrics.org
Cc: cgps at samsung.com
Subject: RE: Samsung Patch for Bus Reset Enhancements

Alex,

I need to wait for Suman to come back to make sure we're in agreement about this but it looks to me like we don't need both. We could possibly replace all occurrences of both with a single var called polledResetInProg (cuz not all reset ops/reinit ops are polled). 

And eliminate the duplicate definition in nvmestd.h and the duplicate initialization in nvmestd.c in NVMeReInitializeController().

Thanks,
Judy

-----Original Message-----
From: nvmewin-bounces at lists.openfabrics.org [mailto:nvmewin-bounces at lists.openfabrics.org] On Behalf Of Alex Chang
Sent: Thursday, October 23, 2014 11:06 AM
To: Foster, Carolyn D; suman.p at samsung.com; nvmewin at lists.openfabrics.org
Cc: cgps at samsung.com
Subject: Re: [nvmewin] Samsung Patch for Bus Reset Enhancements

Hi Suman,

It seems to me that both polledMode and hwResetInProg are used for exactly same purpose. Can you also address the reason why you added polledMode?

Thank you!
Alex

-----Original Message-----
From: Foster, Carolyn D [mailto:carolyn.d.foster at intel.com] 
Sent: Wednesday, October 22, 2014 4:29 PM
To: Alex Chang; suman.p at samsung.com; nvmewin at lists.openfabrics.org
Cc: cgps at samsung.com
Subject: RE: Samsung Patch for Bus Reset Enhancements

Hi Suman,
I have some feedback in addition to Alex's comments.  I believe there is an issue with the loop that was added to NVMeRunningStartAttempt.  The issue is that IoCompletionDpcRoutine was never meant to be called directly.  It was architected and designed to always run from a DPC.  It's possible that a command from the init state machine could generate an interrupt and run the IoCompletionDpcRoutine before it can be called in RunningStartAttempt.  A better solution would be to have a loop similar to the one at the end of NVMePassiveInitialize where RunningStartAttempt is called, and is followed by a loop that waits for the state machine to complete.  As the patch is currently written I am not comfortable approving it.

This change to wait for the state machine's completion could be made in the new ReinitializeController function, and then you wouldn't need the changes to RunningStartAttempt or any of the polledmode code. 

Thanks,
Carolyn

-----Original Message-----
From: nvmewin-bounces at lists.openfabrics.org [mailto:nvmewin-bounces at lists.openfabrics.org] On Behalf Of Alex Chang
Sent: Tuesday, October 21, 2014 1:20 PM
To: suman.p at samsung.com; nvmewin at lists.openfabrics.org
Cc: cgps at samsung.com
Subject: Re: [nvmewin] Samsung Patch for Bus Reset Enhancements

Hi Suman,

(1) There is a call of StorPortResume(pAE) in Line2434 of nvmestd.c, which is redundant because, when NextDriverState is NVMeStartComplete, in the end of NVMeRunning, StorPortResume had been called already.

(2) To comply with our agreed coding style and make the logic easier, may I suggest changing Line#184 of nvmestat.c to:

if (pAE->ntldrDump == FALSE) {
    if (pAE->polledMode == FALSE) {
        NVMeRunning(pAE);
    } else {
        /* 
         * we poll if we're launching the reinit state machine from HwStorResetBus
         * or HwStorAdapterControl->ScsiRestartAdapter path
         */
        NVMeRunning(pAE);
        /* TO val is based on CAP register plus a few, 5, seconds to init post RDY */
        passiveTimeout = pAE->uSecCrtlTimeout + (STORPORT_TIMER_CB_us * MICRO_TO_SEC);
        ...
        return (pAE->DriverState.NextDriverState == NVMeStartComplete) ? TRUE : FALSE;
   }
} else {
    PRES_MAPPING_TBL pRMT = &pAE->ResMapTbl;
    .....
}

Thank you!
Alex

From: SUMAN PRAKASH B [mailto:suman.p at samsung.com]
Sent: Wednesday, October 15, 2014 6:00 AM
To: nvmewin at lists.openfabrics.org
Cc: Alex Chang; cgps at samsung.com
Subject: Samsung Patch for Bus Reset Enhancements

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 Everyone,
 
We have a patch for the Bus Reset Enhancements.
Please find attached the source code. The password is samsung123
 
Please find the change description below - 
 
1. There are multiple paths in the driver that reset the controller and execute the initialization state machine.  Our patch is not concerned with the majority of those paths. Aside from a few additional isolated modifications, our patch focuses on the two paths that are supposed to be synchronous -i.e. they should not return to caller until all work is completed -  but which currently are not so. They are:
a) NVMeResetBus (and)
b) NVMeAdapterControl-> ScsiRestartAdapter We have introduced a new routine NVMeReInitializeController(), which will be invoked from NVMeReseBus() and NVMeAdapterControl() - ScsiRestartAdapter. This routine will reset and initialize the controller and then complete the requests. It will not return until the initialization state machine is complete.
We disallow processing of any SRB in NVMeStartIo() when NextDriverState != NVMeStateComplete. In this way we direct the PowerUp operations to be executed in NVMeAdapterControl() - ScsiRestartAdapter only. When resuming from hibernation for example, NVMeStartio() will not process the POWER SRB. Instead, the Power Up operations will be invoked in NVMeAdapterControl()->ScsiRestartAdapter.
 
Additionally , Miniport drivers should disregard requests to reset the bus when ntldrDump is set to TRUE in NvmeResetBus(). But current implementation processes this request.

2. When pAE->ntldrDump is TRUE, in the NVMeMapCore2Queue() routine, the pPGT value is NULL. Hence a BSOD occurs when executing ULONG coreNum = (ULONG)(pPN->Number + pPGT->BaseProcessor). We fixed the problem by moving access to pPGT when ntldrDump is FALSE.
 
3. In ProcessIo(), when IoStatus is set to NOT_SUBMITTED, the SRB is not completed. Due to this, a BSOD was occuring when executing WHCK test "DP WLK - Hot-Add - Device test". We  fixed the problem by changing the code to complete SRB when IoStatus is NOT_SUBMITTED.
 
4. We changed the use of StorPortBusy()/StorPortReady() to StorPortPause()/StorPortResume(), since StorPortBusy() will not prevent new IOS from coming in once the current ones in the driver have been completed.
 
 
Tested the following on Win7 and Windows 2012R2.
- WHCK
- Install/Uninstall, Enable/Disable, FS Format
- Hibernation/Resume, Sleep/Resume
- IOmeter
 
Thanks,
Suman
 


_______________________________________________
nvmewin mailing list
nvmewin at lists.openfabrics.org
http://lists.openfabrics.org/mailman/listinfo/nvmewin
_______________________________________________
nvmewin mailing list
nvmewin at lists.openfabrics.org
http://lists.openfabrics.org/mailman/listinfo/nvmewin

More information about the nvmewin mailing list