Skip to contentSkip to conversation

8250: serial device file is out of order

PATCH In review Scott Yuan 3 participants 3 messages Aug 25, 2014

Subsystems (4)
lore
Share & tools
lore

Scott Yuan

author
#0 lore raw

PATCH 1/1 8250: serial device file is out of order

From ebb85a61456bf25f87aeb8a31c92c74540bdf306 Mon Sep 17 00:00:00 2001
From: Scott Yuan <[email protected]>
Date: Sun, 24 Aug 2014 13:46:56 +0800
Subject: [PATCH] 8250: serial device file is out of order

On x86 architecture, the configuration of serial device maybe get from ACPI DSDT, but the order of DSDT is not mandatory, result as array serial8250_ports is out of order. This situation is more obvious in multiple serial port mainboard. Sort it by unique id that in DSDT will fix it.

---
 drivers/pnp/pnpacpi/core.c          |  1 +
 drivers/tty/serial/8250/8250_core.c | 40 
++++++++++++++++++++++++++++++++++++-
 drivers/tty/serial/8250/8250_pnp.c  | 12 +++++++++++
 include/linux/pnp.h                 |  1 +
 include/linux/serial_8250.h         |  2 ++
 5 files changed, 55 insertions(+), 1 deletion(-)
drivers/pnp/pnpacpi/core.c +1 -0
index a5c6cb7..84b518e 100644
@@ -254,6 +254,7 @@ static int __init pnpacpi_add_device(struct acpi_device *device)
 		return -ENOMEM;
  	dev->data = device;
+	dev->is_acpi = 1;
 	/* .enabled means the device can decode the resources */
 	dev->active = device->status.enabled;
 	if (acpi_has_method(device->handle, "_SRS"))
drivers/tty/serial/8250/8250_core.c +38 -0
index 7a91c6d..0e5ece5 100644
@@ -3221,6 +3221,41 @@ static struct uart_8250_port *serial8250_find_match_or_unused(struct uart_port *
 	return NULL;
 }
 +static struct uart_8250_port *
+serial8250_find_match_sorted_port(struct uart_port *port)
+{
+	int i, j;
+	struct uart_8250_port *uart;
+
+	uart = container_of(port, struct uart_8250_port, port);
+
+	/*
+	 * We need sorted array serial8250_ports, it is sort by ACPI
+	 * device unique id, so find an apropriate position to insert.
+	 */
+	for (i = 0; i < nr_uarts; i++) {
+		if (serial8250_ports[i].uid == 0 ||
+			uart->uid < serial8250_ports[i].uid)
+			break;
+	}
+
+	/*
+	 * If current uid that in serial8250_ports is big than port uid,
+	 * then move 8250 port data.
+	 */
+	if (i < nr_uarts - 1 &&
+		uart->uid < serial8250_ports[i].uid) {
+		memmove(&serial8250_ports[i+1], &serial8250_ports[i],
+			(nr_uarts - i - 1) * sizeof(struct uart_8250_port));
+
+		/* modify port.line, since data moved */
+		for (j = i + 1; j < nr_uarts; j++)
+			serial8250_ports[j].port.line += 1;
+	}
+	serial8250_ports[i].uid = uart->uid;
+	return &serial8250_ports[i];
+}
+
 /**
  *	serial8250_register_8250_port - register a serial port
  *	@up: serial port template
@@ -3244,7 +3279,10 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
  	mutex_lock(&serial_mutex);
 -	uart = serial8250_find_match_or_unused(&up->port);
+	if (up->is_acpi)
+		uart = serial8250_find_match_sorted_port(&up->port);
+	else
+		uart = serial8250_find_match_or_unused(&up->port);
 	if (uart && uart->port.type != PORT_8250_CIR) {
 		if (uart->port.dev)
 			uart_remove_one_port(&serial8250_reg, &uart->port);
drivers/tty/serial/8250/8250_pnp.c +11 -0
index 682a2fb..c880ed3 100644
@@ -15,6 +15,8 @@
 #include <linux/pci.h>
 #include <linux/pnp.h>
 #include <linux/string.h>
+#include <acpi/acpi.h>
+#include <acpi/acpi_bus.h>
 #include <linux/kernel.h>
 #include <linux/serial_core.h>
 #include <linux/bitops.h>
@@ -427,6 +429,8 @@ static int
 serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
 {
 	struct uart_8250_port uart;
+	struct acpi_device *acpi_dev;
+	char *serial_uid;
 	int ret, line, flags = dev_id->driver_data;
  	if (flags & UNKNOWN_DEV) {
@@ -451,6 +455,14 @@ serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
 	} else
 		return -ENODEV;
 +	if (dev->is_acpi) {
+		acpi_dev = dev->data;
+		serial_uid = acpi_dev->pnp.unique_id;
+		uart.is_acpi = 1;
+		if (kstrtoint(serial_uid, 10, &uart.uid))
+			return -EINVAL;
+	}
+
 #ifdef SERIAL_DEBUG_PNP
 	printk(KERN_DEBUG
 		"Setup PNP port: port %x, mem 0x%lx, irq %d, type %d\n",
include/linux/pnp.h +1 -0
index 195aafc..d745f14 100644
@@ -245,6 +245,7 @@ struct pnp_dev {
 	u64 dma_mask;
 	unsigned int number;		/* used as an index, must be unique */
 	int status;
