dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/1] video: Handle HAS_IOPORT dependencies
@ 2024-04-10 14:23 Niklas Schnelle
  2024-04-10 14:23 ` [PATCH 1/1] " Niklas Schnelle
  0 siblings, 1 reply; 6+ messages in thread
From: Niklas Schnelle @ 2024-04-10 14:23 UTC (permalink / raw)
  To: Helge Deller
  Cc: linux-fbdev, dri-devel, Arnd Bergmann, Heiko Carstens,
	linux-kernel, Niklas Schnelle

Hi Helge (again ;-)),

This is a follow up in my ongoing effort of making inb()/outb() and
similar I/O port accessors compile-time optional. Previously I sent this
as a treewide series titled "treewide: Remove I/O port accessors for
HAS_IOPORT=n" with the latest being its 5th version[0]. With a significant
subset of patches merged I've changed over to per-subsystem series. These
series are stand alone and should be merged via the relevant tree such
that with all subsystems complete we can follow this up with the final
patch that will make the I/O port accessors compile-time optional.

The current state of the full series with changes to the remaining subsystems
and the aforementioned final patch can be found for your convenience on my
git.kernel.org tree in the has_ioport branch[1]. As for compile-time vs runtime
see Linus' reply to my first attempt[2].

Thanks,
Niklas

[0] https://lore.kernel.org/all/20230522105049.1467313-1-schnelle@linux.ibm.com/
[1] https://git.kernel.org/pub/scm/linux/kernel/git/niks/linux.git/log/?h=has_ioport
[2] https://lore.kernel.org/lkml/CAHk-=wg80je=K7madF4e7WrRNp37e3qh6y10Svhdc7O8SZ_-8g@mail.gmail.com/

Niklas Schnelle (1):
  video: Handle HAS_IOPORT dependencies

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

-- 
2.40.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/1] video: Handle HAS_IOPORT dependencies
  2024-04-10 14:23 [PATCH 0/1] video: Handle HAS_IOPORT dependencies Niklas Schnelle
@ 2024-04-10 14:23 ` Niklas Schnelle
  2024-04-11 14:00   ` Helge Deller
  0 siblings, 1 reply; 6+ messages in thread
From: Niklas Schnelle @ 2024-04-10 14:23 UTC (permalink / raw)
  To: Helge Deller
  Cc: linux-fbdev, dri-devel, Arnd Bergmann, Heiko Carstens,
	linux-kernel, Niklas Schnelle

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(-)

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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] video: Handle HAS_IOPORT dependencies
  2024-04-10 14:23 ` [PATCH 1/1] " Niklas Schnelle
@ 2024-04-11 14:00   ` Helge Deller
  2024-04-22  8:34     ` Niklas Schnelle
  0 siblings, 1 reply; 6+ messages in thread
From: Helge Deller @ 2024-04-11 14:00 UTC (permalink / raw)
  To: Niklas Schnelle
  Cc: Helge Deller, linux-fbdev, dri-devel, Arnd Bergmann,
	Heiko Carstens, linux-kernel

* 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
> 

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] video: Handle HAS_IOPORT dependencies
  2024-04-11 14:00   ` Helge Deller
@ 2024-04-22  8:34     ` Niklas Schnelle
  2024-04-22 19:28       ` Arnd Bergmann
  0 siblings, 1 reply; 6+ messages in thread
From: Niklas Schnelle @ 2024-04-22  8:34 UTC (permalink / raw)
  To: Helge Deller
  Cc: Helge Deller, linux-fbdev, dri-devel, Arnd Bergmann,
	Heiko Carstens, linux-kernel

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

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.

Thanks,
Niklas

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] video: Handle HAS_IOPORT dependencies
  2024-04-22  8:34     ` Niklas Schnelle
@ 2024-04-22 19:28       ` Arnd Bergmann
  2024-04-24 19:29         ` Helge Deller
  0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2024-04-22 19:28 UTC (permalink / raw)
  To: Niklas Schnelle, Helge Deller
  Cc: Helge Deller, linux-fbdev, dri-devel, Heiko Carstens, linux-kernel

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] video: Handle HAS_IOPORT dependencies
  2024-04-22 19:28       ` Arnd Bergmann
@ 2024-04-24 19:29         ` Helge Deller
  0 siblings, 0 replies; 6+ messages in thread
From: Helge Deller @ 2024-04-24 19:29 UTC (permalink / raw)
  To: Arnd Bergmann, Niklas Schnelle, Helge Deller
  Cc: linux-fbdev, dri-devel, Heiko Carstens, linux-kernel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-04-24 19:34 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-10 14:23 [PATCH 0/1] video: Handle HAS_IOPORT dependencies Niklas Schnelle
2024-04-10 14:23 ` [PATCH 1/1] " Niklas Schnelle
2024-04-11 14:00   ` Helge Deller
2024-04-22  8:34     ` Niklas Schnelle
2024-04-22 19:28       ` Arnd Bergmann
2024-04-24 19:29         ` Helge Deller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).