[openib-general] Re: [PATCH] [CM] 3/6 SDP updates to bind cm_id's to a device

[Michael S. Tsirkin]

Quoting Sean Hefty <mshefty at ichips.intel.com>:
> The state of the cm_id is controlled by the CM and can change at any time 
> as a result of processing a received MAD.

I see. Lets hide this field then.
At least, this warrants a comment in the header file.

> It's only exposed for debug purposes.

I'd say you cant usefully debug with something that changes at any time, anyway.
Let's just have a compile time flag for dumping cm traffic to syslog.
Makes sense?


[...]

> >I'd prefer separate labels for two errors, instead of testing IS_ERR twice.
> 
> So do I, but I was trying to match the coding style used throughout the SDP 
> code.  Fixing the error handling seems like another set of changes to me.

Lets do it correctly in this function, no need to add more cleanup work.

> >Regarding the question: the first thing we do in sdp_conn_put
> >is sdp_conn_table_remove, so I think this lookup can fail.
> 
> I should have removed that comment.  I'm not overly familiar with the SDP 
> code, but it seems wrong to need to verify that a requested callback can be 
> executed. By returning -EINVAL above, the cm_id associated with the 
>  callback will be destroyed.  This indicates to me that either that cm_id 
> will be destroyed twice by SDP (once when the SDP conn object is destroyed 
> and again here), or that SDP does not usually destroy cm_id's as part of 
> its normal cleanup.

Hmm. Got to think about it.

> I'm also wondering about possible race conditions that 
> could occur between calling table_lookup and conn_lock, but I don't know 
> the code well enough to say if one exists.

No, conn_lock does not handle connection reference counting,
it only protects against two threads running on a connection.

-- 
MST