Re: [PATCH 1/1] tty: serial: handle HAS_IOPORT dependencies

["Arnd Bergmann" <arnd@kernel.org>]

On Fri, Nov 22, 2024, at 21:44, Guenter Roeck wrote:
> On 11/22/24 11:24, Arnd Bergmann wrote:
>> 
>> serial8250_setup_port() is where it ends up using the
>> uninitialized serial8250_ports[index] contents. Since
>> port->iotype is not set to anything here, it defaults to
>> '0', which happens to be UPIO_PORT.
>> 
>> The reason it doesn't immediately crash and burn is that
>> this is still only setting up the structures for later
>> use, but I assume that trying to use console=ttyS0, or
>> opening /dev/ttyS0 on the uninitialized port would
>> then cause an oops.
>> 
>
> All four affected platforms use ttyS0, only it is mmio based,
> not io port based.

Right, so I think the problem is really the 8250_platform.c
file trying to handle all possible cases on x86 (acpi,
isapnp, legacy isa) that may define the same UARTs, but
also trying to handle non-legacy ports with their own
special cases on other architectures.

The patch below is a first idea of how we can skip the
legacy ISA case on architectures that don't define those.
If this works, we can try to come up with a better way of
doing that. Ideally all the pre-DT boardfile machines that
define their own "serial8250" platform_device (with platform_data)
should use a different identifier the x86-legacy case that
uses the platform_device (without platform_data). Similarly,
I think the riscv special ACPI port needs its own trivial
driver without all the ISA magic.

      Arnd

diff --git a/drivers/tty/serial/8250/8250_platform.c b/drivers/tty/serial/8250/8250_platform.c
index 66fd6d5d4835..610b31517734 100644
--- a/drivers/tty/serial/8250/8250_platform.c
+++ b/drivers/tty/serial/8250/8250_platform.c
@@ -101,7 +101,8 @@ static void __init __serial8250_isa_init_ports(void)
 
 void __init serial8250_isa_init_ports(void)
 {
-	DO_ONCE(__serial8250_isa_init_ports);
+	if (ARRAY_SIZE(old_serial_port))
+		DO_ONCE(__serial8250_isa_init_ports);
 }
 
 /*
@@ -283,7 +284,7 @@ static const struct acpi_device_id acpi_platform_serial_table[] = {
 };
 MODULE_DEVICE_TABLE(acpi, acpi_platform_serial_table);
 
-static struct platform_driver serial8250_isa_driver = {
+static struct platform_driver serial8250_platform_driver = {
 	.probe		= serial8250_probe,
 	.remove		= serial8250_remove,
 	.suspend	= serial8250_suspend,
@@ -300,7 +301,7 @@ static struct platform_driver serial8250_isa_driver = {
  */
 struct platform_device *serial8250_isa_devs;
 
-static int __init serial8250_init(void)
+static int __init serial8250_isa_init(void)
 {
 	int ret;
 
@@ -337,11 +338,8 @@ static int __init serial8250_init(void)
 
 	serial8250_register_ports(&serial8250_reg, &serial8250_isa_devs->dev);
 
-	ret = platform_driver_register(&serial8250_isa_driver);
-	if (ret == 0)
-		goto out;
+	return 0;
 
-	platform_device_del(serial8250_isa_devs);
 put_dev:
 	platform_device_put(serial8250_isa_devs);
 unreg_pnp:
@@ -355,6 +353,17 @@ static int __init serial8250_init(void)
 out:
 	return ret;
 }
+
+static int __init serial8250_init(void)
+{
+	if (ARRAY_SIZE(old_serial_port)) {
+		int ret = serial8250_isa_init();
+		if (ret)
+			return ret;
+	}
+
+	return platform_driver_register(&serial8250_platform_driver);
+}
 module_init(serial8250_init);
 
 static void __exit serial8250_exit(void)
@@ -368,7 +377,11 @@ static void __exit serial8250_exit(void)
 	 */
 	serial8250_isa_devs = NULL;
 
-	platform_driver_unregister(&serial8250_isa_driver);
+	platform_driver_unregister(&serial8250_platform_driver);
+
+	if (!ARRAY_SIZE(old_serial_port))
+		return;
+
 	platform_device_unregister(isa_dev);
 
 	serial8250_pnp_exit();