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

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

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
>

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

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.

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?

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.

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.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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?

Thanks,
Niklas

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

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

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

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.

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

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

On Fri, Nov 22, 2024, at 21:44, Guenter Roeck wrote:
> On 11/22/24 11:24, Arnd Bergmann wrote:
>> 
>> 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.

Right, so I think the problem is really the 8250_platform.c
file trying to handle all possible cases on x86 (acpi,
isapnp, legacy isa) that may define the same UARTs, but
also trying to handle non-legacy ports with their own
special cases on other architectures.

The patch below is a first idea of how we can skip the
legacy ISA case on architectures that don't define those.
If this works, we can try to come up with a better way of
doing that. Ideally all the pre-DT boardfile machines that
define their own "serial8250" platform_device (with platform_data)
should use a different identifier the x86-legacy case that
uses the platform_device (without platform_data). Similarly,
I think the riscv special ACPI port needs its own trivial
driver without all the ISA magic.

      Arnd

diff --git a/drivers/tty/serial/8250/8250_platform.c b/drivers/tty/serial/8250/8250_platform.c
index 66fd6d5d4835..610b31517734 100644
--- a/drivers/tty/serial/8250/8250_platform.c
+++ b/drivers/tty/serial/8250/8250_platform.c
@@ -101,7 +101,8 @@ static void __init __serial8250_isa_init_ports(void)
 
 void __init serial8250_isa_init_ports(void)
 {
-	DO_ONCE(__serial8250_isa_init_ports);
+	if (ARRAY_SIZE(old_serial_port))
+		DO_ONCE(__serial8250_isa_init_ports);
 }
 
 /*
@@ -283,7 +284,7 @@ static const struct acpi_device_id acpi_platform_serial_table[] = {
 };
 MODULE_DEVICE_TABLE(acpi, acpi_platform_serial_table);
 
-static struct platform_driver serial8250_isa_driver = {
+static struct platform_driver serial8250_platform_driver = {
 	.probe		= serial8250_probe,
 	.remove		= serial8250_remove,
 	.suspend	= serial8250_suspend,
@@ -300,7 +301,7 @@ static struct platform_driver serial8250_isa_driver = {
  */
 struct platform_device *serial8250_isa_devs;
 
-static int __init serial8250_init(void)
+static int __init serial8250_isa_init(void)
 {
 	int ret;
 
@@ -337,11 +338,8 @@ static int __init serial8250_init(void)
 
 	serial8250_register_ports(&serial8250_reg, &serial8250_isa_devs->dev);
 
-	ret = platform_driver_register(&serial8250_isa_driver);
-	if (ret == 0)
-		goto out;
+	return 0;
 
-	platform_device_del(serial8250_isa_devs);
 put_dev:
 	platform_device_put(serial8250_isa_devs);
 unreg_pnp:
@@ -355,6 +353,17 @@ static int __init serial8250_init(void)
 out:
 	return ret;
 }
+
+static int __init serial8250_init(void)
+{
+	if (ARRAY_SIZE(old_serial_port)) {
+		int ret = serial8250_isa_init();
+		if (ret)
+			return ret;
+	}
+
+	return platform_driver_register(&serial8250_platform_driver);
+}
 module_init(serial8250_init);
 
 static void __exit serial8250_exit(void)
@@ -368,7 +377,11 @@ static void __exit serial8250_exit(void)
 	 */
 	serial8250_isa_devs = NULL;
 
-	platform_driver_unregister(&serial8250_isa_driver);
+	platform_driver_unregister(&serial8250_platform_driver);
+
+	if (!ARRAY_SIZE(old_serial_port))
+		return;
+
 	platform_device_unregister(isa_dev);
 
 	serial8250_pnp_exit();

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

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

On 11/22/24 14:51, Arnd Bergmann wrote:
> On Fri, Nov 22, 2024, at 21:44, Guenter Roeck wrote:
>> On 11/22/24 11:24, Arnd Bergmann wrote:
>>>
>>> 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.
> 
> Right, so I think the problem is really the 8250_platform.c
> file trying to handle all possible cases on x86 (acpi,
> isapnp, legacy isa) that may define the same UARTs, but
> also trying to handle non-legacy ports with their own
> special cases on other architectures.
> 
> The patch below is a first idea of how we can skip the
> legacy ISA case on architectures that don't define those.
> If this works, we can try to come up with a better way of
> doing that. Ideally all the pre-DT boardfile machines that
> define their own "serial8250" platform_device (with platform_data)
> should use a different identifier the x86-legacy case that
> uses the platform_device (without platform_data). Similarly,
> I think the riscv special ACPI port needs its own trivial
> driver without all the ISA magic.
> 
>        Arnd
> 
> diff --git a/drivers/tty/serial/8250/8250_platform.c b/drivers/tty/serial/8250/8250_platform.c
> index 66fd6d5d4835..610b31517734 100644
> --- a/drivers/tty/serial/8250/8250_platform.c
> +++ b/drivers/tty/serial/8250/8250_platform.c

That didn't work.



