[nvmewin] New Patch From Sandisk

Chang, Alex Alex.Chang at idt.com
Thu May 16 13:48:29 PDT 2013 

Hi Sumant/Dharani,

I did not receive your new patch sent out earlier today. Could you please send me a copy?

Thanks,
Alex

________________________________
From: Robles, Raymond C [mailto:raymond.c.robles at intel.com]
Sent: Thursday, May 16, 2013 1:37 PM
To: Sumant Patro; Knoblaugh, Rick; Chang, Alex; Dharani Kotte; nvmewin at lists.openfabrics.org
Subject: RE: New Patch From Sandisk

Hi Sumant/Dharani,

Thanks for the updated patch. A couple of comments/questions:


-          For all the mode sense page translations, the new temp buffer allocated in the SRB extension is used. But for the Modes Sense 0x3C (all mode pages), the SRB data buffer is still memset to 0 at the beginning of SntiReturnAllModePages(). I’m not sure we still need to do this considering how the temp buffer is used to build the mode sense data.

-          If you insist on keeping that memset in there, there is a #define in for all mode pages - MODE_SENSE_ALL_PAGES_LENGTH. It may be better to zero out the buffer based on the min between this define and allocLength.  SRB data transfer length cannot be relied on for “data-in” commands (like mode sense).

Thanks,
Ray

From: Sumant Patro [mailto:Sumant.Patro at sandisk.com]
Sent: Thursday, May 16, 2013 12:43 PM
To: Knoblaugh, Rick; Chang, Alex; Dharani Kotte; Robles, Raymond C; nvmewin at lists.openfabrics.org
Subject: [WARNING - ENCRYPTED ATTACHMENT NOT VIRUS SCANNED] RE: ニ[WARNING - ENCRYPTED ATTACHMENT NOT VIRUS SCANNED] [nvmewin] ***UNCHECKED*** [WARNING - ENCRYPTED ATTACHMENT NOT VIRUS SCANNED] RE: New Patch From Sandisk

Sorry, sent the wrong zip in the previous mail. Please ignore the previous attachment.

Attached is the updated zip file.

Regards,

Sumant

From: nvmewin-bounces at lists.openfabrics.org<mailto:nvmewin-bounces at lists.openfabrics.org> [mailto:nvmewin-bounces at lists.openfabrics.org] On Behalf Of Sumant Patro
Sent: Thursday, May 16, 2013 12:38 PM
To: Knoblaugh, Rick; Chang, Alex; Dharani Kotte; Robles, Raymond C; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: ニ[WARNING - ENCRYPTED ATTACHMENT NOT VIRUS SCANNED] [nvmewin] ***UNCHECKED*** [WARNING - ENCRYPTED ATTACHMENT NOT VIRUS SCANNED] RE: New Patch From Sandisk

Thanks Rick, Alex for your feedback.

Please find attached updated files with following two changes :


1.       Comment in header of SntiCreateModeDataHeader function changed to reflect passing of srb extension

2.      memset(GET_DATA_BUFFER(pSrb), 0, allocLength); modified to use minimum of allocLength and DataTransferLength in SntiReturnAllModePages(..)

Password : sndk1234

Regards,

Sumant

From: nvmewin-bounces at lists.openfabrics.org<mailto:nvmewin-bounces at lists.openfabrics.org> [mailto:nvmewin-bounces at lists.openfabrics.org] On Behalf Of Knoblaugh, Rick
Sent: Thursday, May 16, 2013 11:59 AM
To: Chang, Alex; Dharani Kotte; Robles, Raymond C; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: Re: [nvmewin] New Patch From Sandisk

Hi Dharani,
                         When you change that one, may as well fix the comment in header of SntiCreateModeDataHeader function to reflect that srb extension is now the parameter that’s passed in. Other stuff looks good.

                    Thanks,

                               -Rick


From: nvmewin-bounces at lists.openfabrics.org<mailto:nvmewin-bounces at lists.openfabrics.org> [mailto:nvmewin-bounces at lists.openfabrics.org] On Behalf Of Chang, Alex
Sent: Thursday, May 16, 2013 10:58 AM
To: Dharani Kotte; Robles, Raymond C; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: Re: [nvmewin] New Patch From Sandisk

Hi Dharani,

I tested your new patch and experienced a D1 bugcheck while running SCSICompliance test. Some preliminary debugging leads to Line#3633 of nvmestni.c, where it looks like:
memset(GET_DATA_BUFFER(pSrb), 0, allocLength);

The bugcheck happens when the value of "allocLength" is 0x1000. Apparently, the allocated length in CDB is somewhat a bogus value that is meant to test how driver handles it. Could you please fix it?

Thanks,
Alex

________________________________
From: nvmewin-bounces at lists.openfabrics.org<mailto:nvmewin-bounces at lists.openfabrics.org> [mailto:nvmewin-bounces at lists.openfabrics.org] On Behalf Of Dharani Kotte
Sent: Wednesday, May 01, 2013 8:56 AM
To: Robles, Raymond C; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: [nvmewin] ***UNCHECKED*** [WARNING - ENCRYPTED ATTACHMENT NOT VIRUS SCANNED] New Patch From Sandisk
Hi Ray,

I have incorporated the changes according to your suggestions, I tried to keep up with the coding convention please feel free to let me know if anything has to be changed. I have incorporated one more fix in the nvmeInit.c which can lead to memory corruptions and used the SrbExt for the buffer allocation.

PassWd: sndk1234

Thanks,
Dharani.

