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

[PATCH v3 1/4] 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.1

[PATCH v3 2/4] 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.1

[PATCH v3 3/4] 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

Change since v2:
 * invert condition to initialize built-in ports (as suggested by Ilpo)

 drivers/tty/serial/8250/8250_core.c | 30 +++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index ba48431ec6e2..a8fbc2325244 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1186,22 +1186,26 @@ static int __init serial8250_init(void)
 	if (ret)
 		goto unreg_uart_drv;
 
-	serial8250_isa_devs = platform_device_alloc("serial8250",
-						    PLAT8250_DEV_LEGACY);
-	if (!serial8250_isa_devs) {
-		ret = -ENOMEM;
-		goto unreg_pnp;
+	if (nr_uarts) {
+		serial8250_isa_devs = platform_device_alloc("serial8250",
+							    PLAT8250_DEV_LEGACY);
+		if (!serial8250_isa_devs) {
+			ret = -ENOMEM;
+			goto unreg_pnp;
+		}
+
+		ret = platform_device_add(serial8250_isa_devs);
+		if (ret)
+			goto put_dev;
+
+		serial8250_register_ports(&serial8250_reg, &serial8250_isa_devs->dev);
 	}
 
-	ret = platform_device_add(serial8250_isa_devs);
-	if (ret)
-		goto put_dev;
-
-	serial8250_register_ports(&serial8250_reg, &serial8250_isa_devs->dev);
-
 	ret = platform_driver_register(&serial8250_isa_driver);
 	if (ret == 0)
 		goto out;
+	if (!nr_uarts)
+		goto unreg_pnp;
 
 	platform_device_del(serial8250_isa_devs);
 put_dev:
@@ -1230,7 +1234,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.1

[PATCH v3 4/4] tty: 8250: update description of RUNTIME_PORTS / nr_uarts

[Martin Hundebøll]

The 8250 module has been updated allow configurations with zero builtin
UART ports, so change the description of the parameter to reflect that.

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

Change since v2:
 * new patch

 drivers/tty/serial/8250/8250_core.c |  3 ++-
 drivers/tty/serial/8250/Kconfig     | 10 +++++-----
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index a8fbc2325244..3d8bf0296080 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1257,7 +1257,8 @@ 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/Kconfig b/drivers/tty/serial/8250/Kconfig
index d0b49e15fbf5..65ef03553146 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -191,15 +191,15 @@ config SERIAL_8250_NR_UARTS
 	  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"
+	int "Number of built-in (non-discoverable) UARTs to initialize at boot time"
 	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
+	  Set this to the maximum number of built-in (non-discoverable) serial
+	  ports you want the kernel to initialize 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"
-- 
2.38.1

Re: [PATCH v3 4/4] tty: 8250: update description of RUNTIME_PORTS / nr_uarts

[Ilpo Järvinen]

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

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

> The 8250 module has been updated allow configurations with zero builtin
> UART ports, so change the description of the parameter to reflect that.
> 
> Signed-off-by: Martin Hundebøll <martin@geanix.com>
> ---
> 
> Change since v2:
>  * new patch
> 
>  drivers/tty/serial/8250/8250_core.c |  3 ++-
>  drivers/tty/serial/8250/Kconfig     | 10 +++++-----
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index a8fbc2325244..3d8bf0296080 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -1257,7 +1257,8 @@ 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) ")");

This fails to build. You have dropped the second underscore for some 
reason.

Shouldn't that 1- be also changed to 0- ?

-- 
 i.


>  
>  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/Kconfig b/drivers/tty/serial/8250/Kconfig
> index d0b49e15fbf5..65ef03553146 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -191,15 +191,15 @@ config SERIAL_8250_NR_UARTS
>  	  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"
> +	int "Number of built-in (non-discoverable) UARTs to initialize at boot time"
>  	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
> +	  Set this to the maximum number of built-in (non-discoverable) serial
> +	  ports you want the kernel to initialize 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"
> 

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

[Ilpo Järvinen]

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

On Tue, 25 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 patches 1-3:

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

Please include these tags into the next version of your submission.
Thank you.

-- 
 i.

> ---
> 
> Change since v1:
>  * call serial8250_pnp_init() also when nr_uarts is zero
> 
> Change since v2:
>  * invert condition to initialize built-in ports (as suggested by Ilpo)
> 
>  drivers/tty/serial/8250/8250_core.c | 30 +++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index ba48431ec6e2..a8fbc2325244 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -1186,22 +1186,26 @@ static int __init serial8250_init(void)
>  	if (ret)
>  		goto unreg_uart_drv;
>  
> -	serial8250_isa_devs = platform_device_alloc("serial8250",
> -						    PLAT8250_DEV_LEGACY);
> -	if (!serial8250_isa_devs) {
> -		ret = -ENOMEM;
> -		goto unreg_pnp;
> +	if (nr_uarts) {
> +		serial8250_isa_devs = platform_device_alloc("serial8250",
> +							    PLAT8250_DEV_LEGACY);
> +		if (!serial8250_isa_devs) {
> +			ret = -ENOMEM;
> +			goto unreg_pnp;
> +		}
> +
> +		ret = platform_device_add(serial8250_isa_devs);
> +		if (ret)
> +			goto put_dev;
> +
> +		serial8250_register_ports(&serial8250_reg, &serial8250_isa_devs->dev);
>  	}
>  
> -	ret = platform_device_add(serial8250_isa_devs);
> -	if (ret)
> -		goto put_dev;
> -
> -	serial8250_register_ports(&serial8250_reg, &serial8250_isa_devs->dev);
> -
>  	ret = platform_driver_register(&serial8250_isa_driver);
>  	if (ret == 0)
>  		goto out;
> +	if (!nr_uarts)
> +		goto unreg_pnp;
>  
>  	platform_device_del(serial8250_isa_devs);
>  put_dev:
> @@ -1230,7 +1234,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 v3 3/4] serial: 8250: skip platform device registration with no runtime ports

