Alex Naslednikov wrote on Tue, 9 Nov 2010 at 09:24:26
> Signed-off by: Alexander Naslednikov (xalex at mellanox.co.il)
>
> Index: ulp/ipoib_NDIS6_CM/kernel/ipoib_port.cpp
> =================================================================== ---
> ulp/ipoib_NDIS6_CM/kernel/ipoib_port.cpp (revision 2984) +++
> ulp/ipoib_NDIS6_CM/kernel/ipoib_port.cpp (working copy) @@ -1112,8
> +1112,13 @@
> qp_create.sq_depth = p_port->p_adapter->params.sq_depth;
>
> #define UD_QP_USED_SGE 3
What the heck is UD_QP_USED_SGE? How about a comment?
> - qp_create.sq_sge = MAX_SEND_SGE < p_port->p_ca_attrs->max_sges ?
> - MAX_SEND_SGE : ( p_port->p_ca_attrs->max_sges -
> UD_QP_USED_SGE );
> + if ( MAX_SEND_SGE < p_port->p_ca_attrs->max_sges )
I find this notation very hard to read: "CONSTANT <operator> variable". I
*much* prefer variable <operator> CONSTANT. The argument that it prevents
errors is weak, at best, since compilers will generate warnings for assignment
within conditionals. When looking at this code, I have to think "if
p_port->p_ca_attrs->max_sges > MAX_SEND_SGE" to be able to reason the code flow.
> + {
> + p_port->p_ca_attrs->max_sges = MAX_SEND_SGE;
> + }
> + p_port->p_ca_attrs->max_sges -= UD_QP_USED_SGE;
> + qp_create.sq_sge = p_port->p_ca_attrs->max_sges;
> +
> if ( !p_port->p_ca_attrs->ipoib_csum )
> {
> /* checksum is not supported by device
> @@ -3799,7 +3804,7 @@
> }
>
> /* Remember that one of the DS entries is reserved for the IPoIB
> header. */
> - if( num_pages >= MAX_SEND_SGE )
> + if( num_pages >= p_port->p_ca_attrs->max_sges )
> {
> IPOIB_PRINT(TRACE_LEVEL_INFORMATION, IPOIB_DBG_SEND,
> ("Too many buffers to fit in WR ds_array. Copying
> data.\n") );
> @@ -4270,13 +4275,13 @@
> Copy only N+1-MAX_SEND_SGE, where N is a lenght of SG List
> Remember that one of the DS entries is reserved for the IPoIB header.
> */
> - if( ( p_sgl->NumberOfElements >= MAX_SEND_SGE ||
> + if( ( p_sgl->NumberOfElements >= s_buf->p_port->p_ca_attrs-
> max_sges ||
> p_sgl->Elements[0].Length < sizeof(eth_hdr_t)) )
> {
> IPOIB_PRINT( TRACE_LEVEL_WARNING, IPOIB_DBG_SEND,
> ("Too many buffers %d to fit in WR ds_array[%d] \
> Or buffer[0] length %d < Eth header. Copying
> data.\n",
> - p_sgl->NumberOfElements, MAX_SEND_SGE, p_sgl-
> Elements[0].Length ) );
> + p_sgl->NumberOfElements,
> s_buf->p_port->p_ca_attrs->max_sges , p_sgl->Elements[0].Length ) );
No space before comma. The line is also too long.
>
> if( !s_buf->p_port->p_adapter->params.cm_enabled )
> {
_______________________________________________
ofw mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
