of: Fix crash if an earlycon driver is not found

PATCH Applied Kevin Cernekee 5 participants 14 messages Nov 9, 2014 – Nov 12, 2014

Subsystems (3)

MIPS TTY layer and serial drivers Device tree

lore

Share & tools

lore

Kevin Cernekee

author

2014-11-09 08:55 UTC #0 lore raw

PATCH 1/2 of: Fix crash if an earlycon driver is not found

__earlycon_of_table_sentinel.compatible is a char[128], not a pointer, so it will never be NULL. Checking it against NULL causes the match loop to run past the end of the array, and eventually match a bogus entry, under the following conditions:

- Kernel command line specifies "earlycon" with no parameters
- DT has a stdout-path pointing to a UART node
- The UART driver doesn't use OF_EARLYCON_DECLARE (or maybe the console driver is compiled out)

Fix this by checking to see if match->compatible is a non-empty string.

Signed-off-by Kevin Cernekee <[email protected]>

Cc <[email protected]> # 3.16+

---
 drivers/of/fdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/of/fdt.c +1 -1

index d1ffca8..30e97bc 100644
@@ -773,7 +773,7 @@ int __init early_init_dt_scan_chosen_serial(void)
 	if (offset < 0)
 		return -ENODEV;
 
-	while (match->compatible) {
+	while (match->compatible[0]) {
 		unsigned long addr;
 		if (fdt_node_check_compatible(fdt, offset, match->compatible)) {
 			match++;

Signature · 1 line

2.1.1

Kevin Cernekee

author

2014-11-09 08:55 UTC #1 lore raw

PATCH 2/2 tty: serial: bcm63xx: Allow device nodes to be renamed to /dev/ttyBCM*

By default, bcm63xx_uart.c uses the standard 8250 device naming and major/minor numbers. There are at least two situations where this could be a problem:

1) Multiplatform kernels that need to support some chips that have 8250 UARTs and other chips that have bcm63xx UARTs.

2) Some older chips like BCM7125 have a mix of both UART types.

Add a new Kconfig option to tell the driver whether to register itself as ttyS or ttyBCM. By default it will retain the existing "ttyS" behavior to avoid surprises.

Signed-off-by Kevin Cernekee <[email protected]>

---
 drivers/tty/serial/Kconfig        | 11 +++++++++++
 drivers/tty/serial/bcm63xx_uart.c |  8 ++++++++
 2 files changed, 19 insertions(+)

drivers/tty/serial/Kconfig +11 -0

index fdd851e..c82cfd2 100644
@@ -1302,6 +1302,17 @@ config SERIAL_BCM63XX_CONSOLE
 	  If you have enabled the serial port on the BCM63xx CPU
 	  you can make it the console by answering Y to this option.
 
+config SERIAL_BCM63XX_TTYS
+	bool "Use ttyS[01] device nodes for bcm63xx_uart"
+	depends on SERIAL_BCM63XX
+	default y
+	help
+	  Say Y to name the serial ports /dev/ttyS0, ttyS1, ...
+	  This conflicts with the 8250 driver but may avoid breaking
+	  compatibility with existing init scripts.
+
+	  Say N to name the serial ports /dev/ttyBCM0, ttyBCM1, ...
+
 config SERIAL_GRLIB_GAISLER_APBUART
 	tristate "GRLIB APBUART serial support"
 	depends on OF && SPARC

drivers/tty/serial/bcm63xx_uart.c +8 -0

index e04e580..9f3dcc8 100644
@@ -751,7 +751,11 @@ static int bcm_console_setup(struct console *co, char *options)
 static struct uart_driver bcm_uart_driver;
 
 static struct console bcm63xx_console = {
+#ifdef CONFIG_SERIAL_BCM63XX_TTYS
 	.name		= "ttyS",
+#else
+	.name		= "ttyBCM",
+#endif
 	.write		= bcm_console_write,
 	.device		= uart_console_device,
 	.setup		= bcm_console_setup,

@@ -796,9 +800,13 @@ OF_EARLYCON_DECLARE(bcm63xx_uart, "brcm,bcm6345-uart", bcm_early_console_setup);
 static struct uart_driver bcm_uart_driver = {
 	.owner		= THIS_MODULE,
 	.driver_name	= "bcm63xx_uart",
+#ifdef CONFIG_SERIAL_BCM63XX_TTYS
 	.dev_name	= "ttyS",
 	.major		= TTY_MAJOR,
 	.minor		= 64,
+#else
+	.dev_name	= "ttyBCM",
+#endif
 	.nr		= BCM63XX_NR_UARTS,
 	.cons		= BCM63XX_CONSOLE,
 };

Signature · 1 line

2.1.1

Rob Herring

maintainer

2014-11-10 14:25 UTC #2 lore raw

Re: [PATCH 2/2] tty: serial: bcm63xx: Allow device nodes to be renamed to /dev/ttyBCM*

Quoted 4 lines · Kevin Cernekee

By default, bcm63xx_uart.c uses the standard 8250 device naming and major/minor numbers. There are at least two situations where this could be a problem:

Quoted 8 lines

1) Multiplatform kernels that need to support some chips that have 8250 UARTs and other chips that have bcm63xx UARTs.
2) Some older chips like BCM7125 have a mix of both UART types.
Add a new Kconfig option to tell the driver whether to register itself as ttyS or ttyBCM. By default it will retain the existing "ttyS" behavior to avoid surprises.

