[PATCH 0/2] Move uart_register_driver call to device probe for pl010 and sh-sci

[PATCH 0/2] Move uart_register_driver call to device probe for pl010 and sh-sci

[Sjoerd Simons]

When testing on a Renesas board with the PL010 serial driver enabled
serial output broke. Turns out the minor device numbers for both
drivers happen to overlap, causing whichever driver happened to be the
second one to register to fail.

The attached patches move the uart_register_driver call to probe time
for both drivers avoiding the issue.


Sjoerd Simons (2):
  serial: pl010: Move uart_register_driver call to device probe
  serial: sh-sci: Move uart_register_driver call to device probe

 drivers/tty/serial/amba-pl010.c | 27 +++++++++++++++++----------
 drivers/tty/serial/sh-sci.c     | 21 ++++++++++-----------
 2 files changed, 27 insertions(+), 21 deletions(-)

-- 
2.11.0

[PATCH 1/2] serial: pl010: Move uart_register_driver call to device probe

[Sjoerd Simons]

uart_register_driver call binds the driver to a specific device
node through tty_register_driver call. This should typically
happen during device probe call.

In a multiplatform scenario, it is possible that multiple serial
drivers are part of the kernel. Currently the driver registration fails
if multiple serial drivers with overlapping major/minor numbers are
included.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
---

 drivers/tty/serial/amba-pl010.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/amba-pl010.c b/drivers/tty/serial/amba-pl010.c
index f2f251075109..74a1d3b2e45d 100644
--- a/drivers/tty/serial/amba-pl010.c
+++ b/drivers/tty/serial/amba-pl010.c
@@ -749,6 +749,16 @@ static int pl010_probe(struct amba_device *dev, const struct amba_id *id)
 	amba_ports[i] = uap;
 
 	amba_set_drvdata(dev, uap);
+
+	if (!amba_reg.state) {
+		ret = uart_register_driver(&amba_reg);
+		if (ret < 0) {
+			dev_err(uap->port.dev,
+				"Failed to register AMBA-PL010 driver\n");
+			return ret;
+		}
+	}
+
 	ret = uart_add_one_port(&amba_reg, &uap->port);
 	if (ret)
 		amba_ports[i] = NULL;
@@ -760,12 +770,18 @@ static int pl010_remove(struct amba_device *dev)
 {
 	struct uart_amba_port *uap = amba_get_drvdata(dev);
 	int i;
+	bool busy = false;
 
 	uart_remove_one_port(&amba_reg, &uap->port);
 
 	for (i = 0; i < ARRAY_SIZE(amba_ports); i++)
 		if (amba_ports[i] == uap)
 			amba_ports[i] = NULL;
+		else if (amba_ports[i])
+			busy = true;
+
+	if (!busy)
+		uart_unregister_driver(&amba_reg);
 
 	return 0;
 }
@@ -816,23 +832,14 @@ static struct amba_driver pl010_driver = {
 
 static int __init pl010_init(void)
 {
-	int ret;
-
 	printk(KERN_INFO "Serial: AMBA driver\n");
 
-	ret = uart_register_driver(&amba_reg);
-	if (ret == 0) {
-		ret = amba_driver_register(&pl010_driver);
-		if (ret)
-			uart_unregister_driver(&amba_reg);
-	}
-	return ret;
+	return  amba_driver_register(&pl010_driver);
 }
 
 static void __exit pl010_exit(void)
 {
 	amba_driver_unregister(&pl010_driver);
-	uart_unregister_driver(&amba_reg);
 }
 
 module_init(pl010_init);
-- 
2.11.0

Re: [PATCH 1/2] serial: pl010: Move uart_register_driver call to device probe

[Russell King - ARM Linux]

On Fri, Mar 24, 2017 at 05:26:33PM +0100, Sjoerd Simons wrote:
> uart_register_driver call binds the driver to a specific device
> node through tty_register_driver call. This should typically
> happen during device probe call.
> 
> In a multiplatform scenario, it is possible that multiple serial
> drivers are part of the kernel. Currently the driver registration fails
> if multiple serial drivers with overlapping major/minor numbers are
> included.

This is racy.  The driver model locking is on a _per_ device basis,
it is not on a per driver basis.

It is entirely possible for a driver to be probed by two devices at
once, possibly on two different CPUs.  Checking the state of the
UART driver structure and then registering it means there is a window
where we could end up with two CPUs registering the same UART driver
at the same time, which would cause things to go wrong.

Therefore, this needs some form of locking.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH 2/2] serial: sh-sci: Move uart_register_driver call to device probe

[Sjoerd Simons]

uart_register_driver call binds the driver to a specific device
node through tty_register_driver call. This should typically
happen during device probe call.

In a multiplatform scenario, it is possible that multiple serial
drivers are part of the kernel. Currently the driver registration fails
if multiple serial drivers with overlapping major/minor numbers are
included.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>

---

 drivers/tty/serial/sh-sci.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 9a47cc4f16a2..7c1c0c08459e 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -3063,6 +3063,12 @@ static int sci_probe_single(struct platform_device *dev,
 		return -EINVAL;
 	}
 
+	if (!sci_uart_driver.state) {
+		ret = uart_register_driver(&sci_uart_driver);
+		if (ret)
+			return ret;
+	}
+
 	ret = sci_init_single(dev, sciport, index, p, false);
 	if (ret)
 		return ret;
