netdev - Re: [PATCH 2/2] net: Add Keystone NetCP ethernet driver

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] [thread-next>] [day] [month] [year] [list]

Message-Id: <20140424.124744.1296959126932595844.davem@davemloft.net>
Date:	Thu, 24 Apr 2014 12:47:44 -0400 (EDT)
From:	David Miller <davem@...emloft.net>
To:	santosh.shilimkar@...com
Cc:	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
	sandeep_n@...com, robh+dt@...nel.org, grant.likely@...aro.org
Subject: Re: [PATCH 2/2] net: Add Keystone NetCP ethernet driver

From: Santosh Shilimkar <santosh.shilimkar@...com>
Date: Tue, 22 Apr 2014 17:21:15 -0400

> +struct netcp_tx_pipe {
> +	struct netcp_device	*netcp_device;
> +	void	*dma_queue;

Indent *dma_queue the same as the other struct members.

> +	unsigned		dma_queue_id;

Use explicit "unsigned int".

> +	unsigned		dma_chan_id;

Likewise.

> +struct netcp_addr {
> +	struct netcp_intf	*netcp;
> +	unsigned char		addr[MAX_ADDR_LEN];

If this is just an ethernet driver, ETH_ALEN is more appropriate here.

> +	unsigned		tx_compl_qid;

Explicit "unsigned int" please.  I'm not going to point out all of the other
instances, audit your entire submission for this problem please.

> +static inline u32 *netcp_push_psdata(struct netcp_packet *p_info,
> +				     unsigned bytes)
> +{
> +	u32		*buf;
> +	unsigned	 words;

Do not use tabs between the type and the variable name in local variable
declarations.

Please audit for and fix this in your entire submission.

> +static inline u32 hwval_to_host(bool big_endian, u32 hwval)
> +{
> +	if (big_endian)
> +		return be32_to_cpu(hwval);
> +	else
> +		return le32_to_cpu(hwval);
> +}

You're much better off having a set of methods, one for big endian and
one for little endian, that just straight line codes the appropriate endian
accesses.

These conditionals peppered all over the place are just ugly.
--
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