[Ilpo Järvinen]

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

On Wed, 26 Oct 2022, Ilpo Järvinen wrote:

> On Tue, 25 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 patches 1-3:
> 
> Tested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> Please include these tags into the next version of your submission.
> Thank you.

Actually, I just found out that some set of cmdline parameters do no 
longer work the same. So I'm retracting both of my tags for now.

=0 did work as expected due to this change which I tested and some other 
values >4 but there now seems to be problem of the console not showing up 
like previously when I don't give nr_uarts at all.

-- 
 i.

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

[Ilpo Järvinen]

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

On Fri, 28 Oct 2022, Ilpo Järvinen wrote:

> On Wed, 26 Oct 2022, Ilpo Järvinen wrote:
> 
> > On Tue, 25 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 patches 1-3:
> > 
> > Tested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > 
> > Please include these tags into the next version of your submission.
> > Thank you.
> 
> Actually, I just found out that some set of cmdline parameters do no 
> longer work the same. So I'm retracting both of my tags for now.
> 
> =0 did work as expected due to this change which I tested and some other 
> values >4 but there now seems to be problem of the console not showing up 
> like previously when I don't give nr_uarts at all.

NAK from me until the problem is resolved adequately.

Already the patch 1/4 causes an unacceptable reassignment of ttySx 
targets. This is going to break people's cmdline console setups so you 
need to find a better way.

Before any of these patches:

[    0.000000] Command line: console=ttyS0,115200n8 8250.nr_uarts=4
[    0.021031] Kernel command line: console=ttyS0,115200n8 8250.nr_uarts=4
[    0.441924] printk: console [ttyS0] enabled
[    2.243165] printk: console [ttyS0] disabled
[    2.245682] dw-apb-uart.6: ttyS0 at MMIO 0x4010006000 (irq = 33, base_baud = 115200) is a 16550A
[    4.010237] printk: console [ttyS0] enabled
[    5.933887] dw-apb-uart.7: ttyS1 at MMIO 0x4010007000 (irq = 16, base_baud = 6250000) is a 16550A
[    5.952829] dw-apb-uart.8: ttyS2 at MMIO 0x4010008000 (irq = 17, base_baud = 6250000) is a 16550A

After 1/4 ttyS0 is no longer the same:

[    0.000000] Command line: console=ttyS0,115200n8 8250.nr_uarts=4
[    0.021023] Kernel command line: console=ttyS0,115200n8 8250.nr_uarts=4
[    0.441872] printk: console [ttyS0] enabled
[    2.233584] dw-apb-uart.6: ttyS4 at MMIO 0x4010006000 (irq = 33, base_baud = 115200) is a 16550A
[    2.241955] dw-apb-uart.7: ttyS5 at MMIO 0x4010007000 (irq = 16, base_baud = 6250000) is a 16550A
[    2.249804] dw-apb-uart.8: ttyS6 at MMIO 0x4010008000 (irq = 17, base_baud = 6250000) is a 16550A


-- 
 i.

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

[Martin Hundebøll]

On 28/10/2022 11.40, Ilpo Järvinen wrote:
> On Fri, 28 Oct 2022, Ilpo Järvinen wrote:
> 
>> On Wed, 26 Oct 2022, Ilpo Järvinen wrote:
>>
>>> On Tue, 25 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 patches 1-3:
>>>
>>> Tested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>>>
>>> Please include these tags into the next version of your submission.
>>> Thank you.
>>
>> Actually, I just found out that some set of cmdline parameters do no
>> longer work the same. So I'm retracting both of my tags for now.
>>
>> =0 did work as expected due to this change which I tested and some other
>> values >4 but there now seems to be problem of the console not showing up
>> like previously when I don't give nr_uarts at all.
> 
> NAK from me until the problem is resolved adequately.
> 
> Already the patch 1/4 causes an unacceptable reassignment of ttySx
> targets. This is going to break people's cmdline console setups so you
> need to find a better way.
> 
> Before any of these patches:
> 
> [    0.000000] Command line: console=ttyS0,115200n8 8250.nr_uarts=4
> [    0.021031] Kernel command line: console=ttyS0,115200n8 8250.nr_uarts=4
> [    0.441924] printk: console [ttyS0] enabled
> [    2.243165] printk: console [ttyS0] disabled
> [    2.245682] dw-apb-uart.6: ttyS0 at MMIO 0x4010006000 (irq = 33, base_baud = 115200) is a 16550A
> [    4.010237] printk: console [ttyS0] enabled
> [    5.933887] dw-apb-uart.7: ttyS1 at MMIO 0x4010007000 (irq = 16, base_baud = 6250000) is a 16550A
> [    5.952829] dw-apb-uart.8: ttyS2 at MMIO 0x4010008000 (irq = 17, base_baud = 6250000) is a 16550A
> 
> After 1/4 ttyS0 is no longer the same:
> 
> [    0.000000] Command line: console=ttyS0,115200n8 8250.nr_uarts=4
> [    0.021023] Kernel command line: console=ttyS0,115200n8 8250.nr_uarts=4
> [    0.441872] printk: console [ttyS0] enabled
> [    2.233584] dw-apb-uart.6: ttyS4 at MMIO 0x4010006000 (irq = 33, base_baud = 115200) is a 16550A
> [    2.241955] dw-apb-uart.7: ttyS5 at MMIO 0x4010007000 (irq = 16, base_baud = 6250000) is a 16550A
> [    2.249804] dw-apb-uart.8: ttyS6 at MMIO 0x4010008000 (irq = 17, base_baud = 6250000) is a 16550A

