<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
<meta name="Generator" content="Microsoft Word 14 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
p.emailquote, li.emailquote, div.emailquote
{mso-style-name:emailquote;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:1.0pt;
border:none;
padding:0in;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
span.EmailStyle18
{mso-style-type:personal-reply;
font-family:"Times New Roman","serif";
color:#1F497D;
font-weight:normal;
font-style:normal;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
/* List Definitions */
@list l0
{mso-list-id:229657749;
mso-list-template-ids:1734361410;}
@list l0:level1
{mso-level-start-at:2;
mso-level-tab-stop:.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l1
{mso-list-id:376667440;
mso-list-template-ids:-188971678;}
@list l1:level1
{mso-level-start-at:3;
mso-level-tab-stop:.5in;
mso-level-number-position:left;
text-indent:-.25in;}
@list l2
{mso-list-id:1617327129;
mso-list-template-ids:1387063884;}
ol
{margin-bottom:0in;}
ul
{margin-bottom:0in;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:10.0pt;color:#1F497D">Hi Judy,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;color:#1F497D">Sorry for the late response on this… I’ve been out on medical leave and am currently catching up on all email. See below for my comments in
</span><b><span style="font-size:10.0pt;color:#C00000">BOLD</span></b><span style="font-size:10.0pt;color:#1F497D">.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;color:#1F497D">Thanks,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;color:#1F497D">Ray<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;color:#1F497D"><o:p> </o:p></span></p>
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> nvmewin-bounces@lists.openfabrics.org [mailto:nvmewin-bounces@lists.openfabrics.org]
<b>On Behalf Of </b>Judy Brock-SSI<br>
<b>Sent:</b> Tuesday, January 15, 2013 8:20 AM<br>
<b>To:</b> nvmewin@lists.openfabrics.org<br>
<b>Subject:</b> [nvmewin] OFA Windows NVMe driver question/ feedback<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Calibri","sans-serif"">Hi,<o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Calibri","sans-serif"">I have a question and a little feedback:<o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><o:p></o:p></span></p>
</div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:0in;text-indent:-.25in;mso-list:l2 level1 lfo1">
<![if !supportLists]><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><span style="mso-list:Ignore">1.<span style="font:7.0pt "Times New Roman"">
</span></span></span><![endif]><span style="font-size:10.0pt;font-family:"Calibri","sans-serif"">Currently,</span><span style="font-size:10.0pt;font-family:"Courier New""> SntiBuildFirmwareImageDownloadCmd</span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif"">
() always returns without generating valid PRPs due to the following code (numDwords is always zero):
<o:p></o:p></span></p>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif"">
<span style="color:blue">#define</span> DEBUG_CHECK</span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif"">
<span style="color:blue">#ifdef</span> DEBUG_CHECK</span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif"">
<span style="color:blue">if</span> ((pSgl->NumberOfElements * <span style="color:blue">
sizeof</span>(STOR_SCATTER_GATHER_ELEMENT)) >=</span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif""> (numDwords/<span style="color:blue">sizeof</span>(UINT32))) {
</span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif"">
<span style="color:green">/* In this case, must fail the command or create a temp buffer... */</span></span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif";color:green"> </span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif""> ASSERT(FALSE);</span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif"">
<span style="color:blue">return</span>;</span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif""> }</span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Calibri","sans-serif"">
<span style="color:blue">#endif</span> <span style="color:green">/* DEBUG_CHECK */</span></span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""> <o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""> It looks like the intention was for numDwords to have a valid value that would have allowed the function to continue as long as a temp buffer wasn’t needed
but it was never set…is that the correct?<span style="color:#C00000"> <b><i>[Robles, Raymond C]
</i></b>This is correct. I’m not sure when this was added (was added after the original implementation) but you are correct in your assertion. The check should be to see that DWORD 10 (which does contain the correct value for nunDwords) was used in order to
check if a temp buffer is needed. At the time of the development of this code, this was a command that was not easily testable on the QEMU VM. This is a bug that needs to be fixed and is a good candidate for contribution to the open source driver.<o:p></o:p></span></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#C00000"> </span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif";color:#C00000"><o:p></o:p></span></p>
</div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:0in;text-indent:-.25in;mso-list:l0 level1 lfo2">
<![if !supportLists]><span style="font-size:10.0pt;font-family:"Courier New""><span style="mso-list:Ignore">2.<span style="font:7.0pt "Times New Roman"">
</span></span></span><![endif]><span style="font-size:10.0pt;font-family:"Courier New"">SntiBuildFirmwareImageDownloadCmd</span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""> () also has no return value and the caller is assuming success
, resulting in a malformed NVMe command being generated (PRP1 and PRP2 set all zeros) and sent out on behalf of SCSI write buffer. I believe this function should be changed to return SNTI_TRANSLATION_STATUS and
</span><span style="font-size:10.0pt;font-family:"Courier New"">SntiTranslateWriteBuffer()</span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""> should be modified to check for success/failure rather than forging blindly ahead.<b><i><span style="color:#C00000">[Robles,
Raymond C] </span></i></b><span style="color:#C00000"> Many of the SntiBuildXxx() functions do not have return values. This was by design when I first coded these. The reason being that if the code made it to this point (where all sanity checks had passed
and we just needed to build the SQE), these functions would just build the SQE in the temp SQE unit within the scratch memory space in the SRB extension. There was never any intention to perform any types of checks within these “build” functions. They were
merely helper functions… hence no return values. But as the driver evolved and the robustness grew, some of these functions had checks added to them (just like what you are pointing out) and they really should be returning values now. So, yes, I do agree this
function should return a value. However, it should return SNTI_STATUS. My original intention was that all internal return values for helper functions should try to use SNTI_STATUS and all the first level major interface functions would return SNTI_TRANSLATION_STATUS.
Of course there are always exceptions, but this was the guideline that I tried to implement and follow. This would be a good candidate for a patch from a company using the driver.</span></span><span style="font-size:10.0pt;font-family:"Courier New""><o:p></o:p></span></p>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><o:p></o:p></span></p>
</div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:0in;text-indent:-.25in;mso-list:l1 level1 lfo3">
<![if !supportLists]><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><span style="mso-list:Ignore">3.<span style="font:7.0pt "Times New Roman"">
</span></span></span><![endif]><span style="font-size:10.0pt;font-family:"Calibri","sans-serif"">Seems like a few other routines which don’t have any
</span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">return value should also be changed to return
</span><span style="font-size:10.0pt;font-family:"Courier New"">SNTI_TRANSLATION_STATUS
</span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">value( and all functions which call these routines be modified to check for success/failure):</span><b><i><span style="font-size:10.0pt;font-family:"Calibri","sans-serif";color:#C00000">[Robles,
Raymond C] </span></i></b><span style="font-size:10.0pt;font-family:"Calibri","sans-serif";color:#C00000"> Agreed, with the exception of SntiTranslateSglToPrp(). See above response for all other functions. SntiTranslateSglToPrp() is a special case and does
not return a value by design. I believe this still holds true today. The sole purpose of this function is to translate a Windows SGL to PRPs (entries, list, or combo of both). There are no scenarios in this function that would require a negative return value.
When this function is invoked, a good SGL has been obtained and the needed PRP memory has already been allocated at init time (we wouldn’t be at this point if either of these points were not true). Therefore, the translation process will not fail with the
previous being true. A good SGL will always translate to correct PRP entries/list.</span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><o:p></o:p></span></p>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">
</span><span style="font-size:10.0pt;font-family:"Courier New";color:black">VOID SntiTranslateSglToPrp()</span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Courier New""> VOID SntiBuildFirmwareActivateCmd()</span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Courier New""> VOID SntiBuildSecuritySendReceiveCmd()</span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Thanks,</span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Judy</span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><span style="font-size:10.0pt;font-family:"Calibri","sans-serif""><o:p></o:p></span></p>
</div>
</div>
</body>
</html>