Give fsl_lpuart some love and add break, loopback and sysrq support. While
at it, some errors were noticed, which are also fixed in this series.
The sysrq support was tested on both interrupt driven and DMA based
transfers on the 32bit LPUART.
Michael Walle (8):
serial: fsl_lpuart: don't modify arbitrary data on lpuart32
serial: fsl_lpuart: use UARTDATA_MASK macro
serial: fsl_lpuart: don't restore interrupt state in ISR
serial: fsl_lpuart: handle break and make sysrq work
serial: fsl_lpuart: remove RTSCTS handling from get_mctrl()
serial: fsl_lpuart: remove manual RTSCTS control from 8-bit LPUART
serial: fsl_lpuart: add loopback support
serial: fsl_lpuart: disable DMA for console and fix sysrq
drivers/tty/serial/fsl_lpuart.c | 126 +++++++++++++++++---------------
1 file changed, 69 insertions(+), 57 deletions(-)
--
2.20.1
lpuart_rx_dma_startup() is used for both the 8 bit and the 32 bit
version of the LPUART. Modify the UARTCR only for the 8 bit version.
Fixes: f4eef224a09f ("serial: fsl_lpuart: add sysrq support when using dma")
Signed-off-by: Michael Walle <[email protected]>
---
drivers/tty/serial/fsl_lpuart.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 794035041744..fbf2e4d2d22b 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1625,7 +1625,7 @@ static void lpuart_rx_dma_startup(struct lpuart_port *sport)
sport->lpuart_dma_rx_use = true;
rx_dma_timer_init(sport);
- if (sport->port.has_sysrq) {
+ if (sport->port.has_sysrq && !lpuart_is_32(sport)) {
cr3 = readb(sport->port.membase + UARTCR3);
cr3 |= UARTCR3_FEIE;
writeb(cr3, sport->port.membase + UARTCR3);
--
2.20.1
Use the corresponding macro instead of the magic number. While at it,
drop the useless cast to "unsigned char".
Signed-off-by: Michael Walle <[email protected]>
---
drivers/tty/serial/fsl_lpuart.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index fbf2e4d2d22b..b76ddc0d8edc 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -928,9 +928,9 @@ static void lpuart32_rxint(struct lpuart_port *sport)
*/
sr = lpuart32_read(&sport->port, UARTSTAT);
rx = lpuart32_read(&sport->port, UARTDATA);
- rx &= 0x3ff;
+ rx &= UARTDATA_MASK;
- if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx))
+ if (uart_handle_sysrq_char(&sport->port, rx))
continue;
if (sr & (UARTSTAT_PE | UARTSTAT_OR | UARTSTAT_FE)) {
--
2.20.1
Since commit 81e2073c175b ("genirq: Disable interrupts for force
threaded handlers") interrupt handlers that are not explicitly requested
as threaded are always called with interrupts disabled and there is no
need to save the interrupt state when taking the port lock.
This is a preparation for sysrq handling which uses
uart_unlock_and_check_sysrq();
Signed-off-by: Michael Walle <[email protected]>
---
drivers/tty/serial/fsl_lpuart.c | 22 ++++++++--------------
1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index b76ddc0d8edc..37e02d992c0b 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -824,21 +824,18 @@ static unsigned int lpuart32_tx_empty(struct uart_port *port)
static void lpuart_txint(struct lpuart_port *sport)
{
- unsigned long flags;
-
- spin_lock_irqsave(&sport->port.lock, flags);
+ spin_lock(&sport->port.lock);
lpuart_transmit_buffer(sport);
- spin_unlock_irqrestore(&sport->port.lock, flags);
+ spin_unlock(&sport->port.lock);
}
static void lpuart_rxint(struct lpuart_port *sport)
{
unsigned int flg, ignored = 0, overrun = 0;
struct tty_port *port = &sport->port.state->port;
- unsigned long flags;
unsigned char rx, sr;
- spin_lock_irqsave(&sport->port.lock, flags);
+ spin_lock(&sport->port.lock);
while (!(readb(sport->port.membase + UARTSFIFO) & UARTSFIFO_RXEMPT)) {
flg = TTY_NORMAL;
@@ -896,28 +893,25 @@ static void lpuart_rxint(struct lpuart_port *sport)
writeb(UARTSFIFO_RXOF, sport->port.membase + UARTSFIFO);
}
- spin_unlock_irqrestore(&sport->port.lock, flags);
+ spin_unlock(&sport->port.lock);
tty_flip_buffer_push(port);
}
static void lpuart32_txint(struct lpuart_port *sport)
{
- unsigned long flags;
-
- spin_lock_irqsave(&sport->port.lock, flags);
+ spin_lock(&sport->port.lock);
lpuart32_transmit_buffer(sport);
- spin_unlock_irqrestore(&sport->port.lock, flags);
+ spin_unlock(&sport->port.lock);
}
static void lpuart32_rxint(struct lpuart_port *sport)
{
unsigned int flg, ignored = 0;
struct tty_port *port = &sport->port.state->port;
- unsigned long flags;
unsigned long rx, sr;
- spin_lock_irqsave(&sport->port.lock, flags);
+ spin_lock(&sport->port.lock);
while (!(lpuart32_read(&sport->port, UARTFIFO) & UARTFIFO_RXEMPT)) {
flg = TTY_NORMAL;
@@ -965,7 +959,7 @@ static void lpuart32_rxint(struct lpuart_port *sport)
}
out:
- spin_unlock_irqrestore(&sport->port.lock, flags);
+ spin_unlock(&sport->port.lock);
tty_flip_buffer_push(port);
}
--
2.20.1
Although there is already (broken) sysrq characters handling, a break
condition was never detected. There is also a possible deadlock because
we might call handle_sysrq() while still holding the port lock.
Add support for break detection and use the proper
uart_unlock_and_check_sysrq() to defer calling handle_sysrq().
Signed-off-by: Michael Walle <[email protected]>
---
drivers/tty/serial/fsl_lpuart.c | 36 ++++++++++++++++++++++++---------
1 file changed, 26 insertions(+), 10 deletions(-)
diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 37e02d992c0b..0a578ad31a19 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -910,6 +910,7 @@ static void lpuart32_rxint(struct lpuart_port *sport)
unsigned int flg, ignored = 0;
struct tty_port *port = &sport->port.state->port;
unsigned long rx, sr;
+ bool is_break;
spin_lock(&sport->port.lock);
@@ -924,14 +925,27 @@ static void lpuart32_rxint(struct lpuart_port *sport)
rx = lpuart32_read(&sport->port, UARTDATA);
rx &= UARTDATA_MASK;
- if (uart_handle_sysrq_char(&sport->port, rx))
+ /*
+ * The LPUART can't distinguish between a break and a framing error,
+ * thus we assume it is a break if the received data is zero.
+ */
+ is_break = sr & UARTSTAT_FE && !rx;
+
+ if (is_break && uart_handle_break(&sport->port))
+ continue;
+
+ if (uart_prepare_sysrq_char(&sport->port, rx))
continue;
if (sr & (UARTSTAT_PE | UARTSTAT_OR | UARTSTAT_FE)) {
- if (sr & UARTSTAT_PE)
- sport->port.icount.parity++;
- else if (sr & UARTSTAT_FE)
+ if (sr & UARTSTAT_PE) {
+ if (is_break)
+ sport->port.icount.brk++;
+ else
+ sport->port.icount.parity++;
+ } else if (sr & UARTSTAT_FE) {
sport->port.icount.frame++;
+ }
if (sr & UARTSTAT_OR)
sport->port.icount.overrun++;
@@ -944,22 +958,24 @@ static void lpuart32_rxint(struct lpuart_port *sport)
sr &= sport->port.read_status_mask;
- if (sr & UARTSTAT_PE)
- flg = TTY_PARITY;
- else if (sr & UARTSTAT_FE)
+ if (sr & UARTSTAT_PE) {
+ if (is_break)
+ flg = TTY_BREAK;
+ else
+ flg = TTY_PARITY;
+ } else if (sr & UARTSTAT_FE) {
flg = TTY_FRAME;
+ }
if (sr & UARTSTAT_OR)
flg = TTY_OVERRUN;
-
- sport->port.sysrq = 0;
}
tty_insert_flip_char(port, rx, flg);
}
out:
- spin_unlock(&sport->port.lock);
+ uart_unlock_and_check_sysrq(&sport->port);
tty_flip_buffer_push(port);
}
--
2.20.1
The LPUART doesn't have the ability to control the RTS or CTS line
manually. Instead it will set it automatically when data is send or
handle it when data is received. Thus drop the wrong code in set_mctrl.
For the 32 bit version this was already done in the commit 2b30efe2e88a
("tty: serial: lpuart: Remove unnecessary code from set_mctrl"). Keep
the 8-bit version in sync and remove it there, too.
Signed-off-by: Michael Walle <[email protected]>
---
drivers/tty/serial/fsl_lpuart.c | 28 +---------------------------
1 file changed, 1 insertion(+), 27 deletions(-)
diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 74c04dba02d4..19714047d571 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1403,17 +1403,7 @@ static int lpuart32_config_rs485(struct uart_port *port,
static unsigned int lpuart_get_mctrl(struct uart_port *port)
{
- unsigned int temp = 0;
- unsigned char reg;
-
- reg = readb(port->membase + UARTMODEM);
- if (reg & UARTMODEM_TXCTSE)
- temp |= TIOCM_CTS;
-
- if (reg & UARTMODEM_RXRTSE)
- temp |= TIOCM_RTS;
-
- return temp;
+ return 0;
}
static unsigned int lpuart32_get_mctrl(struct uart_port *port)
@@ -1423,23 +1413,7 @@ static unsigned int lpuart32_get_mctrl(struct uart_port *port)
static void lpuart_set_mctrl(struct uart_port *port, unsigned int mctrl)
{
- unsigned char temp;
- struct lpuart_port *sport = container_of(port,
- struct lpuart_port, port);
-
- /* Make sure RXRTSE bit is not set when RS485 is enabled */
- if (!(sport->port.rs485.flags & SER_RS485_ENABLED)) {
- temp = readb(sport->port.membase + UARTMODEM) &
- ~(UARTMODEM_RXRTSE | UARTMODEM_TXCTSE);
-
- if (mctrl & TIOCM_RTS)
- temp |= UARTMODEM_RXRTSE;
- if (mctrl & TIOCM_CTS)
- temp |= UARTMODEM_TXCTSE;
-
- writeb(temp, port->membase + UARTMODEM);
- }
}
static void lpuart32_set_mctrl(struct uart_port *port, unsigned int mctrl)
--
2.20.1
SYSRQ doesn't work with DMA. This is because there is no error
indication whether a symbol had a framing error or not. Actually,
this is not completely correct, there is a bit in the data register
which is set in this case, but we'd have to read change the DMA access
to 16 bit and we'd need to post process the data, thus make the DMA
pointless in the first place.
Signed-off-by: Michael Walle <[email protected]>
---
Please note, that there is already sysrq/break support in the 8 bit
version. But I think there is a race between the hardware DMA controller
and the ISR in this driver. I'm not sure though and can't test it.
Angelo, maybe you could test it, I'd presume with this patch you don't need
the special handling in the ISR anymore.
drivers/tty/serial/fsl_lpuart.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 5e66f628e895..bf869c8d0897 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1587,6 +1587,9 @@ static void lpuart_tx_dma_startup(struct lpuart_port *sport)
u32 uartbaud;
int ret;
+ if (uart_console(&sport->port))
+ goto err;
+
if (!sport->dma_tx_chan)
goto err;
@@ -1616,6 +1619,9 @@ static void lpuart_rx_dma_startup(struct lpuart_port *sport)
int ret;
unsigned char cr3;
+ if (uart_console(&sport->port))
+ goto err;
+
if (!sport->dma_rx_chan)
goto err;
--
2.20.1
The LPUART can loop the RX and TX signal. Add support for it.
Please note, this was only tested on the 32 bit version of the LPUART.
Signed-off-by: Michael Walle <[email protected]>
---
drivers/tty/serial/fsl_lpuart.c | 36 +++++++++++++++++++++++++++++++--
1 file changed, 34 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 19714047d571..5e66f628e895 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1403,22 +1403,54 @@ static int lpuart32_config_rs485(struct uart_port *port,
static unsigned int lpuart_get_mctrl(struct uart_port *port)
{
- return 0;
+ unsigned int mctrl = 0;
+ u8 reg;
+
+ reg = readb(port->membase + UARTCR1);
+ if (reg & UARTCR1_LOOPS)
+ mctrl |= TIOCM_LOOP;
+
+ return mctrl;
}
static unsigned int lpuart32_get_mctrl(struct uart_port *port)
{
- return 0;
+ unsigned int mctrl = 0;
+ u32 reg;
+
+ reg = lpuart32_read(port, UARTCTRL);
+ if (reg & UARTCTRL_LOOPS)
+ mctrl |= TIOCM_LOOP;
+
+ return mctrl;
}
static void lpuart_set_mctrl(struct uart_port *port, unsigned int mctrl)
{
+ u8 reg;
+
+ reg = readb(port->membase + UARTCR1);
+
+ /* for internal loopback we need LOOPS=1 and RSRC=0 */
+ reg &= ~(UARTCR1_LOOPS | UARTCR1_RSRC);
+ if (mctrl & TIOCM_LOOP)
+ reg |= UARTCR1_LOOPS;
+ writeb(reg, port->membase + UARTCR1);
}
static void lpuart32_set_mctrl(struct uart_port *port, unsigned int mctrl)
{
+ u32 reg;
+
+ reg = lpuart32_read(port, UARTCTRL);
+
+ /* for internal loopback we need LOOPS=1 and RSRC=0 */
+ reg &= ~(UARTCTRL_LOOPS | UARTCTRL_RSRC);
+ if (mctrl & TIOCM_LOOP)
+ reg |= UARTCTRL_LOOPS;
+ lpuart32_write(port, reg, UARTCTRL);
}
static void lpuart_break_ctl(struct uart_port *port, int break_state)
--
2.20.1
The wrong code in set_mctrl() was already removed in commit 2b30efe2e88a
("tty: serial: lpuart: Remove unnecessary code from set_mctrl"), but the
code in get_mctrl() wasn't removed. It will not return the state of the
RTS or CTS line but whether automatic flow control is enabled, which is
wrong for the get_mctrl(). Thus remove it.
Fixes: 2b30efe2e88a ("tty: serial: lpuart: Remove unnecessary code from set_mctrl")
Signed-off-by: Michael Walle <[email protected]>
---
drivers/tty/serial/fsl_lpuart.c | 12 +-----------
1 file changed, 1 insertion(+), 11 deletions(-)
diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 0a578ad31a19..74c04dba02d4 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1418,17 +1418,7 @@ static unsigned int lpuart_get_mctrl(struct uart_port *port)
static unsigned int lpuart32_get_mctrl(struct uart_port *port)
{
- unsigned int temp = 0;
- unsigned long reg;
-
- reg = lpuart32_read(port, UARTMODIR);
- if (reg & UARTMODIR_TXCTSE)
- temp |= TIOCM_CTS;
-
- if (reg & UARTMODIR_RXRTSE)
- temp |= TIOCM_RTS;
-
- return temp;
+ return 0;
}
static void lpuart_set_mctrl(struct uart_port *port, unsigned int mctrl)
--
2.20.1
On Tue, May 11, 2021 at 10:01:43PM +0200, Michael Walle wrote:
> Since commit 81e2073c175b ("genirq: Disable interrupts for force
> threaded handlers") interrupt handlers that are not explicitly requested
> as threaded are always called with interrupts disabled and there is no
> need to save the interrupt state when taking the port lock.
Since you've copied the above words verbatim from commit 75f4e830fa9c
("serial: do not restore interrupt state in sysrq helper") I'd expect
you to use quotes or at least refer to the commit you copied the
rationale from.
> This is a preparation for sysrq handling which uses
> uart_unlock_and_check_sysrq();
Johan
On Tue, May 11, 2021 at 10:01:44PM +0200, Michael Walle wrote:
> Although there is already (broken) sysrq characters handling, a break
> condition was never detected. There is also a possible deadlock because
> we might call handle_sysrq() while still holding the port lock.
Where's the possible deadlock?
First, as you point out above the driver currently doesn't detect breaks
so the sysrq handler is never called and there's no risk for deadlocks
in the console code.
Second, the driver's console implementation explicitly handles being
called recursively so would not deadlock after you start detecting
breaks either.
> Add support for break detection and use the proper
> uart_unlock_and_check_sysrq() to defer calling handle_sysrq().
Johan
Am 2021-05-12 11:25, schrieb Johan Hovold:
> On Tue, May 11, 2021 at 10:01:43PM +0200, Michael Walle wrote:
>> Since commit 81e2073c175b ("genirq: Disable interrupts for force
>> threaded handlers") interrupt handlers that are not explicitly
>> requested
>> as threaded are always called with interrupts disabled and there is no
>> need to save the interrupt state when taking the port lock.
>
> Since you've copied the above words verbatim from commit 75f4e830fa9c
> ("serial: do not restore interrupt state in sysrq helper") I'd expect
> you to use quotes or at least refer to the commit you copied the
> rationale from.
Sure, sorry.
>> This is a preparation for sysrq handling which uses
>> uart_unlock_and_check_sysrq();
-michael
Am 2021-05-12 11:30, schrieb Johan Hovold:
> On Tue, May 11, 2021 at 10:01:44PM +0200, Michael Walle wrote:
>> Although there is already (broken) sysrq characters handling, a break
>> condition was never detected. There is also a possible deadlock
>> because
>> we might call handle_sysrq() while still holding the port lock.
>
> Where's the possible deadlock?
[ 17.866874] ======================================================
[ 17.866876] WARNING: possible circular locking dependency detected
[ 17.866878] 5.13.0-rc1-next-20210511+ #555 Not tainted
[ 17.866880] ------------------------------------------------------
[ 17.866882] sl28-variant.sh/1934 is trying to acquire lock:
[ 17.866884] ffff800011d16a00 (console_owner){-.-.}-{0:0}, at:
console_unlock+0x1c0/0x660
[ 17.866892]
[ 17.866893] but task is already holding lock:
[ 17.866895] ffff0020026ea098 (&port_lock_key){-.-.}-{2:2}, at:
lpuart32_int+0x1b0/0x7c8
[ 17.866902]
[ 17.866904] which lock already depends on the new lock.
[ 17.866906]
[ 17.866907]
[ 17.866909] the existing dependency chain (in reverse order) is:
[ 17.866910]
[ 17.866912] -> #1 (&port_lock_key){-.-.}-{2:2}:
[ 17.866918] _raw_spin_lock_irqsave+0x80/0xd0
[ 17.866920] lpuart32_console_write+0x214/0x2b8
[ 17.866922] console_unlock+0x404/0x660
[ 17.866924] register_console+0x170/0x2a8
[ 17.866925] uart_add_one_port+0x464/0x478
[ 17.866927] lpuart_probe+0x218/0x3a8
[ 17.866928] platform_probe+0x70/0xe0
[ 17.866930] really_probe+0xec/0x3c0
[ 17.866931] driver_probe_device+0x6c/0xd0
[ 17.866933] device_driver_attach+0x7c/0x88
[ 17.866935] __driver_attach+0x6c/0xf8
[ 17.866936] bus_for_each_dev+0x7c/0xd0
[ 17.866938] driver_attach+0x2c/0x38
[ 17.866939] bus_add_driver+0x194/0x1f8
[ 17.866941] driver_register+0x6c/0x128
[ 17.866943] __platform_driver_register+0x30/0x40
[ 17.866944] lpuart_serial_init+0x44/0x6c
[ 17.866946] do_one_initcall+0x90/0x470
[ 17.866948] kernel_init_freeable+0x2d4/0x344
[ 17.866949] kernel_init+0x1c/0x120
[ 17.866951] ret_from_fork+0x10/0x30
[ 17.866952]
[ 17.866953] -> #0 (console_owner){-.-.}-{0:0}:
[ 17.866959] __lock_acquire+0xf60/0x17e8
[ 17.866961] lock_acquire+0x138/0x4c0
[ 17.866963] console_unlock+0x224/0x660
[ 17.866964] vprintk_emit+0x11c/0x338
[ 17.866966] vprintk_default+0x40/0x50
[ 17.866967] vprintk+0xfc/0x320
[ 17.866969] printk+0x6c/0x90
[ 17.866970] __handle_sysrq+0x16c/0x1d8
[ 17.866972] handle_sysrq+0x2c/0x48
[ 17.866973] lpuart32_int+0x70c/0x7c8
[ 17.866975] __handle_irq_event_percpu+0xcc/0x430
[ 17.866977] handle_irq_event_percpu+0x40/0x98
[ 17.866978] handle_irq_event+0x50/0x100
[ 17.866980] handle_fasteoi_irq+0xc0/0x178
[ 17.866981] generic_handle_irq+0x38/0x50
[ 17.866983] __handle_domain_irq+0x6c/0xc8
[ 17.866985] gic_handle_irq+0xdc/0x340
[ 17.866986] el1_irq+0xb8/0x150
[ 17.866988] arch_local_irq_restore+0x8/0x20
[ 17.866989] page_add_file_rmap+0x24/0x1f8
[ 17.866991] do_set_pte+0xd4/0x1a0
[ 17.866992] filemap_map_pages+0x358/0x590
[ 17.866994] __handle_mm_fault+0xbc0/0xdd0
[ 17.866995] handle_mm_fault+0x170/0x3e0
[ 17.866997] do_page_fault+0x1e8/0x448
[ 17.866998] do_translation_fault+0x60/0x70
[ 17.867000] do_mem_abort+0x48/0xb8
[ 17.867001] el0_da+0x44/0x80
[ 17.867002] el0_sync_handler+0x68/0xb8
[ 17.867004] el0_sync+0x178/0x180
[ 17.867005]
[ 17.867007] other info that might help us debug this:
[ 17.867008]
[ 17.867009] Possible unsafe locking scenario:
[ 17.867011]
[ 17.867012] CPU0 CPU1
[ 17.867013] ---- ----
[ 17.867015] lock(&port_lock_key);
[ 17.867019] lock(console_owner);
[ 17.867023] lock(&port_lock_key);
[ 17.867027] lock(console_owner);
[ 17.867030]
[ 17.867031] *** DEADLOCK ***
[ 17.867033]
[ 17.867034] 7 locks held by sl28-variant.sh/1934:
[ 17.867035] #0: ffff002003a37b08 (&mm->mmap_lock){++++}-{3:3}, at:
do_page_fault+0x180/0x448
[ 17.867043] #1: ffff800011d87660 (rcu_read_lock){....}-{1:2}, at:
filemap_map_pages+0x8/0x590
[ 17.867051] #2: ffff0020048d8318 (ptlock_ptr(page)){+.+.}-{2:2}, at:
filemap_map_pages+0x27c/0x590
[ 17.867059] #3: ffff800011d87660 (rcu_read_lock){....}-{1:2}, at:
lock_page_memcg+0x8/0x1d8
[ 17.867067] #4: ffff0020026ea098 (&port_lock_key){-.-.}-{2:2}, at:
lpuart32_int+0x1b0/0x7c8
[ 17.867074] #5: ffff800011d87660 (rcu_read_lock){....}-{1:2}, at:
__handle_sysrq+0x8/0x1d8
[ 17.867082] #6: ffff800011d168a0 (console_lock){+.+.}-{0:0}, at:
vprintk_emit+0x114/0x338
> First, as you point out above the driver currently doesn't detect
> breaks
> so the sysrq handler is never called and there's no risk for deadlocks
> in the console code.
But this commit introduces it? Therefore, I don't get your point.
> Second, the driver's console implementation explicitly handles being
> called recursively so would not deadlock after you start detecting
> breaks either.
See above. Or there is something wrong with the lock debugging.
>> Add support for break detection and use the proper
>> uart_unlock_and_check_sysrq() to defer calling handle_sysrq().
-michael
On Wed, May 12, 2021 at 11:46:28AM +0200, Michael Walle wrote:
> Am 2021-05-12 11:30, schrieb Johan Hovold:
> > On Tue, May 11, 2021 at 10:01:44PM +0200, Michael Walle wrote:
> >> Although there is already (broken) sysrq characters handling, a break
> >> condition was never detected. There is also a possible deadlock
> >> because
> >> we might call handle_sysrq() while still holding the port lock.
> >
> > Where's the possible deadlock?
>
> [ 17.866874] ======================================================
> [ 17.866876] WARNING: possible circular locking dependency detected
> [ 17.866878] 5.13.0-rc1-next-20210511+ #555 Not tainted
> [ 17.866880] ------------------------------------------------------
> [ 17.866882] sl28-variant.sh/1934 is trying to acquire lock:
> [ 17.866884] ffff800011d16a00 (console_owner){-.-.}-{0:0}, at:
> console_unlock+0x1c0/0x660
> [ 17.866892]
> [ 17.866893] but task is already holding lock:
> [ 17.866895] ffff0020026ea098 (&port_lock_key){-.-.}-{2:2}, at:
> lpuart32_int+0x1b0/0x7c8
> [ 17.866902]
> [ 17.866904] which lock already depends on the new lock.
> [ 17.866906]
> [ 17.866907]
> [ 17.866909] the existing dependency chain (in reverse order) is:
> [ 17.866910]
> [ 17.866912] -> #1 (&port_lock_key){-.-.}-{2:2}:
> [ 17.866918] _raw_spin_lock_irqsave+0x80/0xd0
> [ 17.866920] lpuart32_console_write+0x214/0x2b8
> [ 17.866922] console_unlock+0x404/0x660
> [ 17.866924] register_console+0x170/0x2a8
> [ 17.866925] uart_add_one_port+0x464/0x478
> [ 17.866927] lpuart_probe+0x218/0x3a8
> [ 17.866928] platform_probe+0x70/0xe0
> [ 17.866930] really_probe+0xec/0x3c0
> [ 17.866931] driver_probe_device+0x6c/0xd0
> [ 17.866933] device_driver_attach+0x7c/0x88
> [ 17.866935] __driver_attach+0x6c/0xf8
> [ 17.866936] bus_for_each_dev+0x7c/0xd0
> [ 17.866938] driver_attach+0x2c/0x38
> [ 17.866939] bus_add_driver+0x194/0x1f8
> [ 17.866941] driver_register+0x6c/0x128
> [ 17.866943] __platform_driver_register+0x30/0x40
> [ 17.866944] lpuart_serial_init+0x44/0x6c
> [ 17.866946] do_one_initcall+0x90/0x470
> [ 17.866948] kernel_init_freeable+0x2d4/0x344
> [ 17.866949] kernel_init+0x1c/0x120
> [ 17.866951] ret_from_fork+0x10/0x30
> [ 17.866952]
> [ 17.866953] -> #0 (console_owner){-.-.}-{0:0}:
> [ 17.866959] __lock_acquire+0xf60/0x17e8
> [ 17.866961] lock_acquire+0x138/0x4c0
> [ 17.866963] console_unlock+0x224/0x660
> [ 17.866964] vprintk_emit+0x11c/0x338
> [ 17.866966] vprintk_default+0x40/0x50
> [ 17.866967] vprintk+0xfc/0x320
> [ 17.866969] printk+0x6c/0x90
> [ 17.866970] __handle_sysrq+0x16c/0x1d8
> [ 17.866972] handle_sysrq+0x2c/0x48
> [ 17.866973] lpuart32_int+0x70c/0x7c8
> [ 17.866975] __handle_irq_event_percpu+0xcc/0x430
> [ 17.866977] handle_irq_event_percpu+0x40/0x98
> [ 17.866978] handle_irq_event+0x50/0x100
> [ 17.866980] handle_fasteoi_irq+0xc0/0x178
> [ 17.866981] generic_handle_irq+0x38/0x50
> [ 17.866983] __handle_domain_irq+0x6c/0xc8
> [ 17.866985] gic_handle_irq+0xdc/0x340
> [ 17.866986] el1_irq+0xb8/0x150
> [ 17.866988] arch_local_irq_restore+0x8/0x20
> [ 17.866989] page_add_file_rmap+0x24/0x1f8
> [ 17.866991] do_set_pte+0xd4/0x1a0
> [ 17.866992] filemap_map_pages+0x358/0x590
> [ 17.866994] __handle_mm_fault+0xbc0/0xdd0
> [ 17.866995] handle_mm_fault+0x170/0x3e0
> [ 17.866997] do_page_fault+0x1e8/0x448
> [ 17.866998] do_translation_fault+0x60/0x70
> [ 17.867000] do_mem_abort+0x48/0xb8
> [ 17.867001] el0_da+0x44/0x80
> [ 17.867002] el0_sync_handler+0x68/0xb8
> [ 17.867004] el0_sync+0x178/0x180
> [ 17.867005]
> [ 17.867007] other info that might help us debug this:
> [ 17.867008]
> [ 17.867009] Possible unsafe locking scenario:
> [ 17.867011]
> [ 17.867012] CPU0 CPU1
> [ 17.867013] ---- ----
> [ 17.867015] lock(&port_lock_key);
> [ 17.867019] lock(console_owner);
> [ 17.867023] lock(&port_lock_key);
> [ 17.867027] lock(console_owner);
> [ 17.867030]
> [ 17.867031] *** DEADLOCK ***
> [ 17.867033]
> [ 17.867034] 7 locks held by sl28-variant.sh/1934:
> [ 17.867035] #0: ffff002003a37b08 (&mm->mmap_lock){++++}-{3:3}, at:
> do_page_fault+0x180/0x448
> [ 17.867043] #1: ffff800011d87660 (rcu_read_lock){....}-{1:2}, at:
> filemap_map_pages+0x8/0x590
> [ 17.867051] #2: ffff0020048d8318 (ptlock_ptr(page)){+.+.}-{2:2}, at:
> filemap_map_pages+0x27c/0x590
> [ 17.867059] #3: ffff800011d87660 (rcu_read_lock){....}-{1:2}, at:
> lock_page_memcg+0x8/0x1d8
> [ 17.867067] #4: ffff0020026ea098 (&port_lock_key){-.-.}-{2:2}, at:
> lpuart32_int+0x1b0/0x7c8
> [ 17.867074] #5: ffff800011d87660 (rcu_read_lock){....}-{1:2}, at:
> __handle_sysrq+0x8/0x1d8
> [ 17.867082] #6: ffff800011d168a0 (console_lock){+.+.}-{0:0}, at:
> vprintk_emit+0x114/0x338
Note that it says "possible" deadlock; the lockdep validator probably
isn't smart enough to understand the trylock hack in the console write
callback.
> > First, as you point out above the driver currently doesn't detect
> > breaks
> > so the sysrq handler is never called and there's no risk for deadlocks
> > in the console code.
>
> But this commit introduces it? Therefore, I don't get your point.
My point is that your commit message makes it sound like an actual
deadlock in the current code. Something which, for example, can cause
commits to get backported to stable when it is not needed.
> > Second, the driver's console implementation explicitly handles being
> > called recursively so would not deadlock after you start detecting
> > breaks either.
>
> See above. Or there is something wrong with the lock debugging.
Seems to work as intended.
> >> Add support for break detection and use the proper
> >> uart_unlock_and_check_sysrq() to defer calling handle_sysrq().
But you should get rid of the sysrq trylock hack when switching to
uart_unlock_and_check_sysrq().
Johan
[dropped [email protected], mail bounces with 550 5.4.1 Recipient
address rejected: Access denied]
Am 2021-05-12 12:07, schrieb Johan Hovold:
> On Wed, May 12, 2021 at 11:46:28AM +0200, Michael Walle wrote:
>> Am 2021-05-12 11:30, schrieb Johan Hovold:
>> > On Tue, May 11, 2021 at 10:01:44PM +0200, Michael Walle wrote:
>> >> Although there is already (broken) sysrq characters handling, a break
>> >> condition was never detected. There is also a possible deadlock
>> >> because
>> >> we might call handle_sysrq() while still holding the port lock.
>> >
>> > Where's the possible deadlock?
>>
>> [ 17.866874] ======================================================
>> [ 17.866876] WARNING: possible circular locking dependency detected
>> [ 17.866878] 5.13.0-rc1-next-20210511+ #555 Not tainted
>> [ 17.866880] ------------------------------------------------------
>> [ 17.866882] sl28-variant.sh/1934 is trying to acquire lock:
>> [ 17.866884] ffff800011d16a00 (console_owner){-.-.}-{0:0}, at:
>> console_unlock+0x1c0/0x660
>> [ 17.866892]
>> [ 17.866893] but task is already holding lock:
>> [ 17.866895] ffff0020026ea098 (&port_lock_key){-.-.}-{2:2}, at:
>> lpuart32_int+0x1b0/0x7c8
>> [ 17.866902]
>> [ 17.866904] which lock already depends on the new lock.
>> [ 17.866906]
>> [ 17.866907]
>> [ 17.866909] the existing dependency chain (in reverse order) is:
>> [ 17.866910]
>> [ 17.866912] -> #1 (&port_lock_key){-.-.}-{2:2}:
>> [ 17.866918] _raw_spin_lock_irqsave+0x80/0xd0
>> [ 17.866920] lpuart32_console_write+0x214/0x2b8
>> [ 17.866922] console_unlock+0x404/0x660
>> [ 17.866924] register_console+0x170/0x2a8
>> [ 17.866925] uart_add_one_port+0x464/0x478
>> [ 17.866927] lpuart_probe+0x218/0x3a8
>> [ 17.866928] platform_probe+0x70/0xe0
>> [ 17.866930] really_probe+0xec/0x3c0
>> [ 17.866931] driver_probe_device+0x6c/0xd0
>> [ 17.866933] device_driver_attach+0x7c/0x88
>> [ 17.866935] __driver_attach+0x6c/0xf8
>> [ 17.866936] bus_for_each_dev+0x7c/0xd0
>> [ 17.866938] driver_attach+0x2c/0x38
>> [ 17.866939] bus_add_driver+0x194/0x1f8
>> [ 17.866941] driver_register+0x6c/0x128
>> [ 17.866943] __platform_driver_register+0x30/0x40
>> [ 17.866944] lpuart_serial_init+0x44/0x6c
>> [ 17.866946] do_one_initcall+0x90/0x470
>> [ 17.866948] kernel_init_freeable+0x2d4/0x344
>> [ 17.866949] kernel_init+0x1c/0x120
>> [ 17.866951] ret_from_fork+0x10/0x30
>> [ 17.866952]
>> [ 17.866953] -> #0 (console_owner){-.-.}-{0:0}:
>> [ 17.866959] __lock_acquire+0xf60/0x17e8
>> [ 17.866961] lock_acquire+0x138/0x4c0
>> [ 17.866963] console_unlock+0x224/0x660
>> [ 17.866964] vprintk_emit+0x11c/0x338
>> [ 17.866966] vprintk_default+0x40/0x50
>> [ 17.866967] vprintk+0xfc/0x320
>> [ 17.866969] printk+0x6c/0x90
>> [ 17.866970] __handle_sysrq+0x16c/0x1d8
>> [ 17.866972] handle_sysrq+0x2c/0x48
>> [ 17.866973] lpuart32_int+0x70c/0x7c8
>> [ 17.866975] __handle_irq_event_percpu+0xcc/0x430
>> [ 17.866977] handle_irq_event_percpu+0x40/0x98
>> [ 17.866978] handle_irq_event+0x50/0x100
>> [ 17.866980] handle_fasteoi_irq+0xc0/0x178
>> [ 17.866981] generic_handle_irq+0x38/0x50
>> [ 17.866983] __handle_domain_irq+0x6c/0xc8
>> [ 17.866985] gic_handle_irq+0xdc/0x340
>> [ 17.866986] el1_irq+0xb8/0x150
>> [ 17.866988] arch_local_irq_restore+0x8/0x20
>> [ 17.866989] page_add_file_rmap+0x24/0x1f8
>> [ 17.866991] do_set_pte+0xd4/0x1a0
>> [ 17.866992] filemap_map_pages+0x358/0x590
>> [ 17.866994] __handle_mm_fault+0xbc0/0xdd0
>> [ 17.866995] handle_mm_fault+0x170/0x3e0
>> [ 17.866997] do_page_fault+0x1e8/0x448
>> [ 17.866998] do_translation_fault+0x60/0x70
>> [ 17.867000] do_mem_abort+0x48/0xb8
>> [ 17.867001] el0_da+0x44/0x80
>> [ 17.867002] el0_sync_handler+0x68/0xb8
>> [ 17.867004] el0_sync+0x178/0x180
>> [ 17.867005]
>> [ 17.867007] other info that might help us debug this:
>> [ 17.867008]
>> [ 17.867009] Possible unsafe locking scenario:
>> [ 17.867011]
>> [ 17.867012] CPU0 CPU1
>> [ 17.867013] ---- ----
>> [ 17.867015] lock(&port_lock_key);
>> [ 17.867019] lock(console_owner);
>> [ 17.867023] lock(&port_lock_key);
>> [ 17.867027] lock(console_owner);
>> [ 17.867030]
>> [ 17.867031] *** DEADLOCK ***
>> [ 17.867033]
>> [ 17.867034] 7 locks held by sl28-variant.sh/1934:
>> [ 17.867035] #0: ffff002003a37b08 (&mm->mmap_lock){++++}-{3:3}, at:
>> do_page_fault+0x180/0x448
>> [ 17.867043] #1: ffff800011d87660 (rcu_read_lock){....}-{1:2}, at:
>> filemap_map_pages+0x8/0x590
>> [ 17.867051] #2: ffff0020048d8318 (ptlock_ptr(page)){+.+.}-{2:2},
>> at:
>> filemap_map_pages+0x27c/0x590
>> [ 17.867059] #3: ffff800011d87660 (rcu_read_lock){....}-{1:2}, at:
>> lock_page_memcg+0x8/0x1d8
>> [ 17.867067] #4: ffff0020026ea098 (&port_lock_key){-.-.}-{2:2}, at:
>> lpuart32_int+0x1b0/0x7c8
>> [ 17.867074] #5: ffff800011d87660 (rcu_read_lock){....}-{1:2}, at:
>> __handle_sysrq+0x8/0x1d8
>> [ 17.867082] #6: ffff800011d168a0 (console_lock){+.+.}-{0:0}, at:
>> vprintk_emit+0x114/0x338
>
> Note that it says "possible" deadlock; the lockdep validator probably
> isn't smart enough to understand the trylock hack in the console write
> callback.
>
>> > First, as you point out above the driver currently doesn't detect
>> > breaks
>> > so the sysrq handler is never called and there's no risk for deadlocks
>> > in the console code.
>>
>> But this commit introduces it? Therefore, I don't get your point.
>
> My point is that your commit message makes it sound like an actual
> deadlock in the current code. Something which, for example, can cause
> commits to get backported to stable when it is not needed.
I see. I'll rephrase the commit message. FWIW I intentionally didn't
put a Fixes: tag here, because of that.
>> > Second, the driver's console implementation explicitly handles being
>> > called recursively so would not deadlock after you start detecting
>> > breaks either.
>>
>> See above. Or there is something wrong with the lock debugging.
>
> Seems to work as intended.
>
>> >> Add support for break detection and use the proper
>> >> uart_unlock_and_check_sysrq() to defer calling handle_sysrq().
>
> But you should get rid of the sysrq trylock hack when switching to
> uart_unlock_and_check_sysrq().
Ok. But only for the sport->port.sysrq part right? We'll still
need it for oops_in_progress.
Thanks for reviewing,
-michael
On Wed, May 12, 2021 at 12:31:33PM +0200, Michael Walle wrote:
> [dropped [email protected], mail bounces with 550 5.4.1 Recipient
> address rejected: Access denied]
>
> Am 2021-05-12 12:07, schrieb Johan Hovold:
> > But you should get rid of the sysrq trylock hack when switching to
> > uart_unlock_and_check_sysrq().
>
> Ok. But only for the sport->port.sysrq part right? We'll still
> need it for oops_in_progress.
Right.
Johan