[v6,4/5] 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. Some 8250 serial drivers support
MMIO only use, so fence only the parts requiring I/O ports and print an
error message if a device can't be supported with the current
configuration.

Co-developed-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 drivers/tty/Kconfig                   |  4 +--
 drivers/tty/serial/8250/8250_early.c  |  4 +++
 drivers/tty/serial/8250/8250_pci.c    | 49 ++++++++++++++++++++++++++++++++++-
 drivers/tty/serial/8250/8250_pcilib.c |  4 +++
 drivers/tty/serial/8250/8250_port.c   | 47 ++++++++++++++++++++++++++-------
 drivers/tty/serial/8250/Kconfig       |  4 +--
 drivers/tty/serial/Kconfig            |  2 +-
 7 files changed, 98 insertions(+), 16 deletions(-)

On Mon, 7 Oct 2024, Niklas Schnelle wrote:

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

 Gratuitous change here.

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

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

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

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

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

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

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

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

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

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

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

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

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

 Thanks for making the clean-ups we discussed.

  Maciej

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

Agree this looks better, will change it.

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

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

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

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

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

WIll drop this hunk

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

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

Thanks,
Niklas

On Tue, 8 Oct 2024, Niklas Schnelle wrote:

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

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

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

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

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

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

  Maciej