* [PATCH v2 1/2] serial: pl010: Move uart_register_driver call to device probe
From: Sjoerd Simons @ 2017-04-20 12:13 UTC (permalink / raw)
To: linux-serial
Cc: linux-renesas-soc, Greg Kroah-Hartman, linux-kernel, Russell King,
Jiri Slaby
In-Reply-To: <20170420121301.382-1-sjoerd.simons@collabora.co.uk>
uart_register_driver call binds the driver to a specific device
node through tty_register_driver call. This should typically
happen during device probe call.
In a multiplatform scenario, it is possible that multiple serial
drivers are part of the kernel. Currently the driver registration fails
if multiple serial drivers with overlapping major/minor numbers are
included.
Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
---
Changes in v2:
- Add locking to prevent issues when two probes run in parallel
drivers/tty/serial/amba-pl010.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/drivers/tty/serial/amba-pl010.c b/drivers/tty/serial/amba-pl010.c
index f2f251075109..24180adb1cbb 100644
--- a/drivers/tty/serial/amba-pl010.c
+++ b/drivers/tty/serial/amba-pl010.c
@@ -697,6 +697,7 @@ static struct console amba_console = {
#define AMBA_CONSOLE NULL
#endif
+static DEFINE_MUTEX(amba_reg_lock);
static struct uart_driver amba_reg = {
.owner = THIS_MODULE,
.driver_name = "ttyAM",
@@ -749,6 +750,19 @@ static int pl010_probe(struct amba_device *dev, const struct amba_id *id)
amba_ports[i] = uap;
amba_set_drvdata(dev, uap);
+
+ mutex_lock(&amba_reg_lock);
+ if (!amba_reg.state) {
+ ret = uart_register_driver(&amba_reg);
+ if (ret < 0) {
+ mutex_unlock(&amba_reg_lock);
+ dev_err(uap->port.dev,
+ "Failed to register AMBA-PL010 driver\n");
+ return ret;
+ }
+ }
+ mutex_unlock(&amba_reg_lock);
+
ret = uart_add_one_port(&amba_reg, &uap->port);
if (ret)
amba_ports[i] = NULL;
@@ -760,12 +774,18 @@ static int pl010_remove(struct amba_device *dev)
{
struct uart_amba_port *uap = amba_get_drvdata(dev);
int i;
+ bool busy = false;
uart_remove_one_port(&amba_reg, &uap->port);
for (i = 0; i < ARRAY_SIZE(amba_ports); i++)
if (amba_ports[i] == uap)
amba_ports[i] = NULL;
+ else if (amba_ports[i])
+ busy = true;
+
+ if (!busy)
+ uart_unregister_driver(&amba_reg);
return 0;
}
@@ -816,23 +836,14 @@ static struct amba_driver pl010_driver = {
static int __init pl010_init(void)
{
- int ret;
-
printk(KERN_INFO "Serial: AMBA driver\n");
- ret = uart_register_driver(&amba_reg);
- if (ret == 0) {
- ret = amba_driver_register(&pl010_driver);
- if (ret)
- uart_unregister_driver(&amba_reg);
- }
- return ret;
+ return amba_driver_register(&pl010_driver);
}
static void __exit pl010_exit(void)
{
amba_driver_unregister(&pl010_driver);
- uart_unregister_driver(&amba_reg);
}
module_init(pl010_init);
--
2.11.0
^ permalink raw reply related
* [PATCH v2 0/2] Move uart_register_driver call to device probe for pl010 and sh-sci
From: Sjoerd Simons @ 2017-04-20 12:12 UTC (permalink / raw)
To: linux-serial
Cc: linux-renesas-soc, Greg Kroah-Hartman, linux-kernel, Russell King,
Jiri Slaby
When testing on a Renesas board with the PL010 serial driver enabled
serial output broke. Turns out the minor device numbers for both
drivers happen to overlap, causing whichever driver happened to be the
second one to register to fail.
The attached patches move the uart_register_driver call to probe time
for both drivers avoiding the issue.
Changes in v2:
- Add locking to prevent issues when two probes run in parallel
Sjoerd Simons (2):
serial: pl010: Move uart_register_driver call to device probe
serial: sh-sci: Move uart_register_driver call to device probe
drivers/tty/serial/amba-pl010.c | 31 +++++++++++++++++++++----------
drivers/tty/serial/sh-sci.c | 26 +++++++++++++++-----------
2 files changed, 36 insertions(+), 21 deletions(-)
--
2.11.0
^ permalink raw reply
* [PATCH] RFC: serial: core: Dynamic minor support
From: Sjoerd Simons @ 2017-04-20 12:03 UTC (permalink / raw)
To: Greg Kroah-Hartman, linux-serial
Cc: Geert Uytterhoeven, linux-kernel, Jiri Slaby
Quite a few serial port drivers use the "Low-density serial ports" major
which is rather packed. And ofcourse as time passes, things get slightly
less dense. For example the SCI serial port driver has an allocation of
4 minors, but in reality it supports up to 11 ports these days. Causing
it to overlap with the allocations for the AMBA PL010 serial port and
firmware console. While some other drivers are simply using minor
regions never allocated to them (both samsung and AMBA PL011 serial user
minor 64 and onwards while those are allocated to the Altix serial card
driver).
Cleary this doesn't scale.
Furthermore some other drivers rely upon the usage of a dynamic major
e.g. the tegra serial port driver. However as there is only a block of
20 major numbers reserved for dynamic assignment that isn't going to
scale either for multiplatform kernels (I can already easily build 18
different serial drivers on an arm64 kernel today).
This patch tries to address the issue adding a new to configuration
option which will let all serial ports use the "Low-density serial
ports" major and dynamically allocate minor ranges out of that rather
then using the driver defined areas. Given there are 20 bits worth of
minors available this should be enough for everyone... This option is
not enabled by default (for now) to not break setups with a static /dev.
This does not try to address device naming by making all serial ports be
ttyS<value> as that would first need a better way of deterministically
specifying serial console devices. While it would be an option to move
to consistent serial device naming while keeping the current console
names, having both not aligned would probably confuse everyone
massively.
Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
---
drivers/tty/serial/Kconfig | 10 ++++++++++
drivers/tty/serial/serial_core.c | 38 ++++++++++++++++++++++++++++++++++++++
include/linux/serial_core.h | 6 ++++++
3 files changed, 54 insertions(+)
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 6117ac8da48f..65a7261f3060 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -7,6 +7,16 @@ if TTY
menu "Serial drivers"
depends on HAS_IOMEM
+config SERIAL_DYNAMIC_MINORS
+ bool "Dynamic serial minors allocation"
+ help
+ If you say Y here the serial subsystem will use dynamic minors for all
+ serial devices, using the Low density serial port major. Note that
+ this requires the system to a non-static /dev to support a getty on
+ serial console.
+
+ If you are unsure about this, say N here.
+
config SERIAL_EARLYCON
bool
help
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 3fe56894974a..37014c70dd69 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -44,6 +44,32 @@
*/
static DEFINE_MUTEX(port_mutex);
+#ifdef CONFIG_SERIAL_DYNAMIC_MINORS
+/* List of uarts to allow for allocation of dynamic minor ranges*/
+LIST_HEAD(dynamic_uarts);
+DEFINE_MUTEX(dynamic_uarts_mutex);
+
+static void
+uart_setup_major_minor(struct uart_driver *drv)
+{
+ int start = 0;
+ struct uart_driver *d;
+
+ drv->major = LOW_DENSITY_UART_MAJOR;
+ mutex_lock(&dynamic_uarts_mutex);
+
+ list_for_each_entry(d, &dynamic_uarts, dynamic_uarts) {
+ if (start + drv->nr < d->minor)
+ break;
+ start = d->minor + d->nr;
+ }
+ list_add_tail(&drv->dynamic_uarts, &d->dynamic_uarts);
+ drv->minor = start;
+
+ mutex_unlock(&dynamic_uarts_mutex);
+}
+#endif
+
/*
* lockdep: port->lock is initialized in two places, but we
* want only one lock-class:
@@ -2458,6 +2484,11 @@ int uart_register_driver(struct uart_driver *drv)
BUG_ON(drv->state);
+#ifdef CONFIG_SERIAL_DYNAMIC_MINORS
+ INIT_LIST_HEAD(&drv->dynamic_uarts);
+ uart_setup_major_minor(drv);
+#endif
+
/*
* Maybe we should be using a slab cache for this, especially if
* we have a large number of ports to handle.
@@ -2527,6 +2558,13 @@ void uart_unregister_driver(struct uart_driver *drv)
put_tty_driver(p);
for (i = 0; i < drv->nr; i++)
tty_port_destroy(&drv->state[i].port);
+
+#ifdef CONFIG_SERIAL_DYNAMIC_MINORS
+ mutex_lock(&dynamic_uarts_mutex);
+ list_del(&drv->dynamic_uarts);
+ mutex_unlock(&dynamic_uarts_mutex);
+#endif
+
kfree(drv->state);
drv->state = NULL;
drv->tty_driver = NULL;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 58484fb35cc8..890bbefb3f90 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -31,6 +31,8 @@
#include <linux/sysrq.h>
#include <uapi/linux/serial_core.h>
+#define LOW_DENSITY_UART_MAJOR 204
+
#ifdef CONFIG_SERIAL_CORE_CONSOLE
#define uart_console(port) \
((port)->cons && (port)->cons->index == (port)->line)
@@ -313,6 +315,10 @@ struct uart_driver {
*/
struct uart_state *state;
struct tty_driver *tty_driver;
+
+#ifdef CONFIG_SERIAL_DYNAMIC_MINORS
+ struct list_head dynamic_uarts;
+#endif
};
void uart_write_wakeup(struct uart_port *port);
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 1/2] serial: imx: support enabling rs485 at boot time with DT option
From: Greg KH @ 2017-04-18 17:36 UTC (permalink / raw)
To: Alexey Ignatov; +Cc: linux-kernel, linux-serial, Uwe Kleine-König
In-Reply-To: <20170418165434.21252-1-lexszero@gmail.com>
On Tue, Apr 18, 2017 at 07:54:33PM +0300, Alexey Ignatov wrote:
> Signed-off-by: Alexey Ignatov <lexszero@gmail.com>
> ---
> drivers/tty/serial/imx.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
I can't take patches without any changelog text at all, sorry.
And you forgot to cc: the maintainer of the subsystem you are writing a
patch for, not a good way to get it accepted :)
Please use scripts/get_maintainer.pl to help you out.
thanks,
greg k-h
^ permalink raw reply
* [PATCH 2/2] serial: imx: allow enabling RX during TX operation with DT option
From: Alexey Ignatov @ 2017-04-18 16:54 UTC (permalink / raw)
To: linux-kernel, linux-serial; +Cc: Uwe Kleine-König, Alexey Ignatov
In-Reply-To: <20170418165434.21252-1-lexszero@gmail.com>
This commit changes the driver behaviour: new default is no RX during
TX when RS485 is also enabled, so boards requiring this should explicitly
enable rs485-rx-during-tx in their device trees.
Signed-off-by: Alexey Ignatov <lexszero@gmail.com>
---
drivers/tty/serial/imx.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 38c55c309953..a1d4ed71312f 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2050,6 +2050,10 @@ static int serial_imx_probe_dt(struct imx_port *sport,
if (of_property_read_bool(np, "linux,rs485-enabled-at-boot-time"))
sport->port.rs485.flags |= SER_RS485_ENABLED;
+
+ if (of_get_property(np, "rs485-rx-during-tx", NULL))
+ sport->port.rs485.flags |= SER_RS485_RX_DURING_TX;
+
return 0;
}
#else
@@ -2112,7 +2116,7 @@ static int serial_imx_probe(struct platform_device *pdev)
sport->port.ops = &imx_pops;
sport->port.rs485_config = imx_rs485_config;
sport->port.rs485.flags |=
- SER_RS485_RTS_ON_SEND | SER_RS485_RX_DURING_TX;
+ SER_RS485_RTS_ON_SEND;
sport->port.flags = UPF_BOOT_AUTOCONF;
init_timer(&sport->timer);
sport->timer.function = imx_timeout;
--
2.12.2
^ permalink raw reply related
* [PATCH 1/2] serial: imx: support enabling rs485 at boot time with DT option
From: Alexey Ignatov @ 2017-04-18 16:54 UTC (permalink / raw)
To: linux-kernel, linux-serial; +Cc: Uwe Kleine-König, Alexey Ignatov
Signed-off-by: Alexey Ignatov <lexszero@gmail.com>
---
drivers/tty/serial/imx.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index a70356dad1b7..38c55c309953 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2048,6 +2048,8 @@ static int serial_imx_probe_dt(struct imx_port *sport,
if (of_get_property(np, "fsl,dte-mode", NULL))
sport->dte_mode = 1;
+ if (of_property_read_bool(np, "linux,rs485-enabled-at-boot-time"))
+ sport->port.rs485.flags |= SER_RS485_ENABLED;
return 0;
}
#else
@@ -2109,7 +2111,7 @@ static int serial_imx_probe(struct platform_device *pdev)
sport->port.fifosize = 32;
sport->port.ops = &imx_pops;
sport->port.rs485_config = imx_rs485_config;
- sport->port.rs485.flags =
+ sport->port.rs485.flags |=
SER_RS485_RTS_ON_SEND | SER_RS485_RX_DURING_TX;
sport->port.flags = UPF_BOOT_AUTOCONF;
init_timer(&sport->timer);
--
2.12.2
^ permalink raw reply related
* Re: [PATCH v2] serial: 8250_early: Add earlycon support for Palmchip UART
From: Marc Gonzalez @ 2017-04-18 9:27 UTC (permalink / raw)
To: linux-serial
Cc: Greg Kroah-Hartman, Thibaud Cornic, Robin Murphy, Linux ARM,
Mason
In-Reply-To: <7a016ff6-08fc-2811-92e0-7c4603fa8586@sigmadesigns.com>
[ Trimming CC list to minimize noise ]
On 10/04/2017 11:47, Marc Gonzalez wrote:
> Define an OF early console for Palmchip UART, which can be enabled
> by passing "earlycon" on the boot command line.
>
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
> drivers/tty/serial/8250/8250_early.c | 24 ++++++++++++++++++++++++
> drivers/tty/serial/8250/8250_port.c | 4 ++--
> 2 files changed, 26 insertions(+), 2 deletions(-)
Hello Greg,
I'd really like to have earlycon support for this UART in v4.12
Do you see anything wrong with this patch, preventing it from landing?
Do you want me to move the function declarations to a header?
(If so, which one?)
Regards.
^ permalink raw reply
* Re: 8250: Possible race between console message vs DMA?
From: Vignesh R @ 2017-04-18 6:19 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-serial@vger.kernel.org, Peter Hurley, Greg Kroah-Hartman,
linux-omap@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <CAHp75VcCTgMrowhbRd_+B5=2o0utHKt=r=+zCb-NJ1_XVZ=uWA@mail.gmail.com>
On Tuesday 18 April 2017 11:46 AM, Andy Shevchenko wrote:
> On Mon, Apr 10, 2017 at 11:16 AM, Vignesh R <vigneshr@ti.com> wrote:
>> On Sunday 09 April 2017 04:37 PM, Andy Shevchenko wrote:
>>> On Fri, Apr 7, 2017 at 2:08 PM, Vignesh R <vigneshr@ti.com> wrote:
>>>> Hi All,
>>>>
>>>> I seem to be hitting a race condition using 8250_dma (and 8250_omap
>>>> specific dma) support:
>>>>
>>>> Kernel writes log messages to console via
>>>> serial8250_console_write()->serial8250_console_putchar() which directly
>>>> accesses UART_TX register with port->lock acquired.
>>>>
>>>> Now, if the same UART instance is being used by systemd/userspace,
>>>> characters are written to UART_TX register by serial8250_tx_chars(). The
>>>> concurrent access by serial8250_console_write() and
>>>> serial8250_tx_chars() is serialized by the use of port->lock spinlock
>>>> and hence there is no issue with` non DMA case.
>>>>
>>>> But when using DMA with 8250 UART, I see that port->lock is held before
>>>> scheduling of DMA TX transfer and released as soon as the transfer is
>>>> submitted. The lock is not held until the transfer actually completes
>>>> See,
>>>> uart_start()
>>>> ->serial8250_start_tx()->
>>>> __start_tx()
>>>> ->up->dma->tx_dma(up)
>>>> Or
>>>> __dma_tx_complete() in 8250_dma.c that acquires and releases port->lock
>>>> once TX DMA transfer is submitted in serial8250_tx_dma()
>>>>
>>>> So, when the port->lock is released, it is quite possible that DMA is
>>>> still transferring data to UART TX FIFO and UART FIFO might be almost full.
>>>> I see that when DMA is writing to UART TX FIFO,
>>>> serial8250_console_write() may also write kernel log messages to UART TX
>>>> FIFO(as port->lock is now free to be acquired), which is leading to
>>>> overflow and lose of data. serial8250_console_write() checks for
>>>> UART_LSR_THRE to check if Transmit hold register is empty but that may
>>>> not be enough as DMA might put data before CPU write.
>>>>
>>>> It seems that both DMA and CPU might simultaneously put data to UART
>>>> FIFO and lead to potential loss of data.
>>>> Is the expectation that UART instance used to print kernel log messages
>>>> is not intended to use DMA? Or am I missing something?
>>>>
>>>>
>>>> Any help appreciated!
>>>
>>> I have one patch in my tree for a long time already:
>>> https://bitbucket.org/andy-shev/linux/commits/9f86c648e53bd25b8ec374933764577b2a340468?at=topic/uart/rpm
>>
>> I had similar patch in mind. Do you plan to submit above patch to the
>> mailing list? You may also consider to add the issue I mentioned above
>> to the commit description. Thanks!
>
> Yes, I'm planning to do so, but be aware that OMAP has its own DMA
> glue layer and thus my patch doesn't affect it.
>
Yes, I am working on a patch for 8250_omap driver. Thanks!
--
Regards
Vignesh
^ permalink raw reply
* Re: 8250: Possible race between console message vs DMA?
From: Andy Shevchenko @ 2017-04-18 6:16 UTC (permalink / raw)
To: Vignesh R
Cc: linux-serial@vger.kernel.org, Peter Hurley, Greg Kroah-Hartman,
linux-omap@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <69f52038-f0e6-6f56-4f0b-842021152e2c@ti.com>
On Mon, Apr 10, 2017 at 11:16 AM, Vignesh R <vigneshr@ti.com> wrote:
> On Sunday 09 April 2017 04:37 PM, Andy Shevchenko wrote:
>> On Fri, Apr 7, 2017 at 2:08 PM, Vignesh R <vigneshr@ti.com> wrote:
>>> Hi All,
>>>
>>> I seem to be hitting a race condition using 8250_dma (and 8250_omap
>>> specific dma) support:
>>>
>>> Kernel writes log messages to console via
>>> serial8250_console_write()->serial8250_console_putchar() which directly
>>> accesses UART_TX register with port->lock acquired.
>>>
>>> Now, if the same UART instance is being used by systemd/userspace,
>>> characters are written to UART_TX register by serial8250_tx_chars(). The
>>> concurrent access by serial8250_console_write() and
>>> serial8250_tx_chars() is serialized by the use of port->lock spinlock
>>> and hence there is no issue with` non DMA case.
>>>
>>> But when using DMA with 8250 UART, I see that port->lock is held before
>>> scheduling of DMA TX transfer and released as soon as the transfer is
>>> submitted. The lock is not held until the transfer actually completes
>>> See,
>>> uart_start()
>>> ->serial8250_start_tx()->
>>> __start_tx()
>>> ->up->dma->tx_dma(up)
>>> Or
>>> __dma_tx_complete() in 8250_dma.c that acquires and releases port->lock
>>> once TX DMA transfer is submitted in serial8250_tx_dma()
>>>
>>> So, when the port->lock is released, it is quite possible that DMA is
>>> still transferring data to UART TX FIFO and UART FIFO might be almost full.
>>> I see that when DMA is writing to UART TX FIFO,
>>> serial8250_console_write() may also write kernel log messages to UART TX
>>> FIFO(as port->lock is now free to be acquired), which is leading to
>>> overflow and lose of data. serial8250_console_write() checks for
>>> UART_LSR_THRE to check if Transmit hold register is empty but that may
>>> not be enough as DMA might put data before CPU write.
>>>
>>> It seems that both DMA and CPU might simultaneously put data to UART
>>> FIFO and lead to potential loss of data.
>>> Is the expectation that UART instance used to print kernel log messages
>>> is not intended to use DMA? Or am I missing something?
>>>
>>>
>>> Any help appreciated!
>>
>> I have one patch in my tree for a long time already:
>> https://bitbucket.org/andy-shev/linux/commits/9f86c648e53bd25b8ec374933764577b2a340468?at=topic/uart/rpm
>
> I had similar patch in mind. Do you plan to submit above patch to the
> mailing list? You may also consider to add the issue I mentioned above
> to the commit description. Thanks!
Yes, I'm planning to do so, but be aware that OMAP has its own DMA
glue layer and thus my patch doesn't affect it.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [GIT PULL] TTY fix for 4.11-rc7
From: Greg KH @ 2017-04-16 7:56 UTC (permalink / raw)
To: Linus Torvalds, Jiri Slaby; +Cc: Andrew Morton, linux-kernel, linux-serial
The following changes since commit a71c9a1c779f2499fb2afc0553e543f18aff6edf:
Linux 4.11-rc5 (2017-04-02 17:23:54 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/ tags/tty-4.11-rc7
for you to fetch changes up to a8983d01f9b7d600fbdb3916899edf395954cc83:
Revert "tty: don't panic on OOM in tty_set_ldisc()" (2017-04-14 10:59:56 +0200)
----------------------------------------------------------------
TTY fix for 4.11-rc7
Here is a single tty core revert for a patch that was reported to cause
problems. The original issue is one that we have lived with for
decades, so trying to scramble to fix the fix in time for 4.11-final
does not make sense due to the fragility of the tty ldisc layer. Just
reverting it makes sense for now.
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
----------------------------------------------------------------
Greg Kroah-Hartman (1):
Revert "tty: don't panic on OOM in tty_set_ldisc()"
drivers/tty/tty_ldisc.c | 85 +++++++++++++++++++++++++++++++++++++++----------
1 file changed, 69 insertions(+), 16 deletions(-)
^ permalink raw reply
* Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4
From: Greg KH @ 2017-04-14 12:30 UTC (permalink / raw)
To: Vegard Nossum
Cc: Linus Torvalds, Dmitry Vyukov, Jiri Slaby, Andrew Morton, LKML,
linux-serial
In-Reply-To: <CAOMGZ=G9t7ih=ydr=fkKn6cbgu5-qbdQs7XChmn=d598H95QVA@mail.gmail.com>
On Fri, Apr 14, 2017 at 11:41:26AM +0200, Vegard Nossum wrote:
> On 13 April 2017 at 20:34, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Thu, Apr 13, 2017 at 09:07:40AM -0700, Linus Torvalds wrote:
> >> On Thu, Apr 13, 2017 at 3:50 AM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
> >> >
> >> > I've bisected a syzkaller crash down to this commit
> >> > (5362544bebe85071188dd9e479b5a5040841c895). The crash is:
> >> >
> >> > [ 25.137552] BUG: unable to handle kernel paging request at 0000000000002280
> >> > [ 25.137579] IP: mutex_lock_interruptible+0xb/0x30
> >>
> >> It would seem to be the
> >>
> >> if (mutex_lock_interruptible(&ldata->atomic_read_lock))
> >>
> >> call in n_tty_read(), the offset is about right for a NULL 'ldata'
> >> pointer (it's a big structure, it has a couple of character buffers of
> >> size N_TTY_BUF_SIZE).
> >>
> >> I don't see the obvious fix, so I suspect at this point we should just
> >> revert, as that commit seems to introduce worse problems that it is
> >> supposed to fix. Greg?
> >
> > Unless Dmitry has a better idea, I will just revert it and send you the
> > pull request in a day or so.
>
> I don't think we need to rush a revert, I'd hope there's a way to fix
> it properly.
For this late in the release cycle, for something as complex as tty
ldisc handling, for an issue that has been present for over a decade,
the safest thing right now is to go back to the old well-known code by
applying a revert :)
> So the original problem is that the vmalloc() in n_tty_open() can
> fail, and that will panic in tty_set_ldisc()/tty_ldisc_restore()
> because of its unwillingness to proceed if the tty doesn't have an
> ldisc.
>
> Dmitry fixed this by allowing tty->ldisc == NULL in the case of memory
> allocation failure as we can see from the comment in tty_set_ldisc().
>
> Unfortunately, it would appear that some other bits of code do not
> like tty->ldisc == NULL (other than the crash in this thread, I saw
> 2-3 similar crashes in other functions, e.g. poll()). I see two
> possibilities:
>
> 1) make other code handle tty->ldisc == NULL.
>
> 2) don't close/free the old ldisc until the new one has been
> successfully created/initialised/opened/attached to the tty, and
> return an error to userspace if changing it failed.
>
> I'm leaning towards #2 as the more obviously correct fix, it makes
> tty_set_ldisc() transactional, the fix seems limited in scope to
> tty_set_ldisc() itself, and we don't need to make every other bit of
> code that uses tty->ldisc handle the NULL case.
That sounds reasonable to me, care to work on a patch for this?
thanks,
greg k-h
^ permalink raw reply
* Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4
From: Vegard Nossum @ 2017-04-14 9:41 UTC (permalink / raw)
To: Greg KH
Cc: Linus Torvalds, Dmitry Vyukov, Jiri Slaby, Andrew Morton, LKML,
linux-serial
In-Reply-To: <20170413183403.GA16022@kroah.com>
On 13 April 2017 at 20:34, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Apr 13, 2017 at 09:07:40AM -0700, Linus Torvalds wrote:
>> On Thu, Apr 13, 2017 at 3:50 AM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
>> >
>> > I've bisected a syzkaller crash down to this commit
>> > (5362544bebe85071188dd9e479b5a5040841c895). The crash is:
>> >
>> > [ 25.137552] BUG: unable to handle kernel paging request at 0000000000002280
>> > [ 25.137579] IP: mutex_lock_interruptible+0xb/0x30
>>
>> It would seem to be the
>>
>> if (mutex_lock_interruptible(&ldata->atomic_read_lock))
>>
>> call in n_tty_read(), the offset is about right for a NULL 'ldata'
>> pointer (it's a big structure, it has a couple of character buffers of
>> size N_TTY_BUF_SIZE).
>>
>> I don't see the obvious fix, so I suspect at this point we should just
>> revert, as that commit seems to introduce worse problems that it is
>> supposed to fix. Greg?
>
> Unless Dmitry has a better idea, I will just revert it and send you the
> pull request in a day or so.
I don't think we need to rush a revert, I'd hope there's a way to fix
it properly.
So the original problem is that the vmalloc() in n_tty_open() can
fail, and that will panic in tty_set_ldisc()/tty_ldisc_restore()
because of its unwillingness to proceed if the tty doesn't have an
ldisc.
Dmitry fixed this by allowing tty->ldisc == NULL in the case of memory
allocation failure as we can see from the comment in tty_set_ldisc().
Unfortunately, it would appear that some other bits of code do not
like tty->ldisc == NULL (other than the crash in this thread, I saw
2-3 similar crashes in other functions, e.g. poll()). I see two
possibilities:
1) make other code handle tty->ldisc == NULL.
2) don't close/free the old ldisc until the new one has been
successfully created/initialised/opened/attached to the tty, and
return an error to userspace if changing it failed.
I'm leaning towards #2 as the more obviously correct fix, it makes
tty_set_ldisc() transactional, the fix seems limited in scope to
tty_set_ldisc() itself, and we don't need to make every other bit of
code that uses tty->ldisc handle the NULL case.
Vegard
^ permalink raw reply
* Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4
From: Greg KH @ 2017-04-13 18:34 UTC (permalink / raw)
To: Linus Torvalds
Cc: Vegard Nossum, Dmitry Vyukov, Jiri Slaby, Andrew Morton, LKML,
linux-serial
In-Reply-To: <CA+55aFwgiAmy4Gw4RosbNVBJmOG-WQoZKOd76Ziuqx5CzPFMZQ@mail.gmail.com>
On Thu, Apr 13, 2017 at 09:07:40AM -0700, Linus Torvalds wrote:
> On Thu, Apr 13, 2017 at 3:50 AM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
> >
> > I've bisected a syzkaller crash down to this commit
> > (5362544bebe85071188dd9e479b5a5040841c895). The crash is:
> >
> > [ 25.137552] BUG: unable to handle kernel paging request at 0000000000002280
> > [ 25.137579] IP: mutex_lock_interruptible+0xb/0x30
>
> It would seem to be the
>
> if (mutex_lock_interruptible(&ldata->atomic_read_lock))
>
> call in n_tty_read(), the offset is about right for a NULL 'ldata'
> pointer (it's a big structure, it has a couple of character buffers of
> size N_TTY_BUF_SIZE).
>
> I don't see the obvious fix, so I suspect at this point we should just
> revert, as that commit seems to introduce worse problems that it is
> supposed to fix. Greg?
Unless Dmitry has a better idea, I will just revert it and send you the
pull request in a day or so.
thanks,
greg k-h
^ permalink raw reply
* Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4
From: Linus Torvalds @ 2017-04-13 16:07 UTC (permalink / raw)
To: Vegard Nossum
Cc: Greg KH, Dmitry Vyukov, Jiri Slaby, Andrew Morton, LKML,
linux-serial
In-Reply-To: <CAOMGZ=F8xdUHnZaDo18DkkrYgacx6y8Cm=f8-8xnuvPupepnfg@mail.gmail.com>
On Thu, Apr 13, 2017 at 3:50 AM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
>
> I've bisected a syzkaller crash down to this commit
> (5362544bebe85071188dd9e479b5a5040841c895). The crash is:
>
> [ 25.137552] BUG: unable to handle kernel paging request at 0000000000002280
> [ 25.137579] IP: mutex_lock_interruptible+0xb/0x30
It would seem to be the
if (mutex_lock_interruptible(&ldata->atomic_read_lock))
call in n_tty_read(), the offset is about right for a NULL 'ldata'
pointer (it's a big structure, it has a couple of character buffers of
size N_TTY_BUF_SIZE).
I don't see the obvious fix, so I suspect at this point we should just
revert, as that commit seems to introduce worse problems that it is
supposed to fix. Greg?
Linus
^ permalink raw reply
* Re: [PATCH] tty: pl011: use "qdf2400_e44" as the earlycon name for QDF2400 E44
From: Shanker Donthineni @ 2017-04-13 14:35 UTC (permalink / raw)
To: Timur Tabi, linux-serial, Greg Kroah-Hartman, linux-arm-kernel
In-Reply-To: <1492091708-30593-1-git-send-email-timur@codeaurora.org>
Hi Timur,
On 04/13/2017 08:55 AM, Timur Tabi wrote:
> Define a new early console name for Qualcomm Datacenter Technologies
> QDF2400 SOCs affected by erratum 44, instead of piggy-backing on "pl011".
> Previously, to enable traditional (non-SPCR) earlycon, the documentation
> said to specify "earlycon=pl011,<address>,qdf2400_e44", but the code was
> broken and this didn't actually work.
>
> So instead, the method for specifying the E44 work-around with traditional
> earlycon is "earlycon=qdf2400_e44,<address>". Both methods of earlycon
> are now enabled with the same function.
>
> Fixes: e53e597fd4c4 ("tty: pl011: fix earlycon work-around for QDF2400 erratum 44")
> Signed-off-by: Timur Tabi <timur@codeaurora.org>
Tested-by: Shanker Donthineni <shankerd@codeaurora.org>
> ---
> drivers/tty/serial/amba-pl011.c | 31 +++++++++++++++++++++++--------
> 1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 2783539..0875f7d 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -2470,19 +2470,34 @@ static int __init pl011_early_console_setup(struct earlycon_device *device,
> if (!device->port.membase)
> return -ENODEV;
>
> - /* On QDF2400 SOCs affected by Erratum 44, the "qdf2400_e44" must
> - * also be specified, e.g. "earlycon=pl011,<address>,qdf2400_e44".
> - */
> - if (!strcmp(device->options, "qdf2400_e44"))
> - device->con->write = qdf2400_e44_early_write;
> - else
> - device->con->write = pl011_early_write;
> + device->con->write = pl011_early_write;
>
> return 0;
> }
> OF_EARLYCON_DECLARE(pl011, "arm,pl011", pl011_early_console_setup);
> OF_EARLYCON_DECLARE(pl011, "arm,sbsa-uart", pl011_early_console_setup);
> -EARLYCON_DECLARE(qdf2400_e44, pl011_early_console_setup);
> +
> +/*
> + * On Qualcomm Datacenter Technologies QDF2400 SOCs affected by
> + * Erratum 44, traditional earlycon can be enabled by specifying
> + * "earlycon=qdf2400_e44,<address>". Any options are ignored.
> + *
> + * Alternatively, you can just specify "earlycon", and the early console
> + * will be enabled with the information from the SPCR table. In this
> + * case, the SPCR code will detect the need for the E44 work-around,
> + * and set the console name to "qdf2400_e44".
> + */
> +static int __init
> +qdf2400_e44_early_console_setup(struct earlycon_device *device,
> + const char *opt)
> +{
> + if (!device->port.membase)
> + return -ENODEV;
> +
> + device->con->write = qdf2400_e44_early_write;
> + return 0;
> +}
> +EARLYCON_DECLARE(qdf2400_e44, qdf2400_e44_early_console_setup);
>
> #else
> #define AMBA_CONSOLE NULL
--
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* [PATCH] tty: pl011: use "qdf2400_e44" as the earlycon name for QDF2400 E44
From: Timur Tabi @ 2017-04-13 13:55 UTC (permalink / raw)
To: linux-serial, Greg Kroah-Hartman, Shanker Donthineni,
linux-arm-kernel
Define a new early console name for Qualcomm Datacenter Technologies
QDF2400 SOCs affected by erratum 44, instead of piggy-backing on "pl011".
Previously, to enable traditional (non-SPCR) earlycon, the documentation
said to specify "earlycon=pl011,<address>,qdf2400_e44", but the code was
broken and this didn't actually work.
So instead, the method for specifying the E44 work-around with traditional
earlycon is "earlycon=qdf2400_e44,<address>". Both methods of earlycon
are now enabled with the same function.
Fixes: e53e597fd4c4 ("tty: pl011: fix earlycon work-around for QDF2400 erratum 44")
Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
drivers/tty/serial/amba-pl011.c | 31 +++++++++++++++++++++++--------
1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 2783539..0875f7d 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2470,19 +2470,34 @@ static int __init pl011_early_console_setup(struct earlycon_device *device,
if (!device->port.membase)
return -ENODEV;
- /* On QDF2400 SOCs affected by Erratum 44, the "qdf2400_e44" must
- * also be specified, e.g. "earlycon=pl011,<address>,qdf2400_e44".
- */
- if (!strcmp(device->options, "qdf2400_e44"))
- device->con->write = qdf2400_e44_early_write;
- else
- device->con->write = pl011_early_write;
+ device->con->write = pl011_early_write;
return 0;
}
OF_EARLYCON_DECLARE(pl011, "arm,pl011", pl011_early_console_setup);
OF_EARLYCON_DECLARE(pl011, "arm,sbsa-uart", pl011_early_console_setup);
-EARLYCON_DECLARE(qdf2400_e44, pl011_early_console_setup);
+
+/*
+ * On Qualcomm Datacenter Technologies QDF2400 SOCs affected by
+ * Erratum 44, traditional earlycon can be enabled by specifying
+ * "earlycon=qdf2400_e44,<address>". Any options are ignored.
+ *
+ * Alternatively, you can just specify "earlycon", and the early console
+ * will be enabled with the information from the SPCR table. In this
+ * case, the SPCR code will detect the need for the E44 work-around,
+ * and set the console name to "qdf2400_e44".
+ */
+static int __init
+qdf2400_e44_early_console_setup(struct earlycon_device *device,
+ const char *opt)
+{
+ if (!device->port.membase)
+ return -ENODEV;
+
+ device->con->write = qdf2400_e44_early_write;
+ return 0;
+}
+EARLYCON_DECLARE(qdf2400_e44, qdf2400_e44_early_console_setup);
#else
#define AMBA_CONSOLE NULL
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply related
* Re: [PATCH v2] serial: 8250_early: Add earlycon support for Palmchip UART
From: Mason @ 2017-04-13 13:22 UTC (permalink / raw)
To: linux-serial, Greg Kroah-Hartman
Cc: Marc Gonzalez, Peter Hurley, Russell King, Andreas Farber,
Jean Delvare, Rob Herring, Mans Rullgard, Jiri Slaby,
Masahiro Yamada, Vineet Gupta, Scott Wood, Robin Murphy,
Thibaud Cornic, Linux ARM, LKML
In-Reply-To: <7a016ff6-08fc-2811-92e0-7c4603fa8586@sigmadesigns.com>
On 10/04/2017 11:47, Marc Gonzalez wrote:
> @@ -172,3 +179,20 @@ OF_EARLYCON_DECLARE(omap8250, "ti,omap3-uart", early_omap8250_setup);
> OF_EARLYCON_DECLARE(omap8250, "ti,omap4-uart", early_omap8250_setup);
>
> #endif
> +
> +#ifdef CONFIG_SERIAL_8250_RT288X
> +
> +unsigned int au_serial_in(struct uart_port *p, int offset);
> +void au_serial_out(struct uart_port *p, int offset, int value);
Hmmm, I'm thinking that putting declarations in a .c file might
not be a very popular decision... ?
Would there be a header, shared by 8250_early.c and 8250_port.c
where it might be appropriate to declare au_serial_in/out?
> +static int __init early_au_setup(struct earlycon_device *dev, const char *opt)
> +{
> + dev->port.serial_in = au_serial_in;
> + dev->port.serial_out = au_serial_out;
> + dev->port.iotype = UPIO_AU;
> + dev->con->write = early_serial8250_write;
> + return 0;
> +}
> +OF_EARLYCON_DECLARE(palmchip, "ralink,rt2880-uart", early_au_setup);
> +
> +#endif
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 080d5a59d0a7..1f08d22d1a80 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -313,7 +313,7 @@ static const s8 au_io_out_map[8] = {
> -1, /* UART_SCR (unmapped) */
> };
>
> -static unsigned int au_serial_in(struct uart_port *p, int offset)
> +unsigned int au_serial_in(struct uart_port *p, int offset)
> {
> if (offset >= ARRAY_SIZE(au_io_in_map))
> return UINT_MAX;
> @@ -323,7 +323,7 @@ static unsigned int au_serial_in(struct uart_port *p, int offset)
> return __raw_readl(p->membase + (offset << p->regshift));
> }
>
> -static void au_serial_out(struct uart_port *p, int offset, int value)
> +void au_serial_out(struct uart_port *p, int offset, int value)
> {
> if (offset >= ARRAY_SIZE(au_io_out_map))
> return;
>
^ permalink raw reply
* Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4
From: Vegard Nossum @ 2017-04-13 10:50 UTC (permalink / raw)
To: Greg KH, Dmitry Vyukov
Cc: Linus Torvalds, Jiri Slaby, Andrew Morton, LKML, linux-serial
In-Reply-To: <20170326110432.GA9241@kroah.com>
On 26 March 2017 at 13:04, Greg KH <gregkh@linuxfoundation.org> wrote:
> The following changes since commit 4495c08e84729385774601b5146d51d9e5849f81:
>
> Linux 4.11-rc2 (2017-03-12 14:47:08 -0700)
>
> are available in the git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/ tags/tty-4.11-rc4
>
> for you to fetch changes up to a4a3e061149f09c075f108b6f1cf04d9739a6bc2:
>
> tty: fix data race in tty_ldisc_ref_wait() (2017-03-17 14:07:10 +0900)
>
> ----------------------------------------------------------------
> TTY/Serial driver fixes for 4.11-rc4
>
> Here are some tty and serial driver fixes for 4.11-rc4. One of these
> fix a long-standing issue in the ldisc code that was found by Dmitry
> Vyukov with his great fuzzing work. The other fixes resolve other
> reported issues, and there is one revert of a patch in 4.11-rc1 that
> wasn't correct.
>
> All of these have been in linux-next for a while with no reported
> issues.
>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> ----------------------------------------------------------------
> Aleksey Makarov (1):
> Revert "tty: serial: pl011: add ttyAMA for matching pl011 console"
>
> Dmitry Vyukov (2):
> tty: don't panic on OOM in tty_set_ldisc()
I've bisected a syzkaller crash down to this commit
(5362544bebe85071188dd9e479b5a5040841c895). The crash is:
[ 25.137552] BUG: unable to handle kernel paging request at 0000000000002280
[ 25.137579] IP: mutex_lock_interruptible+0xb/0x30
[ 25.137589] PGD 3b0c067
[ 25.137593] PUD 3911067
[ 25.137597] PMD 0
[ 25.137601]
[ 25.137611] Oops: 0002 [#1] PREEMPT SMP KASAN
[ 25.137624] CPU: 1 PID: 3690 Comm: a.out Not tainted 4.11.0-rc2+ #145
[ 25.137631] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
[ 25.137639] task: ffff880003b96400 task.stack: ffff880004e98000
[ 25.137651] RIP: 0010:mutex_lock_interruptible+0xb/0x30
[ 25.137657] RSP: 0018:ffff880004e9fae0 EFLAGS: 00010246
[ 25.137668] RAX: 0000000000000000 RBX: ffff880004e6c000 RCX: ffffffff817bb2a9
[ 25.137675] RDX: ffff880003b96400 RSI: 0000000000000015 RDI: 0000000000002280
[ 25.137696] RBP: ffff880004e9fca0 R08: 0000000000000003 R09: 0000000000000002
[ 25.137703] R10: 0000000000000002 R11: ffffed0000c23fe9 R12: ffff880004e6c000
[ 25.137710] R13: 0000000080045430 R14: ffff880004bac900 R15: ffff880004bacb60
[ 25.137720] FS: 00007f7cac233700(0000) GS:ffff880006100000(0000)
knlGS:0000000000000000
[ 25.137727] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 25.137733] CR2: 0000000000002280 CR3: 0000000003b67000 CR4: 00000000000006e0
[ 25.137746] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 25.137752] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 25.137755] Call Trace:
[ 25.137769] ? n_tty_read+0x15f/0xc70
[ 25.137783] ? preempt_count_add+0xb2/0xe0
[ 25.137793] ? n_tty_flush_buffer+0x90/0x90
[ 25.137806] ? wait_woken+0x100/0x100
[ 25.137817] tty_read+0xd8/0x140
[ 25.137830] __vfs_read+0xd1/0x320
[ 25.137842] ? do_sendfile+0x6c0/0x6c0
[ 25.137853] ? __fsnotify_update_child_dentry_flags+0x30/0x30
[ 25.137864] ? selinux_file_permission+0x1c0/0x210
[ 25.137873] ? __fsnotify_parent+0x27/0x130
[ 25.137882] ? security_file_permission+0xce/0xf0
[ 25.137893] ? rw_verify_area+0x73/0x140
[ 25.137904] vfs_read+0xba/0x1b0
[ 25.137915] SyS_read+0xa0/0x120
[ 25.137926] ? vfs_write+0x260/0x260
[ 25.137938] ? preempt_count_sub+0x13/0xd0
[ 25.137949] entry_SYSCALL_64_fastpath+0x1a/0xa9
[ 25.137957] RIP: 0033:0x7f7caf61351d
[ 25.137963] RSP: 002b:00007f7cac232f20 EFLAGS: 00000293 ORIG_RAX:
0000000000000000
[ 25.137974] RAX: ffffffffffffffda RBX: 00007f7cac233700 RCX: 00007f7caf61351d
[ 25.137980] RDX: 000000000000003e RSI: 0000000080045430 RDI: 0000000000000004
[ 25.137987] RBP: 00007fffb4f21250 R08: 00007f7cac233700 R09: 00007f7cac233700
[ 25.137993] R10: 00007f7cac2339d0 R11: 0000000000000293 R12: 0000000000000000
[ 25.137999] R13: 00007fffb4f2124f R14: 00007f7cac2339c0 R15: 0000000000000000
[ 25.138002] Code: c7 43 20 00 00 00 00 48 89 df e8 91 ff ff ff 5b
41 5c 5d c3 83 e8 01 41 89 44 24 10 eb e1 66 90 65 48 8b 14 25 40 54
01 00 31 c0 <f0> 48 0f b1 17 48 85 c0 74 0a 55 48 89 e5 e8 e2 f4 ff ff
5d f3
[ 25.138218] RIP: mutex_lock_interruptible+0xb/0x30 RSP: ffff880004e9fae0
[ 25.138221] CR2: 0000000000002280
[ 25.138301] ---[ end trace 242fd54c56b177b4 ]---
The syzkaller reproducer is:
# {Threaded:true Collide:true Repeat:true Procs:1 Sandbox:setuid Repro:false}
mmap(&(0x7f0000000000/0x9f000)=nil, (0x9f000), 0x3, 0x32,
0xffffffffffffffff, 0x0)
r0 = openat$ptmx(0xffffffffffffff9c,
&(0x7f0000001000-0xa)="2f6465762f70746d7800", 0x201, 0x0)
ioctl$TIOCSPTLCK(r0, 0x40045431, &(0x7f000009a000)=0x0)
r1 = syz_open_pts(r0, 0x0)
read(r1, &(0x7f0000028000-0x86)="0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
0x3e)
ioctl$TIOCSETD(r1, 0x5423, &(0x7f000009f000-0x4)=0x10080000001)
It takes 10-50 seconds to reproduce, but you can reduce it to ~2
seconds by opening /dev/ptmx only once in each process.
I've verified that reverting the commit from latest mainline makes the
crash go away.
Vegard
^ permalink raw reply
* Re: [PATCHv3 00/10] Nokia H4+ support
From: Sebastian Reichel @ 2017-04-13 0:26 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Greg Kroah-Hartman, Gustavo F. Padovan, Johan Hedberg,
Samuel Thibault, Pavel Machek, Tony Lindgren, Jiri Slaby,
Mark Rutland, open list:BLUETOOTH DRIVERS,
linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
David S. Miller, Rob Herring
In-Reply-To: <C16140C4-0264-411B-9058-0B9013AAC82F-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 3591 bytes --]
Hi Marcel,
On Wed, Apr 12, 2017 at 10:19:21PM +0200, Marcel Holtmann wrote:
> Hi Sebastian,
>
> >>>>>>>> Here is PATCHv3 for the Nokia bluetooth patchset. I addressed all comments from
> >>>>>>>> Rob and Pavel regarding the serdev patches and dropped the *.dts patches, since
> >>>>>>>> they were queued by Tony. I also changed the patch order, so that the serdev
> >>>>>>>> patches come first. All of them have Acked-by from Rob, so I think it makes
> >>>>>>>> sense to merge them to serdev subsystem (now) and provide an immutable branch
> >>>>>>>> for the bluetooth subsystem.
> >>>>>>>
> >>>>>>> Greg doesn't read cover letters generally and since the serdev patches
> >>>>>>> are Cc rather than To him, he's probably not planning to pick them up.
> >>>>>>
> >>>>>> I wonder actually if we should merge all of these via bluetooth-next
> >>>>>> tree with proper Ack from Greg. However it would be good to also get
> >>>>>> buy in from Dave for merging this ultimately through net-next.
> >>>>>
> >>>>> I don't really care where it goes. I can take the whole thing in my
> >>>>> tty/serial tree now if no one objects and I get an ack from the relevant
> >>>>> maintainers {hint...}
> >>>>
> >>>> I think it is better if it goes thru BT tree. I have another driver
> >>>> converted that is dependent on this series. There's a couple other
> >>>> serdev changes on the list too, but this shouldn't depend on them.
> >>>
> >>> Is this waiting for something, or could it be queued to
> >>> bluetooth-next then? It would be nice to finally have
> >>> this in 4.12 :)
> >>
> >> I would prefer if we can get an ACK from Greg. Then I merge it through the bluetooth-next tree.
> >
> > Sorry thought this was coming through mine:
> > Acked-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> >
> > Merge away!
>
> so I have applied patches 1-8 to bluetooth-next tree.
>
> The last 2 I left out since they do cause build issues on non-DT
> platforms. We need to be able to build the driver on all platforms
> so that sanity compile checks happen all the time.
>
> CC drivers/bluetooth/hci_nokia.o
> drivers/bluetooth/hci_nokia.c:802:34: error: array type has incomplete element type ‘struct of_device_id’
> static const struct of_device_id nokia_bluetooth_of_match[] = {
> ^~~~~~~~~~~~~~~~~~~~~~~~
> drivers/bluetooth/hci_nokia.c:803:4: error: field name not in record or union initializer
> { .compatible = "nokia,h4p-bluetooth", },
> ^
> drivers/bluetooth/hci_nokia.c:803:4: note: (near initialization for ‘nokia_bluetooth_of_match’)
> drivers/bluetooth/hci_nokia.c:815:21: error: implicit declaration of function ‘of_match_ptr’ [-Werror=implicit-function-declaration]
> .of_match_table = of_match_ptr(nokia_bluetooth_of_match),
> ^~~~~~~~~~~~
> drivers/bluetooth/hci_nokia.c:802:34: warning: ‘nokia_bluetooth_of_match’ defined but not used [-Wunused-variable]
> static const struct of_device_id nokia_bluetooth_of_match[] = {
> ^~~~~~~~~~~~~~~~~~~~~~~~
Building without CONFIG_OF should work already. Note, that its
actually enabled in your build, since nokia_bluetooth_of_match
is guarded by "#ifdef CONFIG_OF". The actual problem is, that
<linux/of.h> is not included in your build. Looks like it was
implicitly included in my configurations, so I didn't notice.
I will send PATCHv4 with the added include and includes sorted
alphabetically.
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCHv3 00/10] Nokia H4+ support
From: Marcel Holtmann @ 2017-04-12 20:19 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Sebastian Reichel, Gustavo F. Padovan, Johan Hedberg,
Samuel Thibault, Pavel Machek, Tony Lindgren, Jiri Slaby,
Mark Rutland, open list:BLUETOOTH DRIVERS,
linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
David S. Miller, Rob Herring
In-Reply-To: <20170411140637.GA4388-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
Hi Sebastian,
>>>>>>>> Here is PATCHv3 for the Nokia bluetooth patchset. I addressed all comments from
>>>>>>>> Rob and Pavel regarding the serdev patches and dropped the *.dts patches, since
>>>>>>>> they were queued by Tony. I also changed the patch order, so that the serdev
>>>>>>>> patches come first. All of them have Acked-by from Rob, so I think it makes
>>>>>>>> sense to merge them to serdev subsystem (now) and provide an immutable branch
>>>>>>>> for the bluetooth subsystem.
>>>>>>>
>>>>>>> Greg doesn't read cover letters generally and since the serdev patches
>>>>>>> are Cc rather than To him, he's probably not planning to pick them up.
>>>>>>
>>>>>> I wonder actually if we should merge all of these via bluetooth-next
>>>>>> tree with proper Ack from Greg. However it would be good to also get
>>>>>> buy in from Dave for merging this ultimately through net-next.
>>>>>
>>>>> I don't really care where it goes. I can take the whole thing in my
>>>>> tty/serial tree now if no one objects and I get an ack from the relevant
>>>>> maintainers {hint...}
>>>>
>>>> I think it is better if it goes thru BT tree. I have another driver
>>>> converted that is dependent on this series. There's a couple other
>>>> serdev changes on the list too, but this shouldn't depend on them.
>>>
>>> Is this waiting for something, or could it be queued to
>>> bluetooth-next then? It would be nice to finally have
>>> this in 4.12 :)
>>
>> I would prefer if we can get an ACK from Greg. Then I merge it through the bluetooth-next tree.
>
> Sorry thought this was coming through mine:
> Acked-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
>
> Merge away!
so I have applied patches 1-8 to bluetooth-next tree.
The last 2 I left out since they do cause build issues on non-DT platforms. We need to be able to build the driver on all platforms so that sanity compile checks happen all the time.
CC drivers/bluetooth/hci_nokia.o
drivers/bluetooth/hci_nokia.c:802:34: error: array type has incomplete element type ‘struct of_device_id’
static const struct of_device_id nokia_bluetooth_of_match[] = {
^~~~~~~~~~~~~~~~~~~~~~~~
drivers/bluetooth/hci_nokia.c:803:4: error: field name not in record or union initializer
{ .compatible = "nokia,h4p-bluetooth", },
^
drivers/bluetooth/hci_nokia.c:803:4: note: (near initialization for ‘nokia_bluetooth_of_match’)
drivers/bluetooth/hci_nokia.c:815:21: error: implicit declaration of function ‘of_match_ptr’ [-Werror=implicit-function-declaration]
.of_match_table = of_match_ptr(nokia_bluetooth_of_match),
^~~~~~~~~~~~
drivers/bluetooth/hci_nokia.c:802:34: warning: ‘nokia_bluetooth_of_match’ defined but not used [-Wunused-variable]
static const struct of_device_id nokia_bluetooth_of_match[] = {
^~~~~~~~~~~~~~~~~~~~~~~~
Regards
Marcel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/4] Revert "tty_port: register tty ports with serdev bus"
From: Johan Hovold @ 2017-04-12 14:39 UTC (permalink / raw)
To: Rob Herring; +Cc: Johan Hovold, linux-kernel, linux-serial
In-Reply-To: <CAL_JsqJhGDuaPwBH9z0xenr+u=wBp8X9WgDWAm8LuXxg6tA6mA@mail.gmail.com>
On Tue, Apr 11, 2017 at 08:53:32PM -0500, Rob Herring wrote:
> On Tue, Apr 11, 2017 at 12:07 PM, Johan Hovold <johan@kernel.org> wrote:
> > This reverts commit 8ee3fde047589dc9c201251f07d0ca1dc776feca.
> >
> > The new serdev bus hooked into the tty layer in
> > tty_port_register_device() by registering a serdev controller instead of
> > a tty device whenever a serdev client is present, and by deregistering
> > the controller in the tty-port destructor. This is broken in several
> > ways:
>
> Just curious, how are you testing this? greybus?
By unbinding a platform device from its serial driver, and by using some
instrumentation code in USB serial.
> > Firstly, it leads to a NULL-pointer dereference whenever a tty driver
> > later deregisters its devices as no corresponding character device will
> > exist.
> >
> > Secondly, far from every tty driver uses tty-port refcounting (e.g.
> > serial core) so the serdev devices might never be deregistered or
> > deallocated.
> >
> > Thirdly, deregistering at tty-port destruction is too late as the
> > underlying device and structures may be long gone by then. A port is not
> > released before an open tty device is closed, something which a
> > registered serdev client can prevent from ever happening. A driver
> > callback while the device is gone typically also leads to crashes.
> >
> > Many tty drivers even keep their ports around until the driver is
> > unloaded (e.g. serial core), something which even if a late callback
> > never happens, leads to leaks if a device is unbound from its driver and
> > is later rebound.
>
> Isn't this something we want to fix? i.e. everything done in
> probe/release rather than in init/exit.
It keeps some buffers allocated for a while longer than necessary, sure.
But there's quite a few drivers to change and for (non-hotpluggable)
serial drivers it doesn't make that much of difference as I assume
unbinding is mostly done for development purposes.
But we definitely need to make sure all drivers deregisters their
tty/serdev devices before starting to release resources.
> > The right solution here is to add a new tty_port_unregister_device()
> > helper and to never call tty_device_unregister() whenever the port has
> > been claimed by serdev, but since this requires modifying just about
> > every tty driver (and multiple subsystems) it will need to be done
> > incrementally.
> >
> > Reverting the offending patch is the first step in fixing the broken
> > lifetime assumptions. A follow-up patch will add a new pair of
> > tty-device registration helpers, which a vetted tty driver can use to
> > support serdev (initially serial core). When every tty driver uses the
> > serdev helpers (at least for deregistration), we can add serdev
> > registration to tty_port_register_device() again.
>
> Reverting for 4.11 or not probably isn't that important. There aren't
> any users. I guess if you have a DT with a device added, then you
> could still have some problems.
True, but it only takes a DT child node (with a compatible string) to
trigger, and it's also the overhead added for all users of
tty_port_register_device() (e.g. cdc-acm) mentioned below. And we'd also
avoid enabling something in 4.11 which is then again disabled in 4.12
(for most tty drivers).
> > Note that this also fixes another issue with serdev, which currently
> > allocates and registers a serdev controller for every tty device
> > registered using tty_port_device_register() only to immediately
> > deregister and deallocate it when the corresponding OF node or serdev
> > child node is missing. This should be addressed before enabling serdev
> > for hot-pluggable buses.
>
> Yeah, hot-plugging is definitely not supported ATM. Though folks are
> already asking about it. We need to figure out how to switch between a
> cdev and serdev.
Yeah, we need to give hotplugging some thought so we don't paint
ourselves into a corner here.
> Generally, the series looks good to me. Thanks for finding and fixing
> these issues. I'll give it a spin soon.
Thanks,
Johan
^ permalink raw reply
* Re: [PATCH] serial: imx: Enable RTSD only when needed
From: Fabio Estevam @ 2017-04-12 13:38 UTC (permalink / raw)
To: Romain Perier
Cc: Greg Kroah-Hartman, linux-serial@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Nandor Han
In-Reply-To: <20170412133015.23185-1-romain.perier@collabora.com>
On Wed, Apr 12, 2017 at 10:30 AM, Romain Perier
<romain.perier@collabora.com> wrote:
> From: Nandor Han <nandor.han@ge.com>
>
> Currently, this IRQ is always enabled. Some devices might mux these pins
> to other I/Os, like I2C. This could lead to spurious interrupts.
>
> This commit makes this IRQ optional, by using the field have_rtscts.
>
> Signed-off-by: Nandor Han <nandor.han@ge.com>
> Signed-off-by: Romain Perier <romain.perier@collabora.com>
Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
^ permalink raw reply
* [PATCH] serial: imx: Enable RTSD only when needed
From: Romain Perier @ 2017-04-12 13:30 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Romain Perier, linux-arm-kernel, linux-serial, Nandor Han
From: Nandor Han <nandor.han@ge.com>
Currently, this IRQ is always enabled. Some devices might mux these pins
to other I/Os, like I2C. This could lead to spurious interrupts.
This commit makes this IRQ optional, by using the field have_rtscts.
Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
drivers/tty/serial/imx.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index e3e152cb..6dd674f 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1294,7 +1294,9 @@ static int imx_startup(struct uart_port *port)
imx_enable_dma(sport);
temp = readl(sport->port.membase + UCR1);
- temp |= UCR1_RRDYEN | UCR1_RTSDEN | UCR1_UARTEN;
+ temp |= UCR1_RRDYEN | UCR1_UARTEN;
+ if (sport->have_rtscts)
+ temp |= UCR1_RTSDEN;
writel(temp, sport->port.membase + UCR1);
--
2.9.3
^ permalink raw reply related
* Re: [PATCH v9 3/3] printk: fix double printing with earlycon
From: Aleksey Makarov @ 2017-04-12 6:24 UTC (permalink / raw)
To: Petr Mladek
Cc: linux-serial, linux-kernel, Sudeep Holla, Greg Kroah-Hartman,
Peter Hurley, Jiri Slaby, Robin Murphy, Steven Rostedt,
Nair, Jayachandran, Sergey Senozhatsky
In-Reply-To: <20170411074325.GC3452@pathway.suse.cz>
On 04/11/2017 10:43 AM, Petr Mladek wrote:
> On Mon 2017-04-10 21:00:35, Aleksey Makarov wrote:
>>
>>
>> On 04/10/2017 05:22 PM, Petr Mladek wrote:
>>> On Wed 2017-04-05 23:20:00, Aleksey Makarov wrote:
[..]
>> Thank you for review. Can you (or anybody else) ACK it?
>> I am going to resend the whole series without those empty lines.
>> May I add your Acked-by:?
>
> Sure. Feel free to use:
>
> Acked-by: Petr Mladek <pmladek@suse.com>
>
> The meaning of the tags is a bit unclear. Acked-by means that
> the maintainer agrees with the idea. But it does not necessarily
> means that she reviewed the code in details. I agree with the idea
> and did the review, so I used the Reviewed-by tag.
>
> Also you do not need to resend the patchset just because the two
> empty lines. Sergey agrees. I will wait day or two and push
> all three patches into the printk.git if nobody complains
> in the meantime. I could remove the two empty lines when doing so.
Ok, then I will not resend it.
> Thanks a lot for the fix and patience.
Thank you for reviewing and acking it.
Aleksey Makarov
^ permalink raw reply
* [PATCH 4/4] serial: enable serdev support
From: Johan Hovold @ 2017-04-11 17:07 UTC (permalink / raw)
To: Rob Herring, Greg Kroah-Hartman
Cc: Jiri Slaby, linux-serial, linux-kernel, Johan Hovold
In-Reply-To: <20170411170731.4085-1-johan@kernel.org>
Enable serdev support by using the new device-registration helpers.
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/tty/serial/serial_core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 3fe56894974a..9f5eb0ced359 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2785,7 +2785,7 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
* Register the port whether it's detected or not. This allows
* setserial to be used to alter this port's parameters.
*/
- tty_dev = tty_port_register_device_attr(port, drv->tty_driver,
+ tty_dev = tty_port_register_device_attr_serdev(port, drv->tty_driver,
uport->line, uport->dev, port, uport->tty_groups);
if (likely(!IS_ERR(tty_dev))) {
device_set_wakeup_capable(tty_dev, 1);
@@ -2848,7 +2848,7 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
/*
* Remove the devices from the tty layer
*/
- tty_unregister_device(drv->tty_driver, uport->line);
+ tty_port_unregister_device(port, drv->tty_driver, uport->line);
tty = tty_port_tty_get(port);
if (tty) {
--
2.12.2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox