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

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

[Tushar Behera]

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 same default major/minor numbers are
included in the kernel. A typical case is observed with amba-pl011 and
samsung-uart drivers.

This is the 3rd iteration to fix above problem.

Iteration 1:
[PATCH] serial: samsung: Remove hard-coded major/minor numbers
https://lkml.org/lkml/2013/12/27/2

Iteration 2:
[PATCH] tty: Fallback to use dynamic major number
https://lkml.org/lkml/2014/1/16/2

Both these approaches were rejected because they were breaking userspace
interface.

Iteration 3:
uart_register_driver call binds the driver to a specific device
node through tty_driver_register call. This is moved to device probe call.

Tushar Behera (2):
  serial: samsung: Move uart_register_driver call to device probe
  serial: pl011: Move uart_register_driver call to device probe

 drivers/tty/serial/amba-pl011.c |   21 +++++++++++---------
 drivers/tty/serial/samsung.c    |   40 +++++++++++----------------------------
 2 files changed, 23 insertions(+), 38 deletions(-)

-- 
1.7.9.5

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

[Tushar Behera]

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 same default major/minor numbers are
included in the kernel.

A typical case is observed with amba-pl011 and samsung-uart drivers.

Signed-off-by: Tushar Behera <tushar.behera@linaro.org>
---
 drivers/tty/serial/samsung.c |   40 +++++++++++-----------------------------
 1 file changed, 11 insertions(+), 29 deletions(-)

diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
index c1af04d..f8f41dc 100644
--- a/drivers/tty/serial/samsung.c
+++ b/drivers/tty/serial/samsung.c
@@ -1283,6 +1283,14 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto probe_err;
 
+	if (!s3c24xx_uart_drv.state) {
+		ret = uart_register_driver(&s3c24xx_uart_drv);
+		if (ret < 0) {
+			pr_err("Failed to register Samsung UART driver\n");
+			return ret;
+		}
+	}
+
 	dbg("%s: adding port\n", __func__);
 	uart_add_one_port(&s3c24xx_uart_drv, &ourport->port);
 	platform_set_drvdata(pdev, &ourport->port);
@@ -1315,6 +1323,8 @@ static int s3c24xx_serial_remove(struct platform_device *dev)
 		uart_remove_one_port(&s3c24xx_uart_drv, port);
 	}
 
+	uart_unregister_driver(&s3c24xx_uart_drv);
+
 	return 0;
 }
 
@@ -1814,35 +1824,7 @@ static struct platform_driver samsung_serial_driver = {
 	},
 };
 
-/* module initialisation code */
-
-static int __init s3c24xx_serial_modinit(void)
-{
-	int ret;
-
-	ret = uart_register_driver(&s3c24xx_uart_drv);
-	if (ret < 0) {
-		pr_err("Failed to register Samsung UART driver\n");
-		return ret;
-	}
-
-	ret = platform_driver_register(&samsung_serial_driver);
-	if (ret < 0) {
-		pr_err("Failed to register platform driver\n");
-		uart_unregister_driver(&s3c24xx_uart_drv);
-	}
-
-	return ret;
-}
-
-static void __exit s3c24xx_serial_modexit(void)
-{
-	platform_driver_unregister(&samsung_serial_driver);
-	uart_unregister_driver(&s3c24xx_uart_drv);
-}
-
-module_init(s3c24xx_serial_modinit);
-module_exit(s3c24xx_serial_modexit);
+module_platform_driver(samsung_serial_driver);
 
 MODULE_ALIAS("platform:samsung-uart");
 MODULE_DESCRIPTION("Samsung SoC Serial port driver");
-- 
1.7.9.5

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

[Tushar Behera]

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 same default major/minor numbers are
included in the kernel.

A typical case is observed with amba-pl011 and samsung-uart drivers.

Signed-off-by: Tushar Behera <tushar.behera@linaro.org>
---
 drivers/tty/serial/amba-pl011.c |   21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index d58783d..d4eda24 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2154,9 +2154,19 @@ static int pl011_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) {
+			pr_err("Failed to register AMBA-PL011 driver\n");
+			return ret;
+		}
+	}
+
 	ret = uart_add_one_port(&amba_reg, &uap->port);
 	if (ret) {
 		amba_ports[i] = NULL;
+		uart_unregister_driver(&amba_reg);
 		pl011_dma_remove(uap);
 	}
  out:
@@ -2175,6 +2185,7 @@ static int pl011_remove(struct amba_device *dev)
 			amba_ports[i] = NULL;
 
 	pl011_dma_remove(uap);
+	uart_unregister_driver(&amba_reg);
 	return 0;
 }
 
