linux-kernel - Re: [PATCH 1/1] video: Handle HAS

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]

Message-ID: <3eb5b0b4-f025-4396-9d52-05c59d6b904b@gmx.de>
Date: Wed, 24 Apr 2024 21:29:20 +0200
From: Helge Deller <deller@....de>
To: Arnd Bergmann <arnd@...nel.org>, Niklas Schnelle
 <schnelle@...ux.ibm.com>, Helge Deller <deller@...nel.org>
Cc: linux-fbdev@...r.kernel.org, dri-devel@...ts.freedesktop.org,
 Heiko Carstens <hca@...ux.ibm.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] video: Handle HAS_IOPORT dependencies

On 4/22/24 21:28, Arnd Bergmann wrote:
> On Mon, Apr 22, 2024, at 10:34, Niklas Schnelle wrote:
>> On Thu, 2024-04-11 at 16:00 +0200, Helge Deller wrote:
>>> * Niklas Schnelle <schnelle@...ux.ibm.com>:
>>>> In a future patch HAS_IOPORT=n will disable inb()/outb() and friends at
>>>> compile time. We thus need to #ifdef functions and their callsites which
>>>> unconditionally use these I/O accessors. In the include/video/vga.h
>>>> these are conveniently all those functions with the vga_io_* prefix.
>>>
>>> Why don't you code it like in the patch below?
>>> inb_p(), outb_p() and outw() would then need to be defined externally
>>> without an implementation so that they would generate link time errors
>>> (instead of compile time errors).
>>
>> This may be personal preference but I feel like link time errors would
>> be very late to catch a configuration that can't work. Also this would
>> bypass the __compiletime_error("inb()) requires CONFIG_HAS_IOPORT");
>> added instead of the in*()/out*() helpers to make it easy to spot the
>> problem.
>>
>> I'm not a fan of #ifdeffery either but I think in this case it is
>> simple, well enough contained and overall there aren't that many spots
>> where we need to exclude just some sections of code vs entire drivers
>> with vga.h probably being the worst of them all.
>
> Agreed. I also tried to see if we can move stuff out of vga.h
> to have it included in fewer places, as almost everything that
> uses this header already has a HAS_IOPORT dependency, but that
> would be a lot more work.
>
> The other one that gains a few ugly #ifdefs is the 8250 driver,
> everything else is already merged in linux-next or needs a simple
> Kconfig dependency.
>
> I think we can make the vga.h file a little more readable
> by duplicating the functions and still keep the __compiletime_error()
> version in asm/io.h, see below.

Ok.
I assume you or Niklas will then send an updated patch?

Helge


> diff --git a/include/video/vga.h b/include/video/vga.h
> index 947c0abd04ef..7e1d8252b732 100644
> --- a/include/video/vga.h
> +++ b/include/video/vga.h
> @@ -197,6 +197,23 @@ struct vgastate {
>   extern int save_vga(struct vgastate *state);
>   extern int restore_vga(struct vgastate *state);
>
> +static inline unsigned char vga_mm_r (void __iomem *regbase, unsigned short port)
> +{
> +	return readb (regbase + port);
> +}
> +
> +static inline void vga_mm_w (void __iomem *regbase, unsigned short port, unsigned char val)
> +{
> +	writeb (val, regbase + port);
> +}
> +
> +static inline void vga_mm_w_fast (void __iomem *regbase, unsigned short port,
> +				  unsigned char reg, unsigned char val)
> +{
> +	writew (VGA_OUT16VAL (val, reg), regbase + port);
> +}
> +
> +#ifdef CONFIG_HAS_IOPORT
>   /*
>    * generic VGA port read/write
>    */
> @@ -217,22 +234,6 @@ static inline void vga_io_w_fast (unsigned short port, unsigned char reg,
>   	outw(VGA_OUT16VAL (val, reg), port);
>   }
>
> -static inline unsigned char vga_mm_r (void __iomem *regbase, unsigned short port)
> -{
> -	return readb (regbase + port);
> -}
> -
> -static inline void vga_mm_w (void __iomem *regbase, unsigned short port, unsigned char val)
> -{
> -	writeb (val, regbase + port);
> -}
> -
> -static inline void vga_mm_w_fast (void __iomem *regbase, unsigned short port,
> -				  unsigned char reg, unsigned char val)
> -{
> -	writew (VGA_OUT16VAL (val, reg), regbase + port);
> -}
> -
>   static inline unsigned char vga_r (void __iomem *regbase, unsigned short port)
>   {
>   	if (regbase)
> @@ -258,7 +259,25 @@ static inline void vga_w_fast (void __iomem *regbase, unsigned short port,
>   	else
>   		vga_io_w_fast (port, reg, val);
>   }
> +#else
>
> +static inline unsigned char vga_r (void __iomem *regbase, unsigned short port)
> +{
> +	return vga_mm_r(regbase, port);
> +}
> +
> +static inline void vga_w(void __iomem *regbase, unsigned short port, unsigned char val)
> +{
> +	vga_mm_w (regbase, port, val);
> +}
> +
> +static inline void vga_w_fast (void __iomem *regbase, unsigned short port,
> +			       unsigned char reg, unsigned char val)
> +{
> +	vga_mm_w_fast(regbase, port, reg, val);
> +}
> +
> +#endif
>
>   /*
>    * VGA CRTC register read/write