While I understand the desire to have stable names, this is the opposite direction we want to go. Per platform tty names complicates having a generic userspace. It is not so bad since most ARM platforms use ttyS or ttyAMA, but just think what the kernel and userspace side would look like if every single platform did this. We can't change everything to ttyS because the other names are already an ABI.

This can be solved with a udev rule to create sym links. Or if you just need to know which dev node is h/w uart X, you can get that from sysfs.

Rob

Quoted 61 lines

Signed-off-by Kevin Cernekee <[email protected]>

---
 drivers/tty/serial/Kconfig        | 11 +++++++++++
 drivers/tty/serial/bcm63xx_uart.c |  8 ++++++++
 2 files changed, 19 insertions(+)

drivers/tty/serial/Kconfig +11 -0

index fdd851e..c82cfd2 100644
@@ -1302,6 +1302,17 @@ config SERIAL_BCM63XX_CONSOLE
          If you have enabled the serial port on the BCM63xx CPU
          you can make it the console by answering Y to this option.
 
+config SERIAL_BCM63XX_TTYS
+       bool "Use ttyS[01] device nodes for bcm63xx_uart"
+       depends on SERIAL_BCM63XX
+       default y
+       help
+         Say Y to name the serial ports /dev/ttyS0, ttyS1, ...
+         This conflicts with the 8250 driver but may avoid breaking
+         compatibility with existing init scripts.
+
+         Say N to name the serial ports /dev/ttyBCM0, ttyBCM1, ...
+
 config SERIAL_GRLIB_GAISLER_APBUART
        tristate "GRLIB APBUART serial support"
        depends on OF && SPARC

drivers/tty/serial/bcm63xx_uart.c +8 -1

index e04e580..9f3dcc8 100644
@@ -751,7 +751,11 @@ static int bcm_console_setup(struct console *co, char *options)
 static struct uart_driver bcm_uart_driver;
 
 static struct console bcm63xx_console = {
+#ifdef CONFIG_SERIAL_BCM63XX_TTYS
        .name           = "ttyS",
+#else
+       .name           = "ttyBCM",
+#endif
        .write          = bcm_console_write,
        .device         = uart_console_device,
        .setup          = bcm_console_setup,

@@ -796,9 +800,13 @@ OF_EARLYCON_DECLARE(bcm63xx_uart, "brcm,bcm6345-uart", bcm_early_console_setup);
 static struct uart_driver bcm_uart_driver = {
        .owner          = THIS_MODULE,
        .driver_name    = "bcm63xx_uart",
+#ifdef CONFIG_SERIAL_BCM63XX_TTYS
        .dev_name       = "ttyS",
        .major          = TTY_MAJOR,
        .minor          = 64,
+#else
+       .dev_name       = "ttyBCM",
+#endif
        .nr             = BCM63XX_NR_UARTS,
        .cons           = BCM63XX_CONSOLE,
 };
--

2.1.1

Kevin Cernekee

author

2014-11-10 15:05 UTC #3 lore raw

Re: [PATCH 2/2] tty: serial: bcm63xx: Allow device nodes to be renamed to /dev/ttyBCM*

Quoted 14 lines · Rob Herring

Quoted 4 lines · Kevin Cernekee
By default, bcm63xx_uart.c uses the standard 8250 device naming and major/minor numbers. There are at least two situations where this could be a problem:
Quoted 8 lines
1) Multiplatform kernels that need to support some chips that have 8250 UARTs and other chips that have bcm63xx UARTs.
2) Some older chips like BCM7125 have a mix of both UART types.
Add a new Kconfig option to tell the driver whether to register itself as ttyS or ttyBCM. By default it will retain the existing "ttyS" behavior to avoid surprises.

