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

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

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

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

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