netdev - Re: [PATCH v2] net: ezchip: adapt driver to little endian architecture

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: <3132255.zs1gQSacSZ@wuerfel>
Date:	Wed, 02 Mar 2016 21:07:37 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	Lada Trimasova <Lada.Trimasova@...opsys.com>
Cc:	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	Alexey Brodkin <Alexey.Brodkin@...opsys.com>,
	Noam Camus <noamc@...hip.com>, Tal Zilcer <talz@...hip.com>
Subject: Re: [PATCH v2] net: ezchip: adapt driver to little endian architecture

On Wednesday 02 March 2016 21:38:04 Lada Trimasova wrote:
> Since ezchip network driver is written with big endian EZChip platform it
> is necessary to add support for little endian architecture.
> 
> The first issue is that the order of the bits in a bit field is
> implementation specific. So all the bit fields are removed.
> Named constants are used to access necessary fields.
> 
> And the second one is that network byte order is big endian.
> For example, data on ethernet is transmitted with most-significant
> octet (byte) first. So in case of little endian architecture
> it is important to swap data byte order when we read it from
> register. In case of unaligned access we can use "get_unaligned_be32"
> and in other case we should use function "be32_to_cpu" as we read from
> peripheral to CPU.
> 
> And then when we are going to write data to register we need to restore
> byte order using the function "put_unaligned_be32" in case of
> unaligned access and in other case "cpu_to_be32" as
> we write from CPU to peripheral.
> 
> The last little fix is a space between type and pointer to observe
> coding style.
> 
> Signed-off-by: Lada Trimasova <ltrimas@...opsys.com>
> Cc: Alexey Brodkin <abrodkin@...opsys.com>
> Cc: Noam Camus <noamc@...hip.com>
> Cc: Tal Zilcer <talz@...hip.com>
> Cc: Arnd Bergmann <arnd@...db.de>

Looks much better already.

> index b102668..9a5a3bf 100644
> --- a/drivers/net/ethernet/ezchip/nps_enet.c
> +++ b/drivers/net/ethernet/ezchip/nps_enet.c
> @@ -44,19 +44,22 @@ static void nps_enet_read_rx_fifo(struct net_device *ndev,
>  
>  	/* In case dst is not aligned we need an intermediate buffer */
>  	if (dst_is_aligned)
> -		for (i = 0; i < len; i++, reg++)
> +		for (i = 0; i < len; i++, reg++) {
> +			/* In case of LE we need to swap bytes */
>  			*reg = nps_enet_reg_get(priv, NPS_ENET_REG_RX_BUF);
> +			*reg = be32_to_cpu(*reg);
> +		}

This is still not right: please build this with

make C=2 CF="-D__CHECK_ENDIAN__" drivers/net/ethernet/ezchip/nps_enet.o

What is going on here is that NPS_ENET_REG_RX_BUF seems to be
some kind of FIFO register rather than an MMIO register, so
the swap in ioread32be() is wrong.

What you should do instead is to replace the loop
with a call to ioread32_rep(), which will be more efficient
and endian-safe.

	Arnd