[ofa-general] Re: [openib-general] [PATCH] 2.6.20 ib_cm: limit cm message timeouts

[Sean Hefty]

> +#define DRV_NAME        "ib_cm"
> +#define PFX             DRV_NAME ": "

Just define PFX.

> +
> +/*
> + * Limit CM msg timeouts to something reasonable.
> + * 8 seconds, with up to 15 retries, gives per msg timeout of 2 min.
> + */
> +#define IB_CM_MAX_TIMEOUT 21

Thinking out loud... maybe we should make this a module parameter.

> +
>  static void cm_add_one(struct ib_device *device);
>  static void cm_remove_one(struct ib_device *device);
>  
> @@ -887,13 +896,26 @@ static void cm_format_req(struct cm_req_
>  	cm_req_set_local_qpn(req_msg, cpu_to_be32(param->qp_num));
>  	cm_req_set_resp_res(req_msg, param->responder_resources);
>  	cm_req_set_init_depth(req_msg, param->initiator_depth);
> -	cm_req_set_remote_resp_timeout(req_msg,
> -				       param->remote_cm_response_timeout);
> +	if ((u8) IB_CM_MAX_TIMEOUT >= param->remote_cm_response_timeout)

This is a nit, but I find reading the reverse of the above check easier to 
understand and verify for correctness:

if (remote_cm_response_timeout > IB_CM_MAX_TIMEOUT) ...
<or>
if (remote_cm_response_timeout <= IB_CM_MAX_TIMEOUT) ...

That is, we're comparing the user's timeout to the max, not the other way around.

> +	if ((u8) IB_CM_MAX_TIMEOUT >= param->local_cm_response_timeout)

Same nit.

> @@ -2707,7 +2745,13 @@ int ib_send_cm_sidr_req(struct ib_cm_id 
>  
>  	cm_id->service_id = param->service_id;
>  	cm_id->service_mask = __constant_cpu_to_be64(~0ULL);
> -	cm_id_priv->timeout_ms = param->timeout_ms;
> +	if (cm_id_priv->timeout_ms > cm_convert_to_ms(IB_CM_MAX_TIMEOUT))
> +		cm_id_priv->timeout_ms = param->timeout_ms;
> +	else {
> +		cm_id_priv->timeout_ms = cm_convert_to_ms(IB_CM_MAX_TIMEOUT);
> +		printk(KERN_WARNING PFX "Given timeout to ib_send_cm_sidr_req "
> +		       "is too long, setting it to default value");
> +	}

This is keeping the larger of the two values.  I would just remove the line 
after the if() and the else statement.

Thanks for updating the patch.

- Sean