linux-kernel - Re: [PATCH 1/2] leds: bcm6328: improve write and read functions

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 24 Feb 2021 09:45:25 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Pavel Machek <pavel@....cz>
Cc:     Álvaro Fernández Rojas <noltari@...il.com>,
        linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] leds: bcm6328: improve write and read functions



On 2/24/2021 9:36 AM, Pavel Machek wrote:
> Hi!
> 
>>>> Yeah, but ideally you should not be copying comments; there should be
>>>> one central place which does it and does it right.
>>>
>>> I’m open to suggestions :).
>>> Which central place would be a good place for you?
>>
>> I did consider creating an include/linux/brcm/brcm_io.h header or
>> something like that but I am really not sure what the benefit would
>> be.
> 
> Less code duplication? It is immediately clear that driver including
> this is specific for brcm SoCs and would not try to work somewhere else?

Yes maybe, there still does not feel like this deserves a shared header,
but as long as the generated code is the same, why not.

> 
>> As far as using _relaxed() this is absolutely correct because the bus
>> logic that connects the CPU to its on-chip registers is non re-ordering
>> non posted. That is true on the MIPS BE/LE and ARM when configured in LE
>> or BE.
> 
> If that's right on particular SoC, then _relaxed and normal versions
> should be same; drivers still need to use normal versions, because
> they may be running on different SoC...?

readl() includes barriers and read_relaxed() does not, hence the
difference in the name. There is no need to pay the price of a barrier
when a) the bus architecture guarantees non re-ordering and posting and
that statement is true on all the SoCs where these peripherals are used,
and b) you have worked on fine tuning your drivers to get the most
performance out of them.

> 
>> We need the swapping for ARM because when running in ARM BE32, the data
>> is going to be in the host CPU endian, but the register bus is hard
>> wired to little endian.
> 
> Yeah I see you need to do some byteswapping. But I'm pretty sure not
> all MIPS BE boxes do the magic swapping, right? And drivers/leds is
> not a place where you encode knowledge about SoC byte swapping.

The Broadcom MIPS CPUs (we have/had an architectural license) can be
strapped for BE or LE, and when that happens the bridge that connects to
the registers follows the CPU's endian, which is why __raw_{read,write}l
is appropriate for these specific peripherals.

Given these peripherals can only be used on CPUs/SoCs made by Broadcom,
any argument about portability to other SoCs is moot.
-- 
Florian