+	unsigned char is_acpi;		/* is acpi device */
  	struct list_head global_list;	/* node in global list of devices */
 	struct list_head protocol_list;	/* node in list of device's protocol */
include/linux/serial_8250.h +2 -0
index af47a8a..8f1e1bd 100644
@@ -74,7 +74,9 @@ struct uart_8250_port {
 	struct list_head	list;		/* ports on this IRQ */
 	unsigned short		capabilities;	/* port capabilities */
 	unsigned short		bugs;		/* port bugs */
+	int			uid;		/* ACPI device unique id */
 	unsigned int		tx_loadsz;	/* transmit fifo load size */
+	unsigned char		is_acpi;	/* is acpi device */
 	unsigned char		acr;
 	unsigned char		ier;
 	unsigned char		lcr;
Signature · 1 line
1.9.1
#1 lore raw

Hi Scott,

Quoted 9 lines · Scott Yuan

From ebb85a61456bf25f87aeb8a31c92c74540bdf306 Mon Sep 17 00:00:00 2001
From: Scott Yuan <[email protected]>
Date: Sun, 24 Aug 2014 13:46:56 +0800
Subject: [PATCH] 8250: serial device file is out of order

On x86 architecture, the configuration of serial device maybe get from ACPI DSDT, but the order of DSDT is not mandatory, result as array serial8250_ports is out of order. This situation is more obvious in multiple serial port mainboard. Sort it by unique id that in DSDT will fix it.

Why does this matter?

Quoted 40 lines
---
 drivers/pnp/pnpacpi/core.c          |  1 +
 drivers/tty/serial/8250/8250_core.c | 40 ++++++++++++++++++++++++++++++++++++-
 drivers/tty/serial/8250/8250_pnp.c  | 12 +++++++++++
 include/linux/pnp.h                 |  1 +
 include/linux/serial_8250.h         |  2 ++
 5 files changed, 55 insertions(+), 1 deletion(-)
drivers/pnp/pnpacpi/core.c +1 -0
index a5c6cb7..84b518e 100644
@@ -254,6 +254,7 @@ static int __init pnpacpi_add_device(struct acpi_device *device)
         return -ENOMEM;
      dev->data = device;
+    dev->is_acpi = 1;
     /* .enabled means the device can decode the resources */
     dev->active = device->status.enabled;
     if (acpi_has_method(device->handle, "_SRS"))
drivers/tty/serial/8250/8250_core.c +11 -0
index 7a91c6d..0e5ece5 100644
@@ -3221,6 +3221,41 @@ static struct uart_8250_port *serial8250_find_match_or_unused(struct uart_port *
     return NULL;
 }
 +static struct uart_8250_port *
+serial8250_find_match_sorted_port(struct uart_port *port)
+{
+    int i, j;
+    struct uart_8250_port *uart;
+
+    uart = container_of(port, struct uart_8250_port, port);
+
+    /*
+     * We need sorted array serial8250_ports, it is sort by ACPI
+     * device unique id, so find an apropriate position to insert.
+     */

drivers/tty/serial/8250/8250_core.c static struct uart_8250_port *serial8250_find_match_or_unused(struct uart_port * · line 3244

Quoted 10 lines
drivers/tty/serial/8250/8250_core.c +10 -0
+    for (i = 0; i < nr_uarts; i++) {
+        if (serial8250_ports[i].uid == 0 ||
+            uart->uid < serial8250_ports[i].uid)
+            break;
+    }
+
+    /*
+     * If current uid that in serial8250_ports is big than port uid,
+     * then move 8250 port data.
+     */

The port index cannot be changed after the port is registered with the tty core. That's because the tty core associates the index at registration time with a specific tty.

Also, this is thread-unsafe: the 8250 driver uses serial_mutex to protect concurrent add/remove to the serial8250_ports but otherwise does not claim that mutex to dereference that table once a given port index has been assigned.

Quoted 77 lines
drivers/tty/serial/8250/8250_core.c +17 -0
+    if (i < nr_uarts - 1 &&
+        uart->uid < serial8250_ports[i].uid) {
+        memmove(&serial8250_ports[i+1], &serial8250_ports[i],
+            (nr_uarts - i - 1) * sizeof(struct uart_8250_port));
+
+        /* modify port.line, since data moved */
+        for (j = i + 1; j < nr_uarts; j++)
+            serial8250_ports[j].port.line += 1;
+    }
+    serial8250_ports[i].uid = uart->uid;
+    return &serial8250_ports[i];
+}
+
 /**
  *    serial8250_register_8250_port - register a serial port
  *    @up: serial port template
@@ -3244,7 +3279,10 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
      mutex_lock(&serial_mutex);
 -    uart = serial8250_find_match_or_unused(&up->port);
+    if (up->is_acpi)
+        uart = serial8250_find_match_sorted_port(&up->port);
+    else
+        uart = serial8250_find_match_or_unused(&up->port);
     if (uart && uart->port.type != PORT_8250_CIR) {
         if (uart->port.dev)
             uart_remove_one_port(&serial8250_reg, &uart->port);
drivers/tty/serial/8250/8250_pnp.c +11 -0
index 682a2fb..c880ed3 100644
@@ -15,6 +15,8 @@
 #include <linux/pci.h>
 #include <linux/pnp.h>
 #include <linux/string.h>
+#include <acpi/acpi.h>
+#include <acpi/acpi_bus.h>
 #include <linux/kernel.h>
 #include <linux/serial_core.h>
 #include <linux/bitops.h>
@@ -427,6 +429,8 @@ static int
 serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
 {
     struct uart_8250_port uart;
+    struct acpi_device *acpi_dev;
+    char *serial_uid;
     int ret, line, flags = dev_id->driver_data;
      if (flags & UNKNOWN_DEV) {
@@ -451,6 +455,14 @@ serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
     } else
         return -ENODEV;
 +    if (dev->is_acpi) {
+        acpi_dev = dev->data;
+        serial_uid = acpi_dev->pnp.unique_id;
+        uart.is_acpi = 1;
+        if (kstrtoint(serial_uid, 10, &uart.uid))
+            return -EINVAL;
+    }
+
 #ifdef SERIAL_DEBUG_PNP
     printk(KERN_DEBUG
         "Setup PNP port: port %x, mem 0x%lx, irq %d, type %d\n",
include/linux/pnp.h +1 -0
index 195aafc..d745f14 100644
@@ -245,6 +245,7 @@ struct pnp_dev {
     u64 dma_mask;
     unsigned int number;        /* used as an index, must be unique */
     int status;
+    unsigned char is_acpi;        /* is acpi device */
      struct list_head global_list;    /* node in global list of devices */
     struct list_head protocol_list;    /* node in list of device's protocol */
include/linux/serial_8250.h +0 -0
index af47a8a..8f1e1bd 100644

include/linux/serial_8250.h struct uart_8250_port { · line 82

Quoted 9 lines
include/linux/serial_8250.h +2 -0
@@ -74,7 +74,9 @@ struct uart_8250_port {
     struct list_head    list;        /* ports on this IRQ */
     unsigned short        capabilities;    /* port capabilities */
     unsigned short        bugs;        /* port bugs */
+    int            uid;        /* ACPI device unique id */
     unsigned int        tx_loadsz;    /* transmit fifo load size */
+    unsigned char        is_acpi;    /* is acpi device */
     unsigned char        acr;
     unsigned char        ier;
     unsigned char        lcr;

Regards,
Peter Hurley

maintainer
#2 lore raw
In reply to Scott Yuan
Quoted 4 lines · Scott Yuan

From ebb85a61456bf25f87aeb8a31c92c74540bdf306 Mon Sep 17 00:00:00 2001
From: Scott Yuan <[email protected]>
Date: Sun, 24 Aug 2014 13:46:56 +0800
Subject: [PATCH] 8250: serial device file is out of order

Why is this here in the email? Please use git send-email to not send this.

Quoted 21 lines

On x86 architecture, the configuration of serial device maybe get from ACPI DSDT, but the order of DSDT is not mandatory, result as array serial8250_ports
is out of order. This situation is more obvious in multiple serial port mainboard. Sort it by unique id that in DSDT will fix it.

---
 drivers/pnp/pnpacpi/core.c          |  1 +
 drivers/tty/serial/8250/8250_core.c | 40
++++++++++++++++++++++++++++++++++++-
 drivers/tty/serial/8250/8250_pnp.c  | 12 +++++++++++
 include/linux/pnp.h                 |  1 +
 include/linux/serial_8250.h         |  2 ++
 5 files changed, 55 insertions(+), 1 deletion(-)
drivers/pnp/pnpacpi/core.c +0 -0
index a5c6cb7..84b518e 100644

drivers/pnp/pnpacpi/core.c static int __init pnpacpi_add_device(struct acpi_device · line 254

Quoted 1 line
drivers/pnp/pnpacpi/core.c +0 -0
@@ -254,6 +254,7 @@ static int __init pnpacpi_add_device(struct acpi_device
 *device)

Your patch is line-wrapped and can not be applied :(