Thanks for testing this.

The old behavior is wrong: your designware ports replace the built-in ones (0 to 3) instead of using the unused ones (4 to 31). With these patches, it acts as I'd expect: the built-in ports are kept, and any later discovered ports follow.

Yes, breaking existing systems in this way is unacceptable. I'm not sure how to approach this, but I'm inclined to introduce a new config variable to keep the broken behavior?

// Martin


  

Re: [PATCH v3 4/4] tty: 8250: update description of RUNTIME_PORTS / nr_uarts

[Martin Hundebøll]

On 26/10/2022 13.25, Ilpo Järvinen wrote:
> On Tue, 25 Oct 2022, Martin Hundebøll wrote:
> 
>> The 8250 module has been updated allow configurations with zero builtin
>> UART ports, so change the description of the parameter to reflect that.
>>
>> Signed-off-by: Martin Hundebøll<martin@geanix.com>
>> ---
>>
>> Change since v2:
>>   * new patch
>>
>>   drivers/tty/serial/8250/8250_core.c |  3 ++-
>>   drivers/tty/serial/8250/Kconfig     | 10 +++++-----
>>   2 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>> index a8fbc2325244..3d8bf0296080 100644
>> --- a/drivers/tty/serial/8250/8250_core.c
>> +++ b/drivers/tty/serial/8250/8250_core.c
>> @@ -1257,7 +1257,8 @@ 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) ")");
> This fails to build. You have dropped the second underscore for some
> reason.

Running checkpatch after make is dangerous...

> Shouldn't that 1- be also changed to 0- ?

Correct. In fact, it should be 0 - ARRAY_SIZE(old_serial_port) - 1.

// Martin

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

[Ilpo Järvinen]

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

On Tue, 1 Nov 2022, Martin Hundebøll wrote:

> On 28/10/2022 11.40, Ilpo Järvinen wrote:
> > On Fri, 28 Oct 2022, Ilpo Järvinen wrote:
> > 
> > > On Wed, 26 Oct 2022, Ilpo Järvinen wrote:
> > > 
> > > > On Tue, 25 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 patches 1-3:
> > > > 
> > > > Tested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > 
> > > > Please include these tags into the next version of your submission.
> > > > Thank you.
> > > 
> > > Actually, I just found out that some set of cmdline parameters do no
> > > longer work the same. So I'm retracting both of my tags for now.
> > > 
> > > =0 did work as expected due to this change which I tested and some other
> > > values >4 but there now seems to be problem of the console not showing up
> > > like previously when I don't give nr_uarts at all.
> > 
> > NAK from me until the problem is resolved adequately.
> > 
> > Already the patch 1/4 causes an unacceptable reassignment of ttySx
> > targets. This is going to break people's cmdline console setups so you
> > need to find a better way.
> > 
> > Before any of these patches:
> > 
> > [    0.000000] Command line: console=ttyS0,115200n8 8250.nr_uarts=4
> > [    0.021031] Kernel command line: console=ttyS0,115200n8 8250.nr_uarts=4
> > [    0.441924] printk: console [ttyS0] enabled
> > [    2.243165] printk: console [ttyS0] disabled
> > [    2.245682] dw-apb-uart.6: ttyS0 at MMIO 0x4010006000 (irq = 33,
> > base_baud = 115200) is a 16550A
> > [    4.010237] printk: console [ttyS0] enabled
> > [    5.933887] dw-apb-uart.7: ttyS1 at MMIO 0x4010007000 (irq = 16,
> > base_baud = 6250000) is a 16550A
> > [    5.952829] dw-apb-uart.8: ttyS2 at MMIO 0x4010008000 (irq = 17,
> > base_baud = 6250000) is a 16550A
> > 
> > After 1/4 ttyS0 is no longer the same:
> > 
> > [    0.000000] Command line: console=ttyS0,115200n8 8250.nr_uarts=4
> > [    0.021023] Kernel command line: console=ttyS0,115200n8 8250.nr_uarts=4
> > [    0.441872] printk: console [ttyS0] enabled
> > [    2.233584] dw-apb-uart.6: ttyS4 at MMIO 0x4010006000 (irq = 33,
> > base_baud = 115200) is a 16550A
> > [    2.241955] dw-apb-uart.7: ttyS5 at MMIO 0x4010007000 (irq = 16,
> > base_baud = 6250000) is a 16550A
> > [    2.249804] dw-apb-uart.8: ttyS6 at MMIO 0x4010008000 (irq = 17,
> > base_baud = 6250000) is a 16550A
> 
> Thanks for testing this.
> 
> The old behavior is wrong: your designware ports replace the built-in ones (0
> to 3) instead of using the unused ones (4 to 31). With these patches, it acts
> as I'd expect: the built-in ports are kept, and any later discovered ports
> follow.
> 
> Yes, breaking existing systems in this way is unacceptable. I'm not sure how
> to approach this, but I'm inclined to introduce a new config variable to keep
> the broken behavior?

To me this looks now more an attempt to repurpose the nr_uarts to mean 
something it really hasn't. Clearly it hasn't meant non-discoverable 
ports earlier but something related to the maximum number ports. This also 
explains why we had that misunderstanding about the meaning of nr_uarts 
between us earlier.

