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

Michael S. Tsirkin mst at mellanox.co.il
Mon Jan 17 00:43:21 PST 2005


Hello, Roland!
Quoting Roland Dreier
> I reworked this to clean a few things up and match the style of
> mthca_cmd.c a little better.

This is a valid approach. Its your call.

Generally, there is one thing I dislike in the MTHCA_PUT macro -
specifically, that I have to look up the size of the *source* variable
to know which conversion is made. Ideally its the destination
that should dictate the right size.

Consider this:

	MTHCA_PUT(box, in_wc->slid,       MAD_IFC_RLID_OFFSET);

So in_wc->slid happends to have size 16 bit, and so it fits exactly
the inbox format. Its not just luck, but when I am trying to grok
the specific hardware command, I dont want to go look up the type of
structures in ib_verbs.h, which do not have to match hardware exactly
after all.
With the inbox described by a structure, I can see the correct conversion
is being applied at a glance, without going to another file.

Further, I also think that specifying offsets
by means of a structure has an advantage that it is easier to see
that you did not forget to fill in some field.

Maybe not just now, but what about eventually making mthca_cmd.c
use structures along the lines of mthca_cq.c ?

>   Does this still seem OK?

Yes, it looks allright to commit.

> (I got rid of the BUG_ON because I think the test
> 
> 	BUG_ON((!ignore_bkey && !ignore_mkey) && !in_wc);
> 
> is not really correct -- eg passing a MAD known not to be BM-class
> with only ignore_mkey set and no in_wc would be fine -- and I
> couldn't decide on a check that did seem worth oopsing for)

The check seems to be in there. Its not that important either way.

> +	BUG_ON((!ignore_bkey && !ignore_mkey) && !in_wc);
> +

MST



More information about the general mailing list