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@xxxxxxxxx/
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
