[openib-general] Re: [PATCH] CMA and iWARP

[Steve Wise]

Sean,

I'll fix these and we'll re-release an up-to-date patch.

Thanks,

Steve.



---- referencing comments From Sean ----


Tom Tucker wrote:

> +/* Handles an inbound connect request. The function creates a new
> + * iw_cm_id to represent the new connection and inherits the client
> + * callback function and other attributes from the listening parent. 
> + * 
> + * The work item contains a pointer to the listen_cm_id and the event. The
> + * listen_cm_id contains the client cm_handler, context and device. These are
> + * copied when the device is cloned. The event contains the new four tuple.

Does the code take a reference on the listen_cm_id before scheduling the work item?

> + */
> +static int cm_conn_req_handler(struct iwcm_work* work)
> +{
> +	struct iw_cm_id* cm_id;
> +	struct iwcm_id_private* cm_id_priv;
> +	int rc;
> +
> +	/* If the status was not successful, ignore request */
> +	if (work->event.status) {
> +		printk(KERN_ERR "%s:%d Bad status=%d for connection request ... "
> +		       "should be filtered by provider\n",
> +		       __FUNCTION__, __LINE__,
> +		       work->event.status);
> +		return work->event.status;
> +	}
> +	cm_id = iw_create_cm_id(work->cm_id->id.device,	work->cm_id->id.cm_handler,
> +				work->cm_id->id.context);
> +	if (IS_ERR(cm_id)) 
> +		return PTR_ERR(cm_id);
> +
> +	cm_id_priv = container_of(cm_id, struct iwcm_id_private, id);
> +	cm_id_priv->id.local_addr = work->event.local_addr;
> +	cm_id_priv->id.remote_addr = work->event.remote_addr;
> +	cm_id_priv->id.provider_id = work->event.provider_id;
> +	cm_id_priv->id.state = IW_CM_STATE_CONN_RECV;
> +
> +	/* Call the client CM handler */
> +	rc = cm_id->cm_handler(cm_id, &work->event);
> +	if (rc) {
> +		cm_id->state = IW_CM_STATE_IDLE;
> +		iw_destroy_cm_id(cm_id);
> +	}
> +	kfree(work);
> +	return 0;
> +}

> +
> +/*
> + * Handles the transition to established state on the passive side.
> + */
> +static int cm_conn_est_handler(struct iwcm_work* work)
> +{

{snip}

> +	/* Call the client CM handler */
> +	ret = cm_id_priv->id.cm_handler(&cm_id_priv->id, &work->event);

A reference needs to be taken on the cm_id_priv before invoking the callback to 
block destruction.  (I didn't see that a reference was released...)

> +static int cm_conn_rep_handler(struct iwcm_work* work)
> +{
> +	struct iwcm_id_private* cm_id_priv;
> +	unsigned long flags;
> +	int ret = 0;

{snip}

> +
> +	/* Call the client CM handler */
> +	ret = cm_id_priv->id.cm_handler(&cm_id_priv->id, &work->event);
> +	if (ret) {
> +		cm_id_priv->id.state = IW_CM_STATE_IDLE;
> +		iw_destroy_cm_id(&cm_id_priv->id);
> +	}

Same here - a reference is needed to block destruction before invoking the callback.

> +static int cm_disconnect_handler(struct iwcm_work* work) 
> +{
> +	struct iwcm_id_private* cm_id_priv;
> +	int ret = 0;
> +	
> +	cm_id_priv = work->cm_id;
> +
> +	cm_id_priv->id.state = IW_CM_STATE_IDLE;
> +
> +	/* Call the client CM handler */
> +	ret = cm_id_priv->id.cm_handler(&cm_id_priv->id, &work->event);
> +	if (ret)
> +		iw_destroy_cm_id(&cm_id_priv->id);

And here...

> +static void cm_event_handler(struct iw_cm_id* cm_id,
> +			     struct iw_cm_event* event) 
> +{
> +	struct iwcm_work *work;
> +	struct iwcm_id_private* cm_id_priv;
> +
> +	work = kmalloc(sizeof *work, GFP_ATOMIC);
> +	if (!work)
> +		return;
> +
> +	cm_id_priv = container_of(cm_id, struct iwcm_id_private, id);
> +	INIT_WORK(&work->work, cm_work_handler, work);
> +	work->cm_id = cm_id_priv;

Reference the cm_id before queuing the work item.  It needs to be released after 
processing any callbacks.

- Sean