Re: [PATCH 03/16] ehca: structure definitions

[Jörn Engel]

On Thu, 27 April 2006 12:48:13 +0200, Heiko J Schick wrote:
> +
> +#ifdef CONFIG_PPC64
> +#include "ehca_classes_pSeries.h"
> +#endif

Is the #ifdef necessary?  Such conditions around header includes often
indicate problems with the included header.  If that is the case here,
you should fix the header in question in a seperate patch.

> +struct ehca_shca {
> +	struct ib_device ib_device;
> +	struct ibmebus_dev *ibmebus_dev;
> +	u8 num_ports;
        ^^
> +	int hw_level;

This will cause some wasted space due to alignment.  There don't seem
to be other u8 or chars to consolidate with here, though.  Still, you
could take another look that your structures have fields on natural
alignment borders and don't grow without you noticing.

> +struct ehca_mr {
> +	union {
> +		struct ib_mr ib_mr;	/* must always be first in ehca_mr */
> +		struct ib_fmr ib_fmr;	/* must always be first in ehca_mr */
> +	} ib;
> +
> +	spinlock_t mrlock;
> +
> +	/* !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
> +	 * !!! ehca_mr_deletenew() memsets from flags to end of structure
> +	 * !!! DON'T move flags or insert another field before.
> +	 * !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! 
> */

Yuck!  Do you have really good reasons to play such games?

> +struct ehca_pfpd {
> +};
> +
> +struct ehca_pfmr {
> +};
> +
> +struct ehca_pfmw {
> +};

Why these?

Jörn

-- 
Those who come seeking peace without a treaty are plotting.
-- Sun Tzu
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/