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
