[openib-general] [PATCH] [RMPP] receive RMPP support

[Roland Dreier]

I'm not sure if/how RMPP will be used, so it's not clear to me how
useful the RMPP functions are -- I don't feel qualified to have an
opinion yet.

>+static struct ib_ah * create_ah_from_wc(struct ib_pd *pd, struct ib_wc *wc,
>+					u8 port_num)

This function seems reasonably useful.

>+{
>+	struct ib_ah_attr *ah_attr;
>+	struct ib_ah *ah;
>+
>+	ah_attr = kmalloc(sizeof *ah_attr, GFP_KERNEL);

Is it really worth kmalloc() for ah_attr here?  struct ib_ah_attr is
only 32 bytes.  Between the ah_attr pointer and the stack used by the
call to kmalloc(), the current code is probably using at least 16
bytes.  I'd trade 16 bytes of stack for smaller source and object code.

>+	if (!ah_attr)
>+		return ERR_PTR(-ENOMEM);
>+
>+	memset(ah_attr, 0, sizeof *ah_attr);
>+	ah_attr->dlid = wc->slid;
>+	ah_attr->sl = wc->sl;
>+	ah_attr->src_path_bits = wc->dlid_path_bits;
>+	ah_attr->port_num = port_num;

What if the wc has IB_WC_GRH set?  I'm not sure how useful a helper
this is if it doesn't handle the GRH case.

>+
>+	ah = ib_create_ah(pd, ah_attr);
>+	kfree(ah_attr);
>+	return ah;
>+}

Also, while we're looking at the code...

>+	msg->sge.addr = dma_map_single(mad_agent->device->dma_device,
>+				       &msg->mad, sizeof msg->mad,
>+				       DMA_TO_DEVICE);

it's somewhat risky to use dma_map_single() on fields in the middle of
a structure, because you don't know that the field starts and ends on
a cacheline boundary.  In this case you're pretty safe because you're
doing DMA_TO_DEVICE, but if you use the same type of code with
DMA_FROM_DEVICE on a non-cache-coherent arch (e.g. PowerPC 4xx) then
you can get into trouble.  See <http://lwn.net/Articles/2265/> for a
very nice writeup of the problem.

 - R.