@@ -2230,22 +2241,14 @@ static struct amba_driver pl011_driver = {
 
 static int __init pl011_init(void)
 {
-	int ret;
 	printk(KERN_INFO "Serial: AMBA PL011 UART driver\n");
 
-	ret = uart_register_driver(&amba_reg);
-	if (ret == 0) {
-		ret = amba_driver_register(&pl011_driver);
-		if (ret)
-			uart_unregister_driver(&amba_reg);
-	}
-	return ret;
+	return amba_driver_register(&pl011_driver);
 }
 
 static void __exit pl011_exit(void)
 {
 	amba_driver_unregister(&pl011_driver);
-	uart_unregister_driver(&amba_reg);
 }
 
 /*
-- 
1.7.9.5

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

[Russell King - ARM Linux]

On Mon, Jan 20, 2014 at 02:32:35PM +0530, Tushar Behera 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 same default major/minor numbers are
> included in the kernel.
> 
> A typical case is observed with amba-pl011 and samsung-uart drivers.

NAK.  There should not be any other driver using amba-pl011's device numbers.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[Russell King - ARM Linux]

On Mon, Jan 20, 2014 at 02:32:34PM +0530, Tushar Behera 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 same default major/minor numbers are
> included in the kernel.
> 
> A typical case is observed with amba-pl011 and samsung-uart drivers.

The samsung-uart driver is at fault here - the major/minor numbers were
officially registered to amba-pl011.  Samsung needs to be fixed properly.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[Tushar Behera]

On 20 January 2014 15:35, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jan 20, 2014 at 02:32:34PM +0530, Tushar Behera 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 same default major/minor numbers are
>> included in the kernel.
>>
>> A typical case is observed with amba-pl011 and samsung-uart drivers.
>
> The samsung-uart driver is at fault here - the major/minor numbers were
> officially registered to amba-pl011.  Samsung needs to be fixed properly.
>

I had earlier submitted a patch [1] to remove the hard coded
major/minor number for Samsung UART driver, but that was rejected
because of userspace breakage. Without this patch, Samsung UART driver
can't bind to the hard-coded device node. Changing the default
major/minor will also not help to fix the objections raised in [1].

Would you please suggest a way forward?

[1] https://lkml.org/lkml/2013/12/27/2

-- 
Tushar Behera

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

[Russell King - ARM Linux]

On Mon, Jan 20, 2014 at 05:23:21PM +0530, Tushar Behera wrote:
> On 20 January 2014 15:35, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, Jan 20, 2014 at 02:32:34PM +0530, Tushar Behera 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 same default major/minor numbers are
> >> included in the kernel.
> >>
> >> A typical case is observed with amba-pl011 and samsung-uart drivers.
> >
> > The samsung-uart driver is at fault here - the major/minor numbers were
> > officially registered to amba-pl011.  Samsung needs to be fixed properly.
> >
> 
> I had earlier submitted a patch [1] to remove the hard coded
> major/minor number for Samsung UART driver, but that was rejected
> because of userspace breakage. Without this patch, Samsung UART driver
> can't bind to the hard-coded device node. Changing the default
> major/minor will also not help to fix the objections raised in [1].
> 
> Would you please suggest a way forward?

I still assert that it's for Samsung people to sort out their abuse of
the major/minor numbers - we have a procedure in place to allocate these,
I followed it for the AMBA PL011 driver, they didn't.  Their problem -
and they have to suffer with the consequences of that bad decision on
their part, which is that they have to deal with the inevitable breakage
caused by having to renumber.

> [1] https://lkml.org/lkml/2013/12/27/2

lkml.org is dead at the moment - tried from two different ISPs in the UK,
it's nameservers are unreachable, and its https port refuses connections.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[Greg KH]

On Mon, Jan 20, 2014 at 10:05:30AM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 20, 2014 at 02:32:34PM +0530, Tushar Behera 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 same default major/minor numbers are
> > included in the kernel.
> > 
> > A typical case is observed with amba-pl011 and samsung-uart drivers.
> 
> The samsung-uart driver is at fault here - the major/minor numbers were
> officially registered to amba-pl011.  Samsung needs to be fixed properly.

I agree, the Samsung driver is "broken" here, but that's no reason why
these two drivers can't register with the tty layer _after_ the hardware
is detected, and not before.

That saves resources on systems that build the drivers in, yet do not
have the hardware present, which is always a good thing.

thanks,

greg k-h

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

[Russell King - ARM Linux]

On Mon, Jan 20, 2014 at 01:16:01PM -0800, Greg KH wrote:
> On Mon, Jan 20, 2014 at 10:05:30AM +0000, Russell King - ARM Linux wrote:
> > On Mon, Jan 20, 2014 at 02:32:34PM +0530, Tushar Behera 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 same default major/minor numbers are
> > > included in the kernel.
> > > 
> > > A typical case is observed with amba-pl011 and samsung-uart drivers.
> > 
> > The samsung-uart driver is at fault here - the major/minor numbers were
> > officially registered to amba-pl011.  Samsung needs to be fixed properly.
> 
> I agree, the Samsung driver is "broken" here, but that's no reason why
> these two drivers can't register with the tty layer _after_ the hardware
> is detected, and not before.
> 
> That saves resources on systems that build the drivers in, yet do not
> have the hardware present, which is always a good thing.

Great, so what you're saying is that we need to wait until the first
device calls into the probe function.  What about removal... how does
a driver know when it's last device has been removed to de-register
that?

I guess it needs the driver model to provide some way to know when a
driver is completely unbound - but isn't that racy?

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[Alan Cox]

> I had earlier submitted a patch [1] to remove the hard coded
> major/minor number for Samsung UART driver, but that was rejected
> because of userspace breakage. Without this patch, Samsung UART driver
> can't bind to the hard-coded device node. Changing the default
> major/minor will also not help to fix the objections raised in [1].
> 
> Would you please suggest a way forward?
> 
> [1] https://lkml.org/lkml/2013/12/27/2

The dynamic major/minor is the right patch. If the userspace breaks then
the userspace was broken, but I see no evidence in the discussion that
the userspace broke.

204,64 belongs to Altix so neither of the clashing drivers should use it.
We had a table to stop messes like this, and we have dynamic numbering
to stop the table being needed for the future

           50 = /dev/ttyIOC0              Altix serial card
                    ...
           81 = /dev/ttyIOC31             Altix serial card


Thats what the list says. Samsung should have followed the rules, they
didn't so they get to pick up the pieces. The Amba driver wants moving as
well. It's easy. If you want something to be ABI then make sure you get
it upstream first, if not you get to own all the pain down the line.

Hacking up the core code to paper over this crap is not on. We've been
insisting firmly and robustly for years that people don't keep borrowing
names and numbers they are not allocated, and use dynamic values whenever
possible.

If the hardware isn't present then the driver shouldn't even register
with the tty layer in the first place so it doesn't make any resource
differeneces either for properly written code.

Alan

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

[Greg KH]

On Mon, Jan 20, 2014 at 09:32:06PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 20, 2014 at 01:16:01PM -0800, Greg KH wrote:
> > On Mon, Jan 20, 2014 at 10:05:30AM +0000, Russell King - ARM Linux wrote:
> > > On Mon, Jan 20, 2014 at 02:32:34PM +0530, Tushar Behera 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 same default major/minor numbers are
> > > > included in the kernel.
> > > > 
> > > > A typical case is observed with amba-pl011 and samsung-uart drivers.
> > > 
> > > The samsung-uart driver is at fault here - the major/minor numbers were
> > > officially registered to amba-pl011.  Samsung needs to be fixed properly.
> > 
> > I agree, the Samsung driver is "broken" here, but that's no reason why
> > these two drivers can't register with the tty layer _after_ the hardware
> > is detected, and not before.
> > 
> > That saves resources on systems that build the drivers in, yet do not
> > have the hardware present, which is always a good thing.
> 
> Great, so what you're saying is that we need to wait until the first
> device calls into the probe function.  What about removal... how does
> a driver know when it's last device has been removed to de-register
> that?

The "bus" that the device is on handles that, right?

> I guess it needs the driver model to provide some way to know when a
> driver is completely unbound - but isn't that racy?

How is it racy?  That's how the driver model works...

thanks,

greg k-h

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

[Mark Brown]

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

On Mon, Jan 20, 2014 at 09:43:05PM +0000, Alan Cox wrote:

> The dynamic major/minor is the right patch. If the userspace breaks then
> the userspace was broken, but I see no evidence in the discussion that
> the userspace broke.

The userspace breakage is that if someone has a static /dev that doesn't
handle any dynamic devices then renumbering the device will cause that
static /dev to stop matching the kernel.

> Thats what the list says. Samsung should have followed the rules, they
> didn't so they get to pick up the pieces. The Amba driver wants moving as
> well. It's easy. If you want something to be ABI then make sure you get
> it upstream first, if not you get to own all the pain down the line.

This stuff is all upstream already, a quick check suggests both drivers
predate git - it's been noticed because the ARM multiplatform work has
caused people to try booting kernels with both built in.

> If the hardware isn't present then the driver shouldn't even register
> with the tty layer in the first place so it doesn't make any resource
> differeneces either for properly written code.

Right, that's not the idiom that has been followed by any of serial
drivers though so needs fixing too.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Russell King - ARM Linux]

On Mon, Jan 20, 2014 at 03:11:41PM -0800, Greg KH wrote:
> On Mon, Jan 20, 2014 at 09:32:06PM +0000, Russell King - ARM Linux wrote:
> > On Mon, Jan 20, 2014 at 01:16:01PM -0800, Greg KH wrote:
> > > On Mon, Jan 20, 2014 at 10:05:30AM +0000, Russell King - ARM Linux wrote:
> > > > On Mon, Jan 20, 2014 at 02:32:34PM +0530, Tushar Behera 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 same default major/minor numbers are
> > > > > included in the kernel.
> > > > > 
> > > > > A typical case is observed with amba-pl011 and samsung-uart drivers.
> > > > 
> > > > The samsung-uart driver is at fault here - the major/minor numbers were
> > > > officially registered to amba-pl011.  Samsung needs to be fixed properly.
> > > 
> > > I agree, the Samsung driver is "broken" here, but that's no reason why
> > > these two drivers can't register with the tty layer _after_ the hardware
> > > is detected, and not before.
> > > 
> > > That saves resources on systems that build the drivers in, yet do not
> > > have the hardware present, which is always a good thing.
> > 
> > Great, so what you're saying is that we need to wait until the first
> > device calls into the probe function.  What about removal... how does
> > a driver know when it's last device has been removed to de-register
> > that?
> 
> The "bus" that the device is on handles that, right?
> 
> > I guess it needs the driver model to provide some way to know when a
> > driver is completely unbound - but isn't that racy?
> 
> How is it racy?  That's how the driver model works...

Think about what happens when the last device unregisters, but a new
device comes along and is probed.

I don't believe the driver model has any locking to prevent a drivers
->probe function running concurrently with it's ->remove function for
two (or more) devices.

The locking against this is done on a per-device basis, not a per-driver
basis.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[Russell King - ARM Linux]

On Mon, Jan 20, 2014 at 11:14:57PM +0000, Mark Brown wrote:
> On Mon, Jan 20, 2014 at 09:43:05PM +0000, Alan Cox wrote:
> > If the hardware isn't present then the driver shouldn't even register
> > with the tty layer in the first place so it doesn't make any resource
> > differeneces either for properly written code.
> 
> Right, that's not the idiom that has been followed by any of serial
> drivers though so needs fixing too.

It's not followed by serial drivers because it gets f*scking complicated
to do it that way.

In order to do it that way, what we need to do is:

1. On the first device probe, register the UART driver.
2. On subsequent device probes, don't register the UART driver because
   it's already registered.
3. When devices are removed, do nothing until the last device.
4. When the last device is removed, unregister the UART driver.

The first bit is easy... but we need to add locks to every serial
driver to prevent two probes operating concurrently...

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[Alan Cox]

> The first bit is easy... but we need to add locks to every serial
> driver to prevent two probes operating concurrently...

The bus probe should already be serializing surely ?

 

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

[Alan Cox]

On Mon, 20 Jan 2014 23:14:57 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Mon, Jan 20, 2014 at 09:43:05PM +0000, Alan Cox wrote:
> 
> > The dynamic major/minor is the right patch. If the userspace breaks then
> > the userspace was broken, but I see no evidence in the discussion that
> > the userspace broke.
> 
> The userspace breakage is that if someone has a static /dev that doesn't
> handle any dynamic devices then renumbering the device will cause that
> static /dev to stop matching the kernel.

Diddums and you've only provided theoretical cases not real world ones.

They should have followed proper practice and reserved their minors. The
device number belongs to the Altix. The driver should just move.

> > Thats what the list says. Samsung should have followed the rules, they
> > didn't so they get to pick up the pieces. The Amba driver wants moving as
> > well. It's easy. If you want something to be ABI then make sure you get
> > it upstream first, if not you get to own all the pain down the line.
> 
> This stuff is all upstream already, a quick check suggests both drivers
> predate git - it's been noticed because the ARM multiplatform work has
> caused people to try booting kernels with both built in.

So ARM people didn't follow the policy on allocating device minors even
within their own community and got burned by it. That's despite having
previously been burned by abusing the ttyS0 8250 major/minor in the same
way, for the same purposes, and creating the same mess.

{facepalm...}

> > If the hardware isn't present then the driver shouldn't even register
> > with the tty layer in the first place so it doesn't make any resource
> > differeneces either for properly written code.
> 
> Right, that's not the idiom that has been followed by any of serial
> drivers though so needs fixing too.

Actally some drivers do get this right but not many. ehv-bc for example
does.

But yes I agree about the idiom, but a definite NAK to any attempts to
plaster over this grand screwup by crapping in the tty core. Your turd,
deal with it locally in the ARM code if you can't apply common sense and
just go dynamic.

And please, after screwing this up twice - *learn* from the mess.

Alan

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

[Greg KH]

On Mon, Jan 20, 2014 at 11:16:03PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 20, 2014 at 03:11:41PM -0800, Greg KH wrote:
> > On Mon, Jan 20, 2014 at 09:32:06PM +0000, Russell King - ARM Linux wrote:
> > > On Mon, Jan 20, 2014 at 01:16:01PM -0800, Greg KH wrote:
> > > > On Mon, Jan 20, 2014 at 10:05:30AM +0000, Russell King - ARM Linux wrote:
> > > > > On Mon, Jan 20, 2014 at 02:32:34PM +0530, Tushar Behera 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 same default major/minor numbers are
> > > > > > included in the kernel.
> > > > > > 
> > > > > > A typical case is observed with amba-pl011 and samsung-uart drivers.
> > > > > 
> > > > > The samsung-uart driver is at fault here - the major/minor numbers were
> > > > > officially registered to amba-pl011.  Samsung needs to be fixed properly.
> > > > 
> > > > I agree, the Samsung driver is "broken" here, but that's no reason why
> > > > these two drivers can't register with the tty layer _after_ the hardware
> > > > is detected, and not before.
> > > > 
> > > > That saves resources on systems that build the drivers in, yet do not
> > > > have the hardware present, which is always a good thing.
> > > 
> > > Great, so what you're saying is that we need to wait until the first
> > > device calls into the probe function.  What about removal... how does
> > > a driver know when it's last device has been removed to de-register
> > > that?
> > 
> > The "bus" that the device is on handles that, right?
> > 
> > > I guess it needs the driver model to provide some way to know when a
> > > driver is completely unbound - but isn't that racy?
> > 
> > How is it racy?  That's how the driver model works...
> 
> Think about what happens when the last device unregisters, but a new
> device comes along and is probed.
> 
> I don't believe the driver model has any locking to prevent a drivers
> ->probe function running concurrently with it's ->remove function for
> two (or more) devices.

The bus prevents this from happening.

> The locking against this is done on a per-device basis, not a per-driver
> basis.

No, on a per-bus basis.

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Mon, Jan 20, 2014 at 11:35:41PM +0000, Alan Cox wrote:
> > The first bit is easy... but we need to add locks to every serial
> > driver to prevent two probes operating concurrently...
> 
> The bus probe should already be serializing surely ?

Yes, it better be, otherwise that bus is badly broken.

greg k-h

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

[Russell King - ARM Linux]

On Mon, Jan 20, 2014 at 03:51:28PM -0800, Greg KH wrote:
> On Mon, Jan 20, 2014 at 11:16:03PM +0000, Russell King - ARM Linux wrote:
> > I don't believe the driver model has any locking to prevent a drivers
> > ->probe function running concurrently with it's ->remove function for
> > two (or more) devices.
> 
> The bus prevents this from happening.
> 
> > The locking against this is done on a per-device basis, not a per-driver
> > basis.
> 
> No, on a per-bus basis.

I don't see it.

Let's start from driver_register().
This takes no locks and calls bus_add_driver().
This also takes no locks and calls driver_attach().
This walks the list of devices calling __driver_attach() for each.
__driver_attach() tries to match the device against the driver,
locks the parent device if one exists, and the device which is about
to be probed.  It then calls driver_probe_device().
driver_probe_device() inserts a runtime barrier and calls really_probe().
really_probe() ultimately calls either the bus ->probe method or the
driver ->probe method.

At no point in that sequence do I see anything which does any locking
on a per-driver basis.  Let's look at device_add().

device_add() calls bus_probe_device(), which then calls device_attach().
device_attach() takes the device's lock, and walks the list of drivers
calling __device_attach() on each driver.  This then calls down into
driver_probe_device(), and the path is the same as the above.

I don't see any per-driver locking here either.

I've checked the klist stuff, don't see anything there.  Ditto for
bus_for_each_drv().

If you think there's a per-driver lock that's held over probes or removes,
please point it out.  I'm fairly certain that there isn't, because we have
to be able to deal with recursive probes (yes, we've had to deal with
those in the past.)

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[Russell King - ARM Linux]

On Mon, Jan 20, 2014 at 11:47:34PM +0000, Alan Cox wrote:
> But yes I agree about the idiom, but a definite NAK to any attempts to
> plaster over this grand screwup by crapping in the tty core. Your turd,
> deal with it locally in the ARM code if you can't apply common sense and
> just go dynamic.

I believe at the time there was no one maintaining the device list to
_do_ that allocation - AMBA PL011 came along in 2005 after (I believe)
hpa stopped looking after that list.

So, please tell me how a number could be allocated properly without the
device numbers list being maintained?

I've no problem with going dynamic, and I suggest that you get a sense
of perspective rather than just spouting rubbish from on high.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[Greg KH]

On Tue, Jan 21, 2014 at 12:07:06AM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 20, 2014 at 03:51:28PM -0800, Greg KH wrote:
> > On Mon, Jan 20, 2014 at 11:16:03PM +0000, Russell King - ARM Linux wrote:
> > > I don't believe the driver model has any locking to prevent a drivers
> > > ->probe function running concurrently with it's ->remove function for
> > > two (or more) devices.
> > 
> > The bus prevents this from happening.
> > 
> > > The locking against this is done on a per-device basis, not a per-driver
> > > basis.
> > 
> > No, on a per-bus basis.
> 
> I don't see it.
> 
> Let's start from driver_register().

Which happens from module probing, which is single-threaded, right?

Or from module_init callbacks, which is single-threaded.

Normally, busses never add devices (which is what drivers bind to),
except in a single-at-a-time fashion, unless they really know what they
are doing (i.e. PCI had multi-threaded device probing for a while, don't
remember if it still does...)


> This takes no locks and calls bus_add_driver().
> This also takes no locks and calls driver_attach().
> This walks the list of devices calling __driver_attach() for each.
> __driver_attach() tries to match the device against the driver,
> locks the parent device if one exists, and the device which is about
> to be probed.  It then calls driver_probe_device().
> driver_probe_device() inserts a runtime barrier and calls really_probe().
> really_probe() ultimately calls either the bus ->probe method or the
> driver ->probe method.
> 
> At no point in that sequence do I see anything which does any locking
> on a per-driver basis.  Let's look at device_add().
> 
> device_add() calls bus_probe_device(), which then calls device_attach().
> device_attach() takes the device's lock, and walks the list of drivers
> calling __device_attach() on each driver.  This then calls down into
> driver_probe_device(), and the path is the same as the above.
> 
> I don't see any per-driver locking here either.
> 
> I've checked the klist stuff, don't see anything there.  Ditto for
> bus_for_each_drv().
> 
> If you think there's a per-driver lock that's held over probes or removes,
> please point it out.  I'm fairly certain that there isn't, because we have
> to be able to deal with recursive probes (yes, we've had to deal with
> those in the past.)

Hm, you are right, I think that's why we had to remove the locks.  The
klist stuff handles us getting the needed locks for managing our
internal lists of devices and drivers, and those should be fine.

So, let's go back to your original worry, what are you concerned about?
A device being removed while probe() is called?

thanks,

greg k-h

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

[Russell King - ARM Linux]

On Mon, Jan 20, 2014 at 04:26:23PM -0800, Greg KH wrote:
> On Tue, Jan 21, 2014 at 12:07:06AM +0000, Russell King - ARM Linux wrote:
> > On Mon, Jan 20, 2014 at 03:51:28PM -0800, Greg KH wrote:
> > > On Mon, Jan 20, 2014 at 11:16:03PM +0000, Russell King - ARM Linux wrote:
> > > > I don't believe the driver model has any locking to prevent a drivers
> > > > ->probe function running concurrently with it's ->remove function for
> > > > two (or more) devices.
> > > 
> > > The bus prevents this from happening.
> > > 
> > > > The locking against this is done on a per-device basis, not a per-driver
> > > > basis.
> > > 
> > > No, on a per-bus basis.
> > 
> > I don't see it.
> > 
> > Let's start from driver_register().
> 
> Which happens from module probing, which is single-threaded, right?

Yes, to _some_ extent - the driver is added to the bus list of drivers
before existing drivers are probed, so it's always worth bearing in
mind that if a new device comes along, it's possible for that device
to be offered to even a driver which hasn't finished returning from
its module_init().

> > If you think there's a per-driver lock that's held over probes or removes,
> > please point it out.  I'm fairly certain that there isn't, because we have
> > to be able to deal with recursive probes (yes, we've had to deal with
> > those in the past.)
> 
> Hm, you are right, I think that's why we had to remove the locks.  The
> klist stuff handles us getting the needed locks for managing our
> internal lists of devices and drivers, and those should be fine.
> 
> So, let's go back to your original worry, what are you concerned about?
> A device being removed while probe() is called?

My concern is that we're turning something which should be simple into
something unnecessarily complex.  By that, I mean something along the
lines of:

static DEFINE_MUTEX(foo_mutex);
static unsigned foo_devices;

static int foo_probe(struct platform_device *pdev)
{
	int ret;

	mutex_lock(&foo_mutex);
	if (foo_devices++ == 0)
		uart_register_driver(&driver);

	ret = foo_really_probe_device(pdev);
	if (ret) {
		if (--foo_devices == 0)
			uart_unregister_driver(&driver);
	}
	mutex_unlock(&foo_mutex);

	return ret;
}

static int foo_remove(struct platform_device *pdev)
{
	mutex_lock(&foo_mutex);
	foo_really_remove(pdev);
	if (--foo_devices == 0)
		uart_unregister_driver(&driver);
	mutex_unlock(&foo_mutex);

	return 0;
}

in every single serial driver we have...  Wouldn't it just be better to
fix the major/minor number problem rather than have to add all that code
repetitively to all those drivers?

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[Alan Cox]

On Tue, 21 Jan 2014 00:16:57 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

[I did post a reply to this while on my phone but it got rejected]

> On Mon, Jan 20, 2014 at 11:47:34PM +0000, Alan Cox wrote:
> > But yes I agree about the idiom, but a definite NAK to any attempts to
> > plaster over this grand screwup by crapping in the tty core. Your turd,
> > deal with it locally in the ARM code if you can't apply common sense and
> > just go dynamic.
> 
> I believe at the time there was no one maintaining the device list to
> _do_ that allocation - AMBA PL011 came along in 2005 after (I believe)
> hpa stopped looking after that list.

git log Documentation/devices.txt

> So, please tell me how a number could be allocated properly without the
> device numbers list being maintained?

It was being maintained

> I've no problem with going dynamic, and I suggest that you get a sense
> of perspective rather than just spouting rubbish from on high.

I suggest you take a harder look at the actual history rather than your
revisionist one and then apologise.

The "simple" way to sort this out is to go dynamic as first proposed. The
more complicated way *IFF* Ben can show an actual systems that break is
for the ARM folks to bury a "Use ancient static device mapping" KConfig
entry into the arch Kconfig - which can then go away after a while.

Alan

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

[One Thousand Gnomes]

> > So, let's go back to your original worry, what are you concerned about?
> > A device being removed while probe() is called?
> 
> My concern is that we're turning something which should be simple into
> something unnecessarily complex.  By that, I mean something along the
> lines of:

Or in fact more complex in other cases because your remove may well be
refcounted so the stuff may not be going away in the foo_remove() path.

> 
> static DEFINE_MUTEX(foo_mutex);
> static unsigned foo_devices;
> 
> static int foo_probe(struct platform_device *pdev)
> {
> 	int ret;
> 
> 	mutex_lock(&foo_mutex);
> 	if (foo_devices++ == 0)
> 		uart_register_driver(&driver);
> 
> 	ret = foo_really_probe_device(pdev);

We have atomic_inc_and_test and atomic_dec_and_test so it's
fractionally less ugly.

> in every single serial driver we have...  Wouldn't it just be better to
> fix the major/minor number problem rather than have to add all that code
> repetitively to all those drivers?

Quite.

Although for some drivers I suspect what is actually missing when built
in is

module_init() {
	if (not_the_right_platform())
		return -ENOGOOD;
}

 
Going dynamic is the right fix though. Changing how the driver
registration work is a different (and quite independent) problem.

Alan

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

[Russell King - ARM Linux]

On Tue, Jan 21, 2014 at 09:25:31AM +0000, One Thousand Gnomes wrote:
> > static DEFINE_MUTEX(foo_mutex);
> > static unsigned foo_devices;
> > 
> > static int foo_probe(struct platform_device *pdev)
> > {
> > 	int ret;
> > 
> > 	mutex_lock(&foo_mutex);
> > 	if (foo_devices++ == 0)
> > 		uart_register_driver(&driver);
> > 
> > 	ret = foo_really_probe_device(pdev);
> 
> We have atomic_inc_and_test and atomic_dec_and_test so it's
> fractionally less ugly.

How do atomics help here?  If we do this as:

	if (atomic_inc_and_test(&foo_atomic))
		uart_register_driver(&driver);

Then let's think about what can happen:

	CPU0				CPU1
	foo_probe
	atomic_inc_and_test() == true
		uart_register_driver
					foo_probe
					atomic_inc_and_test()
					really_probe_foo()
						*bang*

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[Russell King - ARM Linux]

On Tue, Jan 21, 2014 at 09:03:40AM +0000, Alan Cox wrote:
> On Tue, 21 Jan 2014 00:16:57 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> [I did post a reply to this while on my phone but it got rejected]
> 
> > On Mon, Jan 20, 2014 at 11:47:34PM +0000, Alan Cox wrote:
> > > But yes I agree about the idiom, but a definite NAK to any attempts to
> > > plaster over this grand screwup by crapping in the tty core. Your turd,
> > > deal with it locally in the ARM code if you can't apply common sense and
> > > just go dynamic.
> > 
> > I believe at the time there was no one maintaining the device list to
> > _do_ that allocation - AMBA PL011 came along in 2005 after (I believe)
> > hpa stopped looking after that list.
> 
> git log Documentation/devices.txt

Hmm.  Given that git history starts at 2.6.12-rc2, and this was introduced
in 2.5 kernels, there's no point me looking, because I know the history
I have access to does not go back to that time.

And I know for certain that the devices list *did* fall out of maintainership
for a while.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[Mark Brown]

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

On Mon, Jan 20, 2014 at 11:47:34PM +0000, Alan Cox wrote:
> Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Jan 20, 2014 at 09:43:05PM +0000, Alan Cox wrote:

> > The userspace breakage is that if someone has a static /dev that doesn't
> > handle any dynamic devices then renumbering the device will cause that
> > static /dev to stop matching the kernel.

> Diddums and you've only provided theoretical cases not real world ones.

I'm fairly sure I have some older boards on my desk with static /dev,
I'd need to go and check which is rather more effort than I'm interested
in putting in for point scoring.

> They should have followed proper practice and reserved their minors. The
> device number belongs to the Altix. The driver should just move.

They should have done that about a decade ago when this stuff was
introduced, yes.  At some point one has to accept and deal with the
world as it is.

> But yes I agree about the idiom, but a definite NAK to any attempts to
> plaster over this grand screwup by crapping in the tty core. Your turd,
> deal with it locally in the ARM code if you can't apply common sense and
> just go dynamic.

Local bodges being the first two patches Greg knocked back of course...
In any case, if we can convince people to make the subsystem better and
coincidentally also stop problems occuring on their systems that does
seem like a win overall.

> And please, after screwing this up twice - *learn* from the mess.

One of the things we've been trying to do is to ensure that the all the
driver code that isn't core architecture support (not just for ARM, for
everything) is going into subsystems so we're getting good review from
people who actually know the subsystems and aren't just saying things
are random platform stuff or whatever that we don't care about.  That
has been a source of problems all over, we're trying to avoid it.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Russell King - ARM Linux]

On Tue, Jan 21, 2014 at 04:59:35PM +0000, Mark Brown wrote:
> On Mon, Jan 20, 2014 at 11:47:34PM +0000, Alan Cox wrote:
> > They should have followed proper practice and reserved their minors. The
> > device number belongs to the Altix. The driver should just move.
> 
> They should have done that about a decade ago when this stuff was
> introduced, yes.  At some point one has to accept and deal with the
> world as it is.

Let's look at the facts, shall we?  We have several drivers which I was
involved in, which include the original AMBA PL010 driver, the SA11x0
serial driver, and the Footbridge driver.  Both of those had been around
for some time in earlier kernels.

I remember there being a discussion about the major/minor numbers, and
the conclusion which came out of that was to have this "low density
serial ports" major, and allocate minor numbers from that.

So I applied for minor numbers for these drivers, and HPA at the time
granted them.  Here's an exmaple:
| Russell King wrote:
| > 
| > Hi hpa,
| > 
| > Did you get my previous mail about this device allocation?  If not, here
| > is the request (although its hard to put it together without an up to
| > date devices.txt list):
| > 
| >         Major:                  204
| >         Minor:                  16
| >         Number of minors:       16
| >         Non-devfs name:         ttyAM0 to ttyAM15
| >         Description:            ARM "AMBA" serial ports
| > 
| >         Major:                  205
| >         Minor:                  16
| >         Number of minors:       16
| >         Non-devfs name:         cuaam to cuaam15
| >         Description:            Callout device for ttyAM0 to ttyAM15
| > 
| 
| Approved as written.

Others did the same (previously) for the StrongARM driver (which was
Nicolas).

During the 2.3/2.4 era, the clps711x serial driver was added, and that
re-used the above allocations since it was believed at the time that
there would be no overlap (they'd only appear on SoCs and you would
never have one of each type on each SoC).  So, the clps711x serial
driver never had an offical allocation at that time.  Later on, it
transitioned to using its own minors (which now conflicts).  Exactly
when that happened, I'm not sure - sometime between Jan and July 2002.

However, the clps711x driver went in as a side effect of the need to
replace the old serial.c driver with something better - and it was
decided that my serial_core and serial rewrite was the desired way
forward.  So that stuff got sucked in with no official allocation -
that was an oversight on my part.

At some point after that, the amba PL011 driver was added - I can't
pin that down, because the history from the 2.5/early 2.6 is lost to
me (and I can't sanely get access to the Linux history git... not
with the state of my DSL.)  Looking at the patch archive I have, it
may have been around patch-2.6.0-test2 time when amba-pl011 came into
being, which places that around 5th August 2003, but I can't see when
it went into mainline since I stopped producing patches in December
2003, but we can be certain that it was after 2003.

Now, I'm certain of my facts here that LANANA stopped being useful
around this time - especially so now that I've found an email from
2004:

| Date:   Wed, 23 Jun 2004 10:07:54 -0500
| From:   Erik Jacobson <erikj@subway.americas.sgi.com>
| Subject: Re: [PATCH 2.6] Altix serial driver
| Message-ID: <Pine.SGI.4.53.0406231005110.280312@subway.americas.sgi.com>
| 
| LANANA has been unresponsive and their site states they aren't accepting
| 2.6 submissions.

So here we have evidence that I'm not the only one who stumbled across
LANANA becoming a problem - and it becoming impossible to have additional
numbers allocated in that range.

Here's more evidence, from later on, which appears to show when LANANA
resumed:

| From: David Woodhouse <dwmw2@infradead.org>
| To: device@lanana.org
| Cc: benh@kernel.crashing.org, rmk+serial@arm.linux.org.uk
| Date: Fri, 01 Dec 2006 15:40:23 +0000
| Message-Id: <1164987623.12649.36.camel@pmac.infradead.org>
| 
| Please assign four minor numbers from major 204, for the pmac_zilog
| driver.
| 
| Thanks.

To which LANANA never responded, so it was followed up:

| From: David Woodhouse <dwmw2@infradead.org>
| To: device@lanana.org
| Cc: benh@kernel.crashing.org, rmk+serial@arm.linux.org.uk
| Date: Fri, 30 Mar 2007 04:11:52 +0100
| Message-Id: <1175224312.3122.113.camel@pmac.infradead.org>
| 
| On Fri, 2006-12-01 at 15:40 +0000, David Woodhouse wrote:
| > Please assign four minor numbers from major 204, for the pmac_zilog
| > driver.
| 
| Pretty please?

And this finally resulted in numbers being allocated again, this time by
Torben Mathiasen.

Now, I had - and have - absolutely *no* problem with this process... and
if it had been available for allocating the ttyAMA numbers properly, I
would have definitely used it - for the sole reason that it is the only
sane thing to do.

But it wasn't available, and something had to be done.

Unlike Alan's comments which would have you believe that LANANA has been
operating continuously, it seems that the evidence I have backs up my
claim, and supports the reason why these drivers never had an official
allocation, and it's certainly *not* through lack of common sense or the
intention of creating "a turd" - which I take great offence to those
accusations.  I did the best I could at that time given the circumstances
that were prevailing.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[Alan Cox]

> I had earlier submitted a patch [1] to remove the hard coded
> major/minor number for Samsung UART driver, but that was rejected
> because of userspace breakage. Without this patch, Samsung UART driver
> can't bind to the hard-coded device node. Changing the default
> major/minor will also not help to fix the objections raised in [1].
> 
> Would you please suggest a way forward?
> 
> [1] https://lkml.org/lkml/2013/12/27/2

So to go and try and put this to bed properly I would suggest the
following way forward. 

We add

CONFIG_LEGACY_STATIC_TTY

Some platforms historically used static device nodes for the console
devices. Select this if you are building a kernel for an old system which
has a static /dev.

Note that because some devices historically used incorrect clashing
numbering this may prevent you building a single kernel which can be
booted on multiple platforms.


And then we do

	.nr		= CONFIG_SERIAL_SAMSUNG_UARTS,
 	.cons		= S3C24XX_SERIAL_CONSOLE,
 	.dev_name	= S3C24XX_SERIAL_NAME,
#ifdef CONFIG_LEGACY_STATIC_TTY
	.major		= S3C24XX_SERIAL_MAJOR,
	.minor		= S3C24XX_SERIAL_MINOR,
#endif

for the afflicted ports (and anyone else who wants to migrate)


We can then enable that config option for ARM (and in time for any other
architecture that turns out to need/want it). Eventually it can go away
(not that its exactly doing any harm if it doesnt).


Does that sound a valid way forward for everyone ?

Alan

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

[Mark Brown]

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

On Thu, Jan 23, 2014 at 06:04:23PM +0000, Alan Cox wrote:

> We can then enable that config option for ARM (and in time for any other
> architecture that turns out to need/want it). Eventually it can go away
> (not that its exactly doing any harm if it doesnt).

We'd need to leave it user selectable rather than enabling it for ARM,
the whole reason this got noticed is that people are trying to build
kernels that support a wider range of devices for ARM.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Tomasz Figa]

On 23.01.2014 19:40, Mark Brown wrote:
> On Thu, Jan 23, 2014 at 06:04:23PM +0000, Alan Cox wrote:
>
>> We can then enable that config option for ARM (and in time for any other
>> architecture that turns out to need/want it). Eventually it can go away
>> (not that its exactly doing any harm if it doesnt).
>
> We'd need to leave it user selectable rather than enabling it for ARM,
> the whole reason this got noticed is that people are trying to build
> kernels that support a wider range of devices for ARM.

What about making it depend on !MULTIPLATFORM and enabled by default?

Best regards,
Tomasz

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

[Mark Brown]

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

On Thu, Jan 23, 2014 at 07:47:56PM +0100, Tomasz Figa wrote:
> On 23.01.2014 19:40, Mark Brown wrote:

> >We'd need to leave it user selectable rather than enabling it for ARM,
> >the whole reason this got noticed is that people are trying to build
> >kernels that support a wider range of devices for ARM.

> What about making it depend on !MULTIPLATFORM and enabled by default?

That'd work, but if we're doing that then substituting in the dynamic
assignment only when we hit a collision seems smoother and more general.
Or we could just make the core ignore all hard coded numbers if this is
set rather than putting ifdefs in the drivers.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Alan Cox]

On Thu, 23 Jan 2014 19:36:33 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Thu, Jan 23, 2014 at 07:47:56PM +0100, Tomasz Figa wrote:
> > On 23.01.2014 19:40, Mark Brown wrote:
> 
> > >We'd need to leave it user selectable rather than enabling it for ARM,
> > >the whole reason this got noticed is that people are trying to build
> > >kernels that support a wider range of devices for ARM.
> 
> > What about making it depend on !MULTIPLATFORM and enabled by default?
> 
> That'd work, but if we're doing that then substituting in the dynamic
> assignment only when we hit a collision seems smoother and more general.
> Or we could just make the core ignore all hard coded numbers if this is
> set rather than putting ifdefs in the drivers.

That strikes me as rather more risky. We can propogate it through the
drivers as we are sure it is safe to do so on that platform and encourage
driver authors to migrate. Better than a "big bang" and the inevitable
fallout.

We do want it user selectable, because we want people to leave it off
unless they have a need for it - that then becomes a path towards
eventually getting rid of them static identifiers for good.

Alan

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

[Mark Brown]

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

On Thu, Jan 23, 2014 at 07:51:44PM +0000, Alan Cox wrote:

> That strikes me as rather more risky. We can propogate it through the
> drivers as we are sure it is safe to do so on that platform and encourage
> driver authors to migrate. Better than a "big bang" and the inevitable
> fallout.

I don't see how we can meaningfully test this on a platform - the kernel
would have to be pretty demented to care, it's userspace that cares and
that's not really tied to individual serial drivers but is where we
mainly need coverage.

> We do want it user selectable, because we want people to leave it off
> unless they have a need for it - that then becomes a path towards
> eventually getting rid of them static identifiers for good.

Yup.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Alan Cox]

On Thu, 23 Jan 2014 20:05:09 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Thu, Jan 23, 2014 at 07:51:44PM +0000, Alan Cox wrote:
> 
> > That strikes me as rather more risky. We can propogate it through the
> > drivers as we are sure it is safe to do so on that platform and encourage
> > driver authors to migrate. Better than a "big bang" and the inevitable
> > fallout.
> 
> I don't see how we can meaningfully test this on a platform - the kernel
> would have to be pretty demented to care, it's userspace that cares and
> that's not really tied to individual serial drivers but is where we
> mainly need coverage.

Which is why I think we want to enable it gradually and platform by
platform as that platform or arch maintainer judges it appropriate given
their knowledge of their platform(s).

Alan

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

[Mark Brown]

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

On Thu, Jan 23, 2014 at 09:33:21PM +0000, Alan Cox wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > I don't see how we can meaningfully test this on a platform - the kernel
> > would have to be pretty demented to care, it's userspace that cares and
> > that's not really tied to individual serial drivers but is where we
> > mainly need coverage.

> Which is why I think we want to enable it gradually and platform by
> platform as that platform or arch maintainer judges it appropriate given
> their knowledge of their platform(s).

I don't see how that follows?  For the most part architecture
maintainers aren't going to be able to say too much about which
userspaces are being run on their platforms if the architecture
has any kind of widespread use.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Alan Cox]

On Fri, 24 Jan 2014 12:03:09 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Thu, Jan 23, 2014 at 09:33:21PM +0000, Alan Cox wrote:
> > Mark Brown <broonie@kernel.org> wrote:
> 
> > > I don't see how we can meaningfully test this on a platform - the kernel
> > > would have to be pretty demented to care, it's userspace that cares and
> > > that's not really tied to individual serial drivers but is where we
> > > mainly need coverage.
> 
> > Which is why I think we want to enable it gradually and platform by
> > platform as that platform or arch maintainer judges it appropriate given
> > their knowledge of their platform(s).
> 
> I don't see how that follows?  For the most part architecture
> maintainers aren't going to be able to say too much about which
> userspaces are being run on their platforms if the architecture
> has any kind of widespread use.

For most architectures I suspect that they do have a pretty good idea.
ARM is a bit different but I imagine the various platform specific
maintainers are the ones who have that knowledge

Alan

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

[Russell King - ARM Linux]

On Tue, Jan 21, 2014 at 12:45:05AM +0000, Alan Cox wrote:
> Peter handed it on. Try using git log on Documentation/devices.txt. It
> still gets updates.
> 
> Perhaps you'd care to stick to reality and fix the tree instead of trying
> to excuse the mess ?

Perhaps returning to reality might be advantageous rather than trying
to repeat statements which can't have any bearing on this - especially
as the git history which you're referring to only goes back to 2.6.12-rc2,
and this predates 2.6.12-rc2 by a long shot.

> More importantly certain folks need to stop abusing static numbers
> allocated properly. Repeating it having made a total hash of it before
> is dismal.

And if you continue these stupid accusations which have no basis at all,
we're going to get into a real big argument, because you are soo _wrong_
on that point.  I was always the one arguing /against/ the re-use of
existing major/minor numbers.  I was the one arguing /against/ Nicolas'
patches to make every serial port appear in the 4,64 ttyS namespace.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[Pavel Machek]

On Thu 2014-01-23 19:36:33, Mark Brown wrote:
> On Thu, Jan 23, 2014 at 07:47:56PM +0100, Tomasz Figa wrote:
> > On 23.01.2014 19:40, Mark Brown wrote:
> 
> > >We'd need to leave it user selectable rather than enabling it for ARM,
> > >the whole reason this got noticed is that people are trying to build
> > >kernels that support a wider range of devices for ARM.
> 
> > What about making it depend on !MULTIPLATFORM and enabled by default?
> 
> That'd work, but if we're doing that then substituting in the dynamic
> assignment only when we hit a collision seems smoother and more general.

That seems like a mess. I had enough fun debugging "WTF is
going on with my serials" as is...

Plus... collision can happen at runtime when you insert
BT CF card... at that point it is too late to reassign.

							Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Alan Cox]

On Sun, 26 Jan 2014 22:09:07 +0100
Pavel Machek <pavel@ucw.cz> wrote:

> On Thu 2014-01-23 19:36:33, Mark Brown wrote:
> > On Thu, Jan 23, 2014 at 07:47:56PM +0100, Tomasz Figa wrote:
> > > On 23.01.2014 19:40, Mark Brown wrote:
> > 
> > > >We'd need to leave it user selectable rather than enabling it for ARM,
> > > >the whole reason this got noticed is that people are trying to build
> > > >kernels that support a wider range of devices for ARM.
> > 
> > > What about making it depend on !MULTIPLATFORM and enabled by default?
> > 
> > That'd work, but if we're doing that then substituting in the dynamic
> > assignment only when we hit a collision seems smoother and more general.
> 
> That seems like a mess. I had enough fun debugging "WTF is
> going on with my serials" as is...
> 
> Plus... collision can happen at runtime when you insert
> BT CF card... at that point it is too late to reassign.

Agreed entirely - you want predictability here, plus making it happen by
magic is going to do the nut of anyone trying to figure out why kernel A
works on their platform and kernel B doesn't.

It needs to be something you choose to turn on as compatibility, like old
sysfs files and so forth.

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

[Mark Brown]

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

On Fri, Jan 24, 2014 at 02:38:59PM +0000, Alan Cox wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > I don't see how that follows?  For the most part architecture
> > maintainers aren't going to be able to say too much about which
> > userspaces are being run on their platforms if the architecture
> > has any kind of widespread use.

> For most architectures I suspect that they do have a pretty good idea.
> ARM is a bit different but I imagine the various platform specific
> maintainers are the ones who have that knowledge

I'm still not entirely sure how you expect people to answer this unless
the answer is something along the lines of "all the systems are on my
desk".  This definitely does need to be selectable, I'm just not clear
that making it depend on the architecture is going to help.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Nicolas Pitre]

On Sun, 26 Jan 2014, Russell King - ARM Linux wrote:

> On Tue, Jan 21, 2014 at 12:45:05AM +0000, Alan Cox wrote:
> > Peter handed it on. Try using git log on Documentation/devices.txt. It
> > still gets updates.
> > 
> > Perhaps you'd care to stick to reality and fix the tree instead of trying
> > to excuse the mess ?
> 
> Perhaps returning to reality might be advantageous rather than trying
> to repeat statements which can't have any bearing on this - especially
> as the git history which you're referring to only goes back to 2.6.12-rc2,
> and this predates 2.6.12-rc2 by a long shot.
> 
> > More importantly certain folks need to stop abusing static numbers
> > allocated properly. Repeating it having made a total hash of it before
> > is dismal.
> 
> And if you continue these stupid accusations which have no basis at all,
> we're going to get into a real big argument, because you are soo _wrong_
> on that point.  I was always the one arguing /against/ the re-use of
> existing major/minor numbers.  I was the one arguing /against/ Nicolas'
> patches to make every serial port appear in the 4,64 ttyS namespace.

If you remember correctly, that was my attempt at making serial port 
minor assignment to be dynamic... just like everything else is today.  
And it seemed to me that you thought this was a good idea.

But that was objected by the X86 folks who insisted on always having 
COM1 == ttyS0 and COM2 == ttyS1, whether or not COM1 was present.

Nowdays serial ports have pretty much disappeared from the X86 world.  
But the problem they've created is still with us.

Either everything is dynamic, or everything follows proper minor 
assignment process.


Nicolas

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

[Alan Cox]

> Either everything is dynamic, or everything follows proper minor 
> assignment process.

Ultimately everything should probably be dynamic, but trying to get there
in one step will simply mean we never get there at all.

Alan

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

[Russell King - ARM Linux]

On Sun, Jan 26, 2014 at 11:30:00PM -0500, Nicolas Pitre wrote:
> On Sun, 26 Jan 2014, Russell King - ARM Linux wrote:
> 
> > On Tue, Jan 21, 2014 at 12:45:05AM +0000, Alan Cox wrote:
> > > Peter handed it on. Try using git log on Documentation/devices.txt. It
> > > still gets updates.
> > > 
> > > Perhaps you'd care to stick to reality and fix the tree instead of trying
> > > to excuse the mess ?
> > 
> > Perhaps returning to reality might be advantageous rather than trying
> > to repeat statements which can't have any bearing on this - especially
> > as the git history which you're referring to only goes back to 2.6.12-rc2,
> > and this predates 2.6.12-rc2 by a long shot.
> > 
> > > More importantly certain folks need to stop abusing static numbers
> > > allocated properly. Repeating it having made a total hash of it before
> > > is dismal.
> > 
> > And if you continue these stupid accusations which have no basis at all,
> > we're going to get into a real big argument, because you are soo _wrong_
> > on that point.  I was always the one arguing /against/ the re-use of
> > existing major/minor numbers.  I was the one arguing /against/ Nicolas'
> > patches to make every serial port appear in the 4,64 ttyS namespace.
> 
> If you remember correctly, that was my attempt at making serial port 
> minor assignment to be dynamic... just like everything else is today.  
> And it seemed to me that you thought this was a good idea.

I may have thought that a dynamic space for serial devices was a good
idea, but what I was referring to above was specifically the
implementation.

Unfortunately, there's precious little public evidence of this as the
patches were never posted to a public mailing list.  However, what there
is (as part of another thread) shows that I held that view:

http://archive.arm.linux.org.uk/lurker/message/20041124.164950.92dc25d7.en.html

Plus, of course, the comments in the patch system where I picked out a
number of further technical issues, such as changing inode->i_rdev,
userspace locking, etc.

If you want to review them, they're 1427/1 - 1434/1, 1435/2, 1436/2.
Unfortunately the authorship of those comments was lost.

Hence, my recollection is correct here.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[Nicolas Pitre]

On Mon, 27 Jan 2014, Russell King - ARM Linux wrote:

> On Sun, Jan 26, 2014 at 11:30:00PM -0500, Nicolas Pitre wrote:
> > On Sun, 26 Jan 2014, Russell King - ARM Linux wrote:
> > 
> > > On Tue, Jan 21, 2014 at 12:45:05AM +0000, Alan Cox wrote:
> > > > Peter handed it on. Try using git log on Documentation/devices.txt. It
> > > > still gets updates.
> > > > 
> > > > Perhaps you'd care to stick to reality and fix the tree instead of trying
> > > > to excuse the mess ?
> > > 
> > > Perhaps returning to reality might be advantageous rather than trying
> > > to repeat statements which can't have any bearing on this - especially
> > > as the git history which you're referring to only goes back to 2.6.12-rc2,
> > > and this predates 2.6.12-rc2 by a long shot.
> > > 
> > > > More importantly certain folks need to stop abusing static numbers
> > > > allocated properly. Repeating it having made a total hash of it before
> > > > is dismal.
> > > 
> > > And if you continue these stupid accusations which have no basis at all,
> > > we're going to get into a real big argument, because you are soo _wrong_
> > > on that point.  I was always the one arguing /against/ the re-use of
> > > existing major/minor numbers.  I was the one arguing /against/ Nicolas'
> > > patches to make every serial port appear in the 4,64 ttyS namespace.
> > 
> > If you remember correctly, that was my attempt at making serial port 
> > minor assignment to be dynamic... just like everything else is today.  
> > And it seemed to me that you thought this was a good idea.
> 
> I may have thought that a dynamic space for serial devices was a good
> idea, but what I was referring to above was specifically the
> implementation.

Oh I do not dispute the fact that the implementation at the time was not 
up to needed standards.  That would have been fixed eventually though, 
if it hadn't been for the policy based objection we received.  At which 
point this effort was simply dropped.


Nicolas

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

[Greg KH]

On Mon, Jan 20, 2014 at 10:04:15AM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 20, 2014 at 02:32:35PM +0530, Tushar Behera 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 same default major/minor numbers are
> > included in the kernel.
> > 
> > A typical case is observed with amba-pl011 and samsung-uart drivers.
> 
> NAK.  There should not be any other driver using amba-pl011's device numbers.

I agree, but there is.  And because of that, moving the registration to
the probe call fixes the issue with building a kernel with all of the
drivers built into them, so I'm going to take both of these patches, as
it does solve that problem, while still allowing the device number
collision to happen.

thanks,

greg k-h

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

[Russell King - ARM Linux]

On Thu, Feb 13, 2014 at 10:12:16AM -0800, Greg KH wrote:
> On Mon, Jan 20, 2014 at 10:04:15AM +0000, Russell King - ARM Linux wrote:
> > On Mon, Jan 20, 2014 at 02:32:35PM +0530, Tushar Behera 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 same default major/minor numbers are
> > > included in the kernel.
> > > 
> > > A typical case is observed with amba-pl011 and samsung-uart drivers.
> > 
> > NAK.  There should not be any other driver using amba-pl011's device numbers.
> 
> I agree, but there is.  And because of that, moving the registration to
> the probe call fixes the issue with building a kernel with all of the
> drivers built into them, so I'm going to take both of these patches, as
> it does solve that problem, while still allowing the device number
> collision to happen.

So what happens when two _devices_ are probed by this driver at the same
time?

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[Greg KH]

On Thu, Feb 13, 2014 at 06:15:59PM +0000, Russell King - ARM Linux wrote:
> On Thu, Feb 13, 2014 at 10:12:16AM -0800, Greg KH wrote:
> > On Mon, Jan 20, 2014 at 10:04:15AM +0000, Russell King - ARM Linux wrote:
> > > On Mon, Jan 20, 2014 at 02:32:35PM +0530, Tushar Behera 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 same default major/minor numbers are
> > > > included in the kernel.
> > > > 
> > > > A typical case is observed with amba-pl011 and samsung-uart drivers.
> > > 
> > > NAK.  There should not be any other driver using amba-pl011's device numbers.
> > 
> > I agree, but there is.  And because of that, moving the registration to
> > the probe call fixes the issue with building a kernel with all of the
> > drivers built into them, so I'm going to take both of these patches, as
> > it does solve that problem, while still allowing the device number
> > collision to happen.
> 
> So what happens when two _devices_ are probed by this driver at the same
> time?

The bus that the driver is on will not allow that to happen, I thought
we went through this before...

And yes, devices on different busses could cause problems, but is that
the case here for these devices?  At first glance, I don't think that
can happen for these drivers.

thanks,

greg k-h

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

[Russell King - ARM Linux]

On Thu, Feb 13, 2014 at 10:27:01AM -0800, Greg KH wrote:
> On Thu, Feb 13, 2014 at 06:15:59PM +0000, Russell King - ARM Linux wrote:
> > On Thu, Feb 13, 2014 at 10:12:16AM -0800, Greg KH wrote:
> > > On Mon, Jan 20, 2014 at 10:04:15AM +0000, Russell King - ARM Linux wrote:
> > > > On Mon, Jan 20, 2014 at 02:32:35PM +0530, Tushar Behera 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 same default major/minor numbers are
> > > > > included in the kernel.
> > > > > 
> > > > > A typical case is observed with amba-pl011 and samsung-uart drivers.
> > > > 
> > > > NAK.  There should not be any other driver using amba-pl011's device numbers.
> > > 
> > > I agree, but there is.  And because of that, moving the registration to
> > > the probe call fixes the issue with building a kernel with all of the
> > > drivers built into them, so I'm going to take both of these patches, as
> > > it does solve that problem, while still allowing the device number
> > > collision to happen.
> > 
> > So what happens when two _devices_ are probed by this driver at the same
> > time?
> 
> The bus that the driver is on will not allow that to happen, I thought
> we went through this before...
> 
> And yes, devices on different busses could cause problems, but is that
> the case here for these devices?  At first glance, I don't think that
> can happen for these drivers.

We went through this before, and I stated the paths, and no one disagreed
with that.

It /is/ racy.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[Greg KH]

On Thu, Feb 13, 2014 at 06:42:49PM +0000, Russell King - ARM Linux wrote:
> On Thu, Feb 13, 2014 at 10:27:01AM -0800, Greg KH wrote:
> > On Thu, Feb 13, 2014 at 06:15:59PM +0000, Russell King - ARM Linux wrote:
> > > On Thu, Feb 13, 2014 at 10:12:16AM -0800, Greg KH wrote:
> > > > On Mon, Jan 20, 2014 at 10:04:15AM +0000, Russell King - ARM Linux wrote:
> > > > > On Mon, Jan 20, 2014 at 02:32:35PM +0530, Tushar Behera 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 same default major/minor numbers are
> > > > > > included in the kernel.
> > > > > > 
> > > > > > A typical case is observed with amba-pl011 and samsung-uart drivers.
> > > > > 
> > > > > NAK.  There should not be any other driver using amba-pl011's device numbers.
> > > > 
> > > > I agree, but there is.  And because of that, moving the registration to
> > > > the probe call fixes the issue with building a kernel with all of the
> > > > drivers built into them, so I'm going to take both of these patches, as
> > > > it does solve that problem, while still allowing the device number
> > > > collision to happen.
> > > 
> > > So what happens when two _devices_ are probed by this driver at the same
> > > time?
> > 
> > The bus that the driver is on will not allow that to happen, I thought
> > we went through this before...
> > 
> > And yes, devices on different busses could cause problems, but is that
> > the case here for these devices?  At first glance, I don't think that
> > can happen for these drivers.
> 
> We went through this before, and I stated the paths, and no one disagreed
> with that.
> 
> It /is/ racy.

Ok, I just went and looked at the uart driver register path, and I don't
see the race (note, if there is one, it's there today, regardless of
this patch).

uart_register_driver() fils in a bunch of fields, and passes control off
to tty_register_driver().  If the minor/major number allocation here
fails, due to collisions, then an error occurs, and things back out
safely.

If the allocation succeeds, then we lock the list of drivers, add them
to the list, then register the tty device with the subsystem for how
ever many tty devices were asked for.  Yes, the locking might be wrong
here, in the tty layer, but again, that's nothing new created by this
patch.

So I fail to see the problem with applying this patch, as it solves a
problem people are having, and should be fine given that no hardware
with both of these devices will ever be present at the same time.

thanks,

greg k-h

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

[Russell King - ARM Linux]

On Thu, Feb 13, 2014 at 03:26:06PM -0800, Greg KH wrote:
> On Thu, Feb 13, 2014 at 06:42:49PM +0000, Russell King - ARM Linux wrote:
> > We went through this before, and I stated the paths, and no one disagreed
> > with that.
> > 
> > It /is/ racy.
> 
> Ok, I just went and looked at the uart driver register path, and I don't
> see the race (note, if there is one, it's there today, regardless of
> this patch).

The race isn't the uart code, it's the driver model.

Consider what happens when this happens:

* Two pl011 devices get registered at the same time by two different
  threads.

* Both devices have a lock taken on the _device_ itself before matching
  against the driver.

* Both devices get matched to the same driver.

* Both devices are passed into the driver's probe function.

* Both check uart_reg.state, both call uart_register_driver() on that
  at the same time, which results in two allocations inside
  uart_register_driver(), one gets overwritten...

So, the /only/ thing which stops this happening is that the devices
are generally available before the driver is registered, and driver
registration results in devices being probed serially.  Moreover, both
attempt to call tty_register_driver()... one succeeds, the other fails.

However, what about the userspace bind/unbind methods.  Yes, userspace
can ask the driver core to unbind devices from a driver or bind - and
again, there's no per-driver locking here.  So, if you can trigger two
concurrent binds from userspace, you hit the same race as above.

So, if you want to accept these patches, go ahead, introduce races, but
personally I'd recommend plugging these races.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[Greg KH]

On Fri, Feb 14, 2014 at 12:07:17AM +0000, Russell King - ARM Linux wrote:
> On Thu, Feb 13, 2014 at 03:26:06PM -0800, Greg KH wrote:
> > On Thu, Feb 13, 2014 at 06:42:49PM +0000, Russell King - ARM Linux wrote:
> > > We went through this before, and I stated the paths, and no one disagreed
> > > with that.
> > > 
> > > It /is/ racy.
> > 
> > Ok, I just went and looked at the uart driver register path, and I don't
> > see the race (note, if there is one, it's there today, regardless of
> > this patch).
> 
> The race isn't the uart code, it's the driver model.
> 
> Consider what happens when this happens:
> 
> * Two pl011 devices get registered at the same time by two different
>   threads.

How?  What two different busses will see this same device?  The amba bus
code should prevent that from happening, right?  If not, there's bigger
problems in that bus code :)

That's where this problem should be fixed, if there is one, otherwise
this same issue would be there for any type of driver that calles into
the uart core, right?

> * Both devices have a lock taken on the _device_ itself before matching
>   against the driver.
> 
> * Both devices get matched to the same driver.
> 
> * Both devices are passed into the driver's probe function.
> 
> * Both check uart_reg.state, both call uart_register_driver() on that
>   at the same time, which results in two allocations inside
>   uart_register_driver(), one gets overwritten...
> 
> So, the /only/ thing which stops this happening is that the devices
> are generally available before the driver is registered, and driver
> registration results in devices being probed serially.  Moreover, both
> attempt to call tty_register_driver()... one succeeds, the other fails.
> 
> However, what about the userspace bind/unbind methods.  Yes, userspace
> can ask the driver core to unbind devices from a driver or bind - and
> again, there's no per-driver locking here.  So, if you can trigger two
> concurrent binds from userspace, you hit the same race as above.
> 
> So, if you want to accept these patches, go ahead, introduce races, but
> personally I'd recommend plugging these races.

The only way to solve this would be to do it in the bus, I don't see
anything here that makes it any "racier" than it currently is.

thanks,

greg k-h

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

[Russell King - ARM Linux]

On Thu, Feb 13, 2014 at 04:14:36PM -0800, Greg KH wrote:
> On Fri, Feb 14, 2014 at 12:07:17AM +0000, Russell King - ARM Linux wrote:
> > On Thu, Feb 13, 2014 at 03:26:06PM -0800, Greg KH wrote:
> > > On Thu, Feb 13, 2014 at 06:42:49PM +0000, Russell King - ARM Linux wrote:
> > > > We went through this before, and I stated the paths, and no one disagreed
> > > > with that.
> > > > 
> > > > It /is/ racy.
> > > 
> > > Ok, I just went and looked at the uart driver register path, and I don't
> > > see the race (note, if there is one, it's there today, regardless of
> > > this patch).
> > 
> > The race isn't the uart code, it's the driver model.
> > 
> > Consider what happens when this happens:
> > 
> > * Two pl011 devices get registered at the same time by two different
> >   threads.
> 
> How?  What two different busses will see this same device?  The amba bus
> code should prevent that from happening, right?  If not, there's bigger
> problems in that bus code :)

Where is that requirement documented?  It isn't documented.  No one
implements any kind of locking at the bus level to prevent this, not
PCI, nor platform devices.

> That's where this problem should be fixed, if there is one, otherwise
> this same issue would be there for any type of driver that calles into
> the uart core, right?

And how does bus code prevent this?  By intercepting the driver model
->probe callback and taking its own lock maybe?   Really?  What about
those buses which don't wrap the probe callback?

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[One Thousand Gnomes]

> > How?  What two different busses will see this same device?  The amba bus
> > code should prevent that from happening, right?  If not, there's bigger
> > problems in that bus code :)
> 
> Where is that requirement documented?  It isn't documented.  No one
> implements any kind of locking at the bus level to prevent this, not
> PCI, nor platform devices.

We don't want bus locking. There are lots of busses that can be parallel
probed and in some cases its expensive not to do so. We may well need to
do much more parallel probing in PCI bus in future and we may be parallel
probing multiple bus types at once.

The uart_register hack is simply broken. Nobody can stop you merging it
Greg but in the long term its the wrong approach.

We've identified a correct working approach which is to simply add a
CONFIG entry to the ARM tree and a few ifdefs to the problem drivers to
make the "problem" (in fact a complete fictional non-problem) go away and
to get rid of the mess over time completely as the drivers are set
dynamic and it turns out that all the userspace happens to already handle
this just fine.

Far better than botchfixes to uart registration code that will haunt us
for years.

Alan

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

[One Thousand Gnomes]

And an even simpler broad brush approach will also work I think (untested)

The "zero crap" approach....

commit c5cd0be0576ba9059799ef5383402ff6ffc212a3
Author: Alan <gnomes@lxorguk.ukuu.org.uk>
Date:   Mon Feb 17 15:52:21 2014 +0000

    tty: Allow serial port allocations to be fully dynamic
    
    This switch allows platforms to make their serial port allocations entirely
    dynamic as is needed on some non-x86 platforms which have overlapping static
    minor numbers between some of their devices.
    
    Default to the old static allocations for maximum compatibility with ancient
    userspace.
    
    Signed-off-by: Alan Cox <alan@linux.intel.com>

diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
index 65cd80b..670c076 100644
--- a/drivers/tty/Kconfig
+++ b/drivers/tty/Kconfig
@@ -166,6 +166,16 @@ config LEGACY_PTY_COUNT
 	  When not in use, each legacy PTY occupies 12 bytes on 32-bit
 	  architectures and 24 bytes on 64-bit architectures.
 
+config TTY_SERIAL_LEGACY
+	bool "Allocate legacy fixed minor numbers for serial ports"
+	default y
+	---help---
+	  Modern linux dynamically allocates device numbers. Some very old
+	  Linux distributions only support static device numbering.
+	  Selecting this option will enable support for very old
+	  userspace but on some non PC architectures may prevent you building
+	  a single kernel for multiple machines.
+
 config BFIN_JTAG_COMM
 	tristate "Blackfin JTAG Communication"
 	depends on BLACKFIN
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index ece2049..b78afc1 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2315,8 +2315,10 @@ int uart_register_driver(struct uart_driver *drv)
 
 	normal->driver_name	= drv->driver_name;
 	normal->name		= drv->dev_name;
+#ifdef CONFIG_TTY_SERIAL_LEGACY
 	normal->major		= drv->major;
 	normal->minor_start	= drv->minor;
+#endif	
 	normal->type		= TTY_DRIVER_TYPE_SERIAL;
 	normal->subtype		= SERIAL_TYPE_NORMAL;
 	normal->init_termios	= tty_std_termios;

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

[Mark Brown]

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

On Mon, Feb 17, 2014 at 03:35:18PM +0000, One Thousand Gnomes wrote:

> We've identified a correct working approach which is to simply add a
> CONFIG entry to the ARM tree and a few ifdefs to the problem drivers to
> make the "problem" (in fact a complete fictional non-problem) go away and
> to get rid of the mess over time completely as the drivers are set
> dynamic and it turns out that all the userspace happens to already handle
> this just fine.

It's a very real problem which affects actual kernels that distro style
users are building.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Etched Pixels]

On Tue, 18 Feb 2014 08:50:13 +0900
Mark Brown <broonie@kernel.org> wrote:

> On Mon, Feb 17, 2014 at 03:35:18PM +0000, One Thousand Gnomes wrote:
> 
> > We've identified a correct working approach which is to simply add a
> > CONFIG entry to the ARM tree and a few ifdefs to the problem drivers to
> > make the "problem" (in fact a complete fictional non-problem) go away and
> > to get rid of the mess over time completely as the drivers are set
> > dynamic and it turns out that all the userspace happens to already handle
> > this just fine.
> 
> It's a very real problem which affects actual kernels that distro style
> users are building.

Only because you persist in trying to keep the old static minor
numbers even though they are not needed by anything in the real world
that will ever run such kernels. Also only because you are apparently
too slack to bother to check whether the driver matches the platform or
there is an device tree node before registering it even though that's
trivial to code and is already done by some other platforms and serial
drivers just fine.

It's a core code "problem" that is invented by refusing to do the simple
trivial fixes in the ARM tree. Please clean up your own poop instead of
trying to shovel it into the street.

And the proposed change set is buggy as hell - because we register things
like 8250 devices at least four ways on the same x86 machine all of which
could in theory occur in parallel.

Alan

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

[One Thousand Gnomes]

> > The race isn't the uart code, it's the driver model.
> > 
> > Consider what happens when this happens:
> > 
> > * Two pl011 devices get registered at the same time by two different
> >   threads.
> 
> How?  What two different busses will see this same device?  The amba bus
> code should prevent that from happening, right?  If not, there's bigger
> problems in that bus code :)
> 
> That's where this problem should be fixed, if there is one, otherwise
> this same issue would be there for any type of driver that calles into
> the uart core, right?

I did some more digging into this the problem goes beyond the uart and
driver model code. Even if you serialize every bus and you serialize
every bus against every other bus (which you can't do btw as we get
recursive probes of one bus from a device probe of another) it's still
racy. We have non bus registering paths for some drivers and those could
be user triggered.

An obvious example is 8250.

If the 8250 driver is loaded on a platform where no device is
autodetected (which is true on a few wackier PC laptops with touchscreens
on an 8250) then you hae a race between say a PCMCIA card insertion of an
8250 and a user who creates the first 8250 device using setserial at the
same moment.

There are similar races between the various directly created devices on
some of the serial ports and bus probed ones where the driver mixes bus
probing with olde worlde straight forward hardcoding.

Alan

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

[Mark Brown]

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

On Tue, Feb 18, 2014 at 10:09:43AM +0000, Etched Pixels wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > It's a very real problem which affects actual kernels that distro style
> > users are building.

> Only because you persist in trying to keep the old static minor
> numbers even though they are not needed by anything in the real world
> that will ever run such kernels. Also only because you are apparently
> too slack to bother to check whether the driver matches the platform or
> there is an device tree node before registering it even though that's
> trivial to code and is already done by some other platforms and serial
> drivers just fine.

> It's a core code "problem" that is invented by refusing to do the simple
> trivial fixes in the ARM tree. Please clean up your own poop instead of
> trying to shovel it into the street.

Please try to avoid this sort of misdirected yelling, it's not helping
anything to complain that people are following the recommendations of
the maintainer or to demand that this somehow gets hacked around in
arch/ when we're trying to convince all the architectures to get their
drivers merged into subsystem trees so they're reviewed by the subsystem
maintainers.

If you have some examples of better practice for serial device
registration that you'd like to identify that'd be helpful, but what
you're doing here is making disparaging remarks and telling people to
adopt bad practices because someone else made a mistake a decade ago.
That is how problems like this get created in the first place.

> And the proposed change set is buggy as hell - because we register things
> like 8250 devices at least four ways on the same x86 machine all of which
> could in theory occur in parallel.

Then you need to convince Greg of that.  The most recent set of patches
are exactly what he asked for.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[One Thousand Gnomes]

> Please try to avoid this sort of misdirected yelling, it's not helping

I don't see any misdirected yelling

> anything to complain that people are following the recommendations of
> the maintainer or to demand that this somehow gets hacked around in
> arch/ when we're trying to convince all the architectures to get their
> drivers merged into subsystem trees so they're reviewed by the subsystem
> maintainers.

It's not a case of hacking around it in arch. It's a case of fixing up
problems where they belong, which btw is what Greg has in his tree -
specifically ef2889f7ffee67f0aed49e854c72be63f1466759 and
6f134c3c770355b7e930d3ffc558864668f13055 which keep the handling of the
minor clash cock-up in the drivers affected.

No garbage in the core tty drivers, no racy delayed registration in the
core uart code. In the longer term we should probably just abolish the
fixed minor numbers, nothing in the real world cares about them enough to
justify keeping them forever.

> you're doing here is making disparaging remarks and telling people to
> adopt bad practices because someone else made a mistake a decade ago.

I'm not telling anyone to adopt bad practices - at least its news to me
that "stopping the merge of buggy crap" is now a bad practice.

> That is how problems like this get created in the first place.

We were having a discussion about the fact the proposed patch for
deferring the processing was buggy, and probably unfixably so. That's a
code correctness discussion. Merging overcomplicated buggy crap causes
problems.

> > And the proposed change set is buggy as hell - because we register things
> > like 8250 devices at least four ways on the same x86 machine all of which
> > could in theory occur in parallel.
> 
> Then you need to convince Greg of that.  The most recent set of patches
> are exactly what he asked for.

No there's an assumption that when someone asks for patches the proposed
changes actually *work*. As Russell has demonstrated - the general
deferral patches for the uart/tty layer are broken. The driver specific
fixups in -next on the other hand appear to be fine providing the amba
bus probe remains serialized (and trivially fixed if it doesn't).

Alan

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

[Mark Brown]

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

On Wed, Feb 19, 2014 at 02:47:51PM +0000, One Thousand Gnomes wrote:

> > anything to complain that people are following the recommendations of
> > the maintainer or to demand that this somehow gets hacked around in
> > arch/ when we're trying to convince all the architectures to get their
> > drivers merged into subsystem trees so they're reviewed by the subsystem
> > maintainers.

> It's not a case of hacking around it in arch. It's a case of fixing up
> problems where they belong, which btw is what Greg has in his tree -
> specifically ef2889f7ffee67f0aed49e854c72be63f1466759 and
> 6f134c3c770355b7e930d3ffc558864668f13055 which keep the handling of the
> minor clash cock-up in the drivers affected.

That's more reasonable, what you were saying was to do things in the ARM
tree which reads like you want changes in arch/arm - those changes are
in the serial code.

> > you're doing here is making disparaging remarks and telling people to
> > adopt bad practices because someone else made a mistake a decade ago.

> I'm not telling anyone to adopt bad practices - at least its news to me
> that "stopping the merge of buggy crap" is now a bad practice.

Like I say, it's the "do it in the ARM tree" bit that's bad practice.

> > > And the proposed change set is buggy as hell - because we register things
> > > like 8250 devices at least four ways on the same x86 machine all of which
> > > could in theory occur in parallel.

> > Then you need to convince Greg of that.  The most recent set of patches
> > are exactly what he asked for.

> No there's an assumption that when someone asks for patches the proposed
> changes actually *work*. As Russell has demonstrated - the general
> deferral patches for the uart/tty layer are broken. The driver specific
> fixups in -next on the other hand appear to be fine providing the amba
> bus probe remains serialized (and trivially fixed if it doesn't).

So like I say discuss that with Greg, it's entirely reasonable for a
submitter to trust the maintainer on something like this so it's not
helpful to yell at them (or other people who happen to be working on
the same architecture for that matter).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]