[openib-general] [PATCH] fill in missing parameters for MAD_IFC

[Michael S. Tsirkin]

Hello!
Quoting r. Roland Dreier (roland at topspin.com) "Re: [openib-general] [PATCH] fill in missing parameters for MAD_IFC":
>     Michael> I dont want to go look up the type of structures in
>     Michael> ib_verbs.h, which do not have to match hardware exactly
>     Michael> after all.
> 
> Yeah, I had avoided putting direct dependencies on ib_verbs.h into
> mthca_cmd.c, but I didn't see a nice way to avoid it for the extended
> MAD info passed to MAD_IFC.

Well, the dependency will be there, but I think relying on the
field width in ib_verbs.h matching the hardware is a bit on the ugly side.

>     Michael> Maybe not just now, but what about eventually making
>     Michael> mthca_cmd.c use structures along the lines of mthca_cq.c?
> 
> The thing I don't like about doing that is that things like
> sl_g_mlpath end up being pretty ugly to handle.

It does not have to be that way, I put sl and path in the same
word because they are that way in the cqe, but they could be split.

+		val = in_wc->sl << 4;
+		MTHCA_PUT(box, val,               MAD_IFC_SL_OFFSET);
+
+		val = in_wc->dlid_path_bits |
+			(in_wc->wc_flags & IB_WC_GRH ? 0x80 : 0);
+		MTHCA_PUT(box, val,               MAD_IFC_GRH_OFFSET);

Becomes:

+               info->sl = in_wc->sl << 4;
+               info->sl_g_mlpath = cpu_to_be16(in_wc->dlid_path_bits |
+			(in_wc->wc_flags & IB_WC_GRH ? 0x80 : 0);

Convinced?

>     Michael> The check seems to be in there. Its not that important
>     Michael> either way.
> 
> I did misread the BUG_ON -- but it does seem there are cases where it
> oopses without really needing to.  Rather than a BUG_ON, how about
> just moving the code to make sure we don't check without in_wc into
> the actual MAD_IFC routine:
> 
> 	/*
> 	 * Key check traps can't be generated unless we have in_wc to
> 	 * tell us where to send the trap.
> 	 */
> 	if (ignore_mkey || !in_wc)
> 		op_modifier |= 0x1;
> 	if (ignore_bkey || !in_wc)
> 		op_modifier |= 0x2;

Fine with me.

> Here's my current patch:

OK.

MST