[    0.005735] Console: colour dummy device 80x25
[    0.006393] BUG: spinlock bad magic on CPU#0, swapper/0
[    0.006459]  lock: serial8250_ports+0x0/0x980, .magic: 00000000, .owner: <none>/-1, .owner_cpu: 0
[    0.007102] CPU: 0 UID: 0 PID: 0 Comm: swapper Not tainted 6.12.0-07881-g6206e39999ae #1
[    0.007359] Stack:
[    0.007671] > 00000100 00000000 d09f9e10 d0951c2c
[    0.007765]   d0951c2c 00000000 00000000 d0b06170
[    0.007841]   9003f5ee d09f9e60 00000055 d0dd9900
[    0.007884]   00000000 00000004 d09f9e40 00000000
[    0.007925]   9003f681 d09f9e80 d0dd9900 d08f5924
[    0.007976] > 00000000 d08f5890 ffffffff 00000000
[    0.008019]   906f66b9 d09f9ea0 d0dd9900 00000000
[    0.008059] > 00000000 00000480 00000000 d0a06d74
[    0.008099]   904efe6a d09f9ec0 d0dd9900 00060300
[    0.008139] > 00000000 00000000 00000001 00000000
[    0.008180]   9004368d d09f9ef0 d0aa7238 d7ff70a4
[    0.008220] > d04efe6a 00000000 00000001 00000000
[    0.008260]   00000000 00000000 00000001 00000000
[    0.008301]   90044a84 d09f9f10 d0aa7238 d04efe14
[    0.008340] > d0dd9900 000000c4 d0a06d74 d0a00400
[    0.008381]   90ae6710 d09f9f30 d0aa7238 d0cedea0
[    0.008443] Call Trace:
[    0.008502]  [<d06ed63d>] dump_stack+0x9/0xc
[    0.008624]  [<d003f5ee>] spin_dump+0x4a/0x78
[    0.008677]  [<d003f681>] do_raw_spin_lock+0x1d/0x68
[    0.008727]  [<d06f66b9>] _raw_spin_lock_irqsave+0x41/0x54
[    0.008780]  [<d04efe6a>] univ8250_console_setup+0x56/0x104
[    0.008829]  [<d004368d>] try_enable_preferred_console+0x75/0xf4
[    0.008877]  [<d0044a84>] register_console+0xf4/0x3e4
[    0.008923]  [<d0ae6710>] univ8250_console_init+0x20/0x24
[    0.008970]  [<d0ad44f0>] console_init+0x18/0x20
[    0.009018]  [<d0acfdaf>] start_kernel+0x3db/0x4cc
[    0.009063]  [<d06ee02b>] _startup+0x13b/0x13b

That was xtensa. Others are just silent.

Guenter

On Sat, Nov 23, 2024, at 00:34, Niklas Schnelle wrote:
>
> 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.

Yes, that is certainly the case, sorry if I wasn't clear about this.

The warning shows that there is something wrong with the code,
but that problem has likely existed for a long time. We can
obviously just hide that warning again and ignore the underlying
problem without causing any regressions to the previous state,
but I hope we can improve it in some way that makes it less
broken on non-x86 architectures.

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

The way I was reading the code, all five would be uninitialized
at the time we call __serial8250_isa_init_ports(), and the first
port only gets set later on.

Another thing I see is that the 8250 driver ("serial") is the
only one that ends up registering a lot of ports at boot time,
while the other ones only register the ones they actually detect.

E.g. on my Apple workstation, I get

# head /proc/tty/driver/*
==> /proc/tty/driver/IMX-uart <==
serinfo:1.0 driver revision:

==> /proc/tty/driver/fsl-linflexuart <==
serinfo:1.0 driver revision:

==> /proc/tty/driver/fsl-lpuart <==
serinfo:1.0 driver revision:

==> /proc/tty/driver/msm_serial <==
serinfo:1.0 driver revision:

==> /proc/tty/driver/mvebu_serial <==
serinfo:1.0 driver revision:

==> /proc/tty/driver/qcom_geni_console <==
serinfo:1.0 driver revision:

==> /proc/tty/driver/qcom_geni_uart <==
serinfo:1.0 driver revision:

==> /proc/tty/driver/s3c2410_serial <==
serinfo:1.0 driver revision:
0: uart:APPLE S5L mmio:0x39B200000 irq:42 tx:0 rx:0 CTS|DSR|CD

==> /proc/tty/driver/serial <==
serinfo:1.0 driver revision:
0: uart:unknown port:00000000 irq:0
1: uart:unknown port:00000000 irq:0
2: uart:unknown port:00000000 irq:0
3: uart:unknown port:00000000 irq:0
4: uart:unknown port:00000000 irq:0
5: uart:unknown port:00000000 irq:0
6: uart:unknown port:00000000 irq:0
7: uart:unknown port:00000000 irq:0
8: uart:unknown port:00000000 irq:0

==> /proc/tty/driver/tegra_hsuart <==
serinfo:1.0 driver revision:

==> /proc/tty/driver/usbserial <==
usbserinfo:1.0 driver:2.0

The way that this driver was meant to handle that originally
is that /sbin/setserial can be used at runtime to configure
any UART that is attached to an ISA bus even if it did not
get detected at boot time, by setting the correct port and
IRQ numbers from userspace. This may be required on i486
systems with ISA cards providing additional non-PNP 8250
on a nonstandard port number, but is likely not useful on
anything other than x86-32.

      Arnd

On Fri, Nov 22, 2024 at 08:24:37PM +0100, 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.

Yep. the idea was to purely split this code out of the core
library parts. I was not intending any functional changes.

I believe it's a preexisted bug, but if you can point out to
the breakage I made, please do it, so I would be able to fix
ASAP.

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

Btw, we have a new constant to tell the 8250 core that IO type is not set,
but that one is not used here.

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

On Mon, Nov 25, 2024, at 08:55, Andy Shevchenko wrote:
> On Fri, Nov 22, 2024 at 08:24:37PM +0100, Arnd Bergmann wrote:
>> On Fri, Nov 22, 2024, at 18:22, Guenter Roeck wrote:
>> 
>> 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.
>
> Yep. the idea was to purely split this code out of the core
> library parts. I was not intending any functional changes.

Ok.

> I believe it's a preexisted bug, but if you can point out to
> the breakage I made, please do it, so I would be able to fix
> ASAP.
>
>> 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.
>
> Btw, we have a new constant to tell the 8250 core that IO type is not set,
> but that one is not used here.

So I see serial8250_setup_port() setting "up->cur_iotype = 0xFF"
by first calling serial8250_init_port(), this is the "not
set" value you mean, right?.

Right after that it calls serial8250_set_defaults(),
which sets "up->cur_iotype = p->iotype;", which may or
may not be initialized here.

The possible calls chains I see leading up to
serial8250_setup_port() are:

serial8250_register_8250_port(): here we first initialize
  the iotype incorrectly, then set uart->port.iotype and
  call serial8250_set_defaults() again to fix it.

