[nvmewin] New Patch - Mode Select

Tue Feb 12 16:56:49 PST 2013

Hi Ray,

The way I discovered the bug was via the Policy Tab of the Property sheet of a given volume. The tab allows you to enable/disable write caching of the device, which goes to Mode Select translation in nvme driver. The data types of pModeHeader and pModeHeader10 are PMODE_PARAMETER_HEADER and PMODE_PARAMETER_HEADER10, respectively. When the pointers are increase by 1, they advance in the size of their associated data structure. In other words, they will point to the beginning of Mode Parameter Block(s).

Thanks,
Alex

________________________________
From: Robles, Raymond C [mailto:raymond.c.robles at intel.com]
Sent: Tuesday, February 12, 2013 4:39 PM
To: Chang, Alex; nvmewin at lists.openfabrics.org
Subject: RE: New Patch - Mode Select

Alex,

For the Mode Select command translation, why did you modify the code to just add 1 to the mode header address (in RED below) to get to the mode parameter block? Doing so only adds one byte to the address of pModeHeader6/pModeHeader10. You don't want to add one byte to the pointer address, you want to add the size of the header (6 or 10) to get to the beginning of the mode parameter block (pointer arithmetic). I think the way I had it coded originally was correct. How did you test this?

    if (isModeSelect10 == FALSE) {
        /* Mode Select 6 */
        pModeHeader6 = (PMODE_PARAMETER_HEADER)(GET_DATA_BUFFER(pSrb));
        blockDescLength = pModeHeader6->BlockDescriptorLength;

        /* Set pointer to Mode Param Desc Block - if any */
        pModeParamBlock = (PMODE_PARAMETER_BLOCK)(pModeHeader6 + 1);
    } else {
        /* Mode Select 10 */
        pModeHeader10 = (PMODE_PARAMETER_HEADER10)(GET_DATA_BUFFER(pSrb));

        blockDescLength = (UINT16) ((pModeHeader10->BlockDescriptorLength[BYTE_0] << 8) |
                                    (pModeHeader10->BlockDescriptorLength[BYTE_1]));

        /* Get LONGLBA */
        longLba = pModeHeader10->Reserved[BYTE_0] & LONG_LBA_MASK;

        /* Set pointer to Mode Param Desc Block - if any */
        pModeParamBlock = (PMODE_PARAMETER_BLOCK)(pModeHeader10 + 1);
    }

For setting the pModePagePtr, I believe the numBlockDesc is also incorrect.  You want to advance the pointer to the end of all the block descriptors. The only way to do that is to add the number of block descriptors *times* the size of the block descriptor - pModePagePtr = (PUCHAR)(pModeParamBlock + (numBlockDesc * blockDescLength));

        pModePagePtr = (PUCHAR)(pModeParamBlock + numBlockDesc);

Thanks,
Ray

From: nvmewin-bounces at lists.openfabrics.org [mailto:nvmewin-bounces at lists.openfabrics.org] On Behalf Of Knoblaugh, Rick
Sent: Friday, February 08, 2013 3:09 PM
To: Kong, Kwok; Chang, Alex; nvmewin at lists.openfabrics.org
Subject: Re: [nvmewin] New Patch - Mode Select

Hi Kwok,
                     Yes, we are good with the patch. Greatly appreciate it.

          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 Kong, Kwok
Sent: Friday, February 08, 2013 9:02 AM
To: Chang, Alex; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: Re: [nvmewin] New Patch - Mode Select

Alex,

We cannot check it in yet. We have to wait for the formal approval from intel and LSI.

Intel and LSI, please approve the patch.

Thanks

-Kwok

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, February 07, 2013 6:21 PM
To: nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: [nvmewin] ***UNCHECKED*** RE: New Patch - Mode Select

Hi all,

It's been two weeks and I did not receive more feedbacks. Based on the feedbacks from Dharani and Ray, I modified the patch and re-tested it. Please find the attached patch and review it (password: idt123). Since the changes are pretty straightforward, I'd like to ask Ray's favor to add it to the base if no more feedbacks received by the end of tomorrow. Thanks a lot, Ray.
Here is the summary of the changes:
In function SntiTranslateModeData (nvmesnti.c):
- Fixed some typos.
- Fixed miscalculation of the pointer to Mode Parameter Descriptor Block.
- Fixed DWORD11's value assignment in case of MODE_PAGE_CACHING.
In function NVMeInitCallBack (nvmeinit.c):
- Fixed a typo on Line#1796.

Thanks,
Alex

________________________________
From: Robles, Raymond C [mailto:raymond.c.robles at intel.com]
Sent: Friday, January 25, 2013 5:13 PM
To: Chang, Alex; Dharani Kotte; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: New Patch - Mode Select
Hi Alex,

Yes this is a copy and paste error.  Dharani is correct... it should read pModeHeader10->BlockDescriptorLength;

Did you want to change this in your patch since it is directly related to Mode Select?

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 Chang, Alex
Sent: Friday, January 25, 2013 4:57 PM
To: Dharani Kotte; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: Re: [nvmewin] New Patch - Mode Select

Good catch, Dharani.

Hi Ray,

Is this just a typo?

Thanks,
Alex
________________________________
From: Dharani Kotte [mailto:Dharani.Kotte at sandisk.com]
Sent: Friday, January 25, 2013 3:46 PM
To: Chang, Alex; nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: RE: New Patch - Mode Select
    Hi Alex,

In the file nvmeSnti.c and function SntiTranslateModeData()

/* Mode Select 10 */
        pModeHeader10 = (PMODE_PARAMETER_HEADER10)(GET_DATA_BUFFER(pSrb));

#pragma prefast(suppress:6011,"This pointer is not NULL")
        blockDescLength = pModeHeader6->BlockDescriptorLength;  // BUG -  pModeHeader6 is not initialized probably it should be pModeHeader10

thanks,
Dharani.

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: Friday, January 25, 2013 3:27 PM
To: nvmewin at lists.openfabrics.org<mailto:nvmewin at lists.openfabrics.org>
Subject: [WARNING - ENCRYPTED ATTACHMENT NOT VIRUS SCANNED] [nvmewin] ***UNCHECKED*** New Patch - Mode Select

Hi all,

I am adding a patch to fix a bug related to Mode Select command when users enable/disable Volatile Write Cache. The changes I made are within SntiTranslateModeData routine of nvmeSnti.c to correctly locate the page code and program Dword 11 of NVMe submission entry before issuing Set Features command. The password is idt123. Please review it and provide feedbacks in next couple weeks.

Thanks,
Alex

________________________________

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).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/nvmewin/attachments/20130213/d82bfc00/attachment.html>