[PATCH v2 1/3] serial: 8250: allow use of non-runtime configured uart ports

[PATCH v2 1/3] serial: 8250: allow use of non-runtime configured uart ports

[Martin Hundebøll]

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

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];
 
-- 
2.38.0

[PATCH v2 2/3] serial: 8250: allow zero runtime-configured ports

[Martin Hundebøll]

One should be able to set CONFIG_SERIAL_8250_RUNTIME_UARTS=0 on
platforms with zero built-in 8250-like ports. However, that case was
prohibited in commit 59cfc45f17d6
("serial: 8250: Do nothing if nr_uarts=0"), because of missing array
initialization, effectively disabling the driver entirely.

The missing array initialization has been fixed in the previous commit,
so remove check for zero runtime ports. Said check gets to stay when
initializing early consoles, though, because that makes sense for
built-in ports only.

Signed-off-by: Martin Hundebøll <martin@geanix.com>
---
 drivers/tty/serial/8250/8250_core.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index a166cc66e7d1..ba48431ec6e2 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -680,9 +680,6 @@ static struct console univ8250_console = {
 
 static int __init univ8250_console_init(void)
 {
-	if (nr_uarts == 0)
-		return -ENODEV;
-
 	serial8250_isa_init_ports();
 	register_console(&univ8250_console);
 	return 0;
@@ -1171,9 +1168,6 @@ static int __init serial8250_init(void)
 {
 	int ret;
 
-	if (nr_uarts == 0)
-		return -ENODEV;
-
 	serial8250_isa_init_ports();
 
 	pr_info("Serial: 8250/16550 driver, %d ports, IRQ sharing %sabled\n",
-- 
2.38.0

[PATCH v2 3/3] serial: 8250: skip platform device registration with no runtime ports

[Martin Hundebøll]

Skip registration of the platform device used for built-in ports, if no
such ports are configured/created.

Signed-off-by: Martin Hundebøll <martin@geanix.com>
---

Change since v1:
 * call serial8250_pnp_init() also when nr_uarts is zero

 drivers/tty/serial/8250/8250_core.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index ba48431ec6e2..f4a08fb74c31 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1186,6 +1186,14 @@ static int __init serial8250_init(void)
 	if (ret)
 		goto unreg_uart_drv;
 
+	if (nr_uarts == 0) {
+		ret = platform_driver_register(&serial8250_isa_driver);
+		if (ret)
+			goto unreg_pnp;
+
+		goto out;
+	}
+
 	serial8250_isa_devs = platform_device_alloc("serial8250",
 						    PLAT8250_DEV_LEGACY);
 	if (!serial8250_isa_devs) {
@@ -1230,7 +1238,9 @@ static void __exit serial8250_exit(void)
 	serial8250_isa_devs = NULL;
 
 	platform_driver_unregister(&serial8250_isa_driver);
-	platform_device_unregister(isa_dev);
+
+	if (nr_uarts)
+		platform_device_unregister(isa_dev);
 
 	serial8250_pnp_exit();
 
-- 
2.38.0

Re: [PATCH v2 3/3] serial: 8250: skip platform device registration with no runtime ports

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1672 bytes --]

On Tue, 18 Oct 2022, Martin Hundebøll wrote:

> Skip registration of the platform device used for built-in ports, if no
> such ports are configured/created.
> 
> Signed-off-by: Martin Hundebøll <martin@geanix.com>

For the whole series:

Tested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

> ---
> 
> Change since v1:
>  * call serial8250_pnp_init() also when nr_uarts is zero
> 
>  drivers/tty/serial/8250/8250_core.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index ba48431ec6e2..f4a08fb74c31 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -1186,6 +1186,14 @@ static int __init serial8250_init(void)
>  	if (ret)
>  		goto unreg_uart_drv;
>  
> +	if (nr_uarts == 0) {
> +		ret = platform_driver_register(&serial8250_isa_driver);
> +		if (ret)
> +			goto unreg_pnp;
> +
> +		goto out;
> +	}
> +

I'd reverse the condition:

	if (nr_uarts) {
		platform stuff
	}

	ret = platform_driver_register(&serial8250_isa_driver);
        if (ret == 0)
		goto out;
	if (!nr_uarts)
		goto unreg_pnp;

I believe the logic would be easier to follow if you do that.

-- 
 i.

>  	serial8250_isa_devs = platform_device_alloc("serial8250",
>  						    PLAT8250_DEV_LEGACY);
>  	if (!serial8250_isa_devs) {
> @@ -1230,7 +1238,9 @@ static void __exit serial8250_exit(void)
>  	serial8250_isa_devs = NULL;
>  
>  	platform_driver_unregister(&serial8250_isa_driver);
> -	platform_device_unregister(isa_dev);
> +
> +	if (nr_uarts)
> +		platform_device_unregister(isa_dev);
>  
>  	serial8250_pnp_exit();
>  
> 

Re: [PATCH v2 1/3] serial: 8250: allow use of non-runtime configured uart ports

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2477 bytes --]

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

So nr_uarts no longer means "Maximum number of UARTs supported." ?
Perhaps it should be reworded too.

-- 
 i.
 
> Signed-off-by: Martin Hundebøll <martin@geanix.com>
> ---
>  drivers/tty/serial/8250/8250_core.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> 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];
>  
> 

Re: [PATCH v2 1/3] serial: 8250: allow use of non-runtime configured uart ports

[Martin Hundebøll]

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

Re: [PATCH v2 1/3] serial: 8250: allow use of non-runtime configured uart ports

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1272 bytes --]

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.

Re: [PATCH v2 1/3] serial: 8250: allow use of non-runtime configured uart ports

[Martin Hundebøll]

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");