The code which makes this "wrong behavior" to happen is the last loop in 
serial8250_find_match_or_unused(). Given its comment, I think it has been 
very much intentional behavior. Pretending that non-discoverable port is 
more real/precious than a port that was later _discovered_ and is very 
much a real one is what brings you to this renumbering issue.

I'm not anymore sure about your goals really. Now it sounds one of the 
goals is preserving the non-discoverable ports, whereas previously it was 
just about allowing 0 of them.

How about you add entirely new CONFIG and/or param for this minimum number 
of non-discoverable ports and make the last resort loop (or perhaps all 
but the first loop) in serial8250_find_match_or_unused() to honor that 
(start looking only from port above that index). If it defaults to 0, I 
think this renumbering issue is avoided. Would it work for all the goals 
you have? 

With that change, I don't know what condition should trigger what you do 
in 3/4 though.


-- 
 i.

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

[Martin Hundebøll]

On 01/11/2022 12.44, Ilpo Järvinen wrote:
> On Tue, 1 Nov 2022, Martin Hundebøll wrote:
> 
>> On 28/10/2022 11.40, Ilpo Järvinen wrote:
>>> On Fri, 28 Oct 2022, Ilpo Järvinen wrote:
>>>
>>>> On Wed, 26 Oct 2022, Ilpo Järvinen wrote:
>>>>
>>>>> On Tue, 25 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 patches 1-3:
>>>>>
>>>>> Tested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>>>>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>>>>>
>>>>> Please include these tags into the next version of your submission.
>>>>> Thank you.
>>>>
>>>> Actually, I just found out that some set of cmdline parameters do no
>>>> longer work the same. So I'm retracting both of my tags for now.
>>>>
>>>> =0 did work as expected due to this change which I tested and some other
>>>> values >4 but there now seems to be problem of the console not showing up
>>>> like previously when I don't give nr_uarts at all.
>>>
>>> NAK from me until the problem is resolved adequately.
>>>
>>> Already the patch 1/4 causes an unacceptable reassignment of ttySx
>>> targets. This is going to break people's cmdline console setups so you
>>> need to find a better way.
>>>
>>> Before any of these patches:
>>>
>>> [    0.000000] Command line: console=ttyS0,115200n8 8250.nr_uarts=4
>>> [    0.021031] Kernel command line: console=ttyS0,115200n8 8250.nr_uarts=4
>>> [    0.441924] printk: console [ttyS0] enabled
>>> [    2.243165] printk: console [ttyS0] disabled
>>> [    2.245682] dw-apb-uart.6: ttyS0 at MMIO 0x4010006000 (irq = 33,
>>> base_baud = 115200) is a 16550A
>>> [    4.010237] printk: console [ttyS0] enabled
>>> [    5.933887] dw-apb-uart.7: ttyS1 at MMIO 0x4010007000 (irq = 16,
>>> base_baud = 6250000) is a 16550A
>>> [    5.952829] dw-apb-uart.8: ttyS2 at MMIO 0x4010008000 (irq = 17,
>>> base_baud = 6250000) is a 16550A
>>>
>>> After 1/4 ttyS0 is no longer the same:
>>>
>>> [    0.000000] Command line: console=ttyS0,115200n8 8250.nr_uarts=4
>>> [    0.021023] Kernel command line: console=ttyS0,115200n8 8250.nr_uarts=4
>>> [    0.441872] printk: console [ttyS0] enabled
>>> [    2.233584] dw-apb-uart.6: ttyS4 at MMIO 0x4010006000 (irq = 33,
>>> base_baud = 115200) is a 16550A
>>> [    2.241955] dw-apb-uart.7: ttyS5 at MMIO 0x4010007000 (irq = 16,
>>> base_baud = 6250000) is a 16550A
>>> [    2.249804] dw-apb-uart.8: ttyS6 at MMIO 0x4010008000 (irq = 17,
>>> base_baud = 6250000) is a 16550A
>>
>> Thanks for testing this.
>>
>> The old behavior is wrong: your designware ports replace the built-in ones (0
>> to 3) instead of using the unused ones (4 to 31). With these patches, it acts
>> as I'd expect: the built-in ports are kept, and any later discovered ports
>> follow.
>>
>> Yes, breaking existing systems in this way is unacceptable. I'm not sure how
>> to approach this, but I'm inclined to introduce a new config variable to keep
>> the broken behavior?
> 
> To me this looks now more an attempt to repurpose the nr_uarts to mean
> something it really hasn't. Clearly it hasn't meant non-discoverable
> ports earlier but something related to the maximum number ports. This also
> explains why we had that misunderstanding about the meaning of nr_uarts
> between us earlier.

I was confused about the relation between SERIAL_8250_NR_UARTS and SERIAL_8250_RUNTIME_UARTS, and started digging through the code. The description of NR_UARTS clearly states that it controls the number of *supported* uarts, but I found that it was in fact controlled by RUNTIME_UARTS (8250.nr_uarts).

With the current behavior, ever setting NR_UARTS to anything different from RUNTIME_UARTS makes no sense, since the code only loops until RUNTIME_UARTS. Both Arch Linux and Fedora have come to the same conclusion, since they set both configs to 32. So now I have 32 useless /dev/ttyS* nodes on my laptop. If you run ubuntu on a machine with no uart hardware attached, you'll get 48 useless ttyS* nodes, of which only the first 32 will ever be available for real hardware.

If you look at uevents from 8250 when the designware ports are discovered, you'll see a DELETE of the built-in ttyS0 first, and then an ADD of the designware ttyS0. My understanding is that we should never have had that built-in ttyS0 in the first place?