module_init(serial8250_init): relies on the first
  serial8250_set_defaults() for the ISA ports since they
  are always UPIO_PORT, but sets the other ones (pnp, acpi,
  platform_data) correctly.

early_serial_setup(): called only on non-ISA platforms,
  shouldn't need to call serial8250_isa_init_ports() at
  all.

console_initcall(univ8250_console_init): Not completely
  sure here, it seems this now only allows ports that
  are registered from one of the methods above

Can you have a look at the patch below? I think this
correctly removes the broken serial8250_set_defaults()
while also still adding it in the one case that relies
on the implied version.

This does however revert f4c23a140d80 ("serial: 8250:
fix null-ptr-deref in serial8250_start_tx()") and
might bring back the bug came from opening an
uninitialized uart. On the other hand, I don't see
why that doesn't also crash from accessing the invalid
I/O port on the same architectures.

       Arnd

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 5f9f06911795..5b72309611cb 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -358,8 +358,6 @@ struct uart_8250_port *serial8250_setup_port(int index)
 
 	up->ops = &univ8250_driver_ops;
 
-	serial8250_set_defaults(up);
-
 	return up;
 }
 
diff --git a/drivers/tty/serial/8250/8250_platform.c b/drivers/tty/serial/8250/8250_platform.c
index 66fd6d5d4835..d3c1668add58 100644
--- a/drivers/tty/serial/8250/8250_platform.c
+++ b/drivers/tty/serial/8250/8250_platform.c
@@ -94,6 +94,10 @@ static void __init __serial8250_isa_init_ports(void)
 		port->regshift = old_serial_port[i].iomem_reg_shift;
 
 		port->irqflags |= irqflag;
+
+		serial8250_set_defaults(port);
+
+		/* Allow Intel CE4100 and jailhouse to override defaults */
 		if (serial8250_isa_config != NULL)
 			serial8250_isa_config(i, &up->port, &up->capabilities);
 	}

On Mon, Nov 25, 2024 at 10:53:46AM +0100, Arnd Bergmann wrote:
> On Mon, Nov 25, 2024, at 08:55, Andy Shevchenko wrote:
> > On Fri, Nov 22, 2024 at 08:24:37PM +0100, Arnd Bergmann wrote:
> >> On Fri, Nov 22, 2024, at 18:22, Guenter Roeck wrote:
> >> 
> >> 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.
> >
> > Yep. the idea was to purely split this code out of the core
> > library parts. I was not intending any functional changes.
> 
> Ok.
> 
> > I believe it's a preexisted bug, but if you can point out to
> > the breakage I made, please do it, so I would be able to fix
> > ASAP.
> >
> >> 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.
> >
> > Btw, we have a new constant to tell the 8250 core that IO type is not set,
> > but that one is not used here.
> 
> So I see serial8250_setup_port() setting "up->cur_iotype = 0xFF"
> by first calling serial8250_init_port(), this is the "not
> set" value you mean, right?.

Yes, and we have a constant for that, I'll send a patch to make it clear.

> Right after that it calls serial8250_set_defaults(),
> which sets "up->cur_iotype = p->iotype;", which may or
> may not be initialized here.
> 
> The possible calls chains I see leading up to
> serial8250_setup_port() are:
> 
> serial8250_register_8250_port(): here we first initialize
>   the iotype incorrectly, then set uart->port.iotype and
>   call serial8250_set_defaults() again to fix it.
> 
> module_init(serial8250_init): relies on the first
>   serial8250_set_defaults() for the ISA ports since they
>   are always UPIO_PORT, but sets the other ones (pnp, acpi,
>   platform_data) correctly.
> 
> early_serial_setup(): called only on non-ISA platforms,
>   shouldn't need to call serial8250_isa_init_ports() at
>   all.
> 
> console_initcall(univ8250_console_init): Not completely
>   sure here, it seems this now only allows ports that
>   are registered from one of the methods above
> 
> Can you have a look at the patch below? I think this
> correctly removes the broken serial8250_set_defaults()
> while also still adding it in the one case that relies
> on the implied version.
> 
> This does however revert f4c23a140d80 ("serial: 8250:
> fix null-ptr-deref in serial8250_start_tx()") and
> might bring back the bug came from opening an
> uninitialized uart. On the other hand, I don't see
> why that doesn't also crash from accessing the invalid
> I/O port on the same architectures.

AFAICS it does only partial revert, so I don't see how your patch
may break that.

> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 5f9f06911795..5b72309611cb 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -358,8 +358,6 @@ struct uart_8250_port *serial8250_setup_port(int index)
>  
>  	up->ops = &univ8250_driver_ops;
>  
> -	serial8250_set_defaults(up);
> -
>  	return up;
>  }
>  
> diff --git a/drivers/tty/serial/8250/8250_platform.c b/drivers/tty/serial/8250/8250_platform.c
> index 66fd6d5d4835..d3c1668add58 100644
> --- a/drivers/tty/serial/8250/8250_platform.c
> +++ b/drivers/tty/serial/8250/8250_platform.c
> @@ -94,6 +94,10 @@ static void __init __serial8250_isa_init_ports(void)
>  		port->regshift = old_serial_port[i].iomem_reg_shift;
>  
>  		port->irqflags |= irqflag;
> +
> +		serial8250_set_defaults(port);
> +
> +		/* Allow Intel CE4100 and jailhouse to override defaults */
>  		if (serial8250_isa_config != NULL)
>  			serial8250_isa_config(i, &up->port, &up->capabilities);
>  	}

