| Message ID | 20221018133419.134110-1-martin@geanix.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] serial: 8250: allow use of non-runtime configured uart ports | expand |
On October 20, 2022 2:34:14 PM GMT+02:00, "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com> wrote: >On Tue, 18 Oct 2022, Martin Hundebøll wrote: > >> The logic to find unused ports when registering new 8250 uart ports >> searches only up to CONFIG_SERIAL_8250_RUNTIME_UART, which forces users >> of external 8250 ports to increase the number of runtime ports >> artificially. >> >> Fix this by initializing each allocated port structure with basic >> settings like line number and uart operation callbacks, and by searching >> the entire array of allocated ports to find an unused one. > >So nr_uarts no longer means "Maximum number of UARTs supported." ? >Perhaps it should be reworded too. > It never did. Confusingly, the module parameter name (nr_uarts) corresponds to CONFIG_SERIAL_8250_RUNTIME_UARTS, and controls the number of built-in (non-discoverable) ports. The other config, CONFIG_SERIAL_8250_NR_UARTS, controls the maximum number of ports. We cannot change the module parameter name, so I'm not sure if we should map it to another static variable in the source? // Martin
On 20/10/2022 16.14, Ilpo Järvinen wrote: > On Thu, 20 Oct 2022, Martin Hundebøll wrote: > >> >> >> On October 20, 2022 2:34:14 PM GMT+02:00, "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com> wrote: >>> On Tue, 18 Oct 2022, Martin Hundebøll wrote: >>> >>>> The logic to find unused ports when registering new 8250 uart ports >>>> searches only up to CONFIG_SERIAL_8250_RUNTIME_UART, which forces users >>>> of external 8250 ports to increase the number of runtime ports >>>> artificially. >>>> >>>> Fix this by initializing each allocated port structure with basic >>>> settings like line number and uart operation callbacks, and by searching >>>> the entire array of allocated ports to find an unused one. >>> >>> So nr_uarts no longer means "Maximum number of UARTs supported." ? >>> Perhaps it should be reworded too. >>> >> >> It never did. Confusingly, the module parameter name (nr_uarts) >> corresponds to CONFIG_SERIAL_8250_RUNTIME_UARTS, and controls the number >> of built-in (non-discoverable) ports. The other config, >> CONFIG_SERIAL_8250_NR_UARTS, controls the maximum number of ports. >> >> We cannot change the module parameter name, so I'm not sure if we should >> map it to another static variable in the source? > > I meant that its description should be changed to match what it really > does. I see. Something like this? diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c index f4a08fb74c31..cdac8f11194e 100644 --- a/drivers/tty/serial/8250/8250_core.c +++ b/drivers/tty/serial/8250/8250_core.c @@ -1261,7 +1261,7 @@ module_param_hw(share_irqs, uint, other, 0644); MODULE_PARM_DESC(share_irqs, "Share IRQs with other non-8250/16x50 devices (unsafe)"); module_param(nr_uarts, uint, 0644); -MODULE_PARM_DESC(nr_uarts, "Maximum number of UARTs supported. (1-" __MODULE_STRING(CONFIG_SERIAL_8250_NR_UARTS) ")"); +MODULE_PARM_DESC(nr_uarts, "Number of built-in (non-discoverable) UARTs to initialize. (1-" __MODULE_STRING(CONFIG_SERIAL_8250_NR_UARTS) ")"); module_param(skip_txen_test, uint, 0644); MODULE_PARM_DESC(skip_txen_test, "Skip checking for the TXEN bug at init time");
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c index 94fbf0add2ce..a166cc66e7d1 100644 --- a/drivers/tty/serial/8250/8250_core.c +++ b/drivers/tty/serial/8250/8250_core.c @@ -500,7 +500,7 @@ static void __init serial8250_isa_init_ports(void) if (nr_uarts > UART_NR) nr_uarts = UART_NR; - for (i = 0; i < nr_uarts; i++) { + for (i = 0; i < UART_NR; i++) { struct uart_8250_port *up = &serial8250_ports[i]; struct uart_port *port = &up->port; @@ -926,15 +926,16 @@ static struct uart_8250_port *serial8250_find_match_or_unused(const struct uart_ /* try line number first if still available */ i = port->line; - if (i < nr_uarts && serial8250_ports[i].port.type == PORT_UNKNOWN && + if (i < UART_NR && serial8250_ports[i].port.type == PORT_UNKNOWN && serial8250_ports[i].port.iobase == 0) return &serial8250_ports[i]; + /* * We didn't find a matching entry, so look for the first * free entry. We look for one which hasn't been previously * used (indicated by zero iobase). */ - for (i = 0; i < nr_uarts; i++) + for (i = 0; i < UART_NR; i++) if (serial8250_ports[i].port.type == PORT_UNKNOWN && serial8250_ports[i].port.iobase == 0) return &serial8250_ports[i]; @@ -943,7 +944,7 @@ static struct uart_8250_port *serial8250_find_match_or_unused(const struct uart_ * That also failed. Last resort is to find any entry which * doesn't have a real port associated with it. */ - for (i = 0; i < nr_uarts; i++) + for (i = 0; i < UART_NR; i++) if (serial8250_ports[i].port.type == PORT_UNKNOWN) return &serial8250_ports[i];
The logic to find unused ports when registering new 8250 uart ports searches only up to CONFIG_SERIAL_8250_RUNTIME_UARTS, which forces users of external 8250 ports to increase the number of runtime ports artificially. Fix this by initializing each allocated port structure with basic settings like line number and uart operation callbacks, and by searching the entire array of allocated ports to find an unused one. Signed-off-by: Martin Hundebøll <martin@geanix.com> --- drivers/tty/serial/8250/8250_core.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)