> The code which makes this "wrong behavior" to happen is the last loop in
> serial8250_find_match_or_unused(). Given its comment, I think it has been
> very much intentional behavior. Pretending that non-discoverable port is
> more real/precious than a port that was later _discovered_ and is very
> much a real one is what brings you to this renumbering issue.

The serial8250_find_match_or_unused() logic exists to relate non-discoverable ports to those discovered later through ACPI PNP. Some of my systems announces those built-in ports with ACPI, in which case the discovered ports are assigned to their "built-in" numbers.

I believe all systems running today announce these built-in ports through ACPI PNP, but they are still usable as early consoles, so this special logic is still needed?

> I'm not anymore sure about your goals really. Now it sounds one of the
> goals is preserving the non-discoverable ports, whereas previously it was
> just about allowing 0 of them.

My initial goal was to rely on discoverable ports only. Some of our systems have no built-in uarts, so the bash init script failed to even start, because the kernel command line had console=ttyS0, and bash tried to do set terminal attributes on it. I am only trying to fix the code "the right way", but that might not be possible without breaking numbering fo existing systems.

> How about you add entirely new CONFIG and/or param for this minimum number
> of non-discoverable ports and make the last resort loop (or perhaps all
> but the first loop) in serial8250_find_match_or_unused() to honor that
> (start looking only from port above that index). If it defaults to 0, I
> think this renumbering issue is avoided. Would it work for all the goals
> you have?

That would work for me. But shouldn't we clean up the SERIAL_8250_NR_UARTS vs. SERIAL_8250_RUNTIME_UARTS also?

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

[Ilpo Järvinen]

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

On Tue, 1 Nov 2022, Martin Hundebøll wrote:
> On 01/11/2022 12.44, Ilpo Järvinen wrote:
> > On Tue, 1 Nov 2022, Martin Hundebøll wrote:
> > > On 28/10/2022 11.40, Ilpo Järvinen wrote:
> > > > On Fri, 28 Oct 2022, Ilpo Järvinen wrote:
> > > > > =0 did work as expected due to this change which I tested and some
> > > > > other
> > > > > values >4 but there now seems to be problem of the console not showing
> > > > > up
> > > > > like previously when I don't give nr_uarts at all.
> > > > 
> > > > NAK from me until the problem is resolved adequately.
> > > > 
> > > > Already the patch 1/4 causes an unacceptable reassignment of ttySx
> > > > targets. This is going to break people's cmdline console setups so you
> > > > need to find a better way.
> > > > 
> > > > Before any of these patches:
> > > > 
> > > > [    0.000000] Command line: console=ttyS0,115200n8 8250.nr_uarts=4
> > > > [    0.021031] Kernel command line: console=ttyS0,115200n8
> > > > 8250.nr_uarts=4
> > > > [    0.441924] printk: console [ttyS0] enabled
> > > > [    2.243165] printk: console [ttyS0] disabled
> > > > [    2.245682] dw-apb-uart.6: ttyS0 at MMIO 0x4010006000 (irq = 33,
> > > > base_baud = 115200) is a 16550A
> > > > [    4.010237] printk: console [ttyS0] enabled
> > > > [    5.933887] dw-apb-uart.7: ttyS1 at MMIO 0x4010007000 (irq = 16,
> > > > base_baud = 6250000) is a 16550A
> > > > [    5.952829] dw-apb-uart.8: ttyS2 at MMIO 0x4010008000 (irq = 17,
> > > > base_baud = 6250000) is a 16550A
> > > > 
> > > > After 1/4 ttyS0 is no longer the same:
> > > > 
> > > > [    0.000000] Command line: console=ttyS0,115200n8 8250.nr_uarts=4
> > > > [    0.021023] Kernel command line: console=ttyS0,115200n8
> > > > 8250.nr_uarts=4
> > > > [    0.441872] printk: console [ttyS0] enabled
> > > > [    2.233584] dw-apb-uart.6: ttyS4 at MMIO 0x4010006000 (irq = 33,
> > > > base_baud = 115200) is a 16550A
> > > > [    2.241955] dw-apb-uart.7: ttyS5 at MMIO 0x4010007000 (irq = 16,
> > > > base_baud = 6250000) is a 16550A
> > > > [    2.249804] dw-apb-uart.8: ttyS6 at MMIO 0x4010008000 (irq = 17,
> > > > base_baud = 6250000) is a 16550A
> > > 
> > > Thanks for testing this.
> > > 
> > > The old behavior is wrong: your designware ports replace the built-in ones
> > > (0
> > > to 3) instead of using the unused ones (4 to 31). With these patches, it
> > > acts
> > > as I'd expect: the built-in ports are kept, and any later discovered ports
> > > follow.
> > > 
> > > Yes, breaking existing systems in this way is unacceptable. I'm not sure
> > > how
> > > to approach this, but I'm inclined to introduce a new config variable to
> > > keep
> > > the broken behavior?
> > 
> > To me this looks now more an attempt to repurpose the nr_uarts to mean
> > something it really hasn't. Clearly it hasn't meant non-discoverable
> > ports earlier but something related to the maximum number ports. This also
> > explains why we had that misunderstanding about the meaning of nr_uarts
> > between us earlier.
> 
> I was confused about the relation between SERIAL_8250_NR_UARTS and
> SERIAL_8250_RUNTIME_UARTS, and started digging through the code. The
> description of NR_UARTS clearly states that it controls the number of
> *supported* uarts, but I found that it was in fact controlled by RUNTIME_UARTS
> (8250.nr_uarts).
>
> With the current behavior, ever setting NR_UARTS to anything different from
> RUNTIME_UARTS makes no sense, since the code only loops until RUNTIME_UARTS.

