[1/1] video: Handle HAS_IOPORT dependencies

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.

Co-developed-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
Note: This patch does not depend any not-yet-mainline HAS_IOPORT changes
and may be merged via subsystem specific trees at your earliest
convenience.

 include/video/vga.h | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

* Niklas Schnelle <schnelle@linux.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).

diff --git a/include/video/vga.h b/include/video/vga.h
index 947c0abd04ef..32c915e109fa 100644
--- a/include/video/vga.h
+++ b/include/video/vga.h
@@ -203,18 +203,20 @@ extern int restore_vga(struct vgastate *state);
 
 static inline unsigned char vga_io_r (unsigned short port)
 {
-	return inb_p(port);
+	return IS_ENABLED(CONFIG_HAS_IOPORT) ? inb_p(port) : 0;
 }
 
 static inline void vga_io_w (unsigned short port, unsigned char val)
 {
-	outb_p(val, port);
+	if (IS_ENABLED(CONFIG_HAS_IOPORT))
+		outb_p(val, port);
 }
 
 static inline void vga_io_w_fast (unsigned short port, unsigned char reg,
 				  unsigned char val)
 {
-	outw(VGA_OUT16VAL (val, reg), port);
+	if (IS_ENABLED(CONFIG_HAS_IOPORT))
+		outw(VGA_OUT16VAL (val, reg), port);
 }
 
 static inline unsigned char vga_mm_r (void __iomem *regbase, unsigned short port)