@@ -3186,24 +3192,17 @@ static struct platform_driver sci_driver = {
 
 static int __init sci_init(void)
 {
-	int ret;
-
 	pr_info("%s\n", banner);
 
-	ret = uart_register_driver(&sci_uart_driver);
-	if (likely(ret == 0)) {
-		ret = platform_driver_register(&sci_driver);
-		if (unlikely(ret))
-			uart_unregister_driver(&sci_uart_driver);
-	}
-
-	return ret;
+	return platform_driver_register(&sci_driver);
 }
 
 static void __exit sci_exit(void)
 {
 	platform_driver_unregister(&sci_driver);
-	uart_unregister_driver(&sci_uart_driver);
+
+	if (sci_uart_driver.state)
+		uart_unregister_driver(&sci_uart_driver);
 }
 
 #ifdef CONFIG_SERIAL_SH_SCI_CONSOLE
-- 
2.11.0

Re: [PATCH 0/2] Move uart_register_driver call to device probe for pl010 and sh-sci

[Russell King - ARM Linux]

On Fri, Mar 24, 2017 at 05:26:32PM +0100, Sjoerd Simons wrote:
> When testing on a Renesas board with the PL010 serial driver enabled
> serial output broke. Turns out the minor device numbers for both
> drivers happen to overlap, causing whichever driver happened to be the
> second one to register to fail.

How the **** has the SH serial driver ended up with overlapping device
numbers?

What happened to our maintained list of allocated major/minor device
numbers, which is supposed to stop crap like this happening?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 0/2] Move uart_register_driver call to device probe for pl010 and sh-sci

[Geert Uytterhoeven]

Hi Russell, Sjoerd,

On Fri, Mar 24, 2017 at 5:42 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Fri, Mar 24, 2017 at 05:26:32PM +0100, Sjoerd Simons wrote:
>> When testing on a Renesas board with the PL010 serial driver enabled
>> serial output broke. Turns out the minor device numbers for both
>> drivers happen to overlap, causing whichever driver happened to be the
>> second one to register to fail.
>
> How the **** has the SH serial driver ended up with overlapping device
> numbers?

Interesting...

> What happened to our maintained list of allocated major/minor device
> numbers, which is supposed to stop crap like this happening?

AMBA PL010 has been assigned major 204, minors 16..31,
SCI has been assigned major 204, minors 8..11.

Over the years, Renesas SoCs have been gaining more and more serial
ports, leading to

    #define SCI_NPORTS CONFIG_SERIAL_SH_SCI_NR_UARTS

with CONFIG_SERIAL_SH_SCI_NR_UARTS=20 in shmobile_defconfig and
multi_v7_defconfig (although the maximum value on any supported SoC is 18).

But once the value becomes 5 or more, it starts overflowing into the ttyFWx
and ttyAMx space.

How to solve this?
Time for the serial subsystem to switch to dynamic minors, and get rid of the
what-is-your-serial-port-called-again-on-this-platform
multi-million-euro question?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/2] Move uart_register_driver call to device probe for pl010 and sh-sci

[Russell King - ARM Linux]

On Sun, Mar 26, 2017 at 11:22:57AM +0200, Geert Uytterhoeven wrote:
> Time for the serial subsystem to switch to dynamic minors, and get rid of the
> what-is-your-serial-port-called-again-on-this-platform
> multi-million-euro question?

Dynamic device numbers are fine for those who use devtmpfs / udev, but I
would imagine that there are systems out there which don't, and if they
change, systems become inaccessible (the port becomes dead, even to
sysrq.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 0/2] Move uart_register_driver call to device probe for pl010 and sh-sci

[Greg Kroah-Hartman]

On Sun, Mar 26, 2017 at 11:22:57AM +0200, Geert Uytterhoeven wrote:
> Hi Russell, Sjoerd,
> 
> On Fri, Mar 24, 2017 at 5:42 PM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > On Fri, Mar 24, 2017 at 05:26:32PM +0100, Sjoerd Simons wrote:
> >> When testing on a Renesas board with the PL010 serial driver enabled
> >> serial output broke. Turns out the minor device numbers for both
> >> drivers happen to overlap, causing whichever driver happened to be the
> >> second one to register to fail.
> >
> > How the **** has the SH serial driver ended up with overlapping device
> > numbers?
> 
> Interesting...
> 
> > What happened to our maintained list of allocated major/minor device
> > numbers, which is supposed to stop crap like this happening?
> 
> AMBA PL010 has been assigned major 204, minors 16..31,
> SCI has been assigned major 204, minors 8..11.
> 
> Over the years, Renesas SoCs have been gaining more and more serial
> ports, leading to
> 
>     #define SCI_NPORTS CONFIG_SERIAL_SH_SCI_NR_UARTS
> 
> with CONFIG_SERIAL_SH_SCI_NR_UARTS=20 in shmobile_defconfig and
> multi_v7_defconfig (although the maximum value on any supported SoC is 18).
> 
> But once the value becomes 5 or more, it starts overflowing into the ttyFWx
> and ttyAMx space.
> 
> How to solve this?
> Time for the serial subsystem to switch to dynamic minors, and get rid of the
> what-is-your-serial-port-called-again-on-this-platform
> multi-million-euro question?

Yes, please, we need to do that.  Let's provide a build option for it,
like USB has had for over a decade.  If it's enabled, it's all dynamic,
if not, the "old style" ones are used.  Then we don't accept any new
reservations, making new drivers use the dynamic number, and over a long
time, all should be good.

thanks,

greg k-h