Quoted 8 lines

While I understand the desire to have stable names, this is the opposite direction we want to go. Per platform tty names complicates having a generic userspace. It is not so bad since most ARM platforms use ttyS or ttyAMA, but just think what the kernel and userspace side would look like if every single platform did this. We can't change everything to ttyS because the other names are already an ABI.
This can be solved with a udev rule to create sym links.

Is it safe to register two console drivers named "ttyS" with the same major/minor numbers? Maybe there is a trick to making them coexist?

What I found is that everything worked fine on a system with bcm63xx_uart hardware until I enabled the 8250 driver in .config. At that point the drivers clashed and I had no serial output post-bootconsole. It didn't even get to userland before it failed. There are no DT entries mentioning 8250; the mere presence of the 8250 driver killed my console.

If this behavior is unexpected I can keep digging to find out what went wrong.

Quoted 3 lines

Or if you
just need to know which dev node is h/w uart X, you can get that from sysfs.

Right - I use busybox cttyhack in the init scripts to figure out the console name, so the same rootfs image can be used for both ttyS0 and ttyBCM0:

# Put a shell on the serial port
::respawn:/bin/cttyhack /bin/sh -l

Greg Kroah-Hartman

maintainer

2014-11-10 18:30 UTC #4 lore raw

Re: [PATCH 2/2] tty: serial: bcm63xx: Allow device nodes to be renamed to /dev/ttyBCM*

Quoted 22 lines · Kevin Cernekee

Quoted 21 lines · Rob Herring
Quoted 4 lines · Kevin Cernekee
By default, bcm63xx_uart.c uses the standard 8250 device naming and major/minor numbers. There are at least two situations where this could be a problem:
Quoted 8 lines
1) Multiplatform kernels that need to support some chips that have 8250 UARTs and other chips that have bcm63xx UARTs.
2) Some older chips like BCM7125 have a mix of both UART types.
Add a new Kconfig option to tell the driver whether to register itself as ttyS or ttyBCM. By default it will retain the existing "ttyS" behavior to avoid surprises.
While I understand the desire to have stable names, this is the opposite direction we want to go. Per platform tty names complicates having a generic userspace. It is not so bad since most ARM platforms use ttyS or ttyAMA, but just think what the kernel and userspace side would look like if every single platform did this. We can't change everything to ttyS because the other names are already an ABI.

Quoted 4 lines

Quoted 1 line
This can be solved with a udev rule to create sym links.
Is it safe to register two console drivers named "ttyS" with the same major/minor numbers?

Not at all, think about what you are asking for here.

Is the kernel allowed to register two block devices with the same major/minor numbers?

greg k-h

Kevin Cernekee

author

2014-11-10 19:10 UTC #5 lore raw

Re: [PATCH 2/2] tty: serial: bcm63xx: Allow device nodes to be renamed to /dev/ttyBCM*

Quoted 25 lines · Greg Kroah-Hartman

Quoted 24 lines · Kevin Cernekee
Quoted 22 lines · Rob Herring
Quoted 4 lines · Kevin Cernekee
By default, bcm63xx_uart.c uses the standard 8250 device naming and major/minor numbers. There are at least two situations where this could be a problem:
Quoted 8 lines
1) Multiplatform kernels that need to support some chips that have 8250 UARTs and other chips that have bcm63xx UARTs.
2) Some older chips like BCM7125 have a mix of both UART types.
Add a new Kconfig option to tell the driver whether to register itself as ttyS or ttyBCM. By default it will retain the existing "ttyS" behavior to avoid surprises.
While I understand the desire to have stable names, this is the opposite direction we want to go. Per platform tty names complicates having a generic userspace. It is not so bad since most ARM platforms use ttyS or ttyAMA, but just think what the kernel and userspace side would look like if every single platform did this. We can't change everything to ttyS because the other names are already an ABI.
This can be solved with a udev rule to create sym links.

Quoted 7 lines

