[ofa-general] [PATCHv4 03/10] ib_core: RDMAoE support only QP1

[Eli Cohen]

On Thu, Aug 06, 2009 at 10:52:34AM -0700, Sean Hefty wrote:
> >+	/* Validate device and port */
> >+	port_priv = ib_get_mad_port(device, port_num);
> >+	if (!port_priv) {
> >+		ret = ERR_PTR(-ENODEV);
> >+		goto error1;
> >+	}
> >+
> >+	if (!port_priv->qp_info[qp_type].qp)
> >+		return NULL;
> 
> It seems odd that the first if has 'goto error1', but the second if simply
> returns NULL. 
> 

The original intention was to release the caller from the need to
decide whether to register the mad agent or not and so the NULL
returned would not be treated as error. Thinking it over I realize
that it would be better to let the caller decide (according to the
port protocol) whether or not to register the mad agent. Will fix.


> >@@ -556,6 +559,9 @@ int ib_unregister_mad_agent(struct ib_mad_agent *mad_agent)
> > 	struct ib_mad_agent_private *mad_agent_priv;
> > 	struct ib_mad_snoop_private *mad_snoop_priv;
> >
> >+	if (!mad_agent)
> >+		return 0;
> 
> Why would a kernel client call ib_unregister_mad_agent with a NULL pointer?
> 

Same as above. Goes away after the fix.
> >
> > 	cq_size = (IB_MAD_QP_SEND_SIZE + IB_MAD_QP_RECV_SIZE) * 2;
> >+	has_smi = rdma_port_get_transport(device, port_num) ==
> RDMA_TRANSPORT_IB;
> >+	if (has_smi)
> >+		cq_size *= 2;
> 
> cq_size is doubled twice
> 

This is a bug - I'll fix it  - thanks.

> I really wish there were a cleaner way to add this support that didn't involve
> adding so many checks throughout the code.  It's hard to know if checks were
> added in all the places that were needed.  I can't think of a clever way to
> handle QP 0.

The fix discussed above will eliminate a good portion of these checks.