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@xxxxxxxxxxxxx>:
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
