[ofw] [IPoIB] Fix for improper handling of IPoIB params

Alex Estrin alex.estrin at qlogic.com
Thu Jun 26 05:45:37 PDT 2008


Would you please explain what improper handling this patch is intended
to fix?
 
Thanks,
Alex
________________________________

From: ofw-bounces at lists.openfabrics.org
[mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Alex Naslednikov
Sent: Thursday, June 26, 2008 8:00 AM
To: ofw at lists.openfabrics.org; Tzachi Dar
Subject: [ofw] [IPoIB] Fix for improper handling of IPoIB params



	Reposting this message again (there was a porblem with my first
post)

________________________________

	This patch fixes improper handling of IPoIB parameters received
from registers
	Signed-off by: xalex (xalex at mellanox.co.il
<mailto:xalex at mellanox.co.il> )
	 
	Index: ulp/ipoib/kernel/SOURCES
	
===================================================================
	--- ulp/ipoib/kernel/SOURCES (revision 2629)
	+++ ulp/ipoib/kernel/SOURCES (revision 2630)
	@@ -25,14 +25,16 @@
	 
	 TARGETLIBS= \
	  $(TARGETPATH)\*\complib.lib \
	- $(DDK_LIB_PATH)\ndis.lib
	+ $(DDK_LIB_PATH)\ndis.lib \
	+ $(DDK_LIB_PATH)\ntstrsafe.lib \
	+ $(DDK_LIB_PATH)\strsafe.lib
	 
	 !if !defined(DDK_TARGET_OS) || "$(DDK_TARGET_OS)"=="Win2K"
	 #
	 # The driver is built in the Win2K build environment
	 # - use the library version of safe strings 
	 #
	-TARGETLIBS= $(TARGETLIBS) $(DDK_LIB_PATH)\ntstrsafe.lib
	+#TARGETLIBS= $(TARGETLIBS) $(DDK_LIB_PATH)\ntstrsafe.lib
	 !endif
	 
	 !IFDEF ENABLE_EVENT_TRACING
	Index: ulp/ipoib/kernel/ipoib_driver.c
	
===================================================================
	--- ulp/ipoib/kernel/ipoib_driver.c (revision 2629)
	+++ ulp/ipoib/kernel/ipoib_driver.c (revision 2630)
	@@ -30,7 +30,7 @@
	  * $Id$
	  */
	 
	-
	+#include "limits.h"
	 #include "ipoib_driver.h"
	 #include "ipoib_debug.h"
	 
	@@ -47,8 +47,12 @@
	 #include <complib/cl_init.h>
	 #include <initguid.h>
	 #include <iba/ipoib_ifc.h>
	+#include "ntstrsafe.h"
	+#include "strsafe.h"
	 
	 
	+
	+
	 #if defined(NDIS50_MINIPORT)
	 #define MAJOR_NDIS_VERSION 5
	 #define MINOR_NDIS_VERSION 0
	@@ -59,6 +63,8 @@
	 #error NDIS Version not defined, try defining NDIS50_MINIPORT
or NDIS51_MINIPORT
	 #endif
	 
	+PDRIVER_OBJECT    g_p_drv_obj;
	+
	 static const NDIS_OID SUPPORTED_OIDS[] =
	 {
	  OID_GEN_SUPPORTED_LIST,
	@@ -125,7 +131,72 @@
	 uint32_t  g_ipoib_dbg_flags = 0x00000fff;
	 ipoib_globals_t g_ipoib = {0};
	 
	+typedef struct _IPOIB_REG_ENTRY
	+{
	+ NDIS_STRING RegName;                // variable name text
	+ BOOLEAN     bRequired;              // 1 -> required, 0 ->
optional
	+ UINT        FieldOffset;            // offset in parent struct
	+ UINT        FieldSize;              // size (in bytes) of the
field
	+ UINT        Default;                // default value to use
	+ UINT        Min;                    // minimum value allowed
	+ UINT        Max;                    // maximum value allowed
	+} IPOIB_REG_ENTRY, *PIPOIB_REG_ENTRY;
	 
	+IPOIB_REG_ENTRY HCARegTable[] = {
	+ // reg value name             If Required  Offset in parentr
struct             Field size                  Default         Min
Max
	+ {NDIS_STRING_CONST("RqDepth"),          1,
IPOIB_OFFSET(rq_depth),              IPOIB_SIZE(rq_depth),
512,        128,    1024},
	+ {NDIS_STRING_CONST("RqLowWatermark"),   0,
IPOIB_OFFSET(rq_low_watermark),      IPOIB_SIZE(rq_low_watermark),   4,
2,      8},
	+ {NDIS_STRING_CONST("SqDepth"),          1,
IPOIB_OFFSET(sq_depth),              IPOIB_SIZE(sq_depth),
512,        128,    1024},
	+ {NDIS_STRING_CONST("SendChksum"),       1,
IPOIB_OFFSET(send_chksum_offload),   IPOIB_SIZE(send_chksum_offload),0,
0,      1},
	+ {NDIS_STRING_CONST("RecvChksum"),       1,
IPOIB_OFFSET(recv_chksum_offload),   IPOIB_SIZE(recv_chksum_offload),0,
0,      1},
	+ {NDIS_STRING_CONST("SaTimeout"),        1,
IPOIB_OFFSET(sa_timeout),            IPOIB_SIZE(sa_timeout),
1000,       250,    UINT_MAX},
	+ {NDIS_STRING_CONST("SaRetries"),        1,
IPOIB_OFFSET(sa_retry_cnt),          IPOIB_SIZE(sa_retry_cnt),       10,
1,      UINT_MAX},
	+ {NDIS_STRING_CONST("RecvRatio"),        1,
IPOIB_OFFSET(recv_pool_ratio),       IPOIB_SIZE(recv_pool_ratio),    1,
1,      10},
	+ {NDIS_STRING_CONST("PayloadMtu"),       1,
IPOIB_OFFSET(payload_mtu),           IPOIB_SIZE(payload_mtu),
2044 ,          60 ,   2044}
	+};  
	+
	+#define IPOIB_NUM_REG_PARAMS (sizeof (HCARegTable) /
sizeof(IPOIB_REG_ENTRY))
	+
	+
	+static void
	+ipoib_create_log(
	+ NDIS_HANDLE h_adapter,
	+ UINT ind,
	+ ULONG eventLogMsgId)
	+
	+{
	+#define cMaxStrLen  40
	+#define cArrLen  3
	+
	+ PWCHAR logMsgArray[cArrLen]; 
	+ WCHAR strVal[cMaxStrLen];
	+ NDIS_STRING AdapterInstanceName;
	+
	+ IPOIB_INIT_NDIS_STRING(&AdapterInstanceName);
	+ if (NdisMQueryAdapterInstanceName(&AdapterInstanceName,
h_adapter)!= NDIS_STATUS_SUCCESS ){
	+  ASSERT(FALSE);
	+  IPOIB_PRINT_EXIT( TRACE_LEVEL_ERROR,IPOIB_DBG_ERROR,
("[IPoIB] Init:Failed to retreive adapter name.\n"));
	+  return;
	+ }
	+ logMsgArray[0] = AdapterInstanceName.Buffer;
	+ 
	+ if (RtlStringCbPrintfW(strVal, sizeof(strVal), L"0x%x",
HCARegTable[ind].Default) != STATUS_SUCCESS) {
	+  ASSERT(FALSE);
	+  IPOIB_PRINT_EXIT( TRACE_LEVEL_ERROR,IPOIB_DBG_ERROR,
	+  ("[IPoIB] Init: Problem copying string value: exiting\n"));
	+  return;
	+ }
	+ 
	+ logMsgArray[0] = AdapterInstanceName.Buffer;
	+ logMsgArray[1] = HCARegTable[ind].RegName.Buffer;
	+ logMsgArray[2] = strVal;
	+ 
	+ NdisWriteEventLogEntry(g_p_drv_obj, eventLogMsgId, 0, cArrLen,
&logMsgArray, 0, NULL);
	+
	+}
	+
	+
	+
	 NTSTATUS
	 DriverEntry(
	  IN    PDRIVER_OBJECT    p_drv_obj,
	@@ -248,6 +319,7 @@
	  NDIS_MINIPORT_CHARACTERISTICS characteristics;
	 
	  IPOIB_ENTER( IPOIB_DBG_INIT );
	+ g_p_drv_obj = p_drv_obj;
	 
	 #ifdef _DEBUG_
	  PAGED_CODE();
	@@ -397,6 +469,7 @@
	 }
	 
	 
	+
	 NDIS_STATUS
	 ipoib_get_adapter_params(
	  IN    NDIS_HANDLE* const   wrapper_config_context,
	@@ -405,9 +478,13 @@
	  NDIS_STATUS      status;
	  NDIS_HANDLE      h_config;
	  NDIS_CONFIGURATION_PARAMETER *p_param;
	- NDIS_STRING      keyword;
	  PUCHAR       mac;
	  UINT       len;
	+ UINT                            value;
	+ PIPOIB_REG_ENTRY                pRegEntry;
	+ UINT                            i;
	+ PUCHAR                          structPointer;
	+ int sq_depth_step = 128;
	 
	  IPOIB_ENTER( IPOIB_DBG_INIT );
	 
	@@ -419,124 +496,96 @@
	   return status;
	  }
	 
	- /* Required: Receive queue depth. */
	- RtlInitUnicodeString( &keyword, L"RqDepth" );
	- NdisReadConfiguration(
	-  &status, &p_param, h_config, &keyword, NdisParameterInteger
);
	- if( status != NDIS_STATUS_SUCCESS )
	+ // read all the registry values 
	+ for (i = 0, pRegEntry = HCARegTable; i < IPOIB_NUM_REG_PARAMS;
++i)
	  {
	-  IPOIB_PRINT_EXIT( TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
	-   ("Receive Queue Depth parameter missing.\n") );
	-  return status;
	- }
	- p_adapter->params.rq_depth =
p_param->ParameterData.IntegerData;
	+  // initialize pointer to appropriate place inside 'params'
	+  structPointer = (PUCHAR) &p_adapter->params +
pRegEntry[i].FieldOffset;
	 
	- /* Optional: Receive queue low watermark. */
	- RtlInitUnicodeString( &keyword, L"RqLowWatermark" );
	- NdisReadConfiguration(
	-  &status, &p_param, h_config, &keyword, NdisParameterInteger
);
	- if( status != NDIS_STATUS_SUCCESS ||
!p_param->ParameterData.IntegerData )
	- {
	-  p_adapter->params.rq_low_watermark =
p_adapter->params.rq_depth >> 2;
	- }
	- else
	- {
	-  p_adapter->params.rq_low_watermark =
	-   p_adapter->params.rq_depth /
p_param->ParameterData.IntegerData;
	- }
	+  // Get the configuration value for a specific parameter.
Under NT the
	+  // parameters are all read in as DWORDs.
	+  NdisReadConfiguration(
	+   &status,
	+   &p_param,
	+   h_config,
	+   &pRegEntry[i].RegName,
	+   NdisParameterInteger);
	 
	- /* Required: Send queue depth. */
	- RtlInitUnicodeString( &keyword, L"SqDepth" );
	- NdisReadConfiguration(
	-  &status, &p_param, h_config, &keyword, NdisParameterInteger
);
	- if( status != NDIS_STATUS_SUCCESS )
	- {
	-  IPOIB_PRINT_EXIT( TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
	-   ("Send Queue Depth parameter missing.\n") );
	-  return status;
	- }
	- p_adapter->params.sq_depth =
p_param->ParameterData.IntegerData;
	- /* Send queue depth needs to be a power of two. */
	- if( p_adapter->params.sq_depth <= 128 )
	-  p_adapter->params.sq_depth = 128;
	- else if( p_adapter->params.sq_depth <= 256 )
	-  p_adapter->params.sq_depth = 256;
	- else if( p_adapter->params.sq_depth <= 512 )
	-  p_adapter->params.sq_depth = 512;
	- else
	-  p_adapter->params.sq_depth = 1024;
	+  // If the parameter was present, then check its value for
validity.
	+  if (status == NDIS_STATUS_SUCCESS)
	+  {
	+   // Check that param value is not too small or too large
	+   if (p_param->ParameterData.IntegerData < pRegEntry[i].Min ||
	+    p_param->ParameterData.IntegerData > pRegEntry[i].Max)
	+   {
	+    value = pRegEntry[i].Default;
	+    ipoib_create_log(p_adapter->h_adapter, i,
EVENT_IPOIB_WRONG_PARAMETER_WRN);
	+    IPOIB_PRINT(TRACE_LEVEL_VERBOSE, IPOIB_DBG_INIT, ("Read
configuration.Registry %S value is out of range, setting default value=
0x%x\n", pRegEntry[i].RegName.Buffer, value));

	 
	- /* Required: Send Checksum Offload. */
	- RtlInitUnicodeString( &keyword, L"SendChksum" );
	- NdisReadConfiguration(
	-  &status, &p_param, h_config, &keyword, NdisParameterInteger
);
	- if( status != NDIS_STATUS_SUCCESS )
	- {
	-  IPOIB_PRINT_EXIT( TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
	-   ("Send Checksum Offload parameter missing.\n") );
	-  return status;
	- }
	- p_adapter->params.send_chksum_offload =
(p_param->ParameterData.IntegerData != 0);
	+   }
	+   else
	+   {
	+    value = p_param->ParameterData.IntegerData;
	+    IPOIB_PRINT(TRACE_LEVEL_VERBOSE, IPOIB_DBG_INIT, ("Read
configuration. Registry %S, Value= 0x%x\n", pRegEntry[i].RegName.Buffer,
value));
	+   }
	+  }
	 
	- /* Required: Send Checksum Offload. */
	- RtlInitUnicodeString( &keyword, L"RecvChksum" );
	- NdisReadConfiguration(
	-  &status, &p_param, h_config, &keyword, NdisParameterInteger
);
	- if( status != NDIS_STATUS_SUCCESS )
	- {
	-  IPOIB_PRINT_EXIT( TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
	-   ("Recv Checksum Offload parameter missing.\n") );
	-  return status;
	- }
	- p_adapter->params.recv_chksum_offload =
(p_param->ParameterData.IntegerData != 0);
	+  else
	+  {
	+   value = pRegEntry[i].Default;
	+   status = NDIS_STATUS_SUCCESS;
	+   if (pRegEntry[i].bRequired)
	+   {
	+    ipoib_create_log(p_adapter->h_adapter, i,
EVENT_IPOIB_WRONG_PARAMETER_ERR);
	+    IPOIB_PRINT(TRACE_LEVEL_ERROR, IPOIB_DBG_INIT, ("Read
configuration.Registry %S value not found, setting default value=
0x%x\n", pRegEntry[i].RegName.Buffer, value));
	+   }
	+   else
	+   {
	+    ipoib_create_log(p_adapter->h_adapter, i,
EVENT_IPOIB_WRONG_PARAMETER_INFO);
	+    IPOIB_PRINT(TRACE_LEVEL_VERBOSE, IPOIB_DBG_INIT, ("Read
configuration. Registry %S value not found, Value= 0x%x\n",
pRegEntry[i].RegName.Buffer, value));
	+   }
	 
	- /* Required: SA query timeout, in milliseconds. */
	- RtlInitUnicodeString( &keyword, L"SaTimeout" );
	- NdisReadConfiguration(
	-  &status, &p_param, h_config, &keyword, NdisParameterInteger
);
	- if( status != NDIS_STATUS_SUCCESS )
	- {
	-  IPOIB_PRINT_EXIT( TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
	-   ("SA query timeout parameter missing.\n") );
	-  return status;
	- }
	- p_adapter->params.sa_timeout =
p_param->ParameterData.IntegerData;
	+  }
	+  //
	+  // Store the value in the adapter structure.
	+  //
	+  switch(pRegEntry[i].FieldSize)
	+  {
	+   case 1:
	+    *((PUCHAR) structPointer) = (UCHAR) value;
	+    break;
	 
	- /* Required: SA query retry count. */
	- RtlInitUnicodeString( &keyword, L"SaRetries" );
	- NdisReadConfiguration(
	-  &status, &p_param, h_config, &keyword, NdisParameterInteger
);
	- if( status != NDIS_STATUS_SUCCESS )
	- {
	-  IPOIB_PRINT_EXIT( TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
	-   ("SA query retry count parameter missing.\n") );
	-  return status;
	+   case 2:
	+    *((PUSHORT) structPointer) = (USHORT) value;
	+    break;
	+
	+   case 4:
	+    *((PULONG) structPointer) = (ULONG) value;
	+    break;
	+
	+   default:
	+    IPOIB_PRINT(TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR, ("Bogus
field size %d\n", pRegEntry[i].FieldSize));
	+    break;
	+  }
	  }
	- p_adapter->params.sa_retry_cnt =
p_param->ParameterData.IntegerData;
	 
	- /* Required: Receive pool to queue depth ratio. */
	- RtlInitUnicodeString( &keyword, L"RecvRatio" );
	- NdisReadConfiguration(
	-  &status, &p_param, h_config, &keyword, NdisParameterInteger
);
	- if( status != NDIS_STATUS_SUCCESS )
	- {
	-  IPOIB_PRINT_EXIT( TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
	-   ("Receive pool to queue depth ratio parameter missing.\n")
);
	-  return status;
	+ // Send queue depth needs to be a power of two
	+ //static const INT sq_depth_step = 128;
	+
	+ if (p_adapter->params.sq_depth % sq_depth_step) {
	+  static const c_sq_ind = 2;
	+  p_adapter->params.sq_depth = sq_depth_step *(
	+   p_adapter->params.sq_depth / sq_depth_step + !!(
(p_adapter->params.sq_depth % sq_depth_step) > (sq_depth_step/2) ));
	+  ipoib_create_log(p_adapter->h_adapter, c_sq_ind,
EVENT_IPOIB_WRONG_PARAMETER_WRN);
	+  IPOIB_PRINT(TRACE_LEVEL_VERBOSE, IPOIB_DBG_INIT, ("SQ DEPTH
value was rounded to the closest acceptable value of  0x%x\n",
p_adapter->params.sq_depth ));
	+
	  }
	- p_adapter->params.recv_pool_ratio =
p_param->ParameterData.IntegerData;
	 
	- /* required: MTU size. */
	- RtlInitUnicodeString( &keyword, L"PayloadMtu" );
	- NdisReadConfiguration(
	-  &status, &p_param, h_config, &keyword, NdisParameterInteger
);
	- if( status != NDIS_STATUS_SUCCESS )
	- {
	-  IPOIB_PRINT_EXIT( TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,
	-   ("PayloadMtu parameter missing. Use the default.\n") );
	-  return status;
	- }
	- p_adapter->params.payload_mtu =
p_param->ParameterData.IntegerData;
	+
	+ // Adjusting the low watermark parameter
	+ p_adapter->params.rq_low_watermark =
	+   p_adapter->params.rq_depth /
p_adapter->params.rq_low_watermark;
	+
	  p_adapter->params.xfer_block_size = (sizeof(eth_hdr_t) +
p_adapter->params.payload_mtu);
	  NdisReadNetworkAddress( &status, &mac, &len, h_config );
	 
	@@ -2344,8 +2393,14 @@
	 
	  /* Must cast here because the service name is an array of
unsigned chars but
	   * strcpy want a pointer to a signed char */
	- strcpy( (char *)ib_service.svc_rec.service_name, ATS_NAME );
	-
	+ if ( StringCchCopy( (char *)ib_service.svc_rec.service_name, 
	+  sizeof(ib_service.svc_rec.service_name) / sizeof(char),
ATS_NAME ) != S_OK) {
	+  ASSERT(FALSE);
	+  IPOIB_PRINT_EXIT( TRACE_LEVEL_ERROR,IPOIB_DBG_ERROR,
	+  ("Problem copying ATS name: exiting\n"));
	+  return;
	+ }
	+    
	  /* IP Address in question will be put in below */
	  ib_service.port_guid  = p_adapter->guids.port_guid;
	  ib_service.timeout_ms  = p_adapter->params.sa_timeout;
	Index: ulp/ipoib/kernel/ipoib_log.mc
	
===================================================================
	--- ulp/ipoib/kernel/ipoib_log.mc (revision 2629)
	+++ ulp/ipoib/kernel/ipoib_log.mc (revision 2630)
	@@ -283,3 +283,27 @@
	 Language=English
	 %2: The local port rate is too slow for the existing broadcast
MC group.
	 .
	+
	+MessageId=0x0058
	+Facility=IPoIB
	+Severity=Error
	+SymbolicName=EVENT_IPOIB_WRONG_PARAMETER_ERR
	+Language=English
	+%2: Incorrect value or non-existing registry  for the required
IPoIB parameter %3, overriding it by default value: %4
	+.
	+
	+MessageId=0x0059
	+Facility=IPoIB
	+Severity=Warning
	+SymbolicName=EVENT_IPOIB_WRONG_PARAMETER_WRN
	+Language=English
	+%2: Incorrect value or non-existing registry entry  for the
required IPoIB parameter %3, overriding it by default value: %4
	+.
	+
	+MessageId=0x005A
	+Facility=IPoIB
	+Severity=Informational
	+SymbolicName=EVENT_IPOIB_WRONG_PARAMETER_INFO
	+Language=English
	+%2: Incorrect value or non-existing registry  for the optional
IPoIB parameter %3, overriding it by default value: %4
	+.
	Index: ulp/ipoib/kernel/ipoib_driver.h
	
===================================================================
	--- ulp/ipoib/kernel/ipoib_driver.h (revision 2629)
	+++ ulp/ipoib/kernel/ipoib_driver.h (revision 2630)
	@@ -126,4 +126,13 @@
	 ipoib_resume_oids(
	  IN    ipoib_adapter_t* const  p_adapter );
	 
	+#define IPOIB_OFFSET(field)
((UINT)FIELD_OFFSET(ipoib_params_t,field))
	+#define IPOIB_SIZE(field)
sizeof(((ipoib_params_t*)0)->field)
	+#define IPOIB_INIT_NDIS_STRING(str)                        \
	+    (str)->Length = 0;                                  \
	+    (str)->MaximumLength = 0;                           \
	+    (str)->Buffer = NULL;
	+
	+
	+
	 #endif /* _IPOIB_DRIVER_H_ */
	

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20080626/4e5cae7d/attachment.html>


More information about the ofw mailing list