[openib-general] [PATCH] ib_mad: Fix send only registrations

[Sean Hefty]

On Thu, 14 Oct 2004 14:32:57 -0400
Hal Rosenstock <halr at voltaire.com> wrote:

> What if they didn't fill in any methods in their registration request ?
> Should a recv_handler be required ? Note that the mthca appears to be a
> send only MAD client (for locally generated traps). That's what all this
> stemmed from. Maybe I took it too far.

I was thinking that clients wouldn't provide a mad_reg_req parameter if they were only going to issue sends.  Although, I can see where the documentation says that the parameter _may_ be NULL in that case, rather than _must_ be NULL.  Is there any use for mad_reg_req for clients that only issue sends?
 
> Maybe he sent something and wasn't expecting a response (so didn't
> register a recv handler) but got one anyway. Should the MAD layer crash
> because of this ?

I think that the local client can prevent this by not setting a timeout value.  But, I agree that we shouldn't crash because of bad incoming data.
 
> > We should probably change the send status as well, since no response was delivered.
> 
> OK. What status would you propose ? Do you want to generate a patch for
> this or should I ?

Your guess is as good as mine...  Maybe we can avoid this situation altogether though.  See below.
 
> > A possible fix for this is to check that mad_agent has a send_handler when the send is posted, 
> > rather than waiting until it completes.
> 
> Yes, that's a better solution.

How about this?

In ib_post_send_mad(), we could perform something like:

if (!mad_agent->send_handler ||
    (send_wc->wr.ud.timeout_ms && !mad_agent->recv_handler))
	return -EINVAL;
 
With this check and a check in ib_register_mad_agent() for:

if (mad_reg_req && !recv_handler)
	return -EINVAL

(or something similar, depending on when mad_reg_req is required.)  I think we can safely remove all other checks for valid handlers.  We may still want to keep the other send_handler check after the else to if (mad_reg_req) in ib_register_mad_agent(), but it shouldn't be needed.

- Sean