One is there to size the array (max you can have with a particular build
of a kernel ever). The other is to control the maximum number of ports per 
boot. I'd guess the former is there mainly for legacy reason from the era 
when memory really was scarser resource than it is today.

> Both Arch Linux and Fedora have come to the same conclusion, since they set
> both configs to 32. So now I have 32 useless /dev/ttyS* nodes on my laptop. If
> you run ubuntu on a machine with no uart hardware attached, you'll get 48
> useless ttyS* nodes, of which only the first 32 will ever be available for
> real hardware.

Why load the driver then at all, if there's no need/hw for it? Ironically, 
setting nr_uarts (as it is) to a smaller value would reduce that waste 
and in case of =0 the driver wouldn't be loaded.

I know distros will do it but that's their policy decision that doesn't 
really belong here. If there's hw/need for any users of a distro, they 
provide the support in the most generic way that makes things easy for 
them/users, that is, by having sufficient number of ports for (almost) all 
usecases.

In addition, to me n useless ports just sitting there doesn't sound that 
serious issue that it would warrant breaking setups that currently depend 
on certain ttyS numbers (I suppose you actually agree with this point).

> If you look at uevents from 8250 when the designware ports are discovered,
> you'll see a DELETE of the built-in ttyS0 first, and then an ADD of the
> designware ttyS0. My understanding is that we should never have had that
> built-in ttyS0 in the first place?

I certainly suspect the built-in one really isn't there (in ACPI) but I 
don't want to spend my time on confirming the obvious.

> > The code which makes this "wrong behavior" to happen is the last loop in
> > serial8250_find_match_or_unused(). Given its comment, I think it has been
> > very much intentional behavior. Pretending that non-discoverable port is
> > more real/precious than a port that was later _discovered_ and is very
> > much a real one is what brings you to this renumbering issue.
> 
> The serial8250_find_match_or_unused() logic exists to relate non-discoverable
> ports to those discovered later through ACPI PNP. Some of my systems announces
> those built-in ports with ACPI, in which case the discovered ports are
> assigned to their "built-in" numbers.
> 
> I believe all systems running today announce these built-in ports through ACPI
> PNP, but they are still usable as early consoles, so this special logic is
> still needed?

Unfortunately, I don't know the answer to whether it is a safe assumption 
or not.

> > I'm not anymore sure about your goals really. Now it sounds one of the
> > goals is preserving the non-discoverable ports, whereas previously it was
> > just about allowing 0 of them.
> 
> My initial goal was to rely on discoverable ports only. Some of our systems
> have no built-in uarts, so the bash init script failed to even start, because
> the kernel command line had console=ttyS0, and bash tried to do set terminal
> attributes on it. I am only trying to fix the code "the right way", but that
> might not be possible without breaking numbering fo existing systems.

Again I don't follow the reasoning fully here. What is your end goal here 
in this scenario. You'd either want to have no ports to appear at all or 
have a non-discoverable (=bogus) port as ttyS0. Which way it is?

If you, on the other hand, would have a discoverable port that will take 
over ttyS0, I don't understand why it fails so it seemingly leaves only 
those two options I gave.

> > How about you add entirely new CONFIG and/or param for this minimum number
> > of non-discoverable ports and make the last resort loop (or perhaps all
> > but the first loop) in serial8250_find_match_or_unused() to honor that
> > (start looking only from port above that index). If it defaults to 0, I
> > think this renumbering issue is avoided. Would it work for all the goals
> > you have?
> 
> That would work for me. But shouldn't we clean up the 
> SERIAL_8250_NR_UARTS vs. SERIAL_8250_RUNTIME_UARTS also?

I'm not convinced there's something wrong with it. It's legacy yes, but
the max ports compiled in vs max ports per boot is not wrong/buggy as is.


-- 
 i.

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

[Martin Hundebøll]

