[1/1] tty: serial: handle HAS_IOPORT dependencies

Message ID	20240405152924.252598-2-schnelle@linux.ibm.com
State	New
Headers	show Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2D7DC16F901; Fri, 5 Apr 2024 15:29:36 +0000 (UTC) From: Niklas Schnelle <schnelle@linux.ibm.com> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Jiri Slaby <jirislaby@kernel.org>, =?utf-8?q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com> Cc: linux-serial@vger.kernel.org, Arnd Bergmann <arnd@kernel.org>, Heiko Carstens <hca@linux.ibm.com>, linux-kernel@vger.kernel.org, Niklas Schnelle <schnelle@linux.ibm.com> Subject: [PATCH 1/1] tty: serial: handle HAS_IOPORT dependencies Date: Fri, 5 Apr 2024 17:29:24 +0200 Message-Id: <20240405152924.252598-2-schnelle@linux.ibm.com> In-Reply-To: <20240405152924.252598-1-schnelle@linux.ibm.com> References: <20240405152924.252598-1-schnelle@linux.ibm.com> Content-Transfer-Encoding: 8bit Precedence: bulk MIME-Version: 1.0
Series	tty: Handle HAS_IOPORT dependencies \| expand [0/1] tty: Handle HAS_IOPORT dependencies [1/1] tty: serial: handle HAS_IOPORT dependencies

Niklas Schnelle April 5, 2024, 3:29 p.m. UTC

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. For 8250 based drivers some support
MMIO only use so fence only the parts requiring I/O ports.

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.

Note 2: This was previously acked here:
https://lore.kernel.org/all/2023053050-prodigal-shine-4d1c@gregkh/
Given this was almost a year ago and didn't apply then I didn't
carry the Ack over though.

 drivers/tty/Kconfig                  |  4 +--
 drivers/tty/serial/8250/8250_early.c |  4 +++
 drivers/tty/serial/8250/8250_pci.c   | 14 ++++++++++
 drivers/tty/serial/8250/8250_port.c  | 42 +++++++++++++++++++++++-----
 drivers/tty/serial/8250/Kconfig      |  7 ++---
 drivers/tty/serial/Kconfig           |  2 +-
 6 files changed, 59 insertions(+), 14 deletions(-)

Ilpo Järvinen April 8, 2024, 9:54 a.m. UTC | #1

