linux-kernel - Re: [PATCH v6 5/6] fbdev: Move framebuffer I/O helpers into <asm/fb.h>

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: <238513df-4a39-75d4-9012-20d7d8526706@suse.de>
Date:   Wed, 10 May 2023 17:11:27 +0200
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     deller@....de, javierm@...hat.com, daniel@...ll.ch,
        vgupta@...nel.org, chenhuacai@...nel.org, kernel@...0n.name,
        davem@...emloft.net, James.Bottomley@...senpartnership.com,
        arnd@...db.de, sam@...nborg.org, suijingfeng@...ngson.cn,
        linux-fbdev@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linux-arch@...r.kernel.org, linux-snps-arc@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-ia64@...r.kernel.org,
        loongarch@...ts.linux.dev, linux-m68k@...ts.linux-m68k.org,
        sparclinux@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-parisc@...r.kernel.org
Subject: Re: [PATCH v6 5/6] fbdev: Move framebuffer I/O helpers into
 <asm/fb.h>

Hi Geert

Am 10.05.23 um 16:34 schrieb Geert Uytterhoeven:
> Hi Thomas,
> 
> On Wed, May 10, 2023 at 4:20 PM Thomas Zimmermann <tzimmermann@...e.de> wrote:
>> Am 10.05.23 um 14:34 schrieb Geert Uytterhoeven:
>>> On Wed, May 10, 2023 at 1:06 PM Thomas Zimmermann <tzimmermann@...e.de> wrote:
>>>> Implement framebuffer I/O helpers, such as fb_read*() and fb_write*(),
>>>> in the architecture's <asm/fb.h> header file or the generic one.
>>>>
>>>> The common case has been the use of regular I/O functions, such as
>>>> __raw_readb() or memset_io(). A few architectures used plain system-
>>>> memory reads and writes. Sparc used helpers for its SBus.
>>>>
>>>> The architectures that used special cases provide the same code in
>>>> their __raw_*() I/O helpers. So the patch replaces this code with the
>>>> __raw_*() functions and moves it to <asm-generic/fb.h> for all
>>>> architectures.
>>>>
>>>> v6:
>>>>           * fix fb_readq()/fb_writeq() on 64-bit mips (kernel test robot)
>>>> v5:
>>>>           * include <linux/io.h> in <asm-generic/fb>; fix s390 build
>>>> v4:
>>>>           * ia64, loongarch, sparc64: add fb_mem*() to arch headers
>>>>             to keep current semantics (Arnd)
>>>> v3:
>>>>           * implement all architectures with generic helpers
>>>>           * support reordering and native byte order (Geert, Arnd)
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@...e.de>
>>>> Tested-by: Sui Jingfeng <suijingfeng@...ngson.cn>
>>>> Reviewed-by: Arnd Bergmann <arnd@...db.de>
> 
>>>> --- a/arch/mips/include/asm/fb.h
>>>> +++ b/arch/mips/include/asm/fb.h
>>>> @@ -12,6 +12,28 @@ static inline void fb_pgprotect(struct file *file, struct vm_area_struct *vma,
>>>>    }
>>>>    #define fb_pgprotect fb_pgprotect
>>>>
>>>> +/*
>>>> + * MIPS doesn't define __raw_ I/O macros, so the helpers
>>>> + * in <asm-generic/fb.h> don't generate fb_readq() and
>>>> + * fb_write(). We have to provide them here.
>>>
>>> MIPS does not include <asm-generic/io.h>,  nor define its own
>>
>> I know, that's why the TODO says to convert it to generic I/O.
>>
>>> __raw_readq() and __raw_writeq()...
>>
>> It doesn't define those macros, but it generates function calls of the
>> same names. Follow the macros at
>>
>>
>> https://elixir.bootlin.com/linux/latest/source/arch/mips/include/asm/io.h#L357
>>
>> It expands to a variety of helpers, including __raw_*().
> 
> Thanks, I forgot MIPS is using these grep-unfriendly factories...
> 
>>>> + *
>>>> + * TODO: Convert MIPS to generic I/O. The helpers below can
>>>> + *       then be removed.
>>>> + */
>>>> +#ifdef CONFIG_64BIT
>>>> +static inline u64 fb_readq(const volatile void __iomem *addr)
>>>> +{
>>>> +       return __raw_readq(addr);
>>>
>>> ... so how can this call work?
>>
>> On 64-bit builds, there's __raw_readq() and __raw_writeq().
>>
>> At first, I tried to do the right thing and convert MIPS to work with
>> <asm-generic/io.h>. But that created a ton of follow-up errors in other
>> headers. So for now, it's better to handle this problem in asm/fb.h.
> 
> So isn't just adding
> 
>      #define __raw_readq __raw_readq
>      #define __raw_writeq __raw_writeq
> 
> to arch/mips/include/asm/io.h sufficient to make <asm-generic/fb.h>
> do the right thing?

That works. I had a patch that adds all missing defines to MIPS' io.h. 
Then I went with the current fix, which is self-contained within fbdev. 
But I'd leave it to arch maintainers.

Best regards
Thomas


> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)