[PATCH v6 0/5] treewide: Remove I/O port accessors for HAS_IOPORT=n

[PATCH v6 0/5] treewide: Remove I/O port accessors for HAS_IOPORT=n

[Niklas Schnelle]

Hi All,

This is a follow up in my long running effort of making inb()/outb() and
similar I/O port accessors compile-time optional. After initially
sending this as a treewide series with the latest revision at[0]
we switched to per subsystem series. Now though as we're left with only
5 patches left I'm going back to a single series with Arnd planning
to take this via the the asm-generic tree.

This series may also be viewed for your convenience on my git.kernel.org
tree[1] under the b4/has_ioport branch. 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
[2] https://lore.kernel.org/lkml/CAHk-=wg80je=K7madF4e7WrRNp37e3qh6y10Svhdc7O8SZ_-8g@mail.gmail.com/

Changes since v5 / per subsystem patches:

drm:
- Add HAS_IOPORT dependency for GMA500
tty: serial:
- Make 8250 PCI driver emit an error message when trying to use devices
  which require I/O ports without CONFIG_HAS_IOPORT (Maciej)
- Use early returns + dead code elimination to skip inb()/outb() uses
  in quirks (Arnd)
- In 8250 PCI driver also handle fintek and moxi quirks
- In 8250 ports code handle um's defined(__i385__) &&
  defined(CONFIG_HAS_IOPORT) case
- Use IS_ENABLED() early return also in is_upf_fourport()
  __always_inline to force constant folding

Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
Niklas Schnelle (5):
      hexagon: Don't select GENERIC_IOMAP without HAS_IOPORT support
      Bluetooth: add HAS_IOPORT dependencies
      drm: handle HAS_IOPORT dependencies
      tty: serial: handle HAS_IOPORT dependencies
      asm-generic/io.h: Remove I/O port accessors for HAS_IOPORT=n

 arch/hexagon/Kconfig                  |  1 -
 drivers/bluetooth/Kconfig             |  6 ++--
 drivers/gpu/drm/gma500/Kconfig        |  2 +-
 drivers/gpu/drm/qxl/Kconfig           |  1 +
 drivers/gpu/drm/tiny/bochs.c          | 17 ++++++++++
 drivers/gpu/drm/tiny/cirrus.c         |  2 ++
 drivers/gpu/drm/xe/Kconfig            |  2 +-
 drivers/tty/Kconfig                   |  4 +--
 drivers/tty/serial/8250/8250_early.c  |  4 +++
 drivers/tty/serial/8250/8250_pci.c    | 49 +++++++++++++++++++++++++++-
 drivers/tty/serial/8250/8250_pcilib.c |  4 +++
 drivers/tty/serial/8250/8250_port.c   | 47 +++++++++++++++++++++------
 drivers/tty/serial/8250/Kconfig       |  4 +--
 drivers/tty/serial/Kconfig            |  2 +-
 include/asm-generic/io.h              | 60 +++++++++++++++++++++++++++++++++++
 15 files changed, 183 insertions(+), 22 deletions(-)
---
base-commit: 8cf0b93919e13d1e8d4466eb4080a4c4d9d66d7b
change-id: 20241004-b4-has_ioport-60ac6ce1deb6

Best regards,
-- 
Niklas Schnelle

[PATCH v6 1/5] hexagon: Don't select GENERIC_IOMAP without HAS_IOPORT support

[Niklas Schnelle]

In a future patch HAS_IOPORT=n will disable inb()/outb() and friends at
compile time. As hexagon does not support I/O port access it also
the GENERIC_IOMAP mechanism of dynamically choosing between I/O port and
MMIO access doesn't work so don't select it.

Reviewed-by: Brian Cain <bcain@quicinc.com>
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>
---
 arch/hexagon/Kconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/hexagon/Kconfig b/arch/hexagon/Kconfig
index e233b5efa2761e35c416cbc147f6b6422a7c5b8f..5ea1bf4b7d4f5471a9c39ee9fb5c701455d21342 100644
--- a/arch/hexagon/Kconfig
+++ b/arch/hexagon/Kconfig
@@ -31,7 +31,6 @@ config HEXAGON
 	select HAVE_ARCH_TRACEHOOK
 	select NEED_SG_DMA_LENGTH
 	select NO_IOPORT_MAP
-	select GENERIC_IOMAP
 	select GENERIC_IOREMAP
 	select GENERIC_SMP_IDLE_THREAD
 	select STACKTRACE_SUPPORT

-- 
2.43.0

[PATCH v6 2/5] Bluetooth: add HAS_IOPORT dependencies

[Niklas Schnelle]

In a future patch HAS_IOPORT=n will disable inb()/outb() and friends at
compile time. We thus need to add HAS_IOPORT as dependency for those
drivers using them.

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>
---
 drivers/bluetooth/Kconfig | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index 18767b54df352e929692850dc789d9377839e094..4ab32abf0f48644715d4c27ec0d2fdaccef62b76 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -336,7 +336,7 @@ config BT_HCIBFUSB
 
 config BT_HCIDTL1
 	tristate "HCI DTL1 (PC Card) driver"
-	depends on PCMCIA
+	depends on PCMCIA && HAS_IOPORT
 	help
 	  Bluetooth HCI DTL1 (PC Card) driver.
 	  This driver provides support for Bluetooth PCMCIA devices with
@@ -349,7 +349,7 @@ config BT_HCIDTL1
 
 config BT_HCIBT3C
 	tristate "HCI BT3C (PC Card) driver"
-	depends on PCMCIA
+	depends on PCMCIA && HAS_IOPORT
 	select FW_LOADER
 	help
 	  Bluetooth HCI BT3C (PC Card) driver.
@@ -363,7 +363,7 @@ config BT_HCIBT3C
 
 config BT_HCIBLUECARD
 	tristate "HCI BlueCard (PC Card) driver"
-	depends on PCMCIA
+	depends on PCMCIA && HAS_IOPORT
 	help
 	  Bluetooth HCI BlueCard (PC Card) driver.
 	  This driver provides support for Bluetooth PCMCIA devices with

-- 
2.43.0

[PATCH v6 3/5] drm: handle HAS_IOPORT dependencies

[Niklas Schnelle]

In a future patch HAS_IOPORT=n will disable inb()/outb() and friends at
compile time. We thus need to add HAS_IOPORT as dependency for those
drivers using them. In the bochs driver there is optional MMIO support
detected at runtime, warn if this isn't taken when HAS_IOPORT is not
defined.

There is also a direct and hard coded use in cirrus.c which according to
the comment is only necessary during resume.  Let's just skip this as
for example s390 which doesn't have I/O port support also doesen't
support suspend/resume.

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>
---
 drivers/gpu/drm/gma500/Kconfig |  2 +-
 drivers/gpu/drm/qxl/Kconfig    |  1 +
 drivers/gpu/drm/tiny/bochs.c   | 17 +++++++++++++++++
 drivers/gpu/drm/tiny/cirrus.c  |  2 ++
 drivers/gpu/drm/xe/Kconfig     |  2 +-
 5 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/gma500/Kconfig b/drivers/gpu/drm/gma500/Kconfig
index efb4a2dd2f80885cb59c925d09401002278d7d61..23b7c14de5e29238ece939d5822d8a9ffc4675cc 100644
--- a/drivers/gpu/drm/gma500/Kconfig
+++ b/drivers/gpu/drm/gma500/Kconfig
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 config DRM_GMA500
 	tristate "Intel GMA500/600/3600/3650 KMS Framebuffer"
-	depends on DRM && PCI && X86 && MMU
+	depends on DRM && PCI && X86 && MMU && HAS_IOPORT
 	select DRM_KMS_HELPER
 	select FB_IOMEM_HELPERS if DRM_FBDEV_EMULATION
 	select I2C