From: Dharani Kotte
Sent: Tuesday, April 30, 2013 12:03 PM
To: 'Robles, Raymond C'; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: New Patch From Sandisk

Thank you for the feedback, I agree that the global buffer is not good I have made the changes to allocate this buffer from the srbext I will provide the code with the below modifications soon.

Thanks,
Dharani.

From: Robles, Raymond C [mailto:raymond.c.robles at intel.com]
Sent: Tuesday, April 30, 2013 11:52 AM
To: Dharani Kotte; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: New Patch From Sandisk

Hi Dharani,

Thank you for putting this patch together. Please see my feedback below:


-          Line 52: Note that we have a file called nvmeSntiTypes.h. This file contains all #defines. As per our existing convention, all #defines should be placed in this file (versus at the top of a .c file).

-          Line 3339/3412/3502/3593/3756/5391/5433/5442/5453: Our coding convention states no line be longer than 80 characters. These lines all exceed 80 characters.

-          Line 5410: SntiTranslateAllModePagesResponse() contains new if-else blocks. Our coding convention dictates that open curly braces be placed on the same line as the if or else statement. You can refer to other instances of the code and if-else clauses as examples.

-          Line 4017: SntiCreateModeDataHeader() no longer needs the SRB pointer to be passed in as a parameter since you are using a global temp buffer to build up the mode sense data. That parameter can be removed.

-          General: The method of using a global buffer for a single mode sense command opens up a potential race condition for multiple mode sense commands submitted on top of each other. The idea behind putting the SCSI to NVMe translation phase in the HwStorBuildIo phase was to help performance. We initially knew that we would never be sharing any data structures or memory in calls to BuildIo. There is no spinlock or protection for calls to BuildIo and MSDN makes it clear that these BuildIo calls are not synchronized and that any synchronization of memory/data must be done within the function implementations or just use StartIo instead (since Storport will acquire the StartIoSpinLock).  So there is hole here w/r/t the global buffer being populated for multiple mode sense commands at the same time. I believe you have the right idea, but the correct solution is to create a temp mode sense buffer inside the SRB extension. This way, each command has its own temp mode sense buffer for building the mode sense response, and then when ready to copy over to the SRB data buffer, it’s pulling from a buffer that can only be accessed by the current command (instead of a global buffer that may have been overwritten by another mode sense request call from another BuildIo call). Note that the driver collaboration group made the decision to never acquire any type of lock in BuildIo. Let me know if you have questions on this solution. The additional size to the SRB extension is not an issue. This fix is critical for this patch to be accepted.

Thanks,
Ray

From: nvmewin-bounces at lists.openfabrics.org<mailto:nvmewin-bounces at lists.openfabrics.org> [mailto:nvmewin-bounces at lists.openfabrics.org] On Behalf Of Dharani Kotte
Sent: Tuesday, April 30, 2013 11:14 AM
To: Chang, Alex; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: [nvmewin] ***UNCHECKED*** [WARNING - ENCRYPTED ATTACHMENT NOT VIRUS SCANNED] New Patch From Sandisk

Attaching the code with according to Alex suggestion. Passwd: sndk1234

Thanks,
Dharani.

From: Chang, Alex [mailto:Alex.Chang at idt.com]
Sent: Tuesday, April 30, 2013 9:23 AM
To: Dharani Kotte; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: New Patch From Sandisk

Hi Dharani,

I'd suggest that it's safer to initialize gModeSenseBuf as all zeros each time before it is used, just in case there are some unexpected data in the buffer. Other than that, I am fine with the patch.

Thanks,
Alex

________________________________
From: nvmewin-bounces at lists.openfabrics.org<mailto:nvmewin-bounces at lists.openfabrics.org> [mailto:nvmewin-bounces at lists.openfabrics.org] On Behalf Of Dharani Kotte
Sent: Tuesday, April 30, 2013 8:37 AM
To: nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: [nvmewin] New Patch From Sandisk
Hi all,

It's been almost two weeks after I sent out the patch. please let us know if you're okay with it.

Thanks,
Dharani.

From: Dharani Kotte
Sent: Friday, April 12, 2013 9:58 AM
To: nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: New Patch From Sandisk

Hi all,

I am attaching a new patch that includes the following changes:

1.    In nvmeSnti.c

a.       Allocate a global buffer of 256bytes size for modesense return data preparation

b.      For any mode sense this buffer is used for modesense data along with the requested pages in it.

c.       Copy the required data according to the alloc_length requested in the cdb to the Srb->DataBuffer and update the dataTransferLength accordingly

d.      In function SntiTranslateReturnAllModePagesResponse() the modesense data header offset is calculated wrong which ends up in BSOD in some cases modified code to fix this issue

Please review the changes and provide feedbacks if you have any. If nobody disagrees with the changes, I will remind Ray to merge them in two weeks.

Thanks,
Dharani.


________________________________

PLEASE NOTE: The information contained in this electronic mail message is intended only for the use of the designated recipient(s) named above. If the reader of this message is not the intended recipient, you are hereby notified that you have received this message in error and that any review, dissemination, distribution, or copying of this message is strictly prohibited. If you have received this communication in error, please notify the sender by telephone or e-mail (as shown above) immediately and destroy any and all copies of this message in your possession (whether hard copies or electronically stored copies).



CAUTION: Please confirm that the password protected
.zip attachment which contains the file(s) of type
  source_sndk_05_16_2013.zip
is legitimate prior to opening.  To make sure this
message is not infected with a virus, it is important to
verify that you are expecting the message or else
confirm its legitimacy with the sender.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20130516/103edf81/attachment.html>

More information about the nvmewin mailing list