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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <a2da8ee6-a941-de04-3d45-56fb6cae596a@gmail.com>
Date:   Tue, 23 Feb 2021 09:00:16 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Álvaro Fernández Rojas <noltari@...il.com>,
        Pavel Machek <pavel@....cz>
Cc:     Dan Murphy <dmurphy@...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/23/2021 1:05 AM, Álvaro Fernández Rojas wrote:
> 
> 
>> El 23 feb 2021, a las 9:58, Pavel Machek <pavel@....cz> escribió:
>>
>> Hi!
>>
>>>>> This is proven to work in BMIPS BE/LE and ARM BE/LE, as used in bcm2835-rng
>>>>> and bcmgenet drivers.
>>>>> Both should also be inline functions.
>>>>
>>>>
>>>>
>>>>> -#ifdef CONFIG_CPU_BIG_ENDIAN
>>>>> -	iowrite32be(data, reg);
>>>>> -#else
>>>>> -	writel(data, reg);
>>>>> -#endif
>>>>> +	/* MIPS chips strapped for BE will automagically configure the
>>>>> +	 * peripheral registers for CPU-native byte order.
>>>>> +	 */
>>>>
>>>> Bad comment style.
>>>
>>> I just wanted to copy the same comment as the one in bcm2835-rng and bcmgenet…
>>> https://github.com/torvalds/linux/blob/3b9cdafb5358eb9f3790de2f728f765fef100731/drivers/char/hw_random/bcm2835-rng.c#L42-L60
>>> https://github.com/torvalds/linux/blob/3b9cdafb5358eb9f3790de2f728f765fef100731/drivers/net/ethernet/broadcom/genet/bcmgenet.c#L71-L88
>>>
>>
>> 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.

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.

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.
-- 
Florian