> 
> Co-developed-by: Arnd Bergmann <arnd@kernel.org>
> Signed-off-by: Arnd Bergmann <arnd@kernel.org>
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
> Note: This patch does not depend any not-yet-mainline HAS_IOPORT changes
> and may be merged via subsystem specific trees at your earliest
> convenience.
> 
>  include/video/vga.h | 35 +++++++++++++++++++++++++----------
>  1 file changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/include/video/vga.h b/include/video/vga.h
> index 947c0abd04ef..ed89295941c4 100644
> --- a/include/video/vga.h
> +++ b/include/video/vga.h
> @@ -201,6 +201,7 @@ extern int restore_vga(struct vgastate *state);
>   * generic VGA port read/write
>   */
>  
> +#ifdef CONFIG_HAS_IOPORT
>  static inline unsigned char vga_io_r (unsigned short port)
>  {
>  	return inb_p(port);
> @@ -210,12 +211,12 @@ static inline void vga_io_w (unsigned short port, unsigned char val)
>  {
>  	outb_p(val, port);
>  }
> -
>  static inline void vga_io_w_fast (unsigned short port, unsigned char reg,
>  				  unsigned char val)
>  {
>  	outw(VGA_OUT16VAL (val, reg), port);
>  }
> +#endif /* CONFIG_HAS_IOPORT */
>  
>  static inline unsigned char vga_mm_r (void __iomem *regbase, unsigned short port)
>  {
> @@ -235,28 +236,34 @@ static inline void vga_mm_w_fast (void __iomem *regbase, unsigned short port,
>  
>  static inline unsigned char vga_r (void __iomem *regbase, unsigned short port)
>  {
> -	if (regbase)
> -		return vga_mm_r (regbase, port);
> -	else
> +#ifdef CONFIG_HAS_IOPORT
> +	if (!regbase)
>  		return vga_io_r (port);
> +	else
> +#endif /* CONFIG_HAS_IOPORT */
> +		return vga_mm_r (regbase, port);
>  }
>  
>  static inline void vga_w (void __iomem *regbase, unsigned short port, unsigned char val)
>  {
> -	if (regbase)
> -		vga_mm_w (regbase, port, val);
> -	else
> +#ifdef CONFIG_HAS_IOPORT
> +	if (!regbase)
>  		vga_io_w (port, val);
> +	else
> +#endif /* CONFIG_HAS_IOPORT */
> +		vga_mm_w (regbase, port, val);
>  }
>  
>  
>  static inline void vga_w_fast (void __iomem *regbase, unsigned short port,
>  			       unsigned char reg, unsigned char val)
>  {
> -	if (regbase)
> -		vga_mm_w_fast (regbase, port, reg, val);
> -	else
> +#ifdef CONFIG_HAS_IOPORT
> +	if (!regbase)
>  		vga_io_w_fast (port, reg, val);
> +	else
> +#endif /* CONFIG_HAS_IOPORT */
> +		vga_mm_w_fast (regbase, port, reg, val);
>  }
>  
>  
> @@ -280,6 +287,7 @@ static inline void vga_wcrt (void __iomem *regbase, unsigned char reg, unsigned
>  #endif /* VGA_OUTW_WRITE */
>  }
>  
> +#ifdef CONFIG_HAS_IOPORT
>  static inline unsigned char vga_io_rcrt (unsigned char reg)
>  {
>          vga_io_w (VGA_CRT_IC, reg);
> @@ -295,6 +303,7 @@ static inline void vga_io_wcrt (unsigned char reg, unsigned char val)
>          vga_io_w (VGA_CRT_DC, val);
>  #endif /* VGA_OUTW_WRITE */
>  }
> +#endif /* CONFIG_HAS_IOPORT */
>  
>  static inline unsigned char vga_mm_rcrt (void __iomem *regbase, unsigned char reg)
>  {
> @@ -333,6 +342,7 @@ static inline void vga_wseq (void __iomem *regbase, unsigned char reg, unsigned
>  #endif /* VGA_OUTW_WRITE */
>  }
>  
> +#ifdef CONFIG_HAS_IOPORT
>  static inline unsigned char vga_io_rseq (unsigned char reg)
>  {
>          vga_io_w (VGA_SEQ_I, reg);
> @@ -348,6 +358,7 @@ static inline void vga_io_wseq (unsigned char reg, unsigned char val)
>          vga_io_w (VGA_SEQ_D, val);
>  #endif /* VGA_OUTW_WRITE */
>  }
> +#endif /* CONFIG_HAS_IOPORT */
>  
>  static inline unsigned char vga_mm_rseq (void __iomem *regbase, unsigned char reg)
>  {
> @@ -385,6 +396,7 @@ static inline void vga_wgfx (void __iomem *regbase, unsigned char reg, unsigned
>  #endif /* VGA_OUTW_WRITE */
>  }
>  
> +#ifdef CONFIG_HAS_IOPORT
>  static inline unsigned char vga_io_rgfx (unsigned char reg)
>  {
>          vga_io_w (VGA_GFX_I, reg);
> @@ -400,6 +412,7 @@ static inline void vga_io_wgfx (unsigned char reg, unsigned char val)
>          vga_io_w (VGA_GFX_D, val);
>  #endif /* VGA_OUTW_WRITE */
>  }
> +#endif /* CONFIG_HAS_IOPORT */
>  
>  static inline unsigned char vga_mm_rgfx (void __iomem *regbase, unsigned char reg)
>  {
> @@ -434,6 +447,7 @@ static inline void vga_wattr (void __iomem *regbase, unsigned char reg, unsigned
>          vga_w (regbase, VGA_ATT_W, val);
>  }
>  
> +#ifdef CONFIG_HAS_IOPORT
>  static inline unsigned char vga_io_rattr (unsigned char reg)
>  {
>          vga_io_w (VGA_ATT_IW, reg);
> @@ -445,6 +459,7 @@ static inline void vga_io_wattr (unsigned char reg, unsigned char val)
>          vga_io_w (VGA_ATT_IW, reg);
>          vga_io_w (VGA_ATT_W, val);
>  }
> +#endif /* CONFIG_HAS_IOPORT */
>  
>  static inline unsigned char vga_mm_rattr (void __iomem *regbase, unsigned char reg)
>  {
> -- 
> 2.40.1
>

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@linux.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.

    Arnd


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

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@linux.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

On 4/10/24 16:23, Niklas Schnelle wrote:
> 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.
>
> Co-developed-by: Arnd Bergmann <arnd@kernel.org>
> Signed-off-by: Arnd Bergmann <arnd@kernel.org>
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>

I've applied this patch now to the for-next fbdev git tree.

Please let me know if you prefer another patch....

Thanks!
Helge


> ---
> Note: This patch does not depend any not-yet-mainline HAS_IOPORT changes
> and may be merged via subsystem specific trees at your earliest
> convenience.
>
>   include/video/vga.h | 35 +++++++++++++++++++++++++----------
>   1 file changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/include/video/vga.h b/include/video/vga.h
> index 947c0abd04ef..ed89295941c4 100644
> --- a/include/video/vga.h
> +++ b/include/video/vga.h
> @@ -201,6 +201,7 @@ extern int restore_vga(struct vgastate *state);
>    * generic VGA port read/write
>    */
>
> +#ifdef CONFIG_HAS_IOPORT
>   static inline unsigned char vga_io_r (unsigned short port)
>   {
>   	return inb_p(port);
> @@ -210,12 +211,12 @@ static inline void vga_io_w (unsigned short port, unsigned char val)
>   {
>   	outb_p(val, port);
>   }
> -
>   static inline void vga_io_w_fast (unsigned short port, unsigned char reg,
>   				  unsigned char val)
>   {
>   	outw(VGA_OUT16VAL (val, reg), port);
>   }
> +#endif /* CONFIG_HAS_IOPORT */
>
>   static inline unsigned char vga_mm_r (void __iomem *regbase, unsigned short port)
>   {
> @@ -235,28 +236,34 @@ static inline void vga_mm_w_fast (void __iomem *regbase, unsigned short port,
>
>   static inline unsigned char vga_r (void __iomem *regbase, unsigned short port)
>   {
> -	if (regbase)
> -		return vga_mm_r (regbase, port);
> -	else
> +#ifdef CONFIG_HAS_IOPORT
> +	if (!regbase)
>   		return vga_io_r (port);
> +	else
> +#endif /* CONFIG_HAS_IOPORT */
> +		return vga_mm_r (regbase, port);
>   }
>
>   static inline void vga_w (void __iomem *regbase, unsigned short port, unsigned char val)
>   {
> -	if (regbase)
> -		vga_mm_w (regbase, port, val);
> -	else
> +#ifdef CONFIG_HAS_IOPORT
> +	if (!regbase)
>   		vga_io_w (port, val);
> +	else
> +#endif /* CONFIG_HAS_IOPORT */
> +		vga_mm_w (regbase, port, val);
>   }
>
>
>   static inline void vga_w_fast (void __iomem *regbase, unsigned short port,
>   			       unsigned char reg, unsigned char val)
>   {
> -	if (regbase)
> -		vga_mm_w_fast (regbase, port, reg, val);
> -	else
> +#ifdef CONFIG_HAS_IOPORT
> +	if (!regbase)
>   		vga_io_w_fast (port, reg, val);
> +	else
> +#endif /* CONFIG_HAS_IOPORT */
> +		vga_mm_w_fast (regbase, port, reg, val);
>   }
>
>
> @@ -280,6 +287,7 @@ static inline void vga_wcrt (void __iomem *regbase, unsigned char reg, unsigned
>   #endif /* VGA_OUTW_WRITE */
>   }
>
> +#ifdef CONFIG_HAS_IOPORT
>   static inline unsigned char vga_io_rcrt (unsigned char reg)
>   {
>           vga_io_w (VGA_CRT_IC, reg);
> @@ -295,6 +303,7 @@ static inline void vga_io_wcrt (unsigned char reg, unsigned char val)
>           vga_io_w (VGA_CRT_DC, val);
>   #endif /* VGA_OUTW_WRITE */
>   }
> +#endif /* CONFIG_HAS_IOPORT */
>
>   static inline unsigned char vga_mm_rcrt (void __iomem *regbase, unsigned char reg)
>   {
> @@ -333,6 +342,7 @@ static inline void vga_wseq (void __iomem *regbase, unsigned char reg, unsigned
>   #endif /* VGA_OUTW_WRITE */
>   }
>
> +#ifdef CONFIG_HAS_IOPORT
>   static inline unsigned char vga_io_rseq (unsigned char reg)
>   {
>           vga_io_w (VGA_SEQ_I, reg);
> @@ -348,6 +358,7 @@ static inline void vga_io_wseq (unsigned char reg, unsigned char val)
>           vga_io_w (VGA_SEQ_D, val);
>   #endif /* VGA_OUTW_WRITE */
>   }
> +#endif /* CONFIG_HAS_IOPORT */
>
>   static inline unsigned char vga_mm_rseq (void __iomem *regbase, unsigned char reg)
>   {
> @@ -385,6 +396,7 @@ static inline void vga_wgfx (void __iomem *regbase, unsigned char reg, unsigned
>   #endif /* VGA_OUTW_WRITE */
>   }
>
> +#ifdef CONFIG_HAS_IOPORT
>   static inline unsigned char vga_io_rgfx (unsigned char reg)
>   {
>           vga_io_w (VGA_GFX_I, reg);
> @@ -400,6 +412,7 @@ static inline void vga_io_wgfx (unsigned char reg, unsigned char val)
>           vga_io_w (VGA_GFX_D, val);
>   #endif /* VGA_OUTW_WRITE */
>   }
> +#endif /* CONFIG_HAS_IOPORT */
>
>   static inline unsigned char vga_mm_rgfx (void __iomem *regbase, unsigned char reg)
>   {
> @@ -434,6 +447,7 @@ static inline void vga_wattr (void __iomem *regbase, unsigned char reg, unsigned
>           vga_w (regbase, VGA_ATT_W, val);
>   }
>
> +#ifdef CONFIG_HAS_IOPORT
>   static inline unsigned char vga_io_rattr (unsigned char reg)
>   {
>           vga_io_w (VGA_ATT_IW, reg);
> @@ -445,6 +459,7 @@ static inline void vga_io_wattr (unsigned char reg, unsigned char val)
>           vga_io_w (VGA_ATT_IW, reg);
>           vga_io_w (VGA_ATT_W, val);
>   }
> +#endif /* CONFIG_HAS_IOPORT */
>
>   static inline unsigned char vga_mm_rattr (void __iomem *regbase, unsigned char reg)
>   {