netdev - RE: [PATCH net-next v2] net: bcmgenet: Use correct I/O accessors

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: <9B0290AA-64A9-4DF5-80E5-94AEA41D9D72@gmail.com>
Date:   Wed, 30 Aug 2017 09:00:39 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     David Laight <David.Laight@...LAB.COM>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "opendmb@...il.com" <opendmb@...il.com>,
        "jaedon.shin@...il.com" <jaedon.shin@...il.com>
Subject: RE: [PATCH net-next v2] net: bcmgenet: Use correct I/O accessors

On August 30, 2017 4:39:52 AM PDT, David Laight <David.Laight@...LAB.COM> wrote:
>From: Florian Fainelli
>> Sent: 29 August 2017 20:26
>> The GENET driver currently uses __raw_{read,write}l which means
>> native I/O endian. This works correctly for an ARM LE kernel
>(default)
>> but fails miserably on an ARM BE (BE8) kernel where registers are
>kept
>> little endian, so replace uses with {read,write}l_relaxed here which
>is
>> what we want because this is all performance sensitive code.
>...
>> +	if (IS_ENABLED(CONFIG_MIPS) && IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>> +		__raw_writel(value, offset);
>> +	else
>> +		writel_relaxed(value, offset);
>
>How do you know that all BE MIPS that might have this driver have
>the BE registers of your card?
>(Or that all ARM BE systems have LE registers.)
>

This is the embedded network controller found on Broadcom STB SoCs, they were MIPS-based before, now ARM/ARM64-based. Any MIPS-based SoC that has this controller is using one of Broadcom's BMIPS processor (4350/4380/5000/5200) and all obey the same rule that their endian strap propagates to bus register endian setting as such that the result is always native endian for them. All ARM/ARM64-based SoC are paired with a newer version of the register bus that voluntarily dropped support for changing its endian, such that it is always LE for these newer SoCs.

You won't find this controller in any other product from Broadcom, just like there was not a version designed for e.g: running on a PCI(e) attached FPGA or anything.

>If nothing else the driver code should be predicated on a
>condition set by the kernel config that depends on the cpu build
>rather than embedding that condition in a lot of drivers

The driver is made to build for as many configurations as possible but it won't get probed unless the appropriate DT nodes are populated.

-- 
Florian