On Tue, Nov 01, 2022 at 03:22:33PM +0200, Ilpo Järvinen wrote:
> On Tue, 1 Nov 2022, Martin Hundebøll wrote:
> > On 01/11/2022 12.44, Ilpo Järvinen wrote:
> > > On Tue, 1 Nov 2022, Martin Hundebøll wrote:
> > > > On 28/10/2022 11.40, Ilpo Järvinen wrote:
> > > > > On Fri, 28 Oct 2022, Ilpo Järvinen wrote:
> > > > > > =0 did work as expected due to this change which I tested and some
> > > > > > other
> > > > > > values >4 but there now seems to be problem of the console not showing
> > > > > > up
> > > > > > like previously when I don't give nr_uarts at all.
> > > > > 
> > > > > NAK from me until the problem is resolved adequately.
> > > > > 
> > > > > Already the patch 1/4 causes an unacceptable reassignment of ttySx
> > > > > targets. This is going to break people's cmdline console setups so you
> > > > > need to find a better way.
> > > > > 
> > > > > Before any of these patches:
> > > > > 
> > > > > [    0.000000] Command line: console=ttyS0,115200n8 8250.nr_uarts=4
> > > > > [    0.021031] Kernel command line: console=ttyS0,115200n8
> > > > > 8250.nr_uarts=4
> > > > > [    0.441924] printk: console [ttyS0] enabled
> > > > > [    2.243165] printk: console [ttyS0] disabled
> > > > > [    2.245682] dw-apb-uart.6: ttyS0 at MMIO 0x4010006000 (irq = 33,
> > > > > base_baud = 115200) is a 16550A
> > > > > [    4.010237] printk: console [ttyS0] enabled
> > > > > [    5.933887] dw-apb-uart.7: ttyS1 at MMIO 0x4010007000 (irq = 16,
> > > > > base_baud = 6250000) is a 16550A
> > > > > [    5.952829] dw-apb-uart.8: ttyS2 at MMIO 0x4010008000 (irq = 17,
> > > > > base_baud = 6250000) is a 16550A
> > > > > 
> > > > > After 1/4 ttyS0 is no longer the same:
> > > > > 
> > > > > [    0.000000] Command line: console=ttyS0,115200n8 8250.nr_uarts=4
> > > > > [    0.021023] Kernel command line: console=ttyS0,115200n8
> > > > > 8250.nr_uarts=4
> > > > > [    0.441872] printk: console [ttyS0] enabled
> > > > > [    2.233584] dw-apb-uart.6: ttyS4 at MMIO 0x4010006000 (irq = 33,
> > > > > base_baud = 115200) is a 16550A
> > > > > [    2.241955] dw-apb-uart.7: ttyS5 at MMIO 0x4010007000 (irq = 16,
> > > > > base_baud = 6250000) is a 16550A
> > > > > [    2.249804] dw-apb-uart.8: ttyS6 at MMIO 0x4010008000 (irq = 17,
> > > > > base_baud = 6250000) is a 16550A
> > > > 
> > > > Thanks for testing this.
> > > > 
> > > > The old behavior is wrong: your designware ports replace the built-in ones
> > > > (0
> > > > to 3) instead of using the unused ones (4 to 31). With these patches, it
> > > > acts
> > > > as I'd expect: the built-in ports are kept, and any later discovered ports
> > > > follow.
> > > > 
> > > > Yes, breaking existing systems in this way is unacceptable. I'm not sure
> > > > how
> > > > to approach this, but I'm inclined to introduce a new config variable to
> > > > keep
> > > > the broken behavior?
> > > 
> > > To me this looks now more an attempt to repurpose the nr_uarts to mean
> > > something it really hasn't. Clearly it hasn't meant non-discoverable
> > > ports earlier but something related to the maximum number ports. This also
> > > explains why we had that misunderstanding about the meaning of nr_uarts
> > > between us earlier.
> > 
> > I was confused about the relation between SERIAL_8250_NR_UARTS and
> > SERIAL_8250_RUNTIME_UARTS, and started digging through the code. The
> > description of NR_UARTS clearly states that it controls the number of
> > *supported* uarts, but I found that it was in fact controlled by RUNTIME_UARTS
> > (8250.nr_uarts).
> >
> > With the current behavior, ever setting NR_UARTS to anything different from
> > RUNTIME_UARTS makes no sense, since the code only loops until RUNTIME_UARTS.
> 
> One is there to size the array (max you can have with a particular build
> of a kernel ever). The other is to control the maximum number of ports per 
> boot. I'd guess the former is there mainly for legacy reason from the era 
> when memory really was scarser resource than it is today.

Oh, I see.

> > Both Arch Linux and Fedora have come to the same conclusion, since they set
> > both configs to 32. So now I have 32 useless /dev/ttyS* nodes on my laptop. If
> > you run ubuntu on a machine with no uart hardware attached, you'll get 48
> > useless ttyS* nodes, of which only the first 32 will ever be available for
> > real hardware.
> 
> Why load the driver then at all, if there's no need/hw for it? Ironically, 
> setting nr_uarts (as it is) to a smaller value would reduce that waste 
> and in case of =0 the driver wouldn't be loaded.

We have some PCIe attached UARTs that doesn't appear until after the
system is up and running (they sit behind an FPGA controlled from
userspace.) Some revisions have built-in UARTs while others don't, and I
would like to run the same kernel, rootfs, and boot configuration on all
of them.

> I know distros will do it but that's their policy decision that doesn't 
> really belong here. If there's hw/need for any users of a distro, they 
> provide the support in the most generic way that makes things easy for 
> them/users, that is, by having sufficient number of ports for (almost) all 
> usecases.
> 
> In addition, to me n useless ports just sitting there doesn't sound that 
> serious issue that it would warrant breaking setups that currently depend 
> on certain ttyS numbers (I suppose you actually agree with this point).

Agreed. Although we have seen races between the DELETE and ADD uevents
mentioned below, leading to non-deterministical device numbers.

> > If you look at uevents from 8250 when the designware ports are discovered,
> > you'll see a DELETE of the built-in ttyS0 first, and then an ADD of the
> > designware ttyS0. My understanding is that we should never have had that
> > built-in ttyS0 in the first place?
> 
> I certainly suspect the built-in one really isn't there (in ACPI) but I 
> don't want to spend my time on confirming the obvious.

From a system booted with these patches, and 8250.nr_uarts=0, I get two
ports from ACPI PNP:

# cat /sys/class/tty/ttyS0/port
0x3F8
# cat /sys/class/tty/ttyS1/port
0x2F8

which matches the SERIAL_PORT_DNFS define in arch/x86/include/asm/serial.h