diff --git a/drivers/gpu/drm/qxl/Kconfig b/drivers/gpu/drm/qxl/Kconfig
index ca3f51c2a8fe1a383f8a2479f04b5c0b3fb14e44..d0e0d440c8d96564cb7b8ffd2385c44fc43f873d 100644
--- a/drivers/gpu/drm/qxl/Kconfig
+++ b/drivers/gpu/drm/qxl/Kconfig
@@ -2,6 +2,7 @@
 config DRM_QXL
 	tristate "QXL virtual GPU"
 	depends on DRM && PCI && MMU
+	depends on HAS_IOPORT
 	select DRM_KMS_HELPER
 	select DRM_TTM
 	select DRM_TTM_HELPER
diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
index 31fc5d839e106ea4d5c8fe42d1bfc3c70291e3fb..0ed78d3d5774778f91de972ac27056938036e722 100644
--- a/drivers/gpu/drm/tiny/bochs.c
+++ b/drivers/gpu/drm/tiny/bochs.c
@@ -2,6 +2,7 @@
 
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/bug.h>
 
 #include <drm/drm_aperture.h>
 #include <drm/drm_atomic_helper.h>
@@ -105,7 +106,9 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val)
 
 		writeb(val, bochs->mmio + offset);
 	} else {
+#ifdef CONFIG_HAS_IOPORT
 		outb(val, ioport);
+#endif
 	}
 }
 
@@ -119,7 +122,11 @@ static u8 bochs_vga_readb(struct bochs_device *bochs, u16 ioport)
 
 		return readb(bochs->mmio + offset);
 	} else {
+#ifdef CONFIG_HAS_IOPORT
 		return inb(ioport);
+#else
+		return 0xff;
+#endif
 	}
 }
 
@@ -132,8 +139,12 @@ static u16 bochs_dispi_read(struct bochs_device *bochs, u16 reg)
 
 		ret = readw(bochs->mmio + offset);
 	} else {
+#ifdef CONFIG_HAS_IOPORT
 		outw(reg, VBE_DISPI_IOPORT_INDEX);
 		ret = inw(VBE_DISPI_IOPORT_DATA);
+#else
+		ret = 0xffff;
+#endif
 	}
 	return ret;
 }
@@ -145,8 +156,10 @@ static void bochs_dispi_write(struct bochs_device *bochs, u16 reg, u16 val)
 
 		writew(val, bochs->mmio + offset);
 	} else {
+#ifdef CONFIG_HAS_IOPORT
 		outw(reg, VBE_DISPI_IOPORT_INDEX);
 		outw(val, VBE_DISPI_IOPORT_DATA);
+#endif
 	}
 }
 
@@ -229,6 +242,10 @@ static int bochs_hw_init(struct drm_device *dev)
 			return -ENOMEM;
 		}
 	} else {
+		if (!IS_ENABLED(CONFIG_HAS_IOPORT)) {
+			DRM_ERROR("I/O ports are not supported\n");
+			return -EIO;
+		}
 		ioaddr = VBE_DISPI_IOPORT_INDEX;
 		iosize = 2;
 		if (!request_region(ioaddr, iosize, "bochs-drm")) {
diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c
index 751326e3d9c374baf72115492aeefff2b73869f0..e31e1df029ab0272c4a1ff0ab3eb026ca679b560 100644
--- a/drivers/gpu/drm/tiny/cirrus.c
+++ b/drivers/gpu/drm/tiny/cirrus.c
@@ -509,8 +509,10 @@ static void cirrus_crtc_helper_atomic_enable(struct drm_crtc *crtc,
 
 	cirrus_mode_set(cirrus, &crtc_state->mode);
 
+#ifdef CONFIG_HAS_IOPORT
 	/* Unblank (needed on S3 resume, vgabios doesn't do it then) */
 	outb(VGA_AR_ENABLE_DISPLAY, VGA_ATT_W);
+#endif
 
 	drm_dev_exit(idx);
 }
diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
index 7bbe46a98ff1f449bc2af30686585a00e9e8af93..116f58774135fc3a9f37d6d72d41340f5c812297 100644
--- a/drivers/gpu/drm/xe/Kconfig
+++ b/drivers/gpu/drm/xe/Kconfig
@@ -49,7 +49,7 @@ config DRM_XE
 
 config DRM_XE_DISPLAY
 	bool "Enable display support"
-	depends on DRM_XE && DRM_XE=m
+	depends on DRM_XE && DRM_XE=m && HAS_IOPORT
 	select FB_IOMEM_HELPERS
 	select I2C
 	select I2C_ALGOBIT

-- 
2.43.0

[PATCH v6 4/5] tty: serial: handle HAS_IOPORT dependencies

[Niklas Schnelle]

In a future patch HAS_IOPORT=n will disable inb()/outb() and friends at
compile time. We thus need to add HAS_IOPORT as dependency for those
drivers using them unconditionally. Some 8250 serial drivers support
MMIO only use, so fence only the parts requiring I/O ports and print an
error message if a device can't be supported with the current
configuration.

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>
---
 drivers/tty/Kconfig                   |  4 +--
 drivers/tty/serial/8250/8250_early.c  |  4 +++
 drivers/tty/serial/8250/8250_pci.c    | 49 ++++++++++++++++++++++++++++++++++-
 drivers/tty/serial/8250/8250_pcilib.c |  4 +++
 drivers/tty/serial/8250/8250_port.c   | 47 ++++++++++++++++++++++++++-------
 drivers/tty/serial/8250/Kconfig       |  4 +--
 drivers/tty/serial/Kconfig            |  2 +-
 7 files changed, 98 insertions(+), 16 deletions(-)

diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
index a45d423ad10f02c3a818021bbb18655a8b690500..63a494d36a1fdceba5a7b39f4516060e48af0cc6 100644
--- a/drivers/tty/Kconfig
+++ b/drivers/tty/Kconfig
@@ -220,7 +220,7 @@ config MOXA_INTELLIO
 
 config MOXA_SMARTIO
 	tristate "Moxa SmartIO support v. 2.0"
-	depends on SERIAL_NONSTANDARD && PCI
+	depends on SERIAL_NONSTANDARD && PCI && HAS_IOPORT
 	help
 	  Say Y here if you have a Moxa SmartIO multiport serial card and/or
 	  want to help develop a new version of this driver.
@@ -302,7 +302,7 @@ config GOLDFISH_TTY_EARLY_CONSOLE
 
 config IPWIRELESS
 	tristate "IPWireless 3G UMTS PCMCIA card support"
-	depends on PCMCIA && NETDEVICES
+	depends on PCMCIA && NETDEVICES && HAS_IOPORT
 	select PPP
 	help
 	  This is a driver for 3G UMTS PCMCIA card from IPWireless company. In
diff --git a/drivers/tty/serial/8250/8250_early.c b/drivers/tty/serial/8250/8250_early.c
index 6176083d0341ca10edebe5c4eebfffc922a61472..84242292176570cd2c92afbd4755d303827a4abc 100644
--- a/drivers/tty/serial/8250/8250_early.c
+++ b/drivers/tty/serial/8250/8250_early.c
@@ -46,8 +46,10 @@ static unsigned int serial8250_early_in(struct uart_port *port, int offset)
 		return readl(port->membase + offset);
 	case UPIO_MEM32BE:
 		return ioread32be(port->membase + offset);
+#ifdef CONFIG_HAS_IOPORT
 	case UPIO_PORT:
 		return inb(port->iobase + offset);
+#endif
 	default:
 		return 0;
 	}
@@ -70,9 +72,11 @@ static void serial8250_early_out(struct uart_port *port, int offset, int value)
 	case UPIO_MEM32BE:
 		iowrite32be(value, port->membase + offset);
 		break;
+#ifdef CONFIG_HAS_IOPORT
 	case UPIO_PORT:
 		outb(value, port->iobase + offset);
 		break;
+#endif
 	}
 }
 
diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index 6709b6a5f3011db38acc58dc7223158fe4fcf72e..6a638feb44e443a1998980dd037748f227ec1bc8 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -156,6 +156,14 @@ static const struct pci_device_id pci_use_msi[] = {
 static int pci_default_setup(struct serial_private*,
 	  const struct pciserial_board*, struct uart_8250_port *, int);
 
+static int serial_8250_need_ioport(struct pci_dev *dev)
+{
+	dev_warn(&dev->dev,
+		 "Serial port not supported because of missing I/O resource\n");
+
+	return -ENXIO;
+}
+
 static void moan_device(const char *str, struct pci_dev *dev)
 {
 	pci_err(dev, "%s\n"
@@ -964,6 +972,9 @@ static int pci_ite887x_init(struct pci_dev *dev)
 	struct resource *iobase = NULL;
 	u32 miscr, uartbar, ioport;
 
+	if (!IS_ENABLED(CONFIG_HAS_IOPORT))
+		return serial_8250_need_ioport(dev);
+
 	/* search for the base-ioport */
 	for (i = 0; i < ARRAY_SIZE(inta_addr); i++) {
 		iobase = request_region(inta_addr[i], ITE_887x_IOSIZE,
@@ -1514,6 +1525,9 @@ static int pci_quatech_init(struct pci_dev *dev)
 	const struct pci_device_id *match;
 	bool amcc = false;
 
+	if (!IS_ENABLED(CONFIG_HAS_IOPORT))
+		return serial_8250_need_ioport(dev);
+
 	match = pci_match_id(quatech_cards, dev);
 	if (match)
 		amcc = match->driver_data;
@@ -1538,6 +1552,9 @@ static int pci_quatech_setup(struct serial_private *priv,
 		  const struct pciserial_board *board,
 		  struct uart_8250_port *port, int idx)
 {
+	if (!IS_ENABLED(CONFIG_HAS_IOPORT))
+		return serial_8250_need_ioport(priv->dev);
+
 	/* Needed by pci_quatech calls below */
 	port->port.iobase = pci_resource_start(priv->dev, FL_GET_BASE(board->flags));
 	/* Set up the clocking */
@@ -1655,6 +1672,9 @@ static int pci_fintek_setup(struct serial_private *priv,
 	u8 config_base;
 	u16 iobase;
 
+	if (!IS_ENABLED(CONFIG_HAS_IOPORT))
+		return serial_8250_need_ioport(pdev);
+
 	config_base = 0x40 + 0x08 * idx;
 
 	/* Get the io address from configuration space */
@@ -1686,6 +1706,9 @@ static int pci_fintek_init(struct pci_dev *dev)
 	u8 config_base;
 	struct serial_private *priv = pci_get_drvdata(dev);
 
+	if (!IS_ENABLED(CONFIG_HAS_IOPORT))
+		return serial_8250_need_ioport(dev);
+
 	if (!(pci_resource_flags(dev, 5) & IORESOURCE_IO) ||
 			!(pci_resource_flags(dev, 4) & IORESOURCE_IO) ||
 			!(pci_resource_flags(dev, 3) & IORESOURCE_IO))
@@ -1864,6 +1887,9 @@ static int kt_serial_setup(struct serial_private *priv,
 			   const struct pciserial_board *board,
 			   struct uart_8250_port *port, int idx)
 {
+	if (!IS_ENABLED(CONFIG_HAS_IOPORT))
+		return serial_8250_need_ioport(priv->dev);
+
 	port->port.flags |= UPF_BUG_THRE;
 	port->port.serial_in = kt_serial_in;
 	port->port.handle_break = kt_handle_break;
@@ -1884,6 +1910,9 @@ pci_wch_ch353_setup(struct serial_private *priv,
 		    const struct pciserial_board *board,
 		    struct uart_8250_port *port, int idx)
 {
+	if (!IS_ENABLED(CONFIG_HAS_IOPORT))
+		return serial_8250_need_ioport(priv->dev);
+
 	port->port.flags |= UPF_FIXED_TYPE;
 	port->port.type = PORT_16550A;
 	return pci_default_setup(priv, board, port, idx);
@@ -1894,6 +1923,9 @@ pci_wch_ch355_setup(struct serial_private *priv,
 		const struct pciserial_board *board,
 		struct uart_8250_port *port, int idx)
 {
+	if (!IS_ENABLED(CONFIG_HAS_IOPORT))
+		return serial_8250_need_ioport(priv->dev);
+
 	port->port.flags |= UPF_FIXED_TYPE;
 	port->port.type = PORT_16550A;
 	return pci_default_setup(priv, board, port, idx);
@@ -1904,6 +1936,9 @@ pci_wch_ch38x_setup(struct serial_private *priv,
 		    const struct pciserial_board *board,
 		    struct uart_8250_port *port, int idx)
 {
+	if (!IS_ENABLED(CONFIG_HAS_IOPORT))
+		return serial_8250_need_ioport(priv->dev);
+
 	port->port.flags |= UPF_FIXED_TYPE;
 	port->port.type = PORT_16850;
 	return pci_default_setup(priv, board, port, idx);
@@ -1918,6 +1953,8 @@ static int pci_wch_ch38x_init(struct pci_dev *dev)
 	int max_port;
 	unsigned long iobase;
 
+	if (!IS_ENABLED(CONFIG_HAS_IOPORT))
+		return serial_8250_need_ioport(dev);
 
 	switch (dev->device) {
 	case 0x3853: /* 8 ports */
@@ -1937,11 +1974,15 @@ static void pci_wch_ch38x_exit(struct pci_dev *dev)
 {
 	unsigned long iobase;
 
+	if (!IS_ENABLED(CONFIG_HAS_IOPORT)) {
+		serial_8250_need_ioport(dev);
+		return;
+	}
+
 	iobase = pci_resource_start(dev, 0);
 	outb(0x0, iobase + CH384_XINT_ENABLE_REG);
 }
 
-
 static int
 pci_sunix_setup(struct serial_private *priv,
 		const struct pciserial_board *board,
@@ -2052,6 +2093,9 @@ static int pci_moxa_init(struct pci_dev *dev)
 	unsigned int i, num_ports = moxa_get_nports(device);
 	u8 val, init_mode = MOXA_RS232;
 
+	if (!IS_ENABLED(CONFIG_HAS_IOPORT))
+		return serial_8250_need_ioport(dev);
+
 	if (!(pci_moxa_supported_rs(dev) & MOXA_SUPP_RS232)) {
 		init_mode = MOXA_RS422;
 	}
@@ -2084,6 +2128,9 @@ pci_moxa_setup(struct serial_private *priv,
 	unsigned int bar = FL_GET_BASE(board->flags);
 	int offset;
 
+	if (!IS_ENABLED(CONFIG_HAS_IOPORT))
+		return serial_8250_need_ioport(priv->dev);
+
 	if (board->num_ports == 4 && idx == 3)
 		offset = 7 * board->uart_offset;
 	else
diff --git a/drivers/tty/serial/8250/8250_pcilib.c b/drivers/tty/serial/8250/8250_pcilib.c
index ea906d721b2c3eac15c9e8d62cc6fa56c3ef6150..fc1882d7515b5814ff1240ffdbe1009ab908ad6b 100644
--- a/drivers/tty/serial/8250/8250_pcilib.c
+++ b/drivers/tty/serial/8250/8250_pcilib.c
@@ -28,6 +28,10 @@ int serial8250_pci_setup_port(struct pci_dev *dev, struct uart_8250_port *port,
 		port->port.membase = pcim_iomap_table(dev)[bar] + offset;
 		port->port.regshift = regshift;
 	} else {
+		if (!IS_ENABLED(CONFIG_HAS_IOPORT)) {
+			pr_err("Serial port %lx requires I/O port support\n", port->port.iobase);
+			return -EINVAL;
+		}
 		port->port.iotype = UPIO_PORT;
 		port->port.iobase = pci_resource_start(dev, bar) + offset;
 		port->port.mapbase = 0;
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 3509af7dc52b8816f2f5ab58f0d999696f7483e7..74717675dc41db6a5b5b0dd1da84b4ea23bd9928 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -338,6 +338,7 @@ static void default_serial_dl_write(struct uart_8250_port *up, u32 value)
 	serial_out(up, UART_DLM, value >> 8 & 0xff);
 }
 
+#ifdef CONFIG_HAS_IOPORT
 static unsigned int hub6_serial_in(struct uart_port *p, int offset)
 {
 	offset = offset << p->regshift;
@@ -351,6 +352,7 @@ static void hub6_serial_out(struct uart_port *p, int offset, int value)
 	outb(p->hub6 - 1 + offset, p->iobase);
 	outb(value, p->iobase + 1);
 }
+#endif /* CONFIG_HAS_IOPORT */
 
 static unsigned int mem_serial_in(struct uart_port *p, int offset)
 {
@@ -400,6 +402,7 @@ static unsigned int mem32be_serial_in(struct uart_port *p, int offset)
 	return ioread32be(p->membase + offset);
 }
 
+#ifdef CONFIG_HAS_IOPORT
 static unsigned int io_serial_in(struct uart_port *p, int offset)
 {
 	offset = offset << p->regshift;
@@ -411,6 +414,23 @@ static void io_serial_out(struct uart_port *p, int offset, int value)
 	offset = offset << p->regshift;
 	outb(value, p->iobase + offset);
 }
+#endif
+static unsigned int no_serial_in(struct uart_port *p, int offset)
+{
+	return (unsigned int)-1;
+}
+
+static void no_serial_out(struct uart_port *p, int offset, int value)
+{
+}
+
+static __always_inline bool is_upf_fourport(struct uart_port *port)
+{
+	if (!IS_ENABLED(CONFIG_HAS_IOPORT))
+		return false;
+
+	return port->flags & UPF_FOURPORT;
+}
 
 static int serial8250_default_handle_irq(struct uart_port *port);
 
@@ -422,10 +442,12 @@ static void set_io_from_upio(struct uart_port *p)
 	up->dl_write = default_serial_dl_write;
 
 	switch (p->iotype) {
+#ifdef CONFIG_HAS_IOPORT
 	case UPIO_HUB6:
 		p->serial_in = hub6_serial_in;
 		p->serial_out = hub6_serial_out;
 		break;
+#endif
 
 	case UPIO_MEM:
 		p->serial_in = mem_serial_in;
@@ -446,11 +468,16 @@ static void set_io_from_upio(struct uart_port *p)
 		p->serial_in = mem32be_serial_in;
 		p->serial_out = mem32be_serial_out;
 		break;
-
-	default:
+#ifdef CONFIG_HAS_IOPORT
+	case UPIO_PORT:
 		p->serial_in = io_serial_in;
 		p->serial_out = io_serial_out;
 		break;
+#endif
+	default:
+		WARN(1, "Unsupported UART type %x\n", p->iotype);
+		p->serial_in = no_serial_in;
+		p->serial_out = no_serial_out;
 	}
 	/* Remember loaded iotype */
 	up->cur_iotype = p->iotype;
@@ -1174,7 +1201,7 @@ static void autoconfig(struct uart_8250_port *up)
 		 */
 		scratch = serial_in(up, UART_IER);
 		serial_out(up, UART_IER, 0);
-#ifdef __i386__
+#if defined(__i386__) && defined(CONFIG_HAS_IOPORT)
 		outb(0xff, 0x080);
 #endif
 		/*
@@ -1183,7 +1210,7 @@ static void autoconfig(struct uart_8250_port *up)
 		 */
 		scratch2 = serial_in(up, UART_IER) & UART_IER_ALL_INTR;
 		serial_out(up, UART_IER, UART_IER_ALL_INTR);
-#ifdef __i386__
+#if defined(__i386__) && defined(CONFIG_HAS_IOPORT)
 		outb(0, 0x080);
 #endif
 		scratch3 = serial_in(up, UART_IER) & UART_IER_ALL_INTR;
@@ -1306,12 +1333,12 @@ static void autoconfig_irq(struct uart_8250_port *up)
 {
 	struct uart_port *port = &up->port;
 	unsigned char save_mcr, save_ier;
+	unsigned long irqs;
 	unsigned char save_ICP = 0;
 	unsigned int ICP = 0;
-	unsigned long irqs;
 	int irq;
 
-	if (port->flags & UPF_FOURPORT) {
+	if (is_upf_fourport(port)) {
 		ICP = (port->iobase & 0xfe0) | 0x1f;
 		save_ICP = inb_p(ICP);
 		outb_p(0x80, ICP);
@@ -1330,7 +1357,7 @@ static void autoconfig_irq(struct uart_8250_port *up)
 	irqs = probe_irq_on();
 	serial8250_out_MCR(up, 0);
 	udelay(10);
-	if (port->flags & UPF_FOURPORT) {
+	if (is_upf_fourport(port)) {
 		serial8250_out_MCR(up, UART_MCR_DTR | UART_MCR_RTS);
 	} else {
 		serial8250_out_MCR(up,
@@ -1354,7 +1381,7 @@ static void autoconfig_irq(struct uart_8250_port *up)
 	serial_out(up, UART_IER, save_ier);
 	uart_port_unlock_irq(port);
 
-	if (port->flags & UPF_FOURPORT)
+	if (is_upf_fourport(port))
 		outb_p(save_ICP, ICP);
 
 	port->irq = (irq > 0) ? irq : 0;
@@ -2435,7 +2462,7 @@ int serial8250_do_startup(struct uart_port *port)
 	 */
 	up->ier = UART_IER_RLSI | UART_IER_RDI;
 
-	if (port->flags & UPF_FOURPORT) {
+	if (is_upf_fourport(port)) {
 		unsigned int icp;
 		/*
 		 * Enable interrupts on the AST Fourport board
@@ -2480,7 +2507,7 @@ void serial8250_do_shutdown(struct uart_port *port)
 		serial8250_release_dma(up);
 
 	uart_port_lock_irqsave(port, &flags);
-	if (port->flags & UPF_FOURPORT) {
+	if (is_upf_fourport(port)) {
 		/* reset interrupts on the AST Fourport board */
 		inb((port->iobase & 0xfe0) | 0x1f);
 		port->mctrl |= TIOCM_OUT1;
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 47ff50763c048c89b19b7c9f13f462bc5368ab43..1eb20350fcf432d41cb416bed3be72ed8ab129bb 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -72,7 +72,7 @@ config SERIAL_8250_16550A_VARIANTS
 
 config SERIAL_8250_FINTEK
 	bool "Support for Fintek variants"
-	depends on SERIAL_8250
+	depends on SERIAL_8250 && HAS_IOPORT
 	help
 	  Selecting this option will add support for the RS232 and RS485
 	  capabilities of the Fintek F81216A LPC to 4 UART as well similar
@@ -163,7 +163,7 @@ config SERIAL_8250_HP300
 
 config SERIAL_8250_CS
 	tristate "8250/16550 PCMCIA device support"
-	depends on PCMCIA && SERIAL_8250
+	depends on PCMCIA && SERIAL_8250 && HAS_IOPORT
 	help
 	  Say Y here to enable support for 16-bit PCMCIA serial devices,
 	  including serial port cards, modems, and the modem functions of
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 28e4beeabf8f373fc76e09ea7d1c9d55a66f4964..45f0f779fbf960e9fa66375dbc44c379edc63bef 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -877,7 +877,7 @@ config SERIAL_TXX9_STDSERIAL
 
 config SERIAL_JSM
 	tristate "Digi International NEO and Classic PCI Support"
-	depends on PCI
+	depends on PCI && HAS_IOPORT
 	select SERIAL_CORE
 	help
 	  This is a driver for Digi International's Neo and Classic series

-- 
2.43.0

[PATCH v6 5/5] asm-generic/io.h: Remove I/O port accessors for HAS_IOPORT=n

[Niklas Schnelle]

With all subsystems and drivers either declaring their dependence on
HAS_IOPORT or fencing I/O port specific code sections we can finally
make inb()/outb() and friends compile-time dependent on HAS_IOPORT as
suggested by Linus in the linked mail. The main benefit of this is that
on platforms such as s390 which have no meaningful way of implementing
inb()/outb() their use without the proper HAS_IOPORT dependency will
result in easy to catch and fix compile-time errors instead of compiling
code that can never work.

Link: https://lore.kernel.org/lkml/CAHk-=wg80je=K7madF4e7WrRNp37e3qh6y10Svhdc7O8SZ_-8g@mail.gmail.com/
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>
---
 include/asm-generic/io.h | 60 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 80de699bf6af4b7b77f582c0469c43e978f67a43..1027be6a62bcbcd5f6797e0fa42035208d0ca79f 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -540,6 +540,7 @@ static inline void writesq(volatile void __iomem *addr, const void *buffer,
 
 #if !defined(inb) && !defined(_inb)
 #define _inb _inb
+#ifdef CONFIG_HAS_IOPORT
 static inline u8 _inb(unsigned long addr)
 {
 	u8 val;
@@ -549,10 +550,15 @@ static inline u8 _inb(unsigned long addr)
 	__io_par(val);
 	return val;
 }
+#else
+u8 _inb(unsigned long addr)
+	__compiletime_error("inb()) requires CONFIG_HAS_IOPORT");
+#endif
 #endif
 
 #if !defined(inw) && !defined(_inw)
 #define _inw _inw
+#ifdef CONFIG_HAS_IOPORT
 static inline u16 _inw(unsigned long addr)
 {
 	u16 val;
@@ -562,10 +568,15 @@ static inline u16 _inw(unsigned long addr)
 	__io_par(val);
 	return val;
 }
+#else
+u16 _inw(unsigned long addr)
+	__compiletime_error("inw() requires CONFIG_HAS_IOPORT");
+#endif
 #endif
 
 #if !defined(inl) && !defined(_inl)
 #define _inl _inl
+#ifdef CONFIG_HAS_IOPORT
 static inline u32 _inl(unsigned long addr)
 {
 	u32 val;
@@ -575,36 +586,55 @@ static inline u32 _inl(unsigned long addr)
 	__io_par(val);
 	return val;
 }
+#else
+u32 _inl(unsigned long addr)
+	__compiletime_error("inl() requires CONFIG_HAS_IOPORT");
+#endif
 #endif
 
 #if !defined(outb) && !defined(_outb)
 #define _outb _outb
+#ifdef CONFIG_HAS_IOPORT
 static inline void _outb(u8 value, unsigned long addr)
 {
 	__io_pbw();
 	__raw_writeb(value, PCI_IOBASE + addr);
 	__io_paw();
 }
+#else
+void _outb(u8 value, unsigned long addr)
+	__compiletime_error("outb() requires CONFIG_HAS_IOPORT");
+#endif
 #endif
 
 #if !defined(outw) && !defined(_outw)
 #define _outw _outw
+#ifdef CONFIG_HAS_IOPORT
 static inline void _outw(u16 value, unsigned long addr)
 {
 	__io_pbw();
 	__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
 	__io_paw();
 }
+#else
+void _outw(u16 value, unsigned long addr)
+	__compiletime_error("outw() requires CONFIG_HAS_IOPORT");
+#endif
 #endif
 
 #if !defined(outl) && !defined(_outl)
 #define _outl _outl
+#ifdef CONFIG_HAS_IOPORT
 static inline void _outl(u32 value, unsigned long addr)
 {
 	__io_pbw();
 	__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
 	__io_paw();
 }
+#else
+void _outl(u32 value, unsigned long addr)
+	__compiletime_error("outl() requires CONFIG_HAS_IOPORT");
+#endif
 #endif
 
 #include <linux/logic_pio.h>
@@ -688,53 +718,83 @@ static inline void outl_p(u32 value, unsigned long addr)
 
 #ifndef insb
 #define insb insb
+#ifdef CONFIG_HAS_IOPORT
 static inline void insb(unsigned long addr, void *buffer, unsigned int count)
 {
 	readsb(PCI_IOBASE + addr, buffer, count);
 }
+#else
+void insb(unsigned long addr, void *buffer, unsigned int count)
+	__compiletime_error("insb() requires HAS_IOPORT");
+#endif
 #endif
 
 #ifndef insw
 #define insw insw
+#ifdef CONFIG_HAS_IOPORT
 static inline void insw(unsigned long addr, void *buffer, unsigned int count)
 {
 	readsw(PCI_IOBASE + addr, buffer, count);
 }
+#else
+void insw(unsigned long addr, void *buffer, unsigned int count)
+	__compiletime_error("insw() requires HAS_IOPORT");
+#endif
 #endif
 
 #ifndef insl
 #define insl insl
+#ifdef CONFIG_HAS_IOPORT
 static inline void insl(unsigned long addr, void *buffer, unsigned int count)
 {
 	readsl(PCI_IOBASE + addr, buffer, count);
 }
+#else
+void insl(unsigned long addr, void *buffer, unsigned int count)
+	__compiletime_error("insl() requires HAS_IOPORT");
+#endif
 #endif
 
 #ifndef outsb
 #define outsb outsb
+#ifdef CONFIG_HAS_IOPORT
 static inline void outsb(unsigned long addr, const void *buffer,
 			 unsigned int count)
 {
 	writesb(PCI_IOBASE + addr, buffer, count);
 }
+#else
+void outsb(unsigned long addr, const void *buffer, unsigned int count)
+	__compiletime_error("outsb() requires HAS_IOPORT");
+#endif
 #endif
 
 #ifndef outsw
 #define outsw outsw
+#ifdef CONFIG_HAS_IOPORT
 static inline void outsw(unsigned long addr, const void *buffer,
 			 unsigned int count)
 {
 	writesw(PCI_IOBASE + addr, buffer, count);
 }
+#else
+void outsw(unsigned long addr, const void *buffer, unsigned int count)
+	__compiletime_error("outsw() requires HAS_IOPORT");
+#endif
 #endif
 
 #ifndef outsl
 #define outsl outsl
+#ifdef CONFIG_HAS_IOPORT
 static inline void outsl(unsigned long addr, const void *buffer,
 			 unsigned int count)
 {
 	writesl(PCI_IOBASE + addr, buffer, count);
 }
+#else
+void outsl(unsigned long addr, const void *buffer, unsigned int count)
+	__compiletime_error("outsl() requires HAS_IOPORT");
+#endif
 #endif
 
 #ifndef insb_p

-- 
2.43.0

Re: [PATCH v6 0/5] treewide: Remove I/O port accessors for HAS_IOPORT=n

[Arnd Bergmann]

On Mon, Oct 7, 2024, at 11:40, Niklas Schnelle wrote:
> Hi All,
>
> This is a follow up in my long running effort of making inb()/outb() and
> similar I/O port accessors compile-time optional. After initially
> sending this as a treewide series with the latest revision at[0]
> we switched to per subsystem series. Now though as we're left with only
> 5 patches left I'm going back to a single series with Arnd planning
> to take this via the the asm-generic tree.
>
> This series may also be viewed for your convenience on my git.kernel.org
> tree[1] under the b4/has_ioport branch. As for compile-time vs runtime
> see Linus' reply to my first attempt[2].

This all looks good to me and I'd like to merge it all in the
asm-generic tree in the next few days, assuming nobody has any
further objections.

If something breaks, we can fix it with a patch on top.

I have a few more patches make it possible to build arm64 kernels
without CONFIG_HAS_IOPORT, but we don't need them as part of your
series and can merge them through driver trees independently.

     Arnd

Re: [PATCH v6 3/5] drm: handle HAS_IOPORT dependencies

[Lucas De Marchi]

On Mon, Oct 07, 2024 at 01:40:21PM +0200, Niklas Schnelle wrote:
>In a future patch HAS_IOPORT=n will disable inb()/outb() and friends at
>compile time. We thus need to add HAS_IOPORT as dependency for those
>drivers using them. In the bochs driver there is optional MMIO support
>detected at runtime, warn if this isn't taken when HAS_IOPORT is not
>defined.
>
>There is also a direct and hard coded use in cirrus.c which according to
>the comment is only necessary during resume.  Let's just skip this as
>for example s390 which doesn't have I/O port support also doesen't
>support suspend/resume.
>
>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>
>---
> drivers/gpu/drm/gma500/Kconfig |  2 +-
> drivers/gpu/drm/qxl/Kconfig    |  1 +
> drivers/gpu/drm/tiny/bochs.c   | 17 +++++++++++++++++
> drivers/gpu/drm/tiny/cirrus.c  |  2 ++
> drivers/gpu/drm/xe/Kconfig     |  2 +-

as an example:
$ git grep -lw outb -- drivers/gpu/drm/
drivers/gpu/drm/gma500/cdv_device.c
drivers/gpu/drm/i915/display/intel_vga.c
drivers/gpu/drm/qxl/qxl_cmd.c
drivers/gpu/drm/qxl/qxl_irq.c
drivers/gpu/drm/tiny/bochs.c
drivers/gpu/drm/tiny/cirrus.c

you are adding the dependency on xe, but why are you keeping i915 out?
What approach did you use to determine the dependency?

Lucas De Marchi

Re: [PATCH v6 3/5] drm: handle HAS_IOPORT dependencies

[Arnd Bergmann]

On Mon, Oct 7, 2024, at 14:39, Lucas De Marchi wrote:
> as an example:
> $ git grep -lw outb -- drivers/gpu/drm/
> drivers/gpu/drm/gma500/cdv_device.c
> drivers/gpu/drm/i915/display/intel_vga.c
> drivers/gpu/drm/qxl/qxl_cmd.c
> drivers/gpu/drm/qxl/qxl_irq.c
> drivers/gpu/drm/tiny/bochs.c
> drivers/gpu/drm/tiny/cirrus.c
>
> you are adding the dependency on xe, but why are you keeping i915 out?
> What approach did you use to determine the dependency?

I did a lot of 'randconfig' build testing on earlier versions
(and this version) of the series, which eventually catches
all of them. The i915 driver depends on CONfIG_X86 since it
is only used in Intel PC chipsets that already rely on PIO.

The XE driver is also used for add-on cards, so the drivers
can be built on all architectures including those that do
not support PCI I/O space access. Adding the dependency on
i915 as well wouldn't be wrong, but is not required for
correctness.

I also sent a patch for vmwgfx, which can be used on arm64.
arm64 currently always sets HAS_IOPORT, so my patch is not
required as a dependency for [PATCH v6 5/5], but we eventually
want this so HAS_IOPORT can become optional on arm64.

      Arnd

Re: [PATCH v6 3/5] drm: handle HAS_IOPORT dependencies

[Lucas De Marchi]

On Mon, Oct 07, 2024 at 02:50:11PM +0000, Arnd Bergmann wrote:
>On Mon, Oct 7, 2024, at 14:39, Lucas De Marchi wrote:
>> as an example:
>> $ git grep -lw outb -- drivers/gpu/drm/
>> drivers/gpu/drm/gma500/cdv_device.c
>> drivers/gpu/drm/i915/display/intel_vga.c
>> drivers/gpu/drm/qxl/qxl_cmd.c
>> drivers/gpu/drm/qxl/qxl_irq.c
>> drivers/gpu/drm/tiny/bochs.c
>> drivers/gpu/drm/tiny/cirrus.c
>>
>> you are adding the dependency on xe, but why are you keeping i915 out?
>> What approach did you use to determine the dependency?
>
>I did a lot of 'randconfig' build testing on earlier versions
>(and this version) of the series, which eventually catches
>all of them. The i915 driver depends on CONfIG_X86 since it
>is only used in Intel PC chipsets that already rely on PIO.
>
>The XE driver is also used for add-on cards, so the drivers
>can be built on all architectures including those that do
>not support PCI I/O space access. Adding the dependency on
>i915 as well wouldn't be wrong, but is not required for
>correctness.

ok, makes sense. I missed the indirect dependency already present in
i915. Thanks.

For the xe part,

Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>

Lucas De Marchi

>
>I also sent a patch for vmwgfx, which can be used on arm64.
>arm64 currently always sets HAS_IOPORT, so my patch is not
>required as a dependency for [PATCH v6 5/5], but we eventually
>want this so HAS_IOPORT can become optional on arm64.
>
>      Arnd

Re: [PATCH v6 4/5] tty: serial: handle HAS_IOPORT dependencies

[Maciej W. Rozycki]

On Mon, 7 Oct 2024, Niklas Schnelle wrote:

> diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
> index 6709b6a5f3011db38acc58dc7223158fe4fcf72e..6a638feb44e443a1998980dd037748f227ec1bc8 100644
> --- a/drivers/tty/serial/8250/8250_pci.c
> +++ b/drivers/tty/serial/8250/8250_pci.c
[...]
>  	iobase = pci_resource_start(dev, 0);
>  	outb(0x0, iobase + CH384_XINT_ENABLE_REG);
>  }
>  
> -
>  static int
>  pci_sunix_setup(struct serial_private *priv,
>  		const struct pciserial_board *board,

 Gratuitous change here.

> diff --git a/drivers/tty/serial/8250/8250_pcilib.c b/drivers/tty/serial/8250/8250_pcilib.c
> index ea906d721b2c3eac15c9e8d62cc6fa56c3ef6150..fc1882d7515b5814ff1240ffdbe1009ab908ad6b 100644
> --- a/drivers/tty/serial/8250/8250_pcilib.c
> +++ b/drivers/tty/serial/8250/8250_pcilib.c
> @@ -28,6 +28,10 @@ int serial8250_pci_setup_port(struct pci_dev *dev, struct uart_8250_port *port,
>  		port->port.membase = pcim_iomap_table(dev)[bar] + offset;
>  		port->port.regshift = regshift;
>  	} else {
> +		if (!IS_ENABLED(CONFIG_HAS_IOPORT)) {
> +			pr_err("Serial port %lx requires I/O port support\n", port->port.iobase);
> +			return -EINVAL;
> +		}
>  		port->port.iotype = UPIO_PORT;
>  		port->port.iobase = pci_resource_start(dev, bar) + offset;
>  		port->port.mapbase = 0;

 Can we please flatten this conditional and get rid of the negation, and 
also use `pci_err' for clear identification (`port->port.iobase' may not 
even have been set to anything meaningful if this triggers)?  I.e.:

		/* ... */
	} else if (IS_ENABLED(CONFIG_HAS_IOPORT)) {
		/* ... */
	} else {
		pci_err(dev, "serial port requires I/O port support\n");
		return -EINVAL;
	}

I'd also say "port I/O" (by analogy to "memory-mapped I/O") rather than 
"I/O port", but I can imagine it might be debatable.

> +static __always_inline bool is_upf_fourport(struct uart_port *port)
> +{
> +	if (!IS_ENABLED(CONFIG_HAS_IOPORT))
> +		return false;
> +
> +	return port->flags & UPF_FOURPORT;
> +}

 Can we perhaps avoid adding this helper and then tweaking code throughout 
by having:

#ifdef CONFIG_SERIAL_8250_FOURPORT
#define UPF_FOURPORT		((__force upf_t) ASYNC_FOURPORT       /* 1  */ )
#else
#define UPF_FOURPORT		0
#endif

in include/linux/serial_core.h instead?  I can see the flag is reused by 
drivers/tty/serial/sunsu.c, but from a glance over it seems rubbish to me 
and such a change won't hurt the driver anyway.

> @@ -1174,7 +1201,7 @@ static void autoconfig(struct uart_8250_port *up)
>  		 */
>  		scratch = serial_in(up, UART_IER);
>  		serial_out(up, UART_IER, 0);
> -#ifdef __i386__
> +#if defined(__i386__) && defined(CONFIG_HAS_IOPORT)
>  		outb(0xff, 0x080);
>  #endif
>  		/*
> @@ -1183,7 +1210,7 @@ static void autoconfig(struct uart_8250_port *up)
>  		 */
>  		scratch2 = serial_in(up, UART_IER) & UART_IER_ALL_INTR;
>  		serial_out(up, UART_IER, UART_IER_ALL_INTR);
> -#ifdef __i386__
> +#if defined(__i386__) && defined(CONFIG_HAS_IOPORT)
>  		outb(0, 0x080);
>  #endif
>  		scratch3 = serial_in(up, UART_IER) & UART_IER_ALL_INTR;

 Nah, i386 does have machine OUTB instructions, it has the port I/O 
address space in the ISA, so these two changes make no sense to me.  

 Though this #ifdef should likely be converted to CONFIG_X86_32 via a 
separate change.

> @@ -1306,12 +1333,12 @@ static void autoconfig_irq(struct uart_8250_port *up)
>  {
>  	struct uart_port *port = &up->port;
>  	unsigned char save_mcr, save_ier;
> +	unsigned long irqs;
>  	unsigned char save_ICP = 0;
>  	unsigned int ICP = 0;
> -	unsigned long irqs;
>  	int irq;

 Gratuitous change here (also breaking the reverse Christmas tree order).

 Thanks for making the clean-ups we discussed.

  Maciej

Re: [PATCH v6 4/5] tty: serial: handle HAS_IOPORT dependencies

[Niklas Schnelle]

On Mon, 2024-10-07 at 22:09 +0100, Maciej W. Rozycki wrote:
> On Mon, 7 Oct 2024, Niklas Schnelle wrote:
> 
> > diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
> > index 6709b6a5f3011db38acc58dc7223158fe4fcf72e..6a638feb44e443a1998980dd037748f227ec1bc8 100644
> > --- a/drivers/tty/serial/8250/8250_pci.c
> > +++ b/drivers/tty/serial/8250/8250_pci.c
> [...]
> >  	iobase = pci_resource_start(dev, 0);
> >  	outb(0x0, iobase + CH384_XINT_ENABLE_REG);
> >  }
> >  
> > -
> >  static int
> >  pci_sunix_setup(struct serial_private *priv,
> >  		const struct pciserial_board *board,
> 
>  Gratuitous change here.
> 
> > diff --git a/drivers/tty/serial/8250/8250_pcilib.c b/drivers/tty/serial/8250/8250_pcilib.c
> > index ea906d721b2c3eac15c9e8d62cc6fa56c3ef6150..fc1882d7515b5814ff1240ffdbe1009ab908ad6b 100644
> > --- a/drivers/tty/serial/8250/8250_pcilib.c
> > +++ b/drivers/tty/serial/8250/8250_pcilib.c
> > @@ -28,6 +28,10 @@ int serial8250_pci_setup_port(struct pci_dev *dev, struct uart_8250_port *port,
> >  		port->port.membase = pcim_iomap_table(dev)[bar] + offset;
> >  		port->port.regshift = regshift;
> >  	} else {
> > +		if (!IS_ENABLED(CONFIG_HAS_IOPORT)) {
> > +			pr_err("Serial port %lx requires I/O port support\n", port->port.iobase);
> > +			return -EINVAL;
> > +		}
> >  		port->port.iotype = UPIO_PORT;
> >  		port->port.iobase = pci_resource_start(dev, bar) + offset;
> >  		port->port.mapbase = 0;
> 
>  Can we please flatten this conditional and get rid of the negation, and 
> also use `pci_err' for clear identification (`port->port.iobase' may not 
> even have been set to anything meaningful if this triggers)?  I.e.:
> 
> 		/* ... */
> 	} else if (IS_ENABLED(CONFIG_HAS_IOPORT)) {
> 		/* ... */
> 	} else {
> 		pci_err(dev, "serial port requires I/O port support\n");
> 		return -EINVAL;
> 	}
> 
> I'd also say "port I/O" (by analogy to "memory-mapped I/O") rather than 
> "I/O port", but I can imagine it might be debatable.

Agree this looks better, will change it.

> 
> > +static __always_inline bool is_upf_fourport(struct uart_port *port)
> > +{
> > +	if (!IS_ENABLED(CONFIG_HAS_IOPORT))
> > +		return false;
> > +
> > +	return port->flags & UPF_FOURPORT;
> > +}
> 
>  Can we perhaps avoid adding this helper and then tweaking code throughout 
> by having:
> 
> #ifdef CONFIG_SERIAL_8250_FOURPORT
> #define UPF_FOURPORT		((__force upf_t) ASYNC_FOURPORT       /* 1  */ )
> #else
> #define UPF_FOURPORT		0
> #endif
> 
> in include/linux/serial_core.h instead?  I can see the flag is reused by 
> drivers/tty/serial/sunsu.c, but from a glance over it seems rubbish to me 
> and such a change won't hurt the driver anyway.

I'll look at this, do you think this is okay regarding matching the
user-space definitions in include/uapi/linux/tty_flags.h?

> 
> > @@ -1174,7 +1201,7 @@ static void autoconfig(struct uart_8250_port *up)
> >  		 */
> >  		scratch = serial_in(up, UART_IER);
> >  		serial_out(up, UART_IER, 0);
> > -#ifdef __i386__
> > +#if defined(__i386__) && defined(CONFIG_HAS_IOPORT)
> >  		outb(0xff, 0x080);
> >  #endif
> >  		/*
> > @@ -1183,7 +1210,7 @@ static void autoconfig(struct uart_8250_port *up)
> >  		 */
> >  		scratch2 = serial_in(up, UART_IER) & UART_IER_ALL_INTR;
> >  		serial_out(up, UART_IER, UART_IER_ALL_INTR);
> > -#ifdef __i386__
> > +#if defined(__i386__) && defined(CONFIG_HAS_IOPORT)
> >  		outb(0, 0x080);
> >  #endif
> >  		scratch3 = serial_in(up, UART_IER) & UART_IER_ALL_INTR;
> 
>  Nah, i386 does have machine OUTB instructions, it has the port I/O 
> address space in the ISA, so these two changes make no sense to me.  
> 
>  Though this #ifdef should likely be converted to CONFIG_X86_32 via a 
> separate change.

This is needed for Usermode Linux (UM) which sets __i386__ but also
doesn't have CONFIG_HAS_IOPORT. This was spotted by the kernel test bot
here: https://lore.kernel.org/all/202410031712.BwfGjrQY-lkp@intel.com/

> 
> > @@ -1306,12 +1333,12 @@ static void autoconfig_irq(struct uart_8250_port *up)
> >  {
> >  	struct uart_port *port = &up->port;
> >  	unsigned char save_mcr, save_ier;
> > +	unsigned long irqs;
> >  	unsigned char save_ICP = 0;
> >  	unsigned int ICP = 0;
> > -	unsigned long irqs;
> >  	int irq;
> 
>  Gratuitous change here (also breaking the reverse Christmas tree order).
> 
>  Thanks for making the clean-ups we discussed.
> 
>   Maciej

WIll drop this hunk

Re: [PATCH v6 4/5] tty: serial: handle HAS_IOPORT dependencies

[Niklas Schnelle]

On Tue, 2024-10-08 at 10:16 +0200, Niklas Schnelle wrote:
> On Mon, 2024-10-07 at 22:09 +0100, Maciej W. Rozycki wrote:
> > On Mon, 7 Oct 2024, Niklas Schnelle wrote:
> > 
> > > diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
> > > index 6709b6a5f3011db38acc58dc7223158fe4fcf72e..6a638feb44e443a1998980dd037748f227ec1bc8 100644
> > > --- a/drivers/tty/serial/8250/8250_pci.c
> > > +++ b/drivers/tty/serial/8250/8250_pci.c
> > [...]
> > >  	iobase = pci_resource_start(dev, 0);
> > >  	outb(0x0, iobase + CH384_XINT_ENABLE_REG);
> > >  }
> > >  
> > > -
> > >  static int
> > >  pci_sunix_setup(struct serial_private *priv,
> > >  		const struct pciserial_board *board,
> > 
> >  Gratuitous change here.
> > 
> > > diff --git a/drivers/tty/serial/8250/8250_pcilib.c b/drivers/tty/serial/8250/8250_pcilib.c
> > > index ea906d721b2c3eac15c9e8d62cc6fa56c3ef6150..fc1882d7515b5814ff1240ffdbe1009ab908ad6b 100644
> > > --- a/drivers/tty/serial/8250/8250_pcilib.c
> > > +++ b/drivers/tty/serial/8250/8250_pcilib.c
> > > @@ -28,6 +28,10 @@ int serial8250_pci_setup_port(struct pci_dev *dev, struct uart_8250_port *port,
> > >  		port->port.membase = pcim_iomap_table(dev)[bar] + offset;
> > >  		port->port.regshift = regshift;
> > >  	} else {
> > > +		if (!IS_ENABLED(CONFIG_HAS_IOPORT)) {
> > > +			pr_err("Serial port %lx requires I/O port support\n", port->port.iobase);
> > > +			return -EINVAL;
> > > +		}
> > >  		port->port.iotype = UPIO_PORT;
> > >  		port->port.iobase = pci_resource_start(dev, bar) + offset;
> > >  		port->port.mapbase = 0;
> > 
> >  Can we please flatten this conditional and get rid of the negation, and 
> > also use `pci_err' for clear identification (`port->port.iobase' may not 
> > even have been set to anything meaningful if this triggers)?  I.e.:
> > 
> > 		/* ... */
> > 	} else if (IS_ENABLED(CONFIG_HAS_IOPORT)) {
> > 		/* ... */
> > 	} else {
> > 		pci_err(dev, "serial port requires I/O port support\n");
> > 		return -EINVAL;
> > 	}
> > 
> > I'd also say "port I/O" (by analogy to "memory-mapped I/O") rather than 
> > "I/O port", but I can imagine it might be debatable.
> 
> Agree this looks better, will change it.

While changing this I noticed that this isn't aligned with the other
print. There we use dev_warn() and -ENXIO vs -EINVAL. How about we move
serial_8250_need_ioport() to 8250_pcilib.c and use it here too? Then we
also only have a single place for the message.

Thanks,
Niklas

Re: [PATCH v6 4/5] tty: serial: handle HAS_IOPORT dependencies

[Maciej W. Rozycki]

On Tue, 8 Oct 2024, Niklas Schnelle wrote:

> > > +static __always_inline bool is_upf_fourport(struct uart_port *port)
> > > +{
> > > +	if (!IS_ENABLED(CONFIG_HAS_IOPORT))
> > > +		return false;
> > > +
> > > +	return port->flags & UPF_FOURPORT;
> > > +}
> > 
> >  Can we perhaps avoid adding this helper and then tweaking code throughout 
> > by having:
> > 
> > #ifdef CONFIG_SERIAL_8250_FOURPORT
> > #define UPF_FOURPORT		((__force upf_t) ASYNC_FOURPORT       /* 1  */ )
> > #else
> > #define UPF_FOURPORT		0
> > #endif
> > 
> > in include/linux/serial_core.h instead?  I can see the flag is reused by 
> > drivers/tty/serial/sunsu.c, but from a glance over it seems rubbish to me 
> > and such a change won't hurt the driver anyway.
> 
> I'll look at this, do you think this is okay regarding matching the
> user-space definitions in include/uapi/linux/tty_flags.h?

 With this change UAPI stays the same and setting ASYNC_FOURPORT (with 
`setserial', etc.) will just do nothing with non-port-I/O platforms, as 
expected.  Arguably being able to set it for any serial port and cause the 
driver to poke at random I/O locations is already asking for trouble, but 
that the price of legacy.

> > > @@ -1174,7 +1201,7 @@ static void autoconfig(struct uart_8250_port *up)
> > >  		 */
> > >  		scratch = serial_in(up, UART_IER);
> > >  		serial_out(up, UART_IER, 0);
> > > -#ifdef __i386__
> > > +#if defined(__i386__) && defined(CONFIG_HAS_IOPORT)
> > >  		outb(0xff, 0x080);
> > >  #endif
> > >  		/*
> > > @@ -1183,7 +1210,7 @@ static void autoconfig(struct uart_8250_port *up)
> > >  		 */
> > >  		scratch2 = serial_in(up, UART_IER) & UART_IER_ALL_INTR;
> > >  		serial_out(up, UART_IER, UART_IER_ALL_INTR);
> > > -#ifdef __i386__
> > > +#if defined(__i386__) && defined(CONFIG_HAS_IOPORT)
> > >  		outb(0, 0x080);
> > >  #endif
> > >  		scratch3 = serial_in(up, UART_IER) & UART_IER_ALL_INTR;
> > 
> >  Nah, i386 does have machine OUTB instructions, it has the port I/O 
> > address space in the ISA, so these two changes make no sense to me.  
> > 
> >  Though this #ifdef should likely be converted to CONFIG_X86_32 via a 
> > separate change.
> 
> This is needed for Usermode Linux (UM) which sets __i386__ but also
> doesn't have CONFIG_HAS_IOPORT. This was spotted by the kernel test bot
> here: https://lore.kernel.org/all/202410031712.BwfGjrQY-lkp@intel.com/

 Odd, but I'm not into UML so I need to accept your justification.  My 
reservation about relying on compiler's __i386__ predefine rather than our 
CONFIG_X86_32 setting still stands, but that's beyond the scope of your 
change (as is switching from `#if ...' to `if (...)').  Thanks for your 
attention to such details.

 NB these `outb' calls look to me remarkably like remains of `outb_p' and 
I wonder if they could be abstracted somehow.  For those who don't know: 
the port I/O location 0x80 in the IBM PC address space was reserved for 
use as a diagnostic port.  Despite being in the mainboard's address space 
its chip select line was left floating and one could obtain an ISA option 
card that decoded this location and showed data values written to it on a 
hex display.  As it was a location known to cause no side effect (beyond 
that optional hex display) it was commonly used to incur a small delay in 
execution.  It was also used by BIOS POST to indicate progress.

 I've skimmed over v8 and it seems good to go as far as I'm concerned.  
Any fallout can be dealt with on a case-by-case basis.  Thank you for 
working on these improvements.

  Maciej