Quoted 2 lines
Is it safe to register two console drivers named "ttyS" with the same major/minor numbers?
Not at all, think about what you are asking for here.
Is the kernel allowed to register two block devices with the same major/minor numbers?

So, is there a recommended solution for building a multiplatform kernel that includes more than one serial driver that wants to claim major 4, minor 64? Let's start with the easy case (#1 from above):
only one of the drivers actually has a DT entry and gets probed.

Browsing around drivers/tty/serial I identified several other drivers that use 4/64: apbuart, dz, ip22zilog, m32r_sio, mcf, mrst_max3110, pxa, sunhv, zs. Unfortunately none of these seem to be built as part of ARM's MULTI_V7 kernel.

My other idea was to keep using "ttyS" for bcm63xx_uart, but remove the major/minor numbers from the driver code so the kernel auto-assigns them. In this case I also need to set CONFIG_SERIAL_8250_RUNTIME_UARTS=0 to prevent the 8250 driver from creating ttyS[0-3] on its own, and then I'll need to retest the 8250 based platform to make sure it didn't break. Does that sound plausible?

Rob Herring

maintainer

2014-11-10 19:22 UTC #6 lore raw

Re: [PATCH 2/2] tty: serial: bcm63xx: Allow device nodes to be renamed to /dev/ttyBCM*

In reply to Kevin Cernekee

Quoted 22 lines · Kevin Cernekee

Quoted 21 lines · Rob Herring
Quoted 4 lines · Kevin Cernekee
By default, bcm63xx_uart.c uses the standard 8250 device naming and major/minor numbers. There are at least two situations where this could be a problem:
Quoted 8 lines
1) Multiplatform kernels that need to support some chips that have 8250 UARTs and other chips that have bcm63xx UARTs.
2) Some older chips like BCM7125 have a mix of both UART types.
Add a new Kconfig option to tell the driver whether to register itself as ttyS or ttyBCM. By default it will retain the existing "ttyS" behavior to avoid surprises.
While I understand the desire to have stable names, this is the opposite direction we want to go. Per platform tty names complicates having a generic userspace. It is not so bad since most ARM platforms use ttyS or ttyAMA, but just think what the kernel and userspace side would look like if every single platform did this. We can't change everything to ttyS because the other names are already an ABI.

Quoted 4 lines

Quoted 1 line
This can be solved with a udev rule to create sym links.
Is it safe to register two console drivers named "ttyS" with the same major/minor numbers? Maybe there is a trick to making them coexist?

No, but I think you can do dynamic minor numbers. I seem to recall this coming up with the Samsung UARTs a while back.

Rob

Quoted 19 lines

What I found is that everything worked fine on a system with bcm63xx_uart hardware until I enabled the 8250 driver in .config. At that point the drivers clashed and I had no serial output post-bootconsole. It didn't even get to userland before it failed. There are no DT entries mentioning 8250; the mere presence of the 8250 driver killed my console.
If this behavior is unexpected I can keep digging to find out what went wrong.
Quoted 3 lines
Or if you
just need to know which dev node is h/w uart X, you can get that from sysfs.
Right - I use busybox cttyhack in the init scripts to figure out the console name, so the same rootfs image can be used for both ttyS0 and ttyBCM0:
# Put a shell on the serial port
::respawn:/bin/cttyhack /bin/sh -l

Kevin Cernekee

author

2014-11-10 19:50 UTC #7 lore raw

Re: [PATCH 2/2] tty: serial: bcm63xx: Allow device nodes to be renamed to /dev/ttyBCM*

Quoted 7 lines · Rob Herring

Quoted 4 lines
Quoted 1 line
This can be solved with a udev rule to create sym links.
Is it safe to register two console drivers named "ttyS" with the same major/minor numbers? Maybe there is a trick to making them coexist?
No, but I think you can do dynamic minor numbers. I seem to recall this coming up with the Samsung UARTs a while back.

The other variations I've seen in the tree are:

nwpserial: ttySQ, major 4 minor 68 (not 64)

sunhv, sunsab, sunsu, sunzilog: set uart_driver->major to 4 but let uart_driver->minor default to 0

SERIAL_ATMEL_TTYAT: compile-time selectable between ttySn (4/64) and ttyATn (204/154). txx9 does something similar using SERIAL_TXX9_STDSERIAL.

A whole bunch of other SoC serial drivers use major 204 and a custom name like "ttyAL". Some of these show up in Documentation/devices.txt; others don't. ~3 drivers use 204/64 from the middle of the Altix assigned range.