> > > The code which makes this "wrong behavior" to happen is the last loop in
> > > serial8250_find_match_or_unused(). Given its comment, I think it has been
> > > very much intentional behavior. Pretending that non-discoverable port is
> > > more real/precious than a port that was later _discovered_ and is very
> > > much a real one is what brings you to this renumbering issue.
> > 
> > The serial8250_find_match_or_unused() logic exists to relate non-discoverable
> > ports to those discovered later through ACPI PNP. Some of my systems announces
> > those built-in ports with ACPI, in which case the discovered ports are
> > assigned to their "built-in" numbers.
> > 
> > I believe all systems running today announce these built-in ports through ACPI
> > PNP, but they are still usable as early consoles, so this special logic is
> > still needed?
> 
> Unfortunately, I don't know the answer to whether it is a safe assumption 
> or not.
> 
> > > I'm not anymore sure about your goals really. Now it sounds one of the
> > > goals is preserving the non-discoverable ports, whereas previously it was
> > > just about allowing 0 of them.
> > 
> > My initial goal was to rely on discoverable ports only. Some of our systems
> > have no built-in uarts, so the bash init script failed to even start, because
> > the kernel command line had console=ttyS0, and bash tried to do set terminal
> > attributes on it. I am only trying to fix the code "the right way", but that
> > might not be possible without breaking numbering fo existing systems.
> 
> Again I don't follow the reasoning fully here. What is your end goal here 
> in this scenario. You'd either want to have no ports to appear at all or 
> have a non-discoverable (=bogus) port as ttyS0. Which way it is?
> 
> If you, on the other hand, would have a discoverable port that will take 
> over ttyS0, I don't understand why it fails so it seemingly leaves only 
> those two options I gave.

My endgoal is to avoid a bogus ttyS0, but a working driver (for the PCIe
connected ports).

I still think it seems wrong to have non-working device nodes. They only
confuse both people and software:

% sudo stty -F /dev/ttyS0
stty: /dev/ttyS0: Input/output error

> > > How about you add entirely new CONFIG and/or param for this minimum number
> > > of non-discoverable ports and make the last resort loop (or perhaps all
> > > but the first loop) in serial8250_find_match_or_unused() to honor that
> > > (start looking only from port above that index). If it defaults to 0, I
> > > think this renumbering issue is avoided. Would it work for all the goals
> > > you have?
> > 
> > That would work for me. But shouldn't we clean up the 
> > SERIAL_8250_NR_UARTS vs. SERIAL_8250_RUNTIME_UARTS also?
> 
> I'm not convinced there's something wrong with it. It's legacy yes, but
> the max ports compiled in vs max ports per boot is not wrong/buggy as is.

Your interpretation of those two makes sense, but I don't really see a
usecase for setting 8250.nr_uarts at boot time though.

// Martin

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

[Ilpo Järvinen]

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

On Tue, 1 Nov 2022, Martin Hundebøll wrote:
> On Tue, Nov 01, 2022 at 03:22:33PM +0200, Ilpo Järvinen wrote:
> > On Tue, 1 Nov 2022, Martin Hundebøll wrote:

> > In addition, to me n useless ports just sitting there doesn't sound that 
> > serious issue that it would warrant breaking setups that currently depend 
> > on certain ttyS numbers (I suppose you actually agree with this point).
> 
> Agreed. Although we have seen races between the DELETE and ADD uevents
> mentioned below, leading to non-deterministical device numbers.

No doubt there will be cases were the numbering is not as stable as we'd 
like but it seems an unrelated issue.

> > > If you look at uevents from 8250 when the designware ports are discovered,
> > > you'll see a DELETE of the built-in ttyS0 first, and then an ADD of the
> > > designware ttyS0. My understanding is that we should never have had that
> > > built-in ttyS0 in the first place?
> > 
> > I certainly suspect the built-in one really isn't there (in ACPI) but I 
> > don't want to spend my time on confirming the obvious.
> 
> >From a system booted with these patches, and 8250.nr_uarts=0, I get two
> ports from ACPI PNP:
> 
> # cat /sys/class/tty/ttyS0/port
> 0x3F8
> # cat /sys/class/tty/ttyS1/port
> 0x2F8
> 
> which matches the SERIAL_PORT_DNFS define in arch/x86/include/asm/serial.h

I was talking about the system I was testing with.

> > > > I'm not anymore sure about your goals really. Now it sounds one of the
> > > > goals is preserving the non-discoverable ports, whereas previously it was
> > > > just about allowing 0 of them.
> > > 
> > > My initial goal was to rely on discoverable ports only. Some of our systems
> > > have no built-in uarts, so the bash init script failed to even start, because
> > > the kernel command line had console=ttyS0, and bash tried to do set terminal
> > > attributes on it. I am only trying to fix the code "the right way", but that
> > > might not be possible without breaking numbering fo existing systems.
> > 
> > Again I don't follow the reasoning fully here. What is your end goal here 
> > in this scenario. You'd either want to have no ports to appear at all or 
> > have a non-discoverable (=bogus) port as ttyS0. Which way it is?
> > 
> > If you, on the other hand, would have a discoverable port that will take 
> > over ttyS0, I don't understand why it fails so it seemingly leaves only 
> > those two options I gave.
> 
> My endgoal is to avoid a bogus ttyS0, but a working driver (for the PCIe
> connected ports).
> 
> I still think it seems wrong to have non-working device nodes. They only
> confuse both people and software:
> 
> % sudo stty -F /dev/ttyS0
> stty: /dev/ttyS0: Input/output error

So simply add the maximum number of non-discoverable ports? Something 
which isn't reusing nr_uarts or its related CONFIG, and pick such a 
default which doesn't break existing cmdlines.

> > > That would work for me. But shouldn't we clean up the 
> > > SERIAL_8250_NR_UARTS vs. SERIAL_8250_RUNTIME_UARTS also?
> > 
> > I'm not convinced there's something wrong with it. It's legacy yes, but
> > the max ports compiled in vs max ports per boot is not wrong/buggy as is.
> 
> Your interpretation of those two makes sense, but I don't really see a
> usecase for setting 8250.nr_uarts at boot time though.

You might not but we shouldn't change it to mean something else.


-- 
 i.