On Fri, 5 Apr 2024, 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 unconditionally. For 8250 based drivers some support
> MMIO only use so fence only the parts requiring I/O ports.
> 
> 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.
> 
> Note 2: This was previously acked here:
> https://lore.kernel.org/all/2023053050-prodigal-shine-4d1c@gregkh/
> Given this was almost a year ago and didn't apply then I didn't
> carry the Ack over though.
> 
>  drivers/tty/Kconfig                  |  4 +--
>  drivers/tty/serial/8250/8250_early.c |  4 +++
>  drivers/tty/serial/8250/8250_pci.c   | 14 ++++++++++
>  drivers/tty/serial/8250/8250_port.c  | 42 +++++++++++++++++++++++-----
>  drivers/tty/serial/8250/Kconfig      |  7 ++---
>  drivers/tty/serial/Kconfig           |  2 +-
>  6 files changed, 59 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> index a45d423ad10f..63a494d36a1f 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 e3f482fd3de4..8f9aec2e7c7d 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 0d35c77fad9e..38ac5236d2ea 100644
> --- a/drivers/tty/serial/8250/8250_pci.c
> +++ b/drivers/tty/serial/8250/8250_pci.c
> @@ -928,6 +928,7 @@ static int pci_netmos_init(struct pci_dev *dev)
>  	return num_serial;
>  }
>  
> +#ifdef CONFIG_HAS_IOPORT
>  /*
>   * These chips are available with optionally one parallel port and up to
>   * two serial ports. Unfortunately they all have the same product id.
> @@ -1054,6 +1055,7 @@ static void pci_ite887x_exit(struct pci_dev *dev)
>  	ioport &= 0xffff;
>  	release_region(ioport, ITE_887x_IOSIZE);
>  }
> +#endif /* CONFIG_HAS_IOPORT */
>  
>  /*
>   * Oxford Semiconductor Inc.
> @@ -1328,6 +1330,7 @@ static int pci_oxsemi_tornado_setup(struct serial_private *priv,
>  #define QOPR_CLOCK_X8		0x0003
>  #define QOPR_CLOCK_RATE_MASK	0x0003
>  
> +#ifdef CONFIG_HAS_IOPORT
>  /* Quatech devices have their own extra interface features */
>  static struct pci_device_id quatech_cards[] = {
>  	{ PCI_DEVICE_DATA(QUATECH, QSC100,   1) },
> @@ -1547,6 +1550,7 @@ static int pci_quatech_setup(struct serial_private *priv,
>  		pci_warn(priv->dev, "software control of RS422 features not currently supported.\n");
>  	return pci_default_setup(priv, board, port, idx);
>  }
> +#endif /* CONFIG_HAS_IOPORT */
>  
>  static int pci_default_setup(struct serial_private *priv,
>  		  const struct pciserial_board *board,
> @@ -1826,6 +1830,7 @@ static int skip_tx_en_setup(struct serial_private *priv,
>  	return pci_default_setup(priv, board, port, idx);
>  }
>  
> +#ifdef CONFIG_HAS_IOPORT
>  static void kt_handle_break(struct uart_port *p)
>  {
>  	struct uart_8250_port *up = up_to_u8250p(p);
> @@ -1869,6 +1874,7 @@ static int kt_serial_setup(struct serial_private *priv,
>  	port->port.handle_break = kt_handle_break;
>  	return skip_tx_en_setup(priv, board, port, idx);
>  }
> +#endif /* CONFIG_HAS_IOPORT */
>  
>  static int pci_eg20t_init(struct pci_dev *dev)
>  {
> @@ -1913,6 +1919,7 @@ pci_wch_ch38x_setup(struct serial_private *priv,
>  #define CH384_XINT_ENABLE_REG   0xEB
>  #define CH384_XINT_ENABLE_BIT   0x02
>  
> +#ifdef CONFIG_HAS_IOPORT
>  static int pci_wch_ch38x_init(struct pci_dev *dev)
>  {
>  	int max_port;
> @@ -1940,6 +1947,7 @@ static void pci_wch_ch38x_exit(struct pci_dev *dev)
>  	iobase = pci_resource_start(dev, 0);
>  	outb(0x0, iobase + CH384_XINT_ENABLE_REG);
>  }
> +#endif /* CONFIG_HAS_IOPORT */
>  
>  
>  static int
> @@ -2171,6 +2179,7 @@ static struct pci_serial_quirk pci_serial_quirks[] = {
>  		.subdevice	= PCI_ANY_ID,
>  		.setup		= ce4100_serial_setup,
>  	},
> +#ifdef CONFIG_HAS_IOPORT
>  	{
>  		.vendor		= PCI_VENDOR_ID_INTEL,
>  		.device		= PCI_DEVICE_ID_INTEL_PATSBURG_KT,
> @@ -2190,6 +2199,7 @@ static struct pci_serial_quirk pci_serial_quirks[] = {
>  		.setup		= pci_default_setup,
>  		.exit		= pci_ite887x_exit,
>  	},
> +#endif
>  	/*
>  	 * National Instruments
>  	 */
> @@ -2311,6 +2321,7 @@ static struct pci_serial_quirk pci_serial_quirks[] = {
>  		.exit		= pci_ni8430_exit,
>  	},
>  	/* Quatech */
> +#ifdef CONFIG_HAS_IOPORT
>  	{
>  		.vendor		= PCI_VENDOR_ID_QUATECH,
>  		.device		= PCI_ANY_ID,
> @@ -2319,6 +2330,7 @@ static struct pci_serial_quirk pci_serial_quirks[] = {
>  		.init		= pci_quatech_init,
>  		.setup		= pci_quatech_setup,
>  	},
> +#endif
>  	/*
>  	 * Panacom
>  	 */
> @@ -2836,6 +2848,7 @@ static struct pci_serial_quirk pci_serial_quirks[] = {
>  		.subdevice      = PCI_ANY_ID,
>  		.setup          = pci_wch_ch38x_setup,
>  	},
> +#ifdef CONFIG_HAS_IOPORT
>  	/* WCH CH384 8S card (16850 clone) */
>  	{
>  		.vendor         = PCIE_VENDOR_ID_WCH,
> @@ -2846,6 +2859,7 @@ static struct pci_serial_quirk pci_serial_quirks[] = {
>  		.exit		= pci_wch_ch38x_exit,
>  		.setup          = pci_wch_ch38x_setup,
>  	},
> +#endif
>  	/*
>  	 * Broadcom TruManage (NetXtreme)
>  	 */
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index fc9dd5d45295..1c5e39c1a469 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,24 @@ 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)
> +{
> +}
> +
> +#ifdef CONFIG_HAS_IOPORT
> +static inline bool is_upf_fourport(struct uart_port *port)
> +{
> +	return port->flags & UPF_FOURPORT;
> +}
> +#else
> +#define is_upf_fourport(x)	false
> +#endif
>  
>  static int serial8250_default_handle_irq(struct uart_port *port);
>  
> @@ -422,10 +443,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 +469,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;
> @@ -1318,7 +1346,7 @@ static void autoconfig_irq(struct uart_8250_port *up)
>  	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);
> @@ -1337,7 +1365,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,
> @@ -1361,7 +1389,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;
> @@ -2438,7 +2466,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
> @@ -2483,7 +2511,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 47ff50763c04..54bf98869abf 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -6,7 +6,6 @@
>  
>  config SERIAL_8250
>  	tristate "8250/16550 and compatible serial support"
> -	depends on !S390

Why? Your changelogs gives zero insight on this change.

>  	select SERIAL_CORE
>  	select SERIAL_MCTRL_GPIO if GPIOLIB
>  	help
> @@ -72,7 +71,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
> @@ -136,7 +135,7 @@ config SERIAL_8250_PCILIB
>  
>  config SERIAL_8250_PCI
>  	tristate "8250/16550 PCI device support"
> -	depends on SERIAL_8250 && PCI
> +	depends on SERIAL_8250 && PCI && HAS_IOPORT
>  	select SERIAL_8250_PCILIB
>  	default SERIAL_8250
>  	help
> @@ -163,7 +162,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

What about drivers that use SERIAL8250_PORT()?

Also port provided in 8250_PNP might expect it I think.

> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index ffcf4882b25f..92c85e805c2a 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -874,7 +874,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
>

Arnd Bergmann April 8, 2024, 10:17 a.m. UTC | #2

On Mon, Apr 8, 2024, at 11:54, Ilpo Järvinen wrote:
> On Fri, 5 Apr 2024, Niklas Schnelle wrote:

>>  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
>
> What about drivers that use SERIAL8250_PORT()?

It probably makes sense to hide these, since they won't ever
work. I probably missed them in my initial series because they
don't cause a compile-time error, but I agree that there is no
use in showing the options here.

> Also port provided in 8250_PNP might expect it I think.

I don't think these need any change: 8250_pnp.c supports
both IORESOURCE_IO and IORESOURCE_MEM based ports. It will
still create a 8250 port for the I/O based ones but they
will now correctly fail to probe in the main driver rather
than crashing the kernel. PNP devices that only use
memory BARs will keep working as before, on both machines
with and without CONFIG_HAS_IOPORT.

I think that most 8250_pnp variants are probably used only
with ISAPNP or PNPBIOS, neither of which exists without
HAS_IOPORT, but you could certainly have PNPACPI on arm
or riscv machines that don't have port I/O but come with
a memory-mapped 8250 port described by firmware.

    Arnd

Ilpo Järvinen April 8, 2024, 10:25 a.m. UTC | #3

On Mon, 8 Apr 2024, Arnd Bergmann wrote:

> On Mon, Apr 8, 2024, at 11:54, Ilpo Järvinen wrote:
> > On Fri, 5 Apr 2024, Niklas Schnelle wrote:
> 
> >>  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
> >
> > What about drivers that use SERIAL8250_PORT()?
> 
> It probably makes sense to hide these, since they won't ever
> work. I probably missed them in my initial series because they
> don't cause a compile-time error, but I agree that there is no
> use in showing the options here.
> 
> > Also port provided in 8250_PNP might expect it I think.
> 
> I don't think these need any change: 8250_pnp.c supports
> both IORESOURCE_IO and IORESOURCE_MEM based ports. It will
> still create a 8250 port for the I/O based ones but they
> will now correctly fail to probe in the main driver rather
> than crashing the kernel. PNP devices that only use
> memory BARs will keep working as before, on both machines
> with and without CONFIG_HAS_IOPORT.
> 
> I think that most 8250_pnp variants are probably used only
> with ISAPNP or PNPBIOS, neither of which exists without
> HAS_IOPORT,

Okay, seems fine then if that dependency is handled somewhere.

Niklas Schnelle April 8, 2024, 3:35 p.m. UTC | #4

On Mon, 2024-04-08 at 12:54 +0300, Ilpo Järvinen wrote:
> On Fri, 5 Apr 2024, 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 unconditionally. For 8250 based drivers some support
> > MMIO only use so fence only the parts requiring I/O ports.
> > 
> > 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.
> > 
> > Note 2: This was previously acked here:
> > https://lore.kernel.org/all/2023053050-prodigal-shine-4d1c@gregkh/
> > Given this was almost a year ago and didn't apply then I didn't
> > carry the Ack over though.
> > 
> > 
---8<---
> > diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> > index 47ff50763c04..54bf98869abf 100644
> > --- a/drivers/tty/serial/8250/Kconfig
> > +++ b/drivers/tty/serial/8250/Kconfig
> > @@ -6,7 +6,6 @@
> >  
> >  config SERIAL_8250
> >  	tristate "8250/16550 and compatible serial support"
> > -	depends on !S390
> 
> Why? Your changelogs gives zero insight on this change.

I used this for compile testing since I build on s390 natively and this
would have hidden the missing HAS_IOPORT dependencies I'm pretty sure
it was added because of the I/O port problem too. I'll either add to
the commit description that it is no longer needed or drop this. Any
preference?

Ilpo Järvinen April 8, 2024, 3:41 p.m. UTC | #5

On Mon, 8 Apr 2024, Niklas Schnelle wrote:

> On Mon, 2024-04-08 at 12:54 +0300, Ilpo Järvinen wrote:
> > On Fri, 5 Apr 2024, 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 unconditionally. For 8250 based drivers some support
> > > MMIO only use so fence only the parts requiring I/O ports.
> > > 
> > > 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.
> > > 
> > > Note 2: This was previously acked here:
> > > https://lore.kernel.org/all/2023053050-prodigal-shine-4d1c@gregkh/
> > > Given this was almost a year ago and didn't apply then I didn't
> > > carry the Ack over though.
> > > 
> > > 
> ---8<---
> > > diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> > > index 47ff50763c04..54bf98869abf 100644
> > > --- a/drivers/tty/serial/8250/Kconfig
> > > +++ b/drivers/tty/serial/8250/Kconfig
> > > @@ -6,7 +6,6 @@
> > >  
> > >  config SERIAL_8250
> > >  	tristate "8250/16550 and compatible serial support"
> > > -	depends on !S390
> > 
> > Why? Your changelogs gives zero insight on this change.
> 
> I used this for compile testing since I build on s390 natively and this
> would have hidden the missing HAS_IOPORT dependencies I'm pretty sure
> it was added because of the I/O port problem too. I'll either add to
> the commit description that it is no longer needed or drop this. Any
> preference?

Okay, we might never know the reason for sure if that's old enough.
I think the best approach would be to put it into own patch so this 
guessimation is limited to a single liner patch instead of it being 
hidden among the other clearer cases.

Arnd Bergmann April 8, 2024, 3:50 p.m. UTC | #6

On Mon, Apr 8, 2024, at 17:41, Ilpo Järvinen wrote:
> On Mon, 8 Apr 2024, Niklas Schnelle wrote:
>> On Mon, 2024-04-08 at 12:54 +0300, Ilpo Järvinen wrote:
>> > On Fri, 5 Apr 2024, Niklas Schnelle wrote:
>> ---8<---
>> > > diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
>> > > index 47ff50763c04..54bf98869abf 100644
>> > > --- a/drivers/tty/serial/8250/Kconfig
>> > > +++ b/drivers/tty/serial/8250/Kconfig
>> > > @@ -6,7 +6,6 @@
>> > >  
>> > >  config SERIAL_8250
>> > >  	tristate "8250/16550 and compatible serial support"
>> > > -	depends on !S390
>> > 
>> > Why? Your changelogs gives zero insight on this change.
>> 
>> I used this for compile testing since I build on s390 natively and this
>> would have hidden the missing HAS_IOPORT dependencies I'm pretty sure
>> it was added because of the I/O port problem too. I'll either add to
>> the commit description that it is no longer needed or drop this. Any
>> preference?
>
> Okay, we might never know the reason for sure if that's old enough.
> I think the best approach would be to put it into own patch so this 
> guessimation is limited to a single liner patch instead of it being 
> hidden among the other clearer cases.

Maciej W. Rozycki May 23, 2024, 2:11 a.m. UTC | #7

On Fri, 5 Apr 2024, Niklas Schnelle wrote:

> diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
> index 0d35c77fad9e..38ac5236d2ea 100644
> --- a/drivers/tty/serial/8250/8250_pci.c
> +++ b/drivers/tty/serial/8250/8250_pci.c
> @@ -928,6 +928,7 @@ static int pci_netmos_init(struct pci_dev *dev)
>  	return num_serial;
>  }
>  
> +#ifdef CONFIG_HAS_IOPORT
>  /*
>   * These chips are available with optionally one parallel port and up to
>   * two serial ports. Unfortunately they all have the same product id.
[...]
> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index 47ff50763c04..54bf98869abf 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -136,7 +135,7 @@ config SERIAL_8250_PCILIB
>  
>  config SERIAL_8250_PCI
>  	tristate "8250/16550 PCI device support"
> -	depends on SERIAL_8250 && PCI
> +	depends on SERIAL_8250 && PCI && HAS_IOPORT
>  	select SERIAL_8250_PCILIB
>  	default SERIAL_8250
>  	help

 This is clearly wrong, there is PCIe 8250 serial hardware that does MMIO 
only, so the option has to stay possible to enable.  I have such hardware 
as shown in this log:

Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
serial 0001:01:00.0: enabling device (0140 -> 0142)
serial 0001:01:00.0: detected caps 00000700 should be 00000500
0001:01:00.0: ttyS0 at MMIO 0x600c080401000 (irq = 40, base_baud = 15625000) is a 16C950/954
serial 0001:01:00.0: detected caps 00000700 should be 00000500
0001:01:00.0: ttyS1 at MMIO 0x600c080401200 (irq = 40, base_baud = 15625000) is a 16C950/954

which is from a POWER9 system.  Which as you may know has no PCI port I/O 
support in hardware, so it is quite relevant here.  I'd like to keep this 
PCIe serial option functional with my system.

 Also your change itself modifies 8250_pci.c (cited above for reference), 
which would make no sense if SERIAL_8250_PCI was permanently disabled for 
!HAS_IOPORT.  Shall I take it than that the Kconfig change I question has 
been made merely by mistake?

  Maciej

Niklas Schnelle Oct. 1, 2024, 9:04 a.m. UTC | #8

On Mon, 2024-04-08 at 12:17 +0200, Arnd Bergmann wrote:
> On Mon, Apr 8, 2024, at 11:54, Ilpo Järvinen wrote:
> > On Fri, 5 Apr 2024, Niklas Schnelle wrote:
> 
> > >  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
> > 
> > What about drivers that use SERIAL8250_PORT()?
> 
> It probably makes sense to hide these, since they won't ever
> work. I probably missed them in my initial series because they
> don't cause a compile-time error, but I agree that there is no
> use in showing the options here.
> 

As far as I can tell SERTIAL8250_PORT() is used by SERIAL_8250_ACCENT,
SERIAL_8250_BOCA, and SERIAL_8250_EXAR_ST16C554 all of these already
depend on ISA which implies HAS_IOPORT. So I don't think we need to add
HAS_IOPORT dependencies here?

Thanks,
Niklas

Niklas Schnelle Oct. 1, 2024, 11:21 a.m. UTC | #9

On Thu, 2024-05-23 at 03:11 +0100, Maciej W. Rozycki wrote:
> On Fri, 5 Apr 2024, Niklas Schnelle wrote:
> 
> > diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
> > index 0d35c77fad9e..38ac5236d2ea 100644
> > --- a/drivers/tty/serial/8250/8250_pci.c
> > +++ b/drivers/tty/serial/8250/8250_pci.c
> > @@ -928,6 +928,7 @@ static int pci_netmos_init(struct pci_dev *dev)
> >  	return num_serial;
> >  }
> >  
> > +#ifdef CONFIG_HAS_IOPORT
> >  /*
> >   * These chips are available with optionally one parallel port and up to
> >   * two serial ports. Unfortunately they all have the same product id.
> [...]
> > diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> > index 47ff50763c04..54bf98869abf 100644
> > --- a/drivers/tty/serial/8250/Kconfig
> > +++ b/drivers/tty/serial/8250/Kconfig
> > @@ -136,7 +135,7 @@ config SERIAL_8250_PCILIB
> >  
> >  config SERIAL_8250_PCI
> >  	tristate "8250/16550 PCI device support"
> > -	depends on SERIAL_8250 && PCI
> > +	depends on SERIAL_8250 && PCI && HAS_IOPORT
> >  	select SERIAL_8250_PCILIB
> >  	default SERIAL_8250
> >  	help
> 
>  This is clearly wrong, there is PCIe 8250 serial hardware that does MMIO 
> only, so the option has to stay possible to enable.  I have such hardware 
> as shown in this log:
> 
> Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
> serial 0001:01:00.0: enabling device (0140 -> 0142)
> serial 0001:01:00.0: detected caps 00000700 should be 00000500
> 0001:01:00.0: ttyS0 at MMIO 0x600c080401000 (irq = 40, base_baud = 15625000) is a 16C950/954
> serial 0001:01:00.0: detected caps 00000700 should be 00000500
> 0001:01:00.0: ttyS1 at MMIO 0x600c080401200 (irq = 40, base_baud = 15625000) is a 16C950/954
> 
> which is from a POWER9 system.  Which as you may know has no PCI port I/O 
> support in hardware, so it is quite relevant here.  I'd like to keep this 
> PCIe serial option functional with my system.
> 
>  Also your change itself modifies 8250_pci.c (cited above for reference), 
> which would make no sense if SERIAL_8250_PCI was permanently disabled for 
> !HAS_IOPORT.  Shall I take it than that the Kconfig change I question has 
> been made merely by mistake?
> 
>   Maciej

Hi Maciej,

With 2 more HAS_IOPORT patches having gone into v6.12-rc1 I'm looking
at what's left and we're down to 4 prerequisite patches[0] before being
able to compile-time disable inb()/outb()/…. This one being by far the
largest of these. Looking at your suggestion it seems that to compile
8250_pci.c without HAS_IOPORT I'll have to add #ifdef CONFIG_HAS_IOPORT
around the MOXI section as that uses I/O ports unconditionally. The
rest seems fine and I guess would theoretically work on a system with
!HAS_IOPORT. I'll send a v2 with that included. 

Note however that even though your POWER9 system does not have I/O port
support in hardware we still have HAS_IOPORT enabled for arch/powerpc
if PCI is enabed so even with this patch as is your POWER9 system
should not be affected.

Thanks,
Niklas

[0]
https://git.kernel.org/pub/scm/linux/kernel/git/niks/linux.git/log/?h=has_ioport

Arnd Bergmann Oct. 1, 2024, 3:31 p.m. UTC | #10

On Tue, Oct 1, 2024, at 11:21, Niklas Schnelle wrote:
> On Thu, 2024-05-23 at 03:11 +0100, Maciej W. Rozycki wrote:
>
> With 2 more HAS_IOPORT patches having gone into v6.12-rc1 I'm looking
> at what's left and we're down to 4 prerequisite patches[0] before being
> able to compile-time disable inb()/outb()/…. This one being by far the
> largest of these. Looking at your suggestion it seems that to compile
> 8250_pci.c without HAS_IOPORT I'll have to add #ifdef CONFIG_HAS_IOPORT
> around the MOXI section as that uses I/O ports unconditionally. The
> rest seems fine and I guess would theoretically work on a system with
> !HAS_IOPORT. I'll send a v2 with that included. 

I think that is the correct approach, yes. From what I can tell, the
older version of the 8250 patch added the #ifdef blocks for all other
port types that need port I/O, but the moxa version was added later
than that and just needs the same change.

      Arnd

Maciej W. Rozycki Oct. 1, 2024, 4:41 p.m. UTC | #11

Hi Niklas,

> With 2 more HAS_IOPORT patches having gone into v6.12-rc1 I'm looking
> at what's left and we're down to 4 prerequisite patches[0] before being
> able to compile-time disable inb()/outb()/…. This one being by far the
> largest of these. Looking at your suggestion it seems that to compile
> 8250_pci.c without HAS_IOPORT I'll have to add #ifdef CONFIG_HAS_IOPORT
> around the MOXI section as that uses I/O ports unconditionally. The
> rest seems fine and I guess would theoretically work on a system with
> !HAS_IOPORT. I'll send a v2 with that included. 

 I've skimmed over and I agree, though I'd place some of the #ifdefs 
differently, e.g. above #define QPCR_TEST_FOR1.  Overall I think it would 
make sense to reorder code and group stuff that depends on port I/O such 
as to minimise the number of #ifdefs.

 Ideally we could come with a slightly user-friendlier change that would 
report the inability to handle port I/O devices as they are discovered 
rather than just silently ignoring them.

> Note however that even though your POWER9 system does not have I/O port
> support in hardware we still have HAS_IOPORT enabled for arch/powerpc
> if PCI is enabed so even with this patch as is your POWER9 system
> should not be affected.

 I think we need to get this right regardless.  And also while I run a 
generic distribution kernel on my POWER9 as a production system, I'd love 
to see an option to build a tailored configuration that would indeed 
remove support for port I/O from the kernel side for systems like mine 
that have no provision for port I/O in hardware, knowing that such a 
kernel will only ever run on such hardware and discarding compiled code 
that won't ever be used.

  Maciej

Niklas Schnelle Oct. 2, 2024, 12:44 p.m. UTC | #12

On Tue, 2024-10-01 at 17:41 +0100, Maciej W. Rozycki wrote:
> Hi Niklas,
> 
> > With 2 more HAS_IOPORT patches having gone into v6.12-rc1 I'm looking
> > at what's left and we're down to 4 prerequisite patches[0] before being
> > able to compile-time disable inb()/outb()/…. This one being by far the
> > largest of these. Looking at your suggestion it seems that to compile
> > 8250_pci.c without HAS_IOPORT I'll have to add #ifdef CONFIG_HAS_IOPORT
> > around the MOXI section as that uses I/O ports unconditionally. The
> > rest seems fine and I guess would theoretically work on a system with
> > !HAS_IOPORT. I'll send a v2 with that included. 
> 
>  I've skimmed over and I agree, though I'd place some of the #ifdefs 
> differently, e.g. above #define QPCR_TEST_FOR1.  Overall I think it would 
> make sense to reorder code and group stuff that depends on port I/O such 
> as to minimise the number of #ifdefs.

I just noticed that while not causing compile errors pci_fintek_setup()
also sets iotype = UPIO_PORT so the devices using this won't work
without HAS_IOPORT either. 

> 
>  Ideally we could come with a slightly user-friendlier change that would 
> report the inability to handle port I/O devices as they are discovered 
> rather than just silently ignoring them.

I think this would generally get quite ugly as one would have to keep
around enough of the drivers which can't possibly work in that
!HAS_IOPORT kernel to identify the device and print some error. It's
also not what happens when anything else isn't supported by your kernel
build. And I don't think we can just look for any I/O ports either
because they could be an alternative access method that isn't required.

As an example for the ugliness, for 8250 one could get something to
work as it supports both I/O port and MMIO devices. First one would
need to not #ifdef the setup routines and keep the quirk entry for
devices that use UPIO_PORT and then in pciserial_init_ports() one could
check with !IS_ENABLED(CONFIG_HAS_IOPORT) && uart.port.iotype ==
UPIO_PORT. But then for moxa one would have to keep pci_moxa_setup() to
set iotype = UPIO_PORT but would have to #ifdef in pci_moxa_init()
because the initialization already uses I/O ports and init is part of
the quirk.

> 
> > Note however that even though your POWER9 system does not have I/O port
> > support in hardware we still have HAS_IOPORT enabled for arch/powerpc
> > if PCI is enabed so even with this patch as is your POWER9 system
> > should not be affected.
> 
>  I think we need to get this right regardless.  And also while I run a 
> generic distribution kernel on my POWER9 as a production system, I'd love 
> to see an option to build a tailored configuration that would indeed 
> remove support for port I/O from the kernel side for systems like mine 
> that have no provision for port I/O in hardware, knowing that such a 
> kernel will only ever run on such hardware and discarding compiled code 
> that won't ever be used.
> 
>   Maciej

I think allowing for such custom configs is a possible second step and
I agree it would be nice and probably becomes more useful as more and
more platforms lose I/O port support. First we need to be able to build
without HAS_IOPORT on architectures that just can't do I/O port access
though, and I'd like not delay this any more.

Thanks,
Niklas

Maciej W. Rozycki Oct. 2, 2024, 6:12 p.m. UTC | #13

On Wed, 2 Oct 2024, Niklas Schnelle wrote:

> >  Ideally we could come with a slightly user-friendlier change that would 
> > report the inability to handle port I/O devices as they are discovered 
> > rather than just silently ignoring them.
> 
> I think this would generally get quite ugly as one would have to keep
> around enough of the drivers which can't possibly work in that
> !HAS_IOPORT kernel to identify the device and print some error. It's
> also not what happens when anything else isn't supported by your kernel
> build. And I don't think we can just look for any I/O ports either
> because they could be an alternative access method that isn't required.

 There might be corner cases, but offhand I think it's simpler than you 
outline.  There are two cases to handle here:

1. Code you've #ifdef'd out that explicitly refers port I/O resources.
   So rather than having struct entries referring to problematic `*_init' 
   handlers #ifdef'd out we can keep them and make them call an error 
   reporting function if (!IS_ENABLED(CONFIG_HAS_IOPORT)).  As a side 
   effect the structure of code will improve as we don't really like 
   #ifdefs sprinkled throughout.

2. Code that infers the access type required from BARs.  It has to handle 
   the unsupported case anyway, so rather than doing it silently it can 
   call the same error reporting function.

Yes, there's some work to be done here, but nothing exceedingly tough I 
believe.

 Also I think this case is a bit special, because it's different from a 
missing driver.  The driver is there and the hardware is there visible in 
the PCI hierarchy, there's nothing reported and other serial ports work, 
or a similar serial port works elsewhere, so why doesn't this one?  The 
user may not necessarily be aware of the peculiarity that the lack of 
support for port I/O is.

 I was not and discovered it the hard way in the course of installing my 
POWER9 system and trying to make the defxx driver work as supplied by the 
distribution.  It took me a few days to conclude there is no bug anywhere 
except for the system lacking support for port I/O and the driver having 
been configured by the packager via a Kconfig option to use that access 
type.  Also I had PHB4 documentation to hand to refer to and track down 
the relevant bits.

 I ended up updating the driver to choose the access type automatically 
(as the board resources are dual-mapped, via both a port I/O and an MMIO 
BAR), and would have done so long before if I was aware of the existence 
of such systems.

 Now I consider myself a reasonably seasoned systems software developer, 
so what can an ordinary user say?  They might be utterly confused and 
either report it as a system bug (if they were so determined) or just 
conclude Linux is junk.

 A message such as:

serial 0001:01:00.0: cannot handle, no port I/O support in the system

would definitely help.

> As an example for the ugliness, for 8250 one could get something to
> work as it supports both I/O port and MMIO devices. First one would
> need to not #ifdef the setup routines and keep the quirk entry for
> devices that use UPIO_PORT and then in pciserial_init_ports() one could
> check with !IS_ENABLED(CONFIG_HAS_IOPORT) && uart.port.iotype ==
> UPIO_PORT. But then for moxa one would have to keep pci_moxa_setup() to
> set iotype = UPIO_PORT but would have to #ifdef in pci_moxa_init()
> because the initialization already uses I/O ports and init is part of
> the quirk.

 I don't think it has to be as complex as that.  The OxSemi Tornado driver 
which I care about a lot is an example of one that handles hardware that 
can be strapped for either access type (and I have cards with actual pin 
header jumpers and associated documentation for the user to configure 
that), so you only know at run time from the type of BAR 0 whether you 
need port I/O or MMIO.  So it falls into #2 above, and all you need is to 
handle this in `serial8250_pci_setup_port', which I can see your change 
doesn't take into account anyway, whether silently or aloud.

> I think allowing for such custom configs is a possible second step and
> I agree it would be nice and probably becomes more useful as more and
> more platforms lose I/O port support. First we need to be able to build
> without HAS_IOPORT on architectures that just can't do I/O port access
> though, and I'd like not delay this any more.

 I agree it will best be done in steps, no worry.

  Maciej

Arnd Bergmann Oct. 2, 2024, 10 p.m. UTC | #14

On Wed, Oct 2, 2024, at 18:12, Maciej W. Rozycki wrote:
> On Wed, 2 Oct 2024, Niklas Schnelle wrote:
>
>> >  Ideally we could come with a slightly user-friendlier change that would 
>> > report the inability to handle port I/O devices as they are discovered 
>> > rather than just silently ignoring them.
>> 
>> I think this would generally get quite ugly as one would have to keep
>> around enough of the drivers which can't possibly work in that
>> !HAS_IOPORT kernel to identify the device and print some error. It's
>> also not what happens when anything else isn't supported by your kernel
>> build. And I don't think we can just look for any I/O ports either
>> because they could be an alternative access method that isn't required.
>
>  There might be corner cases, but offhand I think it's simpler than you 
> outline.  There are two cases to handle here:
>
> 1. Code you've #ifdef'd out that explicitly refers port I/O resources.
>    So rather than having struct entries referring to problematic `*_init' 
>    handlers #ifdef'd out we can keep them and make them call an error 
>    reporting function if (!IS_ENABLED(CONFIG_HAS_IOPORT)).  As a side 
>    effect the structure of code will improve as we don't really like 
>    #ifdefs sprinkled throughout.
>
> 2. Code that infers the access type required from BARs.  It has to handle 
>    the unsupported case anyway, so rather than doing it silently it can 
>    call the same error reporting function.
>
> Yes, there's some work to be done here, but nothing exceedingly tough I 
> believe.

I agree that this shouldn't be hard to finish. The IS_ENABLED()
check is not that easy to do as I think we need to keep calling
inb()/outb() outside of an #ifdef a compile-time error.

However, I think most of the inb/outb usage in 8250_pci.c can
just be converted to either serial_port_in()/serial_port_out(),
using the 8250 specific wrappers, or to ioread8()/iowrite8()
in combination with pci_iomap().

It might help to add a UPIO_IOMAP type to replace UPIO_PORT
for the PCI drivers and just use pci_iomap() exclusively in that
driver.

>  Also I think this case is a bit special, because it's different from a 
> missing driver.  The driver is there and the hardware is there visible in 
> the PCI hierarchy, there's nothing reported and other serial ports work, 
> or a similar serial port works elsewhere, so why doesn't this one?  The 
> user may not necessarily be aware of the peculiarity that the lack of 
> support for port I/O is.

Part of the problem that Niklas is trying to solve with the
CONFIG_HAS_IOPORT annotations is to prevent an invalid inb()/outb()
from turning into a NULL pointer dereference as it currently does
on architectures that have no way to support PIO but get the
default implementation from asm-generic/io.h.

It's not clear if having a silently non-working driver or one
that crashes makes it easier to debug for users. Having a clear
warning message in the PCI probe code is probably the best
we can hope for.

>  I was not and discovered it the hard way in the course of installing my 
> POWER9 system and trying to make the defxx driver work as supplied by the 
> distribution.  It took me a few days to conclude there is no bug anywhere 
> except for the system lacking support for port I/O and the driver having 
> been configured by the packager via a Kconfig option to use that access 
> type.  Also I had PHB4 documentation to hand to refer to and track down 
> the relevant bits.
>
>  I ended up updating the driver to choose the access type automatically 
> (as the board resources are dual-mapped, via both a port I/O and an MMIO 
> BAR), and would have done so long before if I was aware of the existence 
> of such systems.
>
>  Now I consider myself a reasonably seasoned systems software developer, 
> so what can an ordinary user say?  They might be utterly confused and 
> either report it as a system bug (if they were so determined) or just 
> conclude Linux is junk.

I think that anyone using hardware that relies on port I/O on
non-x86 is at this point going to have a reasonable understanding
of the system, so I'm not too worried here. ;-)

>  A message such as:
>
> serial 0001:01:00.0: cannot handle, no port I/O support in the system
>
> would definitely help.

Right.

       Arnd

Maciej W. Rozycki Oct. 2, 2024, 10:59 p.m. UTC | #15

On Wed, 2 Oct 2024, Arnd Bergmann wrote:

> I agree that this shouldn't be hard to finish. The IS_ENABLED()
> check is not that easy to do as I think we need to keep calling
> inb()/outb() outside of an #ifdef a compile-time error.

 Can we just provide dummy prototypes with `__attribute__((error("...")))' 
instead?  This will surely prevent us from having to bend backwards so as 
to make sure the compiler won't see any spurious references to these 
inexistent functions or macros.  We already have a `__compiletime_error()' 
macro for this purpose even.

> Part of the problem that Niklas is trying to solve with the
> CONFIG_HAS_IOPORT annotations is to prevent an invalid inb()/outb()
> from turning into a NULL pointer dereference as it currently does
> on architectures that have no way to support PIO but get the
> default implementation from asm-generic/io.h.

 It can be worse than that.  Part of my confusion with the defxx driver 
trying to do port I/O with my POWER9 system came from the mapping actually 
resulting in non-NULL invalid pointers, dereferencing which caused a flood 
of obscure messages produced to the system console by the system firmware:

LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
IPMI: dropping non severe PEL event
LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
IPMI: dropping non severe PEL event
LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
IPMI: dropping non severe PEL event
LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
IPMI: dropping non severe PEL event
[...]

from which it was all but obvious that they were caused by an attempt to 
use PCI port I/O with a system lacking support for it.

> It's not clear if having a silently non-working driver or one
> that crashes makes it easier to debug for users. Having a clear
> warning message in the PCI probe code is probably the best
> we can hope for.

 I agree.  Enthusiastically.

> I think that anyone using hardware that relies on port I/O on
> non-x86 is at this point going to have a reasonable understanding
> of the system, so I'm not too worried here. ;-)

 Well, virtually all non-x86 systems continue supporting PCI/e port I/O 
via a suitably decoded CPU-side MMIO window, so I think coming across one 
that doesn't can still be a nasty surprise even in 2024.  For instance 
I've been happily using a PC parallel port PCIe option card, one of the 
very few interfaces if not the only one remaining that have not ever seen 
an MMIO variant, with my RISC-V hardware, newer than said POWER9 system.

 So far it's been the s390 and a couple of POWER system implementations 
that have support for PCI/e port I/O removed.  Have I missed anything?

  Maciej

Arnd Bergmann Oct. 4, 2024, 6:53 a.m. UTC | #16

On Wed, Oct 2, 2024, at 22:59, Maciej W. Rozycki wrote:
> On Wed, 2 Oct 2024, Arnd Bergmann wrote:
>> Part of the problem that Niklas is trying to solve with the
>> CONFIG_HAS_IOPORT annotations is to prevent an invalid inb()/outb()
>> from turning into a NULL pointer dereference as it currently does
>> on architectures that have no way to support PIO but get the
>> default implementation from asm-generic/io.h.
>
>  It can be worse than that.  Part of my confusion with the defxx driver 
> trying to do port I/O with my POWER9 system came from the mapping actually 
> resulting in non-NULL invalid pointers, dereferencing which caused a flood 
> of obscure messages produced to the system console by the system firmware:
>
> LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> IPMI: dropping non severe PEL event
> LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> IPMI: dropping non severe PEL event
> LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> IPMI: dropping non severe PEL event
> LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> IPMI: dropping non severe PEL event
> [...]
>
> from which it was all but obvious that they were caused by an attempt to 
> use PCI port I/O with a system lacking support for it.

Ah, that's too bad. I think this is a result of the patch that
Michael did to shut up the NULL pointer warning we get, see
be140f1732b5 ("powerpc/64: Set _IO_BASE to POISON_POINTER_DELTA
not 0 for CONFIG_PCI=n"). I really wish we could have finished
Niklas' series earlier to avoid this.

>> I think that anyone using hardware that relies on port I/O on
>> non-x86 is at this point going to have a reasonable understanding
>> of the system, so I'm not too worried here. ;-)
>
>  Well, virtually all non-x86 systems continue supporting PCI/e port I/O 
> via a suitably decoded CPU-side MMIO window, so I think coming across one 
> that doesn't can still be a nasty surprise even in 2024.  For instance 
> I've been happily using a PC parallel port PCIe option card, one of the 
> very few interfaces if not the only one remaining that have not ever seen 
> an MMIO variant, with my RISC-V hardware, newer than said POWER9 system.
>
>  So far it's been the s390 and a couple of POWER system implementations 
> that have support for PCI/e port I/O removed.  Have I missed anything?

I meant PCIe cards with I/O space here, not host bridges. I know you
have a lot of them, but what I've heard from Arm platform maintainers
is that they tend to struggle finding any PCIe cards to test their
hsot bridge drivers on, and I expect that a lot of them are actually
broken because they have never been tested and just copied the
implementation badly from some other driver.

I think the only new PCIe devices you can find today that still use
I/O space are ones with compatibility registers for IBM PC style
hardware (vga, uart, parport), but most users would never have used
one of those and instead use the native register interface of their
GPU (on non-x86), USB-serial and no parport. Other devices that
needed I/O space never worked on PCIe anyway because of the lack
of ISA style DMA.

There are also a lot of Arm systems that have no I/O space support at
all, such as the Apple M2 I'm using at the moment.

      Arnd

Niklas Schnelle Oct. 4, 2024, 10:09 a.m. UTC | #17

On Wed, 2024-10-02 at 23:59 +0100, Maciej W. Rozycki wrote:
> On Wed, 2 Oct 2024, Arnd Bergmann wrote:
> 
> > I agree that this shouldn't be hard to finish. The IS_ENABLED()
> > check is not that easy to do as I think we need to keep calling
> > inb()/outb() outside of an #ifdef a compile-time error.
> 
>  Can we just provide dummy prototypes with `__attribute__((error("...")))' 
> instead?  This will surely prevent us from having to bend backwards so as 
> to make sure the compiler won't see any spurious references to these 
> inexistent functions or macros.  We already have a `__compiletime_error()' 
> macro for this purpose even.

This is already done in the final patch of my series when disabling
inb()/outb() and friends.

> 
> > Part of the problem that Niklas is trying to solve with the
> > CONFIG_HAS_IOPORT annotations is to prevent an invalid inb()/outb()
> > from turning into a NULL pointer dereference as it currently does
> > on architectures that have no way to support PIO but get the
> > default implementation from asm-generic/io.h.
> 
>  It can be worse than that.  Part of my confusion with the defxx driver 
> trying to do port I/O with my POWER9 system came from the mapping actually 
> resulting in non-NULL invalid pointers, dereferencing which caused a flood 
> of obscure messages produced to the system console by the system firmware:
> 
> LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> IPMI: dropping non severe PEL event
> LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> IPMI: dropping non severe PEL event
> LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> IPMI: dropping non severe PEL event
> LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> IPMI: dropping non severe PEL event
> [...]
> 
> from which it was all but obvious that they were caused by an attempt to 
> use PCI port I/O with a system lacking support for it.
> 
> > It's not clear if having a silently non-working driver or one
> > that crashes makes it easier to debug for users. Having a clear
> > warning message in the PCI probe code is probably the best
> > we can hope for.
> 
>  I agree.  Enthusiastically.

I think there was also a bit of a misunderstanding. My argument that
this would be very ugly in the general case was really meant as general
case outside of drivers like 8250 that deals with both I/O port and
MMIO i.e. we can't warn/error when !HAS_IOPORT deactivates a whole
driver because seeing an I/O port BAR in common PCI code doesn't mean
that it is required for use of the device.

I'm working on a new proposal for 8250 now. Basically I think we can
put the warning/error in serial8250_pci_setup_port(). And then for
those 8250_pci.c "sub drivers" that require I/O ports instead of just
ifdeffing out their setup/init/exit we can define anything but setup to
NULL and setup to pci_default_setup() such that the latter will find
the I/O port BAR via FL_GET_BASE() and subsequently cause the error
print in serial8250_pci_setup_port(). It's admittedly a bit odd but it
also keeps the #ifdefs to just around the code that wouldn't compile.

One thing I'm not happy with yet is the handling around
is_upf_fourport(port) in 8250_pci.c. With !HAS_IOPORT this is defined
as false. With that it makes sure that inb_p()/outb_p() aren't called
but I think this only works because the compiler usually drops the dead
if clause. I think there were previous discussions around this but I
think just like IS_ENABLED() checks this isn't quite correct.

Thanks,
Niklas

Arnd Bergmann Oct. 4, 2024, 12:48 p.m. UTC | #18

On Fri, Oct 4, 2024, at 10:09, Niklas Schnelle wrote:

> I'm working on a new proposal for 8250 now. Basically I think we can
> put the warning/error in serial8250_pci_setup_port(). And then for
> those 8250_pci.c "sub drivers" that require I/O ports instead of just
> ifdeffing out their setup/init/exit we can define anything but setup to
> NULL and setup to pci_default_setup() such that the latter will find
> the I/O port BAR via FL_GET_BASE() and subsequently cause the error
> print in serial8250_pci_setup_port(). It's admittedly a bit odd but it
> also keeps the #ifdefs to just around the code that wouldn't compile.

Right, makes sense.

> One thing I'm not happy with yet is the handling around
> is_upf_fourport(port) in 8250_pci.c. With !HAS_IOPORT this is defined
> as false. With that it makes sure that inb_p()/outb_p() aren't called
> but I think this only works because the compiler usually drops the dead
> if clause. I think there were previous discussions around this but I
> think just like IS_ENABLED() checks this isn't quite correct.

Not sure what you mean, we rely on dead code elimination in all
kinds of places. The only bit to be aware of is that normal
'inline' functions may not get constant-folded all the time,
but anything that is either a macro or __always_inline does
work.

I just verified that the version below does what Maciej
suggested with IS_ENABLED() in 8250-pci, and this works fine.

       Arnd

diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index 6709b6a5f301..784190824aad 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -928,6 +928,14 @@ static int pci_netmos_init(struct pci_dev *dev)
 	return num_serial;
 }
 
+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;
+}
+
 /*
  * These chips are available with optionally one parallel port and up to
  * two serial ports. Unfortunately they all have the same product id.
@@ -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,
@@ -1049,6 +1060,10 @@ static int pci_ite887x_init(struct pci_dev *dev)
 static void pci_ite887x_exit(struct pci_dev *dev)
 {
 	u32 ioport;
+
+	if (!IS_ENABLED(CONFIG_HAS_IOPORT))
+		serial_8250_need_ioport(dev);
+
 	/* the ioport is bit 0-15 in POSIO0R */
 	pci_read_config_dword(dev, ITE_887x_POSIO0, &ioport);
 	ioport &= 0xffff;
@@ -1514,6 +1529,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 +1556,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(dev);
+
 	/* Needed by pci_quatech calls below */
 	port->port.iobase = pci_resource_start(priv->dev, FL_GET_BASE(board->flags));
 	/* Set up the clocking */
@@ -1864,6 +1885,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(dev);
+
 	port->port.flags |= UPF_BUG_THRE;
 	port->port.serial_in = kt_serial_in;
 	port->port.handle_break = kt_handle_break;
@@ -1918,6 +1942,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,6 +1963,9 @@ static void pci_wch_ch38x_exit(struct pci_dev *dev)
 {
 	unsigned long iobase;
 
+	if (!IS_ENABLED(CONFIG_HAS_IOPORT))
+		serial_8250_need_ioport(dev);
+
 	iobase = pci_resource_start(dev, 0);
 	outb(0x0, iobase + CH384_XINT_ENABLE_REG);
 }
@@ -2052,6 +2081,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 +2116,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(dev);
+
 	if (board->num_ports == 4 && idx == 3)
 		offset = 7 * board->uart_offset;
 	else

Niklas Schnelle Oct. 4, 2024, 2:44 p.m. UTC | #19

On Fri, 2024-10-04 at 12:09 +0200, Niklas Schnelle wrote:
> On Wed, 2024-10-02 at 23:59 +0100, Maciej W. Rozycki wrote:
> > On Wed, 2 Oct 2024, Arnd Bergmann wrote:
> > 
---8<---
> > > Part of the problem that Niklas is trying to solve with the
> > > CONFIG_HAS_IOPORT annotations is to prevent an invalid inb()/outb()
> > > from turning into a NULL pointer dereference as it currently does
> > > on architectures that have no way to support PIO but get the
> > > default implementation from asm-generic/io.h.
> > 
> >  It can be worse than that.  Part of my confusion with the defxx driver 
> > trying to do port I/O with my POWER9 system came from the mapping actually 
> > resulting in non-NULL invalid pointers, dereferencing which caused a flood 
> > of obscure messages produced to the system console by the system firmware:
> > 
> > LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> > IPMI: dropping non severe PEL event
> > LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> > IPMI: dropping non severe PEL event
> > LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> > IPMI: dropping non severe PEL event
> > LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> > IPMI: dropping non severe PEL event
> > [...]
> > 
> > from which it was all but obvious that they were caused by an attempt to 
> > use PCI port I/O with a system lacking support for it.
> > 
> > > It's not clear if having a silently non-working driver or one
> > > that crashes makes it easier to debug for users. Having a clear
> > > warning message in the PCI probe code is probably the best
> > > we can hope for.
> > 
> >  I agree.  Enthusiastically.
> 
> I think there was also a bit of a misunderstanding. My argument that
> this would be very ugly in the general case was really meant as general
> case outside of drivers like 8250 that deals with both I/O port and
> MMIO i.e. we can't warn/error when !HAS_IOPORT deactivates a whole
> driver because seeing an I/O port BAR in common PCI code doesn't mean
> that it is required for use of the device.
> 
> I'm working on a new proposal for 8250 now. Basically I think we can
> put the warning/error in serial8250_pci_setup_port(). And then for
> those 8250_pci.c "sub drivers" that require I/O ports instead of just
> ifdeffing out their setup/init/exit we can define anything but setup to
> NULL and setup to pci_default_setup() such that the latter will find
> the I/O port BAR via FL_GET_BASE() and subsequently cause the error
> print in serial8250_pci_setup_port(). It's admittedly a bit odd but it
> also keeps the #ifdefs to just around the code that wouldn't compile.

I've now got this to compile on s390x with the !S390 check removed in
Kconfig. Instead of "misusing" pci_default_setup() I instead added a
pci_fail_io_port_setup() helper that prints the error and returns -
EINVAL.

> 
> One thing I'm not happy with yet is the handling around
> is_upf_fourport(port) in 8250_pci.c. With !HAS_IOPORT this is defined
> as false. With that it makes sure that inb_p()/outb_p() aren't called
> but I think this only works because the compiler usually drops the dead
> if clause. I think there were previous discussions around this but I
> think just like IS_ENABLED() checks this isn't quite correct.

It's not the nicest but I added the necessary #ifdefs in 8250_port.c
and at least they are small sections. As Arnd said we will go back to a
single series. So I've also switched to a b4 prep workflow which I
usually use nowadays and so the current code can be found here:

https://git.kernel.org/pub/scm/linux/kernel/git/niks/linux.git/log/?h=b4/has_ioport

I plan to resend on Monday based on v6.12-rc2 which will also include
the bcachefs fix to fix building on Big Endian which I was previously
carrying for my s390x development convenience.

Thanks,
Niklas

Niklas Schnelle Oct. 4, 2024, 4:03 p.m. UTC | #20

On Fri, 2024-10-04 at 12:48 +0000, Arnd Bergmann wrote:
> On Fri, Oct 4, 2024, at 10:09, Niklas Schnelle wrote:
> 
> > I'm working on a new proposal for 8250 now. Basically I think we can
> > put the warning/error in serial8250_pci_setup_port(). And then for
> > those 8250_pci.c "sub drivers" that require I/O ports instead of just
> > ifdeffing out their setup/init/exit we can define anything but setup to
> > NULL and setup to pci_default_setup() such that the latter will find
> > the I/O port BAR via FL_GET_BASE() and subsequently cause the error
> > print in serial8250_pci_setup_port(). It's admittedly a bit odd but it
> > also keeps the #ifdefs to just around the code that wouldn't compile.
> 
> Right, makes sense.
> 
> > One thing I'm not happy with yet is the handling around
> > is_upf_fourport(port) in 8250_pci.c. With !HAS_IOPORT this is defined
> > as false. With that it makes sure that inb_p()/outb_p() aren't called
> > but I think this only works because the compiler usually drops the dead
> > if clause. I think there were previous discussions around this but I
> > think just like IS_ENABLED() checks this isn't quite correct.
> 
> Not sure what you mean, we rely on dead code elimination in all
> kinds of places. The only bit to be aware of is that normal
> 'inline' functions may not get constant-folded all the time,
> but anything that is either a macro or __always_inline does
> work.

Ah ok, didn't know this was okay to rely on. Then I can roll the extra
#ifdefs back. Need to address the test bot's finding anyway. There we
can get #ifdef __i386__ but also !HAS_IOPORT on um.

> 
> I just verified that the version below does what Maciej
> suggested with IS_ENABLED() in 8250-pci, and this works fine.
> 
>        Arnd

Your version below is also pretty nice a bit more spread out but less
#ifdefs. That said at least in my NeoVim setup the #ifdefs are nicely
grayed out via clang-analyzer / lsp which to me actually makes #ifdefs
quite easy to see at a glance but that's probably a niche opinion.

> 
> diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
> index 6709b6a5f301..784190824aad 100644
> --- a/drivers/tty/serial/8250/8250_pci.c
> +++ b/drivers/tty/serial/8250/8250_pci.c
> @@ -928,6 +928,14 @@ static int pci_netmos_init(struct pci_dev *dev)
>  	return num_serial;
>  }
>  
> +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;
> +}
> +
---8<---

Maciej W. Rozycki Oct. 4, 2024, 4:24 p.m. UTC | #21

On Fri, 4 Oct 2024, Arnd Bergmann wrote:

> >  It can be worse than that.  Part of my confusion with the defxx driver 
> > trying to do port I/O with my POWER9 system came from the mapping actually 
> > resulting in non-NULL invalid pointers, dereferencing which caused a flood 
> > of obscure messages produced to the system console by the system firmware:
> >
> > LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> > IPMI: dropping non severe PEL event
> > LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> > IPMI: dropping non severe PEL event
> > LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> > IPMI: dropping non severe PEL event
> > LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> > IPMI: dropping non severe PEL event
> > [...]
> >
> > from which it was all but obvious that they were caused by an attempt to 
> > use PCI port I/O with a system lacking support for it.
> 
> Ah, that's too bad. I think this is a result of the patch that
> Michael did to shut up the NULL pointer warning we get, see
> be140f1732b5 ("powerpc/64: Set _IO_BASE to POISON_POINTER_DELTA
> not 0 for CONFIG_PCI=n"). I really wish we could have finished
> Niklas' series earlier to avoid this.

 That was back in 2020 (5.9.0), so long before be140f1732b5, and it's a 
production system, so I don't want to fiddle with it beyond necessity:

$ uptime
 16:53:27 up 466 days,  9:49,  5 users,  load average: 0.14, 0.12, 0.09
$ 

 Essentially defxx has this code (not changed much since, except that 
`dfx_use_mmio' is now always true for PCI):

	if (!dfx_use_mmio)
		region = request_region(bar_start[0], bar_len[0],
					bdev->driver->name);
	if (!region) {
		dfx_register_res_err(print_name, dfx_use_mmio,
				     bar_start[0], bar_len[0]);
		err = -EBUSY;
		goto err_out_disable;
	}
	/* ... */
	if (dfx_use_mmio) {
		/* ... */
	} else {
		bp->base.port = bar_start[0];
		dev->base_addr = bar_start[0];
	}

so whatever came out of BAR[1]:

pci 0031:02:04.0: [1011:000f] type 00 class 0x020200
pci 0031:02:04.0: reg 0x10: [mem 0x620c080020000-0x620c08002007f]
pci 0031:02:04.0: reg 0x14: [io  0x0000-0x007f]
pci 0031:02:04.0: reg 0x18: [mem 0x620c080030000-0x620c08003ffff]
pci 0031:02:04.0: BAR0 [mem size 0x00000080]: requesting alignment to 0x10000

was supplied to `inl'/`outl' and wreaked havoc.

 First of all I think `request_region', etc. ought to fail in the first 
place with non-port-I/O systems: why does a request for a resource that 
cannot be satisfied succeed?  It would have already solved the issue with 
defxx, making the driver gracefully fail to register the device.  I don't 
know if this has been fixed since.

 Second, rather than relying on a magic mapping in the physical space 
causing a bus error (unless you are absolutely sure it is going to work in 
100% cases), I think it would make sense to make port I/O accessors call 
BUG_ON(no_port_io) explicitly for platforms where it is only known at run 
time whether PCI/e port I/O is available or not.  Port I/O is the opposite 
of performance already, so a couple of extra instructions will be lost in 
the latency and at least POWER has conditional traps, so there'd be no 
branch penalty.

> >  Well, virtually all non-x86 systems continue supporting PCI/e port I/O 
> > via a suitably decoded CPU-side MMIO window, so I think coming across one 
> > that doesn't can still be a nasty surprise even in 2024.  For instance 
> > I've been happily using a PC parallel port PCIe option card, one of the 
> > very few interfaces if not the only one remaining that have not ever seen 
> > an MMIO variant, with my RISC-V hardware, newer than said POWER9 system.
> >
> >  So far it's been the s390 and a couple of POWER system implementations 
> > that have support for PCI/e port I/O removed.  Have I missed anything?
> 
> I meant PCIe cards with I/O space here, not host bridges. I know you
> have a lot of them, but what I've heard from Arm platform maintainers
> is that they tend to struggle finding any PCIe cards to test their
> hsot bridge drivers on, and I expect that a lot of them are actually
> broken because they have never been tested and just copied the
> implementation badly from some other driver.

 I would expect serial ports to be the most common PCIe options still 
using port I/O.  Sadly OxSemi was acquired and their line of devices, 
which support MMIO, cancelled at one point and my observations seem to 
indicate that what is still manufactured uses port I/O (correct me if I'm 
wrong please).  Last time I checked OxSemi-based option cards were still 
available though, but one may have to check with the supplier as to 
whether they have been configured for MMIO or port I/O, as they're not 
dual-mapped.

> I think the only new PCIe devices you can find today that still use
> I/O space are ones with compatibility registers for IBM PC style
> hardware (vga, uart, parport), but most users would never have used
> one of those and instead use the native register interface of their
> GPU (on non-x86), USB-serial and no parport. Other devices that
> needed I/O space never worked on PCIe anyway because of the lack
> of ISA style DMA.

 I think serial port options are the most likely devices still in use, 
given that UARTs continue being widely used in industrial applications.  
Depending on application a USB serial adapter may or may not be suitable 
to interface those.

> There are also a lot of Arm systems that have no I/O space support at
> all, such as the Apple M2 I'm using at the moment.

 Thanks for letting me know.  Is it AArch64 only that has no port I/O 
support in the PCIe root complex nowadays, or is it 32-bit ARM as well?

  Maciej

Maciej W. Rozycki Oct. 4, 2024, 4:34 p.m. UTC | #22

On Fri, 4 Oct 2024, Niklas Schnelle wrote:

> >  Can we just provide dummy prototypes with `__attribute__((error("...")))' 
> > instead?  This will surely prevent us from having to bend backwards so as 
> > to make sure the compiler won't see any spurious references to these 
> > inexistent functions or macros.  We already have a `__compiletime_error()' 
> > macro for this purpose even.
> 
> This is already done in the final patch of my series when disabling
> inb()/outb() and friends.

 Good!

> >  I agree.  Enthusiastically.
> 
> I think there was also a bit of a misunderstanding. My argument that
> this would be very ugly in the general case was really meant as general
> case outside of drivers like 8250 that deals with both I/O port and
> MMIO i.e. we can't warn/error when !HAS_IOPORT deactivates a whole
> driver because seeing an I/O port BAR in common PCI code doesn't mean
> that it is required for use of the device.

 Absolutely.  Just seeing an I/O bar does not mean it's ever going to be 
used.  Even conventional PCI hardware is often dual-mapped and it's up to 
the driver to choose which mapping to use.

> I'm working on a new proposal for 8250 now. Basically I think we can
> put the warning/error in serial8250_pci_setup_port(). And then for
> those 8250_pci.c "sub drivers" that require I/O ports instead of just
> ifdeffing out their setup/init/exit we can define anything but setup to
> NULL and setup to pci_default_setup() such that the latter will find
> the I/O port BAR via FL_GET_BASE() and subsequently cause the error
> print in serial8250_pci_setup_port(). It's admittedly a bit odd but it
> also keeps the #ifdefs to just around the code that wouldn't compile.

 I'd rather you did what Arnd example patch does and just made original 
handlers bail out right away unless IS_ENABLED(CONFIG_HAS_IOPORT).  I do 
hope it will make no #ifdef necessary in 8250_pci.c at all.

  Maciej

Arnd Bergmann Oct. 4, 2024, 4:57 p.m. UTC | #23

On Fri, Oct 4, 2024, at 16:24, Maciej W. Rozycki wrote:
> On Fri, 4 Oct 2024, Arnd Bergmann wrote:
>> There are also a lot of Arm systems that have no I/O space support at
>> all, such as the Apple M2 I'm using at the moment.
>
>  Thanks for letting me know.  Is it AArch64 only that has no port I/O 
> support in the PCIe root complex nowadays, or is it 32-bit ARM as well?

I'm fairly sure we have some on 32-bit as well, it's certainly
up to the specific SoC rather than the architecture.

I think I've seen three different cases:

- Apple leaves out every feature from hardware that they don't
  have to do for compliance, this also includes CPU stuff like
  32-bit mode, big-endian or cacheable PCI mappings. I think IBMs
  ppc64 chips are in a similar situation

- Some devices can probably do I/O space in hardware, but this is
  left out from the driver or the DT because it has not been
  validated to work

- Some devices have a limited number of mapping windows that
  are shared between prefetchable-memory, non-prefetchable-memory,
  config and io space. Leaving out I/O space completely is
  easier than runtime remapping of the windows

      Arnd

Guenter Roeck Nov. 22, 2024, 3:18 p.m. UTC | #24

On Fri, Apr 05, 2024 at 05:29:24PM +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 unconditionally. For 8250 based drivers some support
> MMIO only use so fence only the parts requiring I/O ports.
> 
> 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>
...
> @@ -422,10 +443,12 @@ static void set_io_from_upio(struct uart_port *p)
>  	up->dl_write = default_serial_dl_write;
>  
> +	default:
> +		WARN(1, "Unsupported UART type %x\n", p->iotype);

So, according to this patch, the serial uart on microblaze, nios2,
openrisc, xtensa, and possibly others is not or no longer supported.

WARNING: CPU: 0 PID: 0 at drivers/tty/serial/8250/8250_port.c:470 serial8250_set_defaults+0x1a8/0x22c
Unsupported UART type 0

Any special reason ?

Guenter

Arnd Bergmann Nov. 22, 2024, 4:31 p.m. UTC | #25

On Fri, Nov 22, 2024, at 16:35, Niklas Schnelle wrote:
> On Fri, 2024-11-22 at 07:18 -0800, Guenter Roeck wrote:
>> On Fri, Apr 05, 2024 at 05:29:24PM +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 unconditionally. For 8250 based drivers some support
>> > MMIO only use so fence only the parts requiring I/O ports.
>> > 
>> > 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>
>> ...
>> > @@ -422,10 +443,12 @@ static void set_io_from_upio(struct uart_port *p)
>> >  	up->dl_write = default_serial_dl_write;
>> >  
>> > +	default:
>> > +		WARN(1, "Unsupported UART type %x\n", p->iotype);
>> 
>> So, according to this patch, the serial uart on microblaze, nios2,
>> openrisc, xtensa, and possibly others is not or no longer supported.
>> 
>> WARNING: CPU: 0 PID: 0 at drivers/tty/serial/8250/8250_port.c:470 serial8250_set_defaults+0x1a8/0x22c
>> Unsupported UART type 0
>> 
>> Any special reason ?
>
> So according to the warning the p->iotype is 0 which is UPIO_PORT.
> For UPIO_PORT the switch above the WARN would pick io_serial_in() and
> io_serial_out() as handlers. These use inb() respectively outb() to
> access the serial so I don't see how they would work with !HAS_IOPORT
> and it most definitely won't work for s390.
>
> Now for Microblaze Kconfig says to select HAS_IOPORT if PCI so I'd
> assume that it can use inb()/outb() and maybe the PCI requirement is
> not correct if this isn't a PCI device and it used to work with
> inb()/outb()? For nios2, openrisc, and xtensa they don't select
> HAS_IOPORT so either it really won't work anyway or they should select
> it. Can you tell us more about the devices involved where you saw this?

I've tried to have a quick look at the four architectures, here
is what I see in the sources:

- on microblaze, the default uart is xlnx,xps-uartlite-1.00.a,
  and there is no 8250. The PCI code that theoretically supports
  I/O port access through an ISA bridge (copied from powerpc),
  but there is currently no code to set up the I/O space window,
  so in practice the port I/O is just a NULL pointer dereference.

- nios2 has a 16550 compatible UART listed in the devicetree
  file, but this uses normal UPIO_MEM registers, not ISA-style
  I/O ports. There is no support for ISA or PCI.

- openrisc has no support for port I/O, like on nios2 the
  16550 style uart is on UPIO_MEM according to the devicetree

- xtensa manually sets up a UPIO_MEM uart in its board files
  and in its dts files. The PCI support does have code to
  hand port I/O, but it looks incorrect to me and either broke
  at some point or never worked. Most likely this was copied
  incorrectly from old powerpc or sparc code where it worked.

So in all four cases, the normal uart should keep working,
and if something tried to start up an ISA style 8250, I
would expect to see the new version produce the WARN()
in place of what was a NULL pointer dereference (reading
from (u8 *)0x2f8) before.

Since there are so many ways to set up a broken 8250,
it is still possible that something tries to add another
UPIO_PORT uart, and that this causes the WARN() to trigger,
if we find any of those, the fix is to no stop registering
broken ports.

     Arnd

Guenter Roeck Nov. 22, 2024, 5:22 p.m. UTC | #26

On 11/22/24 08:31, Arnd Bergmann wrote:
> On Fri, Nov 22, 2024, at 16:35, Niklas Schnelle wrote:
>> On Fri, 2024-11-22 at 07:18 -0800, Guenter Roeck wrote:
>>> On Fri, Apr 05, 2024 at 05:29:24PM +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 unconditionally. For 8250 based drivers some support
>>>> MMIO only use so fence only the parts requiring I/O ports.
>>>>
>>>> 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>
>>> ...
>>>> @@ -422,10 +443,12 @@ static void set_io_from_upio(struct uart_port *p)
>>>>   	up->dl_write = default_serial_dl_write;
>>>>   
>>>> +	default:
>>>> +		WARN(1, "Unsupported UART type %x\n", p->iotype);
>>>
>>> So, according to this patch, the serial uart on microblaze, nios2,
>>> openrisc, xtensa, and possibly others is not or no longer supported.
>>>
>>> WARNING: CPU: 0 PID: 0 at drivers/tty/serial/8250/8250_port.c:470 serial8250_set_defaults+0x1a8/0x22c
>>> Unsupported UART type 0
>>>
>>> Any special reason ?
>>
>> So according to the warning the p->iotype is 0 which is UPIO_PORT.
>> For UPIO_PORT the switch above the WARN would pick io_serial_in() and
>> io_serial_out() as handlers. These use inb() respectively outb() to
>> access the serial so I don't see how they would work with !HAS_IOPORT
>> and it most definitely won't work for s390.
>>
>> Now for Microblaze Kconfig says to select HAS_IOPORT if PCI so I'd
>> assume that it can use inb()/outb() and maybe the PCI requirement is
>> not correct if this isn't a PCI device and it used to work with
>> inb()/outb()? For nios2, openrisc, and xtensa they don't select
>> HAS_IOPORT so either it really won't work anyway or they should select
>> it. Can you tell us more about the devices involved where you saw this?
> 
> I've tried to have a quick look at the four architectures, here
> is what I see in the sources:
> 
> - on microblaze, the default uart is xlnx,xps-uartlite-1.00.a,
>    and there is no 8250. The PCI code that theoretically supports
>    I/O port access through an ISA bridge (copied from powerpc),
>    but there is currently no code to set up the I/O space window,
>    so in practice the port I/O is just a NULL pointer dereference.
> 
> - nios2 has a 16550 compatible UART listed in the devicetree
>    file, but this uses normal UPIO_MEM registers, not ISA-style
>    I/O ports. There is no support for ISA or PCI.
> 
> - openrisc has no support for port I/O, like on nios2 the
>    16550 style uart is on UPIO_MEM according to the devicetree
> 
> - xtensa manually sets up a UPIO_MEM uart in its board files
>    and in its dts files. The PCI support does have code to
>    hand port I/O, but it looks incorrect to me and either broke
>    at some point or never worked. Most likely this was copied
>    incorrectly from old powerpc or sparc code where it worked.
> 
> So in all four cases, the normal uart should keep working,
> and if something tried to start up an ISA style 8250, I
> would expect to see the new version produce the WARN()
> in place of what was a NULL pointer dereference (reading
> from (u8 *)0x2f8) before.
> 
> Since there are so many ways to set up a broken 8250,
> it is still possible that something tries to add another
> UPIO_PORT uart, and that this causes the WARN() to trigger,
> if we find any of those, the fix is to no stop registering
> broken ports.
> 

The call chain in all cases is

[    0.013596] Call Trace:
[    0.013796]  [<d06eb249>] dump_stack+0x9/0xc
[    0.014115]  [<d000c12c>] __warn+0x94/0xbc
[    0.014332]  [<d000c29c>] warn_slowpath_fmt+0x148/0x184
[    0.014560]  [<d04f03d8>] set_io_from_upio+0x70/0x98
[    0.014781]  [<d04f0459>] serial8250_set_defaults+0x59/0x8c
[    0.015013]  [<d04efa6a>] serial8250_setup_port+0x6e/0x90
[    0.015240]  [<d0ae2a12>] serial8250_isa_init_ports+0x32/0x5c
[    0.015473]  [<d0ae28a1>] univ8250_console_init+0x15/0x24
[    0.015698]  [<d0ad0684>] console_init+0x18/0x20
[    0.015926]  [<d0acbf43>] start_kernel+0x3db/0x4cc
[    0.016145]  [<d06ebc37>] _startup+0x13b/0x13b

That seems unconditional. What is the architecture expected to do to
prevent this call chain from being executed ?

Guenter

Guenter Roeck Nov. 22, 2024, 8:44 p.m. UTC | #27

On 11/22/24 11:24, Arnd Bergmann wrote:
> On Fri, Nov 22, 2024, at 18:22, Guenter Roeck wrote:
>> On 11/22/24 08:31, Arnd Bergmann wrote:
>>> On Fri, Nov 22, 2024, at 16:35, Niklas Schnelle wrote:
>>>> On Fri, 2024-11-22 at 07:18 -0800, Guenter Roeck wrote:
>>> So in all four cases, the normal uart should keep working,
>>> and if something tried to start up an ISA style 8250, I
>>> would expect to see the new version produce the WARN()
>>> in place of what was a NULL pointer dereference (reading
>>> from (u8 *)0x2f8) before.
>>>
>>> Since there are so many ways to set up a broken 8250,
>>> it is still possible that something tries to add another
>>> UPIO_PORT uart, and that this causes the WARN() to trigger,
>>> if we find any of those, the fix is to no stop registering
>>> broken ports.
>>>
>>
>> The call chain in all cases is
>>
>> [    0.013596] Call Trace:
>> [    0.013796]  [<d06eb249>] dump_stack+0x9/0xc
>> [    0.014115]  [<d000c12c>] __warn+0x94/0xbc
>> [    0.014332]  [<d000c29c>] warn_slowpath_fmt+0x148/0x184
>> [    0.014560]  [<d04f03d8>] set_io_from_upio+0x70/0x98
>> [    0.014781]  [<d04f0459>] serial8250_set_defaults+0x59/0x8c
>> [    0.015013]  [<d04efa6a>] serial8250_setup_port+0x6e/0x90
>> [    0.015240]  [<d0ae2a12>] serial8250_isa_init_ports+0x32/0x5c
>> [    0.015473]  [<d0ae28a1>] univ8250_console_init+0x15/0x24
>> [    0.015698]  [<d0ad0684>] console_init+0x18/0x20
>> [    0.015926]  [<d0acbf43>] start_kernel+0x3db/0x4cc
>> [    0.016145]  [<d06ebc37>] _startup+0x13b/0x13b
>>
>> That seems unconditional. What is the architecture expected to do to
>> prevent this call chain from being executed ?
> 
> This looks like a bug in drivers/tty/serial/8250/8250_platform.c
> to me, not something the architectures did wrong. My impression
> from __serial8250_isa_init_ports() is that this is supposed
> to initialize exactly the ports in the old_serial_port[]
> array, which is filled only on alpha, m68k and x86 but not
> on the other ones.
> 
> Andy moved this code in ffd8e8bd26e9 ("serial: 8250: Extract
> platform driver"), but I don't think this move by itself
> changed anything.
> 
> serial8250_setup_port() is where it ends up using the
> uninitialized serial8250_ports[index] contents. Since
> port->iotype is not set to anything here, it defaults to
> '0', which happens to be UPIO_PORT.
> 
> The reason it doesn't immediately crash and burn is that
> this is still only setting up the structures for later
> use, but I assume that trying to use console=ttyS0, or
> opening /dev/ttyS0 on the uninitialized port would
> then cause an oops.
> 

All four affected platforms use ttyS0, only it is mmio based,
not io port based.

Guenter

> The bit I'm less sure about is why the
> serial8250_setup_port() function is called here in
> the first place. I assume it does something for
> /some/ architecture, but it's clearly wrong for
> most of them.
> 
>         Arnd

Niklas Schnelle Nov. 22, 2024, 11:27 p.m. UTC | #28

On Fri, 2024-11-22 at 09:07 -0800, Guenter Roeck wrote:
> On 11/22/24 07:35, Niklas Schnelle wrote:
> > On Fri, 2024-11-22 at 07:18 -0800, Guenter Roeck wrote:
> > > On Fri, Apr 05, 2024 at 05:29:24PM +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 unconditionally. For 8250 based drivers some
> > > > support
> > > > MMIO only use so fence only the parts requiring I/O ports.
> > > > 
> > > > 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>
> > > ...
> > > > @@ -422,10 +443,12 @@ static void set_io_from_upio(struct
> > > > uart_port *p)
> > > >   	up->dl_write = default_serial_dl_write;
> > > >   
> > > > +	default:
> > > > +		WARN(1, "Unsupported UART type %x\n", p-
> > > > >iotype);
> > > 
> > > So, according to this patch, the serial uart on microblaze,
> > > nios2,
> > > openrisc, xtensa, and possibly others is not or no longer
> > > supported.
> > > 
> > > WARNING: CPU: 0 PID: 0 at drivers/tty/serial/8250/8250_port.c:470
> > > serial8250_set_defaults+0x1a8/0x22c
> > > Unsupported UART type 0
> > > 
> > > Any special reason ?
> > > 
> > > Guenter
> > 
> > So according to the warning the p->iotype is 0 which is UPIO_PORT.
> > For UPIO_PORT the switch above the WARN would pick io_serial_in()
> > and
> > io_serial_out() as handlers. These use inb() respectively outb() to
> > access the serial so I don't see how they would work with
> > !HAS_IOPORT
> > and it most definitely won't work for s390.
> > 
> > Now for Microblaze Kconfig says to select HAS_IOPORT if PCI so I'd
> > assume that it can use inb()/outb() and maybe the PCI requirement
> > is
> > not correct if this isn't a PCI device and it used to work with
> > inb()/outb()? For nios2, openrisc, and xtensa they don't select
> > HAS_IOPORT so either it really won't work anyway or they should
> > select
> > it. Can you tell us more about the devices involved where you saw
> > this?
> > 
> 
> This is seen when booting the affected architectures with qemu.
> 
> Logs:
> 
> https://kerneltests.org/builders/qemu-microblaze-master/builds/327/steps/qemubuildcommand/logs/stdio
> https://kerneltests.org/builders/qemu-nios2-master/builds/314/steps/qemubuildcommand/logs/stdio
> https://kerneltests.org/builders/qemu-openrisc-master/builds/301
> https://kerneltests.org/builders/qemu-xtensa-master/builds/311/steps/qemubuildcommand/logs/stdio
> 
> Guenter

Am I seeing it right that despite the warning and the code setting
no_serial_in / no_serial_out the console=ttyS0 in the above qemu boots
still worked? Also for example in the nios2 case I see the warning 4
times. So this makes me wonder since the warning is new is it possible
that set_io_from_upio() has been called with an invalid / all
zero port before but it was invisible.

The way I'm reading __serial8250_isa_init_ports() and in particular the
first loop if nr_uarts is e.g. 5 in the nios case but only the first
entry in serial8250_ports[] has the IOMEM 8250 it will still call
serial8250_setup_port() on the 4 other unintalized/zero elements which
would explain the iotype being 0. And as far as I can see nr_uarts is
just set to the value of CONFIG_SERIAL_8250_RUNTIME_UARTS in
8250_platform.c.

I may be totally off though this console_init() stuff has me a little
confused and it's been a long day.

Regards,
Niklas

[1/1] tty: serial: handle HAS_IOPORT dependencies

Commit Message

Comments

Patch