In a multi-platform scenario, the hard-coded major/minor numbers in
serial drivers may conflict with each other. A typical scenario is
observed with amba-pl011 and samsung-uart drivers, both of these
drivers use same set of major/minor numbers. If both of these drivers
are enabled, probe of samsung-uart driver fails because the desired
node is busy.
The issue is fixed by adding a fallback in driver core, so that we can
use dynamic major number in case device node allocation fails for
hard-coded major/minor number.
Signed-off-by: Tushar Behera <[email protected]>
---
Hi Greg,
This is my second attempt at getting this fixed. Let me know if you are
okay with this.
Initial approach to fix this problem was by forcing samsung-uart driver
to use dynamic major number, but it was rejected [1] because of possible
user-space breakage. Current approach falls back to dynamic major
number as a fallback in case of failure.
[1] https://lkml.org/lkml/2013/12/27/2
drivers/tty/tty_io.c | 19 ++++++++++++++++---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index c74a00a..0c692be 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3341,6 +3341,22 @@ int tty_register_driver(struct tty_driver *driver)
dev_t dev;
struct device *d;
+ if (driver->major) {
+ dev = MKDEV(driver->major, driver->minor_start);
+ error = register_chrdev_region(dev, driver->num, driver->name);
+ /* In case of error, fall back to dynamic allocation */
+ if (error < 0) {
+ pr_warn("Default device node (%d:%d) for %s is busy, using dynamic node\n",
+ driver->major, driver->minor_start,
+ driver->name);
+ driver->major = 0;
+ }
+ }
+
+ /*
+ * Don't replace the following check with an else to above if statement,
+ * as it may also be called as a fallback.
+ */
if (!driver->major) {
error = alloc_chrdev_region(&dev, driver->minor_start,
driver->num, driver->name);
@@ -3348,9 +3364,6 @@ int tty_register_driver(struct tty_driver *driver)
driver->major = MAJOR(dev);
driver->minor_start = MINOR(dev);
}
- } else {
- dev = MKDEV(driver->major, driver->minor_start);
- error = register_chrdev_region(dev, driver->num, driver->name);
}
if (error < 0)
goto err;
--
1.7.9.5
On Thu, Jan 16, 2014 at 10:33:22AM +0530, Tushar Behera wrote:
> In a multi-platform scenario, the hard-coded major/minor numbers in
> serial drivers may conflict with each other. A typical scenario is
> observed with amba-pl011 and samsung-uart drivers, both of these
> drivers use same set of major/minor numbers. If both of these drivers
> are enabled, probe of samsung-uart driver fails because the desired
> node is busy.
Why would both drivers ever be loaded at the same time? Don't they bind
to different hardware devices, and as such, never will be in the same
system?
The driver shouldn't be registering it's tty devices if the hardware
isn't present in the system, so how can this codepath ever happen?
> The issue is fixed by adding a fallback in driver core, so that we can
> use dynamic major number in case device node allocation fails for
> hard-coded major/minor number.
Did you test this out? You still get userspace breakage with it :(
> Signed-off-by: Tushar Behera <[email protected]>
> ---
> Hi Greg,
>
> This is my second attempt at getting this fixed. Let me know if you are
> okay with this.
>
> Initial approach to fix this problem was by forcing samsung-uart driver
> to use dynamic major number, but it was rejected [1] because of possible
> user-space breakage.
"possible"? Heh, no, that should be "real".
Sorry, but I think you are trying to fix a problem that never actually
happens in real life, or in valid configurations...
greg k-h
On Thu, Jan 16, 2014 at 08:18:41AM -0800, Greg KH wrote:
> On Thu, Jan 16, 2014 at 10:33:22AM +0530, Tushar Behera wrote:
> > In a multi-platform scenario, the hard-coded major/minor numbers in
> > serial drivers may conflict with each other. A typical scenario is
> > observed with amba-pl011 and samsung-uart drivers, both of these
> > drivers use same set of major/minor numbers. If both of these drivers
> > are enabled, probe of samsung-uart driver fails because the desired
> > node is busy.
> Why would both drivers ever be loaded at the same time? Don't they bind
> to different hardware devices, and as such, never will be in the same
> system?
No, the issue is happening because the drivers are registering things
at module load time and not at device instantiation time. While the
drivers won't actually be run together things like the multiplatform
defconfig and distro configs will build them in since people tend to
like to get serial early on.
> The driver shouldn't be registering it's tty devices if the hardware
> isn't present in the system, so how can this codepath ever happen?
A quick and unscientific poll of serial drivers seems to suggest that
uart_register_driver() is normally called when the module is loaded (if
it shouldn't be this is at least a very common error pattern). This in
turn calls tty_register_driver() which checks for duplicates in major
prior to any device being instantiated. Simply having the module
present is enough to cause problems.
> > The issue is fixed by adding a fallback in driver core, so that we can
> > use dynamic major number in case device node allocation fails for
> > hard-coded major/minor number.
> Did you test this out? You still get userspace breakage with it :(
Yes. I should put my hands up and say that this was my idea. The
theory is that any system using static allocation for a device shouldn't
be affected (modulo races, it's not perfect obviously), any currently
broken system with a userspace with a dynamic dev will start working and
any breakage is confined to drivers which duplicated major numbers (and
even then only on systems with static /dev).
The other solutions I can think of are moving tty_register_driver() to
device probe time, allowing tty_register_driver() to accept duplicate
majors and the complaining when the devices actually get instantiated
or changing major numbers where there is a duplicate which is guaranteed
to break at least some userspaces with static devices (which was the
original patch you complained about). The first two solutions look a
bit fun though perhaps they fall out more easily than I suspect.
On Thu, Jan 16, 2014 at 05:08:14PM +0000, Mark Brown wrote:
> On Thu, Jan 16, 2014 at 08:18:41AM -0800, Greg KH wrote:
> > On Thu, Jan 16, 2014 at 10:33:22AM +0530, Tushar Behera wrote:
> > > In a multi-platform scenario, the hard-coded major/minor numbers in
> > > serial drivers may conflict with each other. A typical scenario is
> > > observed with amba-pl011 and samsung-uart drivers, both of these
> > > drivers use same set of major/minor numbers. If both of these drivers
> > > are enabled, probe of samsung-uart driver fails because the desired
> > > node is busy.
>
> > Why would both drivers ever be loaded at the same time? Don't they bind
> > to different hardware devices, and as such, never will be in the same
> > system?
>
> No, the issue is happening because the drivers are registering things
> at module load time and not at device instantiation time.
That's a driver bug that could easily be fixed, instead of hacking up
the tty core :)
> While the drivers won't actually be run together things like the
> multiplatform defconfig and distro configs will build them in since
> people tend to like to get serial early on.
Build them into the kernel and not as modules?
> > The driver shouldn't be registering it's tty devices if the hardware
> > isn't present in the system, so how can this codepath ever happen?
>
> A quick and unscientific poll of serial drivers seems to suggest that
> uart_register_driver() is normally called when the module is loaded (if
> it shouldn't be this is at least a very common error pattern).
It's a common error pattern :)
> > > The issue is fixed by adding a fallback in driver core, so that we can
> > > use dynamic major number in case device node allocation fails for
> > > hard-coded major/minor number.
>
> > Did you test this out? You still get userspace breakage with it :(
>
> Yes. I should put my hands up and say that this was my idea. The
> theory is that any system using static allocation for a device shouldn't
> be affected (modulo races, it's not perfect obviously), any currently
> broken system with a userspace with a dynamic dev will start working and
> any breakage is confined to drivers which duplicated major numbers (and
> even then only on systems with static /dev).
>
> The other solutions I can think of are moving tty_register_driver() to
> device probe time, allowing tty_register_driver() to accept duplicate
> majors and the complaining when the devices actually get instantiated
> or changing major numbers where there is a duplicate which is guaranteed
> to break at least some userspaces with static devices (which was the
> original patch you complained about). The first two solutions look a
> bit fun though perhaps they fall out more easily than I suspect.
How about fixing the drivers as I mentioned above, to not register with
the uart or tty core until they know that their hardware is actually
present in the system? That would keep any tty core "hacks" from being
needed.
thanks,
greg k-h
On Thu, Jan 16, 2014 at 09:22:40AM -0800, Greg KH wrote:
> On Thu, Jan 16, 2014 at 05:08:14PM +0000, Mark Brown wrote:
> > No, the issue is happening because the drivers are registering things
> > at module load time and not at device instantiation time.
> That's a driver bug that could easily be fixed, instead of hacking up
> the tty core :)
Looking at the code it really, really looked intentional - in any case
that seems worthwhile, Tushar this looks like a way forwards?
> > While the drivers won't actually be run together things like the
> > multiplatform defconfig and distro configs will build them in since
> > people tend to like to get serial early on.
> Build them into the kernel and not as modules?
Yes. It's the common case for embedded stuff to just build everything
in so people tend to do it.
> > The other solutions I can think of are moving tty_register_driver() to
> > device probe time, allowing tty_register_driver() to accept duplicate
> > majors and the complaining when the devices actually get instantiated
> > or changing major numbers where there is a duplicate which is guaranteed
> > to break at least some userspaces with static devices (which was the
> > original patch you complained about). The first two solutions look a
> > bit fun though perhaps they fall out more easily than I suspect.
> How about fixing the drivers as I mentioned above, to not register with
> the uart or tty core until they know that their hardware is actually
> present in the system? That would keep any tty core "hacks" from being
> needed.
Certainly makes sense to me. I have to confess that looking at the code
it looked totally intentional and I remember dragons from the last time
I had to peer into that stuff.
On 16 January 2014 23:45, Mark Brown <[email protected]> wrote:
> On Thu, Jan 16, 2014 at 09:22:40AM -0800, Greg KH wrote:
>> On Thu, Jan 16, 2014 at 05:08:14PM +0000, Mark Brown wrote:
>
>> > No, the issue is happening because the drivers are registering things
>> > at module load time and not at device instantiation time.
>
>> That's a driver bug that could easily be fixed, instead of hacking up
>> the tty core :)
>
> Looking at the code it really, really looked intentional - in any case
> that seems worthwhile, Tushar this looks like a way forwards?
>
Ok. I will prepare a patch with this approach for amba-pl011 and
samsung-uart drivers. If that works well, I will extend that to other
serial drivers.
Thanks to both of you for your time.
--
Tushar Behera