lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <1308324821.16582.31.camel@Joe-Laptop>
Date:	Fri, 17 Jun 2011 08:33:41 -0700
From:	Joe Perches <joe@...ches.com>
To:	Sebastian Pöhn 
	<sebastian.belden@...glemail.com>
Cc:	Linux Netdev <netdev@...r.kernel.org>,
	Sebastian Pöhn <sebastian.poehn@...den.com>
Subject: Re: [PATCH] gianfar v4: implement nfc

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.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