On Thu, 14 Oct 2004 13:33:08 -0400
Hal Rosenstock <[EMAIL PROTECTED]> wrote:
> /* Validate MAD registration request if supplied */
> if (mad_reg_req) {
> - if (!recv_handler ||
> - mad_reg_req->mgmt_class_version >= MAX_MGMT_VERSION) {
> + if (mad_reg_req->mgmt_class_version >= MAX_MGMT_VERSION) {
> ret = ERR_PTR(-EINVAL);
> goto error1;
> }
> + if (!bitmap_empty(mad_reg_req->method_mask,
> + IB_MGMT_MAX_METHODS)) {
> + if (!recv_handler) {
> + ret = ERR_PTR(-EINVAL);
> + goto error1;
> + }
> + } else {
> + if (!send_handler) {
> + ret = ERR_PTR(-EINVAL);
> + goto error1;
> + }
> + }
I'm not quite understanding this change. If the user has provided a mad_reg_req, they
are indicating that they want to receive unsolicited MADs. A recv_handler should be
required. Am I missing something?
> /* Validate device and port */
> @@ -842,7 +854,9 @@
> spin_unlock_irqrestore(&mad_agent_priv->lock, flags);
>
> /* Defined behavior is to complete response before request */
> - mad_agent_priv->agent.recv_handler(&mad_agent_priv->agent,
> + if (mad_agent_priv->agent.recv_handler)
> + mad_agent_priv->agent.recv_handler(
> + &mad_agent_priv->agent,
> &recv->header.recv_wc);
> atomic_dec(&mad_agent_priv->refcount);
If I understand this change, a client sent a MAD, expecting a response, got one, but
didn't register with a receive handler. As a side thought, I'm wondering how much
protection we need to build into the code to handle kernel clients that don't provide
all of the necessary parameters, but we can discuss this.
The only real problem with this change is that the receive buffer needs to be released
if it is not given to a client. We should probably change the send status as well,
since no response was delivered.
> @@ -851,7 +865,9 @@
> mad_send_wc.wr_id = mad_send_wr->wr_id;
> ib_mad_complete_send_wr(mad_send_wr, &mad_send_wc);
> } else {
> - mad_agent_priv->agent.recv_handler(&mad_agent_priv->agent,
> + if (mad_agent_priv->agent.recv_handler)
> + mad_agent_priv->agent.recv_handler(
> + &mad_agent_priv->agent,
> &recv->header.recv_wc);
Need to free the receive buffer here as well if not delivered.
> - if (!mad_agent) {
> + if (!mad_agent || !mad_agent->agent.recv_handler) {
This appears to be where the receive buffer would have been freed, but...
We can't safely walk into the mad_agent structure after calling
ib_mad_complete_recv(). Immediately above this code a reference is taken on the
mad_agent. That reference is released in ib_mad_complete_recv(), which would allow
the user to destroy the mad_agent before returning back to this call and the if
statement above.
I'm more in favor of removing checks for a recv_handler completely, but if we want to
keep it, we can move it into find_mad_agent(), and just not report a mad_agent if it
doesn't have a recv_handler.
> if (mad_send_wr->status != IB_WC_SUCCESS )
> mad_send_wc->status = mad_send_wr->status;
> - mad_agent_priv->agent.send_handler(&mad_agent_priv->agent,
> mad_send_wc);
> + if (mad_agent_priv->agent.send_handler)
> + mad_agent_priv->agent.send_handler(&mad_agent_priv->agent,
> + mad_send_wc);
This has similar problems to the receive handling. If a client issued a send, but
doesn't have a send_handler, there's nothing we can do with the send buffer, which
needs to be freed. I think that a client who does this is causing more problems then
we can deal with in the access layer.
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.
> /* Release reference on agent taken when sending */
> if (atomic_dec_and_test(&mad_agent_priv->refcount))
> @@ -1135,7 +1153,9 @@
> list_for_each_entry_safe(mad_send_wr, temp_mad_send_wr,
> &cancel_list, agent_list) {
> mad_send_wc.wr_id = mad_send_wr->wr_id;
> - mad_agent_priv->agent.send_handler(&mad_agent_priv->agent,
> + if (mad_agent_priv->agent.send_handler)
> + mad_agent_priv->agent.send_handler(
> + &mad_agent_priv->agent,
> &mad_send_wc);
Same issue as above.
> list_del(&mad_send_wr->agent_list);
> @@ -1196,8 +1216,9 @@
> mad_send_wc.status = IB_WC_WR_FLUSH_ERR;
> mad_send_wc.vendor_err = 0;
> mad_send_wc.wr_id = mad_send_wr->wr_id;
> - mad_agent_priv->agent.send_handler(&mad_agent_priv->agent,
> - &mad_send_wc);
> + if (mad_agent_priv->agent.send_handler)
> + mad_agent_priv->agent.send_handler(&mad_agent_priv->agent,
> + &mad_send_wc);
Ditto.
_______________________________________________
openib-general mailing list
[EMAIL PROTECTED]
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