On Mon, Nov 25, 2024, at 11:33, Andy Shevchenko wrote:
> On Mon, Nov 25, 2024 at 10:53:46AM +0100, Arnd Bergmann wrote:
>> 
>> This does however revert f4c23a140d80 ("serial: 8250:
>> fix null-ptr-deref in serial8250_start_tx()") and
>> might bring back the bug came from opening an
>> uninitialized uart. On the other hand, I don't see
>> why that doesn't also crash from accessing the invalid
>> I/O port on the same architectures.
>
> AFAICS it does only partial revert, so I don't see how your patch
> may break that.

I think it's a complete revert, it's just not entirely obvious
since serial8250_setup_port() got split out by 9d86719f8769
("serial: 8250: Allow using ports higher than
SERIAL_8250_RUNTIME_UARTS") and the original callsite got
moved by your ffd8e8bd26e9 ("serial: 8250: Extract platform
driver").

What I suspect is going on with the f4c23a140d80 commit
is the same bug I mentioned earlier in this thread, where
__serial8250_isa_init_ports() just always registers
'nr_uarts' (CONFIG_SERIAL_8250_RUNTIME_UARTS) ports,
unlike any other serial driver.

This used to be required before 9d86719f8769 ("serial:
8250: Allow using ports higher than SERIAL_8250_RUNTIME_UARTS"),
but I don't see why this is still a thing now, other than
for using setserial on i486-class PCs with nonstandard ISA
ports.

On non-x86 machines, it only ever seems to create extra
ports that are likely to crash the system if opened, either
because they lack proper serial_in/serial_out callbacks,
or because the default UPIO_PORT callbacks end up poking
unmapped memory.

Do you see any reason why we can't just do the version below?

        Arnd

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 5f9f06911795..5b72309611cb 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -358,8 +358,6 @@ struct uart_8250_port *serial8250_setup_port(int index)
 
 	up->ops = &univ8250_driver_ops;
 
-	serial8250_set_defaults(up);
-
 	return up;
 }
 
diff --git a/drivers/tty/serial/8250/8250_platform.c b/drivers/tty/serial/8250/8250_platform.c
index 66fd6d5d4835..7675536b6396 100644
--- a/drivers/tty/serial/8250/8250_platform.c
+++ b/drivers/tty/serial/8250/8250_platform.c
@@ -33,8 +33,6 @@
 unsigned int share_irqs = SERIAL8250_SHARE_IRQS;
 unsigned int skip_txen_test;
 
-unsigned int nr_uarts = CONFIG_SERIAL_8250_RUNTIME_UARTS;
-
 #include <asm/serial.h>
 
 /*
@@ -50,6 +48,8 @@ static const struct old_serial_port old_serial_port[] = {
 	SERIAL_PORT_DFNS /* defined in asm/serial.h */
 };
 
+unsigned int nr_uarts = ARRAY_SIZE(old_serial_port);;
+
 serial8250_isa_config_fn serial8250_isa_config;
 void serial8250_set_isa_configurator(serial8250_isa_config_fn v)
 {
@@ -65,9 +65,9 @@ static void __init __serial8250_isa_init_ports(void)
 		nr_uarts = UART_NR;
 
 	/*
-	 * Set up initial ISA ports based on nr_uart module param, or else
-	 * default to CONFIG_SERIAL_8250_RUNTIME_UARTS. Note that we do not
-	 * need to increase nr_uarts when setting up the initial ISA ports.
+	 * Set up initial ISA ports based on nr_uart module param. Note that 
+	 * we do not need to increase nr_uarts when setting up the initial
+	 * ISA ports.
 	 */
 	for (i = 0; i < nr_uarts; i++)
 		serial8250_setup_port(i);
@@ -94,6 +94,10 @@ static void __init __serial8250_isa_init_ports(void)
 		port->regshift = old_serial_port[i].iomem_reg_shift;
 
 		port->irqflags |= irqflag;
+
+		serial8250_set_defaults(up);
+
+		/* Allow Intel CE4100 and jailhouse to override defaults */
 		if (serial8250_isa_config != NULL)
 			serial8250_isa_config(i, &up->port, &up->capabilities);
 	}
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 5b1cc009b977..3bf27cecf82e 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -198,17 +198,6 @@ config SERIAL_8250_NR_UARTS
 	  PCI enumeration and any ports that may be added at run-time
 	  via hot-plug, or any ISA multi-port serial cards.
 
-config SERIAL_8250_RUNTIME_UARTS
-	int "Number of 8250/16550 serial ports to register at runtime"
-	depends on SERIAL_8250
-	range 0 SERIAL_8250_NR_UARTS
-	default "4"
-	help
-	  Set this to the maximum number of serial ports you want
-	  the kernel to register at boot time.  This can be overridden
-	  with the module parameter "nr_uarts", or boot-time parameter
-	  8250.nr_uarts
-
 config SERIAL_8250_EXTENDED
 	bool "Extended 8250/16550 serial driver options"
 	depends on SERIAL_8250

On Mon, Nov 25, 2024 at 12:06:16PM +0100, Arnd Bergmann wrote:
> On Mon, Nov 25, 2024, at 11:33, Andy Shevchenko wrote:
> > On Mon, Nov 25, 2024 at 10:53:46AM +0100, Arnd Bergmann wrote:
> >> 
> >> This does however revert f4c23a140d80 ("serial: 8250:
> >> fix null-ptr-deref in serial8250_start_tx()") and
> >> might bring back the bug came from opening an
> >> uninitialized uart. On the other hand, I don't see
> >> why that doesn't also crash from accessing the invalid
> >> I/O port on the same architectures.
> >
> > AFAICS it does only partial revert, so I don't see how your patch
> > may break that.
> 
> I think it's a complete revert, it's just not entirely obvious
> since serial8250_setup_port() got split out by 9d86719f8769
> ("serial: 8250: Allow using ports higher than
> SERIAL_8250_RUNTIME_UARTS") and the original callsite got
> moved by your ffd8e8bd26e9 ("serial: 8250: Extract platform
> driver").

Ah, okay.

> What I suspect is going on with the f4c23a140d80 commit
> is the same bug I mentioned earlier in this thread, where
> __serial8250_isa_init_ports() just always registers
> 'nr_uarts' (CONFIG_SERIAL_8250_RUNTIME_UARTS) ports,
> unlike any other serial driver.

But the configuration can give less than old_serial_port contains.
See dozens of the explicit settings in the defconfigs.

> This used to be required before 9d86719f8769 ("serial:
> 8250: Allow using ports higher than SERIAL_8250_RUNTIME_UARTS"),
> but I don't see why this is still a thing now, other than
> for using setserial on i486-class PCs with nonstandard ISA
> ports.
> 
> On non-x86 machines, it only ever seems to create extra
> ports that are likely to crash the system if opened, either
> because they lack proper serial_in/serial_out callbacks,
> or because the default UPIO_PORT callbacks end up poking
> unmapped memory.
> 
> Do you see any reason why we can't just do the version below?

Perhaps we may do this way (it seems better to me than previous suggestions),
but it also needs to be carefully checked against those configurations that set
it explicitly.

...

If we go this way, it would be also nice to add a comment explaining that
this is module parameter (as it's done for those ones above).

> +unsigned int nr_uarts = ARRAY_SIZE(old_serial_port);;

...

>  	/*
> -	 * Set up initial ISA ports based on nr_uart module param, or else
> -	 * default to CONFIG_SERIAL_8250_RUNTIME_UARTS. Note that we do not
> -	 * need to increase nr_uarts when setting up the initial ISA ports.

> +	 * Set up initial ISA ports based on nr_uart module param. Note that 

Just in case it has a trailing whitespace.

> +	 * we do not need to increase nr_uarts when setting up the initial
> +	 * ISA ports.
>  	 */

On Mon, Nov 25, 2024, at 12:26, Andy Shevchenko wrote:
> On Mon, Nov 25, 2024 at 12:06:16PM +0100, Arnd Bergmann wrote:
>> What I suspect is going on with the f4c23a140d80 commit
>> is the same bug I mentioned earlier in this thread, where
>> __serial8250_isa_init_ports() just always registers
>> 'nr_uarts' (CONFIG_SERIAL_8250_RUNTIME_UARTS) ports,
>> unlike any other serial driver.
>
> But the configuration can give less than old_serial_port contains.
> See dozens of the explicit settings in the defconfigs.

I don't see any of the upstream defconfigs doing this
though, the only ones setting CONFIG_SERIAL_8250_RUNTIME_UARTS
are those that have an empty old_serial_port[]. 

Note that SERIAL_PORT_DFNS is only defined on x86, alpha
and m68k (for q40), which are the main PC-like platforms.
I see that all three have identical definitions of
SERIAL_PORT_DFNS, so I think these should just be moved
next to the __serial8250_isa_init_ports definition, with
the entire thing moved into a separate ISA driver or
an #ifdef around it. This is of course not the problem
at hand, but it would help separate the x86/isa and
non-x86 platform device cases further.

>> This used to be required before 9d86719f8769 ("serial:
>> 8250: Allow using ports higher than SERIAL_8250_RUNTIME_UARTS"),
>> but I don't see why this is still a thing now, other than
>> for using setserial on i486-class PCs with nonstandard ISA
>> ports.
>> 
>> On non-x86 machines, it only ever seems to create extra
>> ports that are likely to crash the system if opened, either
>> because they lack proper serial_in/serial_out callbacks,
>> or because the default UPIO_PORT callbacks end up poking
>> unmapped memory.
>> 
>> Do you see any reason why we can't just do the version below?
>
> Perhaps we may do this way (it seems better to me than previous 
> suggestions), but it also needs to be carefully checked against
> those configurations that set it explicitly.

Yes, at least to make sure that the numbering of the uarts
does not change. I expect it's actually the same, but don't
know for sure.

> If we go this way, it would be also nice to add a comment explaining that
> this is module parameter (as it's done for those ones above).
>
>> +unsigned int nr_uarts = ARRAY_SIZE(old_serial_port);;

Right.

     Arnd

On Mon, Nov 25, 2024 at 02:50:56PM +0100, Arnd Bergmann wrote:
> On Mon, Nov 25, 2024, at 12:26, Andy Shevchenko wrote:
> > On Mon, Nov 25, 2024 at 12:06:16PM +0100, Arnd Bergmann wrote:

...

> >> What I suspect is going on with the f4c23a140d80 commit
> >> is the same bug I mentioned earlier in this thread, where
> >> __serial8250_isa_init_ports() just always registers
> >> 'nr_uarts' (CONFIG_SERIAL_8250_RUNTIME_UARTS) ports,
> >> unlike any other serial driver.
> >
> > But the configuration can give less than old_serial_port contains.
> > See dozens of the explicit settings in the defconfigs.
> 
> I don't see any of the upstream defconfigs doing this
> though, the only ones setting CONFIG_SERIAL_8250_RUNTIME_UARTS
> are those that have an empty old_serial_port[]. 

A-ha, a good catch. I haven't checked the actual contents of the
old_serial_port for those configurations.

> Note that SERIAL_PORT_DFNS is only defined on x86, alpha
> and m68k (for q40), which are the main PC-like platforms.
> I see that all three have identical definitions of
> SERIAL_PORT_DFNS, so I think these should just be moved
> next to the __serial8250_isa_init_ports definition, with
> the entire thing moved into a separate ISA driver or
> an #ifdef around it. This is of course not the problem
> at hand, but it would help separate the x86/isa and
> non-x86 platform device cases further.

It's nice idea, but yes, we can think about it later.

> >> This used to be required before 9d86719f8769 ("serial:
> >> 8250: Allow using ports higher than SERIAL_8250_RUNTIME_UARTS"),
> >> but I don't see why this is still a thing now, other than
> >> for using setserial on i486-class PCs with nonstandard ISA
> >> ports.
> >> 
> >> On non-x86 machines, it only ever seems to create extra
> >> ports that are likely to crash the system if opened, either
> >> because they lack proper serial_in/serial_out callbacks,
> >> or because the default UPIO_PORT callbacks end up poking
> >> unmapped memory.
> >> 
> >> Do you see any reason why we can't just do the version below?
> >
> > Perhaps we may do this way (it seems better to me than previous 
> > suggestions), but it also needs to be carefully checked against
> > those configurations that set it explicitly.
> 
> Yes, at least to make sure that the numbering of the uarts
> does not change. I expect it's actually the same, but don't
> know for sure.

Me neither. And the issue with NULL pointer dereference needs to be retested.

On Mon, Nov 25, 2024, at 12:06, Arnd Bergmann wrote:
> +unsigned int nr_uarts = ARRAY_SIZE(old_serial_port);;
> +

Unfortunately, this breaks on non-x86 because of the check
added in 59cfc45f17d6 ("serial: 8250: Do nothing if nr_uarts=0").

I still think it's the right idea, but need to unwind further
to make this possible, and find a different fix for the bug
from that commit.

      Arnd

On Mon, 25 Nov 2024, Arnd Bergmann wrote:

> > But the configuration can give less than old_serial_port contains.
> > See dozens of the explicit settings in the defconfigs.
> 
> I don't see any of the upstream defconfigs doing this
> though, the only ones setting CONFIG_SERIAL_8250_RUNTIME_UARTS
> are those that have an empty old_serial_port[]. 
> 
> Note that SERIAL_PORT_DFNS is only defined on x86, alpha
> and m68k (for q40), which are the main PC-like platforms.

 May I suggest to call `serial8250_isa_init_ports':

	if (IS_ENABLED(CONFIG_ISA) || IS_ENABLED(CONFIG_ALPHA) ||
	    IS_ENABLED(CONFIG_M68K) || IS_ENABLED(CONFIG_X86))

then (or have an equivalent `select' in the relevant Kconfig files)?

 The whole point of this legacy setup is to poke at ISA serial ports that 
have been wired by jumpers or similar means (sometimes just hardwired) to 
their designated ISA port I/O locations.  Sometimes it means LPC rather 
than real ISA, but LPC stuff should mostly be covered by platform bindings 
rather than just blind poking, which may only be needed for platforms that 
have some kind of a generic config and no DT or other way (such as ACPI or 
ISA PNP) to discover actual ports.

> I see that all three have identical definitions of
> SERIAL_PORT_DFNS, so I think these should just be moved
> next to the __serial8250_isa_init_ports definition, with
> the entire thing moved into a separate ISA driver or
> an #ifdef around it. This is of course not the problem
> at hand, but it would help separate the x86/isa and
> non-x86 platform device cases further.

 This SERIAL_PORT_DFNS definition is just original ISA stuff, so it does 
apply universally across CONFIG_ISA platforms.  Original ISA option cards 
have had no way to discover other than by blind-poking (or giving port I/O 
locations by hand via a kernel parameter).

  Maciej

On Mon, Nov 25, 2024, at 17:54, Maciej W. Rozycki wrote:
> On Mon, 25 Nov 2024, Arnd Bergmann wrote:
>
>> > But the configuration can give less than old_serial_port contains.
>> > See dozens of the explicit settings in the defconfigs.
>> 
>> I don't see any of the upstream defconfigs doing this
>> though, the only ones setting CONFIG_SERIAL_8250_RUNTIME_UARTS
>> are those that have an empty old_serial_port[]. 
>> 
>> Note that SERIAL_PORT_DFNS is only defined on x86, alpha
>> and m68k (for q40), which are the main PC-like platforms.
>
>  May I suggest to call `serial8250_isa_init_ports':
>
> 	if (IS_ENABLED(CONFIG_ISA) || IS_ENABLED(CONFIG_ALPHA) ||
> 	    IS_ENABLED(CONFIG_M68K) || IS_ENABLED(CONFIG_X86))
>
> then (or have an equivalent `select' in the relevant Kconfig files)?

Right, I think that makes sense, but I'm a little worried
about renumbering or incorrectly configuring the uarts on
a non-x86 system that might have ISA slots and also register
a 8250 console.

E.g. on the RM200, two serial ports get registered on
MMIO addresses:

arch/mips/sni/rm200.c:static struct serial8250_platform_data rm200_data[] = {
arch/mips/sni/rm200.c-  MEMPORT(0x160003f8, RM200_I8259A_IRQ_BASE + 4),
arch/mips/sni/rm200.c-  MEMPORT(0x160002f8, RM200_I8259A_IRQ_BASE + 3),
arch/mips/sni/rm200.c-  { },
arch/mips/sni/rm200.c-};

so these would become ports ttyS4 and ttyS5 if the first four
ports get reserved for ISA cards, or disappear when using the
default CONFIG_SERIAL_8250_NR_UARTS=4.

      Arnd

On Mon, 25 Nov 2024, Arnd Bergmann wrote:

> >  May I suggest to call `serial8250_isa_init_ports':
> >
> > 	if (IS_ENABLED(CONFIG_ISA) || IS_ENABLED(CONFIG_ALPHA) ||
> > 	    IS_ENABLED(CONFIG_M68K) || IS_ENABLED(CONFIG_X86))
> >
> > then (or have an equivalent `select' in the relevant Kconfig files)?
> 
> Right, I think that makes sense, but I'm a little worried
> about renumbering or incorrectly configuring the uarts on
> a non-x86 system that might have ISA slots and also register
> a 8250 console.
> 
> E.g. on the RM200, two serial ports get registered on
> MMIO addresses:
> 
> arch/mips/sni/rm200.c:static struct serial8250_platform_data rm200_data[] = {
> arch/mips/sni/rm200.c-  MEMPORT(0x160003f8, RM200_I8259A_IRQ_BASE + 4),
> arch/mips/sni/rm200.c-  MEMPORT(0x160002f8, RM200_I8259A_IRQ_BASE + 3),
> arch/mips/sni/rm200.c-  { },
> arch/mips/sni/rm200.c-};
> 
> so these would become ports ttyS4 and ttyS5 if the first four
> ports get reserved for ISA cards, or disappear when using the
> default CONFIG_SERIAL_8250_NR_UARTS=4.

 I think it's still the correct thing to do or ISA card ports won't be 
accessible.  Then where there are platform ports, they ought to be given 
precedence so that they are given lower indices.  I'm undecided as to 
whether we actually need to get it sorted now or wait for someone with an 
RM200 system to trigger the problem (with the possible workarounds being:

1. Do not plug any ISA cards with serial ports.

2. Set CONFIG_SERIAL_8250_RUNTIME_UARTS or "8250.nr_uarts" to 0.

).  It might be quite easy to get sorted though, so I'd be leaning towards 
at least checking how feasible it would be.

  Maciej

On 11/25/24 09:54, Arnd Bergmann wrote:
> On Mon, Nov 25, 2024, at 17:54, Maciej W. Rozycki wrote:
>> On Mon, 25 Nov 2024, Arnd Bergmann wrote:
>>
>>>> But the configuration can give less than old_serial_port contains.
>>>> See dozens of the explicit settings in the defconfigs.
>>>
>>> I don't see any of the upstream defconfigs doing this
>>> though, the only ones setting CONFIG_SERIAL_8250_RUNTIME_UARTS
>>> are those that have an empty old_serial_port[].
>>>
>>> Note that SERIAL_PORT_DFNS is only defined on x86, alpha
>>> and m68k (for q40), which are the main PC-like platforms.
>>
>>   May I suggest to call `serial8250_isa_init_ports':
>>
>> 	if (IS_ENABLED(CONFIG_ISA) || IS_ENABLED(CONFIG_ALPHA) ||
>> 	    IS_ENABLED(CONFIG_M68K) || IS_ENABLED(CONFIG_X86))
>>
>> then (or have an equivalent `select' in the relevant Kconfig files)?
> 
> Right, I think that makes sense, but I'm a little worried
> about renumbering or incorrectly configuring the uarts on
> a non-x86 system that might have ISA slots and also register
> a 8250 console.
> 
> E.g. on the RM200, two serial ports get registered on
> MMIO addresses:
> 
> arch/mips/sni/rm200.c:static struct serial8250_platform_data rm200_data[] = {
> arch/mips/sni/rm200.c-  MEMPORT(0x160003f8, RM200_I8259A_IRQ_BASE + 4),
> arch/mips/sni/rm200.c-  MEMPORT(0x160002f8, RM200_I8259A_IRQ_BASE + 3),
> arch/mips/sni/rm200.c-  { },
> arch/mips/sni/rm200.c-};
> 
> so these would become ports ttyS4 and ttyS5 if the first four
> ports get reserved for ISA cards, or disappear when using the
> default CONFIG_SERIAL_8250_NR_UARTS=4.
> 

Any progress ?

Guenter

On Mon, Nov 25, 2024 at 04:59:00PM +0100, Arnd Bergmann wrote:
> On Mon, Nov 25, 2024, at 12:06, Arnd Bergmann wrote:
> > +unsigned int nr_uarts = ARRAY_SIZE(old_serial_port);;
> > +
> 
> Unfortunately, this breaks on non-x86 because of the check
> added in 59cfc45f17d6 ("serial: 8250: Do nothing if nr_uarts=0").
> 
> I still think it's the right idea, but need to unwind further
> to make this possible, and find a different fix for the bug
> from that commit.
> 

I decided to apply the patch below to my fixes branch. It doesn't change
the code, it just gets rid of the warning backtrace.

Guenter

---

On Wed, Dec 4, 2024, at 22:09, Guenter Roeck wrote:
> On Mon, Nov 25, 2024 at 04:59:00PM +0100, Arnd Bergmann wrote:
>> On Mon, Nov 25, 2024, at 12:06, Arnd Bergmann wrote:
>> > +unsigned int nr_uarts = ARRAY_SIZE(old_serial_port);;
>> > +
>> 
>> Unfortunately, this breaks on non-x86 because of the check
>> added in 59cfc45f17d6 ("serial: 8250: Do nothing if nr_uarts=0").
>> 
>> I still think it's the right idea, but need to unwind further
>> to make this possible, and find a different fix for the bug
>> from that commit.
>> 
>
> I decided to apply the patch below to my fixes branch. It doesn't change
> the code, it just gets rid of the warning backtrace.

I got stuck in this rabbit hole of running into more issues
with the 8250 driver. Any time you touch something, it breaks
elsewhere.

I've uploaded what I have here now:

https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=8250-cleanup

but this probably needs more testing, and a few smaller changes

>  drivers/tty/serial/8250/8250_port.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/8250/8250_port.c 
> b/drivers/tty/serial/8250/8250_port.c
> index 4d63d80e78a9..649e74e9b52f 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -467,7 +467,8 @@ static void set_io_from_upio(struct uart_port *p)
>  		break;
>  #endif
>  	default:
> -		WARN(1, "Unsupported UART type %x\n", p->iotype);
> +		WARN(p->iotype != UPIO_PORT || p->iobase,
> +		     "Unsupported UART type %x\n", p->iotype);
>  		p->serial_in = no_serial_in;
>  		p->serial_out = no_serial_out;
>  	}

I had considered this at first but didn't really want to do
it like this because we should not be registering empty ports
on platforms that don't use the setserial style override for
the port configuration.

It does of course address the warning, just not the
underlying bug.

      Arnd

On 12/4/24 14:17, Arnd Bergmann wrote:
> On Wed, Dec 4, 2024, at 22:09, Guenter Roeck wrote:
>> On Mon, Nov 25, 2024 at 04:59:00PM +0100, Arnd Bergmann wrote:
>>> On Mon, Nov 25, 2024, at 12:06, Arnd Bergmann wrote:
>>>> +unsigned int nr_uarts = ARRAY_SIZE(old_serial_port);;
>>>> +
>>>
>>> Unfortunately, this breaks on non-x86 because of the check
>>> added in 59cfc45f17d6 ("serial: 8250: Do nothing if nr_uarts=0").
>>>
>>> I still think it's the right idea, but need to unwind further
>>> to make this possible, and find a different fix for the bug
>>> from that commit.
>>>
>>
>> I decided to apply the patch below to my fixes branch. It doesn't change
>> the code, it just gets rid of the warning backtrace.
> 
> I got stuck in this rabbit hole of running into more issues
> with the 8250 driver. Any time you touch something, it breaks
> elsewhere.
> 
> I've uploaded what I have here now:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=8250-cleanup
> 
> but this probably needs more testing, and a few smaller changes
> 
>>   drivers/tty/serial/8250/8250_port.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_port.c
>> b/drivers/tty/serial/8250/8250_port.c
>> index 4d63d80e78a9..649e74e9b52f 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -467,7 +467,8 @@ static void set_io_from_upio(struct uart_port *p)
>>   		break;
>>   #endif
>>   	default:
>> -		WARN(1, "Unsupported UART type %x\n", p->iotype);
>> +		WARN(p->iotype != UPIO_PORT || p->iobase,
>> +		     "Unsupported UART type %x\n", p->iotype);
>>   		p->serial_in = no_serial_in;
>>   		p->serial_out = no_serial_out;
>>   	}
> 
> I had considered this at first but didn't really want to do
> it like this because we should not be registering empty ports
> on platforms that don't use the setserial style override for
> the port configuration.
> 
> It does of course address the warning, just not the
> underlying bug.
> 

I had a look myself, and concluded that a clean fix will likely require
substantial changes to avoid regressions. Your patch series pretty much
confirms that. This is why I came up with the hack below. Yes, it doesn't
get rid of the underlying problem, but IMO it is good enough for 6.13,
or at least not worse than 6.12, while at the same time avoiding the
warning backtrace. It seems to me that a clean fix would be 6.14 material,
and I really don't want those backtraces to clog up test logs until then.

Thanks,
Guenter

On Wed, Dec 4, 2024, at 23:44, Guenter Roeck wrote:
> On 12/4/24 14:17, Arnd Bergmann wrote:
>> On Wed, Dec 4, 2024, at 22:09, Guenter Roeck wrote:
>> 
>> I had considered this at first but didn't really want to do
>> it like this because we should not be registering empty ports
>> on platforms that don't use the setserial style override for
>> the port configuration.
>> 
>> It does of course address the warning, just not the
>> underlying bug.
>> 
>
> I had a look myself, and concluded that a clean fix will likely require
> substantial changes to avoid regressions. Your patch series pretty much
> confirms that. This is why I came up with the hack below. Yes, it doesn't
> get rid of the underlying problem, but IMO it is good enough for 6.13,
> or at least not worse than 6.12, while at the same time avoiding the
> warning backtrace. It seems to me that a clean fix would be 6.14 material,
> and I really don't want those backtraces to clog up test logs until then.

Fair enough, let's use your patch then for 6,13,

Acked-by: Arnd Bergmann <arnd@arndb.de>

On 12/4/24 23:08, Arnd Bergmann wrote:
> On Wed, Dec 4, 2024, at 23:44, Guenter Roeck wrote:
>> On 12/4/24 14:17, Arnd Bergmann wrote:
>>> On Wed, Dec 4, 2024, at 22:09, Guenter Roeck wrote:
>>>
>>> I had considered this at first but didn't really want to do
>>> it like this because we should not be registering empty ports
>>> on platforms that don't use the setserial style override for
>>> the port configuration.
>>>
>>> It does of course address the warning, just not the
>>> underlying bug.
>>>
>>
>> I had a look myself, and concluded that a clean fix will likely require
>> substantial changes to avoid regressions. Your patch series pretty much
>> confirms that. This is why I came up with the hack below. Yes, it doesn't
>> get rid of the underlying problem, but IMO it is good enough for 6.13,
>> or at least not worse than 6.12, while at the same time avoiding the
>> warning backtrace. It seems to me that a clean fix would be 6.14 material,
>> and I really don't want those backtraces to clog up test logs until then.
> 
> Fair enough, let's use your patch then for 6,13,
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>


Ok, I just sent it out as real patch with your Ack applied.

Thanks,
Guenter

On Wed, Dec 04, 2024 at 11:17:56PM +0100, Arnd Bergmann wrote:
> On Wed, Dec 4, 2024, at 22:09, Guenter Roeck wrote:

...

> I got stuck in this rabbit hole of running into more issues
> with the 8250 driver. Any time you touch something, it breaks
> elsewhere.
> 
> I've uploaded what I have here now:

FWIW, I have briefly looked at it, some patches I appreciate very much, some of
them I think need more testing and one thing I don't really like is putting
code back to 8250_core. Thinking about that, perhaps we need to restore 8250.c
(or alike) for collecting non-library / non-_particular_-driver-leaf?

> https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=8250-cleanup
> 
> but this probably needs more testing, and a few smaller changes

Definitely!

On Fri, Dec 6, 2024, at 16:44, Andy Shevchenko wrote:
> On Wed, Dec 04, 2024 at 11:17:56PM +0100, Arnd Bergmann wrote:
>> On Wed, Dec 4, 2024, at 22:09, Guenter Roeck wrote:
>
>> I got stuck in this rabbit hole of running into more issues
>> with the 8250 driver. Any time you touch something, it breaks
>> elsewhere.
>> 
>> I've uploaded what I have here now:
>
> FWIW, I have briefly looked at it, some patches I appreciate very much, some of
> them I think need more testing and one thing I don't really like is putting
> code back to 8250_core. Thinking about that, perhaps we need to restore 8250.c
> (or alike) for collecting non-library / non-_particular_-driver-leaf?

Thanks for taking a look!

I thought you might have different ideas on where some of the
code needs to go,  as you were the one who moved it out
previously.

However, I think the two bits that I ended up moving back
really belong in the core:

- serial8250_setup_ports() is needed to get the
  serial8250_ports[] into a sane state. Ideally this
  array would just be statically initialized, but I don't
  think we can actually express that in C code.

- serial8250_init()/serial8250_exit() are required for
  registering the driver itself (serial8250_reg) Having
  that part in the ISA driver made no sense to me.

The bit that is definitely ugly is how serial8250_isa_init()
gets called from serial8250_init() and univ8250_console_init(),
and I would prefer to remove that, but any attempt to do
that made it worse.

The problem here is that we can't just rely on link order:
For the console initcall and  serial8250_isa_init_ports(),
we have to call this before the core driver initialization,
but the serial8250-isa platform device has to be registered
after the core driver does. If we link both files into
a loadable module, there is an additional problem of
not being able to have more than one module_init() call.

      Arnd