netdev - Re: [PATCH v2 1/5] ethernet: add sun8i-emac 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]

Date:	Fri, 29 Jul 2016 11:12:02 +0100
From:	Andre Przywara <andre.przywara@....com>
To:	maxime.ripard@...e-electrons.com,
	LABBE Corentin <clabbe.montjoie@...il.com>
Cc:	robh+dt@...nel.org, mark.rutland@....com, wens@...e.org,
	linux@...linux.org.uk, davem@...emloft.net, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, linux-sunxi@...glegroups.com
Subject: Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver

Hi,

On 25/07/16 20:54, Maxime Ripard wrote:
> On Wed, Jul 20, 2016 at 10:03:16AM +0200, LABBE Corentin wrote:
>> This patch add support for sun8i-emac ethernet MAC hardware.
>> It could be found in Allwinner H3/A83T/A64 SoCs.
>>
>> It supports 10/100/1000 Mbit/s speed with half/full duplex.
>> It can use an internal PHY (MII 10/100) or an external PHY
>> via RGMII/RMII.
>>
>> Signed-off-by: LABBE Corentin <clabbe.montjoie@...il.com>
>> ---
>>  drivers/net/ethernet/allwinner/Kconfig      |   13 +
>>  drivers/net/ethernet/allwinner/Makefile     |    1 +
>>  drivers/net/ethernet/allwinner/sun8i-emac.c | 2129 +++++++++++++++++++++++++++
>>  3 files changed, 2143 insertions(+)
>>  create mode 100644 drivers/net/ethernet/allwinner/sun8i-emac.c

...

>> diff --git a/drivers/net/ethernet/allwinner/sun8i-emac.c b/drivers/net/ethernet/allwinner/sun8i-emac.c
>> new file mode 100644
>> index 0000000..fc0c1dd
>> --- /dev/null
>> +++ b/drivers/net/ethernet/allwinner/sun8i-emac.c

...

>> +
>> +/* struct dma_desc - Structure of DMA descriptor used by the hardware
>> + * @status: Status of the frame written by HW, so RO for the
>> + *	driver (except for BIT(31) which is R/W)
>> + * @ctl: Information on the frame written by the driver (INT, len,...)
>> + * @buf_addr: physical address of the frame data
>> + * @next: physical address of next dma_desc
>> + */
>> +struct dma_desc {
>> +	u32 status;
>> +	u32 ctl;
>> +	u32 buf_addr;
>> +	u32 next;
>> +};
> 
> You should use the endian-aware variants here.

For the records: just doing the sparse annotation with __le32 here will
of course not be sufficient to make it work on BE kernels. I added
proper endianness conversion to all accesses to the descriptors and got
it to work with an arm64 big-endian kernel on the Pine64.

I put a patch here:
https://gist.github.com/apritzel/bc792c4dbbd8789f5f18aef538e8c440

This particular version is untested (though it compiles), since I just
adapted the working patch against the newer driver code and couldn't
test it yet.
I am not really an endianness expert, so don't know if there are smarter
ways to tackle this, if we should for instance provide access wrappers
to the DMA descriptor fields.

I will try to test this later today, if that works, feel free to merge
those changes into your driver.

Cheers,
Andre.