Re: [PATCH] gianfar v4: implement nfc

[Joe Perches <joe@perches.com>]

On Fri, 2011-06-17 at 11:53 +0200, Sebastian Pöhn wrote:
> This patch adds all missing functionalities for nfc except GRXFH.

Just some trivial comments.

> diff --git a/drivers/net/gianfar_ethtool.c b/drivers/net/gianfar_ethtool.c
[]
> +static void gfar_swap(void *a, void *b, int size)
> +{
> +	u32 t1 = *(u32 *) a;
> +	u32 t2 = *(u32 *) (a + 4);
> +	u32 t3 = *(u32 *) (a + 8);
> +	u32 t4 = *(u32 *) (a + 12);
> +	*(u32 *) a = *(u32 *) b;
> +	*(u32 *) (a + 4) = *(u32 *) (b + 4);
> +	*(u32 *) (a + 8) = *(u32 *) (b + 8);
> +	*(u32 *) (a + 12) = *(u32 *) (b + 12);
> +	*(u32 *) b = t1;
> +	*(u32 *) (b + 4) = t2;
> +	*(u32 *) (b + 8) = t3;
> +	*(u32 *) (b + 12) = t4;
> +}

Doesn't this read better by casting a and b
to temporary pointers and using swap?

static void gfar_swap(void *a, void *b, int size)
{
	u32 *_a = a;
	u32 *_b = b;

	swap(_a[0], _b[0]);
	swap(_a[1], _b[1]);
	swap(_a[2], _b[2]);
	swap(_a[3], _b[3]);
}

[]

> +	mask_table = kzalloc(
> +			sizeof(struct gfar_mask_entry) * (MAX_FILER_CACHE_IDX
> +					/ 2 + 1), GFP_KERNEL);

	mask_table = kcalloc(MAX_FILER_CACHE_IDX / 2 + 1,
			     sizeof(struct gfar_mask_entry), GFP_KERNEL)

[]

> +			netdev_err(priv->ndev,
> +				"Adding this rule is not possible,"
> +				" because this flow-type is not supported"
> +				" by hardware!\n");

Could you review your logging messages for grammar
and spacing please.

For all these logging messages, you really should
ignore what is now an arbitrary 80 column limit and
not break the format string up on multiple lines.

It makes grep harder and when you write them, you don't
quite read them back to yourself normally.  You wouldn't
generally use a comma between possible and because.

			netdev_err(priv->ndev,
				   "Adding this rule is not possible because this flow-type is not supported by hardware!\n");

or maybe:

			netdev_err(priv->ndev,
				   "Rule not added.  Flow-type not supported by hardware!\n");

Maybe it should be:

	"Flow-type (%(some type)) not supported by hardware!\n",
	flow->some_type

> +				netdev_err(priv->ndev,
> +				"There is already an element on ID %d\n",
> +				flow->location);

I think it nicer to not align any function params
on indent level but where possible on open paren.

				netdev_err(priv->ndev,
					   "There is already an element on ID %d\n",
					   flow->location);

Aligning on indent level is confusing.
If you really need or want to to this, it's
better to change the indent level inwards
for extra long lines.

			netdev_err(priv->ndev,
"Adding this rule is not possible because this flow-type is not supported by hardware!\n");

That way you can see if the line is too long for
a message logging form and should be shortened
somehow.