What is the current best practice for new drivers?

Rob Herring

maintainer

2014-11-11 17:35 UTC #8 lore raw

Re: [PATCH 2/2] tty: serial: bcm63xx: Allow device nodes to be renamed to /dev/ttyBCM*

Quoted 20 lines · Kevin Cernekee

Quoted 7 lines · Rob Herring
Quoted 4 lines
Quoted 1 line
This can be solved with a udev rule to create sym links.
Is it safe to register two console drivers named "ttyS" with the same major/minor numbers? Maybe there is a trick to making them coexist?
No, but I think you can do dynamic minor numbers. I seem to recall this coming up with the Samsung UARTs a while back.
The other variations I've seen in the tree are:
nwpserial: ttySQ, major 4 minor 68 (not 64)
sunhv, sunsab, sunsu, sunzilog: set uart_driver->major to 4 but let uart_driver->minor default to 0
SERIAL_ATMEL_TTYAT: compile-time selectable between ttySn (4/64) and ttyATn (204/154). txx9 does something similar using SERIAL_TXX9_STDSERIAL.

Quoted 6 lines

A whole bunch of other SoC serial drivers use major 204 and a custom name like "ttyAL". Some of these show up in Documentation/devices.txt; others don't. ~3 drivers use 204/64 from the middle of the Altix assigned range.
What is the current best practice for new drivers?

