[openib-general] [PATCH 3/3] iWARP CM

[Sean Hefty]

>Well, it depends on our philosophy. If kernel users are trusted -- no.
>If not, then we can either check them here and fail gracefully, or bug-
>check the app later. Currently, _we_ are the app, so maybe we should
>dump-em.

The philosophy is that kernel users are trusted.  A bug check lets us discover
the problem earlier.

 > +		if (ret == 0) {
>> > +			struct iw_cm_event event;
>> > +			event.event = IW_CM_EVENT_LLP_DISCONNECT;
>> > +			event.provider_id = cm_id->provider_id;
>> > +			event.status = 0;
>> > +			event.local_addr = cm_id->local_addr;
>> > +			event.remote_addr = cm_id->remote_addr;
>> > +			event.private_data = 0;
>> > +			event.private_data_len = 0;
>> > +			cm_event_handler(cm_id, &event);
>>
>> See comments in iw_cm_accept().
>>
>
>> > +		}
>> > +		cm_id->state = IW_CM_STATE_IDLE;
>>
>> Why is state set to CLOSING above, then set it to IDLE?  There may be a valid
>> reason, I'm just trying to understand why.
>>
>
>While in the client callback handler and through potential downcalls, I
>wanted to have a separate state. IDLE means it can be reused. I thought
>we should not be able to do so until after we've popped back up through
>this call chain.... that was the idea anyway.

If I understood correctly, cm_event_handler invokes a callback via another
thread.  The state is being changed in a separate thread here.  We don't know
what state the cm_id will be when the user's callback is invoked.  How can a
user tell when the cm_id can be reused?

>> > +	switch (work->event.event) {
>> > +	case IW_CM_EVENT_CONNECT_REQUEST:
>> > +		rc = cm_conn_req_handler(work);
>> > +		break;
>> > +	case IW_CM_EVENT_CONNECT_REPLY:
>> > +		rc = cm_conn_rep_handler(work);
>> > +		break;
>> > +	case IW_CM_EVENT_ESTABLISHED:
>> > +		rc = cm_conn_est_handler(work);
>> > +		break;
>> > +	case IW_CM_EVENT_LLP_DISCONNECT:
>> > +	case IW_CM_EVENT_LLP_TIMEOUT:
>> > +	case IW_CM_EVENT_LLP_RESET:
>> > +	case IW_CM_EVENT_CLOSE:
>> > +		rc = cm_disconnect_handler(work);
>> > +		break;
>> > +	}
>> > +
>> > +	/* The reference was done in cm_event_handler. */
>> > +	iwcm_deref_id(cm_id_priv);
>>
>> The cm_*_handlers need to release the reference in order to process
>destruction.
>>   That or use their return code to determine if the reference should be
>> released, or if cleanup needs to be invoked.
>What I'm going to do is have the return value specify whether or not the
>cm_id should be destroyed. Also, the kfree of the work is being moved
>here.

Okay - note that cm_conn_req_handler() needs to result in destroying the newly
created cm_id, and not the one stored in work->cm_id, which should be the
listen.

- Sean