I think it would be using dynamic numbering, but would be good to have others weigh in here. It looks like a dynamic major would solve your problem. See tty_register_driver. Also, there was a patch to make this the fallback behavior instead of an error[1], but it was never merged (and it's not clear why). This was the Samsung related change I was remembering.

Rob

[1] http://lists.linaro.org/pipermail/linaro-kernel/2014-January/010383.html

Kevin Cernekee

author

2014-11-11 19:50 UTC #9 lore raw

Re: [PATCH 2/2] tty: serial: bcm63xx: Allow device nodes to be renamed to /dev/ttyBCM*

Quoted 9 lines · Rob Herring

Quoted 1 line
What is the current best practice for new drivers?
I think it would be using dynamic numbering, but would be good to have others weigh in here. It looks like a dynamic major would solve your problem. See tty_register_driver. Also, there was a patch to make this the fallback behavior instead of an error[1], but it was never merged (and it's not clear why). This was the Samsung related change I was remembering.

Quoted 3 lines

Rob
[1] http://lists.linaro.org/pipermail/linaro-kernel/2014-January/010383.html

This doesn't seem to fix the coexistence problems with the 8250 driver. Perhaps it is time to write a simpler version of the 8250 driver geared toward embedded applications? Some items on my wishlist include:

- OF_EARLYCON support
- Native-endian and big-endian register support
- Move to a pure runtime-enumerated model and drop the ISA code
- Drop support for obsolete chips (16450, 8250) and quirks for obsolete PCs

It looks like sunsu, serial-tegra, omap-serial, and possibly others have already started down this path. Or I could just write a dedicated uart_bcm7xxx driver if that's easier...

Here are my results after grabbing V2 of Tushar's patch from the list archives and reverting the bcm63xx_uart driver to claim "ttyS" with major 4, minor 64:

- Building with CONFIG_SERIAL_8250_NR_UARTS=0 rendered the 8250-based
system unbootable (it dies with no output right after the bootconsole is disabled)

- Building with CONFIG_SERIAL_8250_NR_UARTS=1 rendered the
bcm63xx_uart-based system unbootable, with the following output:

Serial: 8250/16550 driver, 1 ports, IRQ sharing disabled Default device node (4:64) for ttyS is busy, using dynamic major number bcm63xx_uart 10000100.serial: ttyS0 at MMIO 0x10000100 (irq = 28, base_baud = 1562500) is a bcm63xx_uart
console [ttyS0] enabled
console [ttyS0] enabled
bootconsole [uart0] disabled
bootconsole [uart0] disabled

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x64/0xbc()
sysfs: cannot create duplicate filename '/class/tty/ttyS0'
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.18.0-rc4+ #384
Stack : 00000000 00000004 00000000 80075444 00000000 00000000 00000000 00000000
      809e5012 0000003a 00000001 00000000 00010000 87c282b8 8056dac4 805b9ce7
      00000001 00000000 809e39d8 87c282b8 87c6ae10 00000001 87c6ae10 804edeb0
      805c0000 80032984 00000000 00000000 80571d28 87c2da84 87c2da84 8056dac4
      00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
      ...
Call Trace:
[<8001a00c>] show_stack+0x64/0x7c
[<804eff94>] dump_stack+0xc8/0xfc
[<80032bd8>] warn_slowpath_common+0x7c/0xac
[<80032c68>] warn_slowpath_fmt+0x2c/0x38
[<8015c784>] sysfs_warn_dup+0x64/0xbc
[<8015cabc>] sysfs_do_create_link_sd.isra.2+0xec/0xf4
[<802c193c>] device_add+0x4b0/0x59c
[<802a452c>] tty_register_device_attr+0xe4/0x278
[<802b625c>] uart_add_one_port+0x344/0x474
[<802bbffc>] bcm_uart_probe+0x14c/0x240
[<802c6c20>] platform_drv_probe+0x54/0xc4
[<802c51bc>] really_probe+0xa0/0x2cc
[<802c5544>] __driver_attach+0xd0/0xd8
[<802c3660>] bus_for_each_dev+0x68/0xb0
[<802c4160>] bus_add_driver+0x168/0x230
[<802c5f6c>] driver_register+0x84/0x138
[<805e86ec>] bcm_uart_init+0x30/0x58
[<805d4ce0>] do_one_initcall+0x148/0x208
[<805d4f0c>] kernel_init_freeable+0x16c/0x228
[<804eb6dc>] kernel_init+0x10/0x100
[<80013ce8>] ret_from_kernel_thread+0x14/0x1c

---[ end trace dc92a8c272541296 ]---

bcm63xx_uart 10000100.serial: Cannot register tty device on line 0
libphy: Fixed MDIO Bus: probed
usbcore: registered new interface driver asix
usbcore: registered new interface driver ax88179_178a
usbcore: registered new interface driver cdc_ether
usbcore: registered new interface driver net1080
usbcore: registered new interface driver cdc_subset
usbcore: registered new interface driver zaurus
usbcore: registered new interface driver cdc_ncm
ehci_hcd: USB 2.0 'Enhanced' Host Controller (EHCI) Driver
ehci-platform: EHCI generic platform driver
ohci_hcd: USB 1.1 'Open' Host Controller (OHCI) Driver
ohci-platform: OHCI generic platform driver
usbcore: registered new interface driver usb-storage
TCP: cubic registered
NET: Registered protocol family 10
sit: IPv6 over IPv4 tunneling driver
NET: Registered protocol family 17
Freeing unused kernel memory: 4144K (805d4000 - 809e0000) starting pid 797, tty '': '/etc/init.d/rcS'
Mounting virtual filesystems
Starting mdev
Configuring lo interface
Configuring sit0 interface
Starting network services
starting pid 826, tty '': '/bin/cttyhack /bin/sh -l'
process '/bin/cttyhack /bin/sh -l' (pid 826) exited. Scheduling for restart. starting pid 828, tty '': '/bin/cttyhack /bin/sh -l'
process '/bin/cttyhack /bin/sh -l' (pid 828) exited. Scheduling for restart. starting pid 830, tty '': '/bin/cttyhack /bin/sh -l'

Rob Herring

maintainer

2014-11-10 14:14 UTC #10 lore raw

In reply to Kevin Cernekee

Quoted 10 lines · Kevin Cernekee

__earlycon_of_table_sentinel.compatible is a char[128], not a pointer, so it will never be NULL. Checking it against NULL causes the match loop to run past the end of the array, and eventually match a bogus entry, under the following conditions:
- Kernel command line specifies "earlycon" with no parameters
- DT has a stdout-path pointing to a UART node
- The UART driver doesn't use OF_EARLYCON_DECLARE (or maybe the console driver is compiled out)

Quoted 4 lines

Fix this by checking to see if match->compatible is a non-empty string.
Signed-off-by Kevin Cernekee <[email protected]>
Cc <[email protected]> # 3.16+

Thanks. I'll queue this up.

BTW, you should not add stable CC when submitting for review, but rather add a note for the maintainer to apply to stable. Only if a commit is in mainline already and not flagged for stable, then you send the patch with the stable tag to get the commit added to stable. It's a bit confusing...

Rob

Quoted 20 lines

---
 drivers/of/fdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/of/fdt.c +1 -2

index d1ffca8..30e97bc 100644
@@ -773,7 +773,7 @@ int __init early_init_dt_scan_chosen_serial(void)
        if (offset < 0)
                return -ENODEV;
 
-       while (match->compatible) {
+       while (match->compatible[0]) {
                unsigned long addr;
                if (fdt_node_check_compatible(fdt, offset, match->compatible)) {
                        match++;
--

2.1.1

Sergei Shtylyov

2014-11-10 19:01 UTC #11 lore raw

Quoted 15 lines · Rob Herring

Quoted 10 lines
__earlycon_of_table_sentinel.compatible is a char[128], not a pointer, so it will never be NULL. Checking it against NULL causes the match loop to run past the end of the array, and eventually match a bogus entry, under the following conditions:
- Kernel command line specifies "earlycon" with no parameters
- DT has a stdout-path pointing to a UART node
- The UART driver doesn't use OF_EARLYCON_DECLARE (or maybe the console driver is compiled out)
Quoted 4 lines
Fix this by checking to see if match->compatible is a non-empty string.
Signed-off-by Kevin Cernekee <[email protected]>
Cc <[email protected]> # 3.16+

Quoted 7 lines

Thanks. I'll queue this up.
BTW, you should not add stable CC when submitting for review, but rather add a note for the maintainer to apply to stable. Only if a commit is in mainline already and not flagged for stable, then you send the patch with the stable tag to get the commit added to stable. It's a bit confusing...

It's actually OK to tag the patch for stable (not really send to stable),
so that that list gets automatically notified when the comment lands in the mainline. Unless the maintainer doesn't have his own rules about stable patches (like e.g. DaveM). Do you have alike rules?

Quoted 1 line

Rob

WBR, Sergei

Grant Likely

2014-11-12 17:12 UTC #12 lore raw

In reply to Rob Herring

Quoted 16 lines · Rob Herring

Quoted 10 lines · Kevin Cernekee
__earlycon_of_table_sentinel.compatible is a char[128], not a pointer, so it will never be NULL. Checking it against NULL causes the match loop to run past the end of the array, and eventually match a bogus entry, under the following conditions:
- Kernel command line specifies "earlycon" with no parameters
- DT has a stdout-path pointing to a UART node
- The UART driver doesn't use OF_EARLYCON_DECLARE (or maybe the console driver is compiled out)
Quoted 4 lines
Fix this by checking to see if match->compatible is a non-empty string.
Signed-off-by Kevin Cernekee <[email protected]>
Cc <[email protected]> # 3.16+

Quoted 7 lines

Thanks. I'll queue this up.
BTW, you should not add stable CC when submitting for review, but rather add a note for the maintainer to apply to stable. Only if a commit is in mainline already and not flagged for stable, then you send the patch with the stable tag to get the commit added to stable. It's a bit confusing...

Oops, since you've picked it up I'll drop it from my tree. I'll let you send the pull req to Linus.

Grant Likely

Applied

2014-11-12 17:11 UTC #13 lore raw

In reply to Kevin Cernekee

Quoted 10 lines · Kevin Cernekee

__earlycon_of_table_sentinel.compatible is a char[128], not a pointer, so it will never be NULL. Checking it against NULL causes the match loop to run past the end of the array, and eventually match a bogus entry, under the following conditions:
- Kernel command line specifies "earlycon" with no parameters
- DT has a stdout-path pointing to a UART node
- The UART driver doesn't use OF_EARLYCON_DECLARE (or maybe the console driver is compiled out)

Quoted 4 lines

Fix this by checking to see if match->compatible is a non-empty string.
Signed-off-by Kevin Cernekee <[email protected]>
Cc <[email protected]> # 3.16+

Applied, thanks. Although a more robust fix would probably to be checking for the end address of the section. This is a safe fix though, so I'm picking it up.

Quoted 20 lines

---
 drivers/of/fdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/of/fdt.c +1 -1

index d1ffca8..30e97bc 100644
@@ -773,7 +773,7 @@ int __init early_init_dt_scan_chosen_serial(void)
 	if (offset < 0)
 		return -ENODEV;
 
-	while (match->compatible) {
+	while (match->compatible[0]) {
 		unsigned long addr;
 		if (fdt_node_check_compatible(fdt, offset, match->compatible)) {
 			match++;

Signature · 1 line

2.1.1