Hello,
RFC
printk_safe buffers help us to avoid printk() deadlocks that are
caused by recursive printk() calls, e.g.
printk()
vprintk_emit()
spin_lock(&logbuf_lock)
vsnprintf()
format_decode()
warn_slowpath_fmt()
vprintk_emit()
spin_lock(&logbuf_lock) << deadlock
It doesn't come as a surprise that recursive printk() calls are not the
only way for us to deadlock in printk() and we still have a whole bunch
of other printk() deadlock scenarios. For instance, those that involve
TTY port->lock spin_lock and UART port->lock spin_lock.
syzbot recently hit one of such scenarios [1]:
CPU0 CPU1
tty_ioctl()
spin_lock(&tty_port->lock) IRQ
kmalloc() foo_console_handle_IRQ()
printk() spin_lock(&uart_port->lock)
call_console_drivers() tty_wakeup()
foo_console_driver()
spin_lock(&uart_port->lock) spin_lock(&tty_port->lock)
So the idea of this patch set is to take tty_port->lock and
uart_port->lock from printk_safe context and to eliminate some
of non-recursive printk() deadlocks - the ones that don't start
in printk(), but involve console related locks and thus eventually
deadlock us in printk(). For this purpose the patch set introduces
several helper macros:
- uart_port_lock_irq() / uart_port_unlock_irq()
uart_port_lock_irqsave() / uart_port_unlock_irqrestore()
To lock/unlock uart_port->lock spin_lock from printk_safe
context.
And
- tty_port_lock_irq() / tty_port_unlock_irq()
tty_port_lock_irqsave() / tty_port_unlock_irqrestore()
To lock/unlock tty_port->lock spin_lock from printk_safe
context.
I converted tty/pty/serial_core to use those helper macros.
Now, the boring part is that all serial console drivers must be converted
to use uart_port_lock macros. In this patch set I only modified the 8250
serial console [since this is RFC patch set], but if the patch set will
not see a lot of opposition I can do so for the rest of serial consoles
[or ask the maintainers to help me out]. The conversion is pretty
simple.
It would be great if we could "forbid" direct tty_port->lock and
uart_port->lock manipulation [I don't know, rename them to ->__lock]
and enforce uart_port_lock/tty_port_lock macros usage.
There are some other cases [2] that we can handle with this
patch set. For instance:
IRQ
spin_lock(&uart_port->lock)
tty_wakeup()
tty_port_default_wakeup()
tty_port_tty_get()
refcount_inc()
WARN_ONCE()
printk()
call_console_drivers()
foo_console_driver()
spin_lock(&uart_port->lock) << deadlock
Of course, TTY and UART port spin_locks are not the only locks that
we can deadlock on. So this patch set does not address all deadlock
scenarios, it just makes a small step forward.
Any opinions?
[1]: lkml.kernel.org/r/[email protected]
[2]: lkml.kernel.org/r/20180607051019.GA10406@jagdpanzerIV
Sergey Senozhatsky (6):
printk: move printk_safe macros to printk header
tty: add tty_port locking helpers
tty/pty: switch to tty_port helpers
serial: add uart port locking helpers
serial: switch to uart_port locking helpers
tty: 8250: switch to uart_port locking helpers
drivers/tty/pty.c | 4 +-
drivers/tty/serial/8250/8250_core.c | 8 +-
drivers/tty/serial/8250/8250_dma.c | 4 +-
drivers/tty/serial/8250/8250_port.c | 74 +++++++++----------
drivers/tty/serial/serial_core.c | 109 ++++++++++++++--------------
drivers/tty/tty_port.c | 38 +++++-----
include/linux/printk.h | 40 ++++++++++
include/linux/serial_core.h | 35 +++++++++
include/linux/tty.h | 24 ++++++
kernel/printk/internal.h | 37 ----------
10 files changed, 218 insertions(+), 155 deletions(-)
--
2.17.1
Make printk_safe_enter_irqsave()/etc macros available to the
rest of the kernel, so we can use them in TTY and UART code.
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
include/linux/printk.h | 40 ++++++++++++++++++++++++++++++++++++++++
kernel/printk/internal.h | 37 -------------------------------------
2 files changed, 40 insertions(+), 37 deletions(-)
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 6d7e800affd8..8b4e1e667919 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -153,6 +153,46 @@ static inline void printk_nmi_enter(void) { }
static inline void printk_nmi_exit(void) { }
#endif /* PRINTK_NMI */
+#ifdef CONFIG_PRINTK
+extern void __printk_safe_enter(void);
+extern void __printk_safe_exit(void);
+
+#define printk_safe_enter_irqsave(flags) \
+ do { \
+ local_irq_save(flags); \
+ __printk_safe_enter(); \
+ } while (0)
+
+#define printk_safe_exit_irqrestore(flags) \
+ do { \
+ __printk_safe_exit(); \
+ local_irq_restore(flags); \
+ } while (0)
+
+#define printk_safe_enter_irq() \
+ do { \
+ local_irq_disable(); \
+ __printk_safe_enter(); \
+ } while (0)
+
+#define printk_safe_exit_irq() \
+ do { \
+ __printk_safe_exit(); \
+ local_irq_enable(); \
+ } while (0)
+#else
+/*
+ * On !PRINTK builds we still export console output related locks
+ * and some functions (console_unlock()/tty/etc.), so printk-safe
+ * must preserve the existing local IRQ guarantees.
+ */
+#define printk_safe_enter_irqsave(flags) local_irq_save(flags)
+#define printk_safe_exit_irqrestore(flags) local_irq_restore(flags)
+
+#define printk_safe_enter_irq() local_irq_disable()
+#define printk_safe_exit_irq() local_irq_enable()
+#endif
+
#ifdef CONFIG_PRINTK
asmlinkage __printf(5, 0)
int vprintk_emit(int facility, int level,
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 2a7d04049af4..cf4f85e53cc2 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -27,46 +27,9 @@ extern raw_spinlock_t logbuf_lock;
__printf(1, 0) int vprintk_default(const char *fmt, va_list args);
__printf(1, 0) int vprintk_deferred(const char *fmt, va_list args);
__printf(1, 0) int vprintk_func(const char *fmt, va_list args);
-void __printk_safe_enter(void);
-void __printk_safe_exit(void);
-
-#define printk_safe_enter_irqsave(flags) \
- do { \
- local_irq_save(flags); \
- __printk_safe_enter(); \
- } while (0)
-
-#define printk_safe_exit_irqrestore(flags) \
- do { \
- __printk_safe_exit(); \
- local_irq_restore(flags); \
- } while (0)
-
-#define printk_safe_enter_irq() \
- do { \
- local_irq_disable(); \
- __printk_safe_enter(); \
- } while (0)
-
-#define printk_safe_exit_irq() \
- do { \
- __printk_safe_exit(); \
- local_irq_enable(); \
- } while (0)
#else
__printf(1, 0) int vprintk_func(const char *fmt, va_list args) { return 0; }
-/*
- * In !PRINTK builds we still export logbuf_lock spin_lock, console_sem
- * semaphore and some of console functions (console_unlock()/etc.), so
- * printk-safe must preserve the existing local IRQ guarantees.
- */
-#define printk_safe_enter_irqsave(flags) local_irq_save(flags)
-#define printk_safe_exit_irqrestore(flags) local_irq_restore(flags)
-
-#define printk_safe_enter_irq() local_irq_disable()
-#define printk_safe_exit_irq() local_irq_enable()
-
#endif /* CONFIG_PRINTK */
--
2.17.1
TTY port spin_lock is one of the locks that can cause a
printk-recursion deadlock, thus we need to lock it from
printk_safe context. E.g.
ioctl()
tty_ioctl()
spin_lock(&tty_port->lock)
printk()
call_console_drivers()
foo_console_write()
spin_lock(&uart_port->lock)
tty_wakeup()
spin_lock(&tty_port->lock) << deadlock
This patch adds tty_port->lock helper macros which take care
of printk_safe context and redirect potentially unsafe printk()
calls to per-CPU buffers, which we flush later from safe a
context. The helper macros will turn the above mentioned deadlock
scenario into:
ioctl()
tty_ioctl()
printk_safe_enter()
spin_lock(&tty_port->lock)
printk()
printk_safe_log_store()
irq_work_queue()
spin_unlock(&tty_port->lock)
printk_safe_exit()
IRQ
printk_safe_flush_buffer()
vprintk_deferred()
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
include/linux/tty.h | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/include/linux/tty.h b/include/linux/tty.h
index c56e3978b00f..828f91ab680b 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -252,6 +252,30 @@ struct tty_port {
void *client_data;
};
+#define tty_port_lock_irq(lock) \
+ do { \
+ printk_safe_enter_irq(); \
+ spin_lock(lock); \
+ } while (0)
+
+#define tty_port_unlock_irq(lock) \
+ do { \
+ spin_unlock(lock); \
+ printk_safe_exit_irq(); \
+ } while (0)
+
+#define tty_port_lock_irqsave(lock, flags) \
+ do { \
+ printk_safe_enter_irqsave(flags); \
+ spin_lock(lock); \
+ } while (0)
+
+#define tty_port_unlock_irqrestore(lock, flags) \
+ do { \
+ spin_unlock(lock); \
+ printk_safe_exit_irqrestore(flags); \
+ } while (0)
+
/* tty_port::iflags bits -- use atomic bit ops */
#define TTY_PORT_INITIALIZED 0 /* device is initialized */
#define TTY_PORT_SUSPENDED 1 /* device is suspended */
--
2.17.1
UART port spin_lock is one of the locks that can cause a
printk-recursion deadlock, thus we need to lock it from
printk_safe context. An example of a possible deadlock:
IRQ
foo_console_handle_IRQ()
spin_lock(&uart_port->lock)
tty_wakeup()
printk()
call_console_drivers()
foo_console_write()
spin_lock(&uart_port->lock) << deadlock
This patch adds uart_port->lock helper macros which take care
of printk_safe context and redirect potentially unsafe printk()
calls to per-CPU buffers, which we flush later from safe a
context. The helper macros will turn the above mentioned deadlock
scenario into:
IRQ
foo_console_handle_IRQ()
printk_safe_enter()
spin_lock(&uart_port->lock)
tty_wakeup()
printk()
printk_safe_log_store()
irq_work_queue()
spin_unlock(&uart_port->lock)
printk_safe_exit()
iret
IRQ
printk_safe_flush_buffer()
vprintk_deferred()
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
include/linux/serial_core.h | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 06ea4eeb09ab..deb464520946 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -256,6 +256,41 @@ struct uart_port {
void *private_data; /* generic platform data pointer */
};
+#define uart_port_lock_irq(lock) \
+ do { \
+ printk_safe_enter_irq(); \
+ spin_lock(lock); \
+ } while (0)
+
+#define uart_port_unlock_irq(lock) \
+ do { \
+ spin_unlock(lock); \
+ printk_safe_exit_irq(); \
+ } while (0)
+
+#define uart_port_lock_irqsave(lock, flags) \
+ do { \
+ printk_safe_enter_irqsave(flags); \
+ spin_lock(lock); \
+ } while (0)
+
+#define uart_port_trylock_irqsave(lock, flags) \
+ ({ \
+ int locked; \
+ \
+ printk_safe_enter_irqsave(flags); \
+ locked = spin_trylock(lock); \
+ if (!locked) \
+ printk_safe_exit_irqrestore(flags); \
+ locked; \
+ })
+
+#define uart_port_unlock_irqrestore(lock, flags) \
+ do { \
+ spin_unlock(lock); \
+ printk_safe_exit_irqrestore(flags); \
+ } while (0)
+
static inline int serial_port_in(struct uart_port *up, int offset)
{
return up->serial_in(up, offset);
--
2.17.1
Do not directly lock/unlock uart_port->lock, but use uart_port_lock
helper macros.
The patch also renames serial_core internal macros uart_port_lock()
and uart_port_unlock() to uart_port_ref_lock() and to
uart_port_unlock_deref() correspondingly.
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
drivers/tty/serial/serial_core.c | 101 ++++++++++++++++---------------
1 file changed, 51 insertions(+), 50 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 48b3377d7f77..48624ca485e5 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -65,19 +65,20 @@ static inline void uart_port_deref(struct uart_port *uport)
wake_up(&uport->state->remove_wait);
}
-#define uart_port_lock(state, flags) \
+#define uart_port_ref_lock(state, flags) \
({ \
struct uart_port *__uport = uart_port_ref(state); \
if (__uport) \
- spin_lock_irqsave(&__uport->lock, flags); \
+ uart_port_lock_irqsave(&__uport->lock, flags); \
__uport; \
})
-#define uart_port_unlock(uport, flags) \
+#define uart_port_unlock_deref(uport, flags) \
({ \
struct uart_port *__uport = uport; \
if (__uport) { \
- spin_unlock_irqrestore(&__uport->lock, flags); \
+ uart_port_unlock_irqrestore(&__uport->lock, \
+ flags); \
uart_port_deref(__uport); \
} \
})
@@ -109,10 +110,10 @@ static void uart_stop(struct tty_struct *tty)
struct uart_port *port;
unsigned long flags;
- port = uart_port_lock(state, flags);
+ port = uart_port_ref_lock(state, flags);
if (port)
port->ops->stop_tx(port);
- uart_port_unlock(port, flags);
+ uart_port_unlock_deref(port, flags);
}
static void __uart_start(struct tty_struct *tty)
@@ -130,9 +131,9 @@ static void uart_start(struct tty_struct *tty)
struct uart_port *port;
unsigned long flags;
- port = uart_port_lock(state, flags);
+ port = uart_port_ref_lock(state, flags);
__uart_start(tty);
- uart_port_unlock(port, flags);
+ uart_port_unlock_deref(port, flags);
}
static void
@@ -141,12 +142,12 @@ uart_update_mctrl(struct uart_port *port, unsigned int set, unsigned int clear)
unsigned long flags;
unsigned int old;
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(&port->lock, flags);
old = port->mctrl;
port->mctrl = (old & ~clear) | set;
if (old != port->mctrl)
port->ops->set_mctrl(port, port->mctrl);
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(&port->lock, flags);
}
#define uart_set_mctrl(port, set) uart_update_mctrl(port, set, 0)
@@ -499,7 +500,7 @@ static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
/*
* Set modem status enables based on termios cflag
*/
- spin_lock_irq(&uport->lock);
+ uart_port_lock_irq(&uport->lock);
if (termios->c_cflag & CRTSCTS)
uport->status |= UPSTAT_CTS_ENABLE;
else
@@ -521,7 +522,7 @@ static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
if (hw_stopped)
__uart_start(tty);
}
- spin_unlock_irq(&uport->lock);
+ uart_port_unlock_irq(&uport->lock);
}
static int uart_put_char(struct tty_struct *tty, unsigned char c)
@@ -536,13 +537,13 @@ static int uart_put_char(struct tty_struct *tty, unsigned char c)
if (!circ->buf)
return 0;
- port = uart_port_lock(state, flags);
+ port = uart_port_ref_lock(state, flags);
if (port && uart_circ_chars_free(circ) != 0) {
circ->buf[circ->head] = c;
circ->head = (circ->head + 1) & (UART_XMIT_SIZE - 1);
ret = 1;
}
- uart_port_unlock(port, flags);
+ uart_port_unlock_deref(port, flags);
return ret;
}
@@ -573,7 +574,7 @@ static int uart_write(struct tty_struct *tty,
if (!circ->buf)
return 0;
- port = uart_port_lock(state, flags);
+ port = uart_port_ref_lock(state, flags);
while (port) {
c = CIRC_SPACE_TO_END(circ->head, circ->tail, UART_XMIT_SIZE);
if (count < c)
@@ -588,7 +589,7 @@ static int uart_write(struct tty_struct *tty,
}
__uart_start(tty);
- uart_port_unlock(port, flags);
+ uart_port_unlock_deref(port, flags);
return ret;
}
@@ -599,9 +600,9 @@ static int uart_write_room(struct tty_struct *tty)
unsigned long flags;
int ret;
- port = uart_port_lock(state, flags);
+ port = uart_port_ref_lock(state, flags);
ret = uart_circ_chars_free(&state->xmit);
- uart_port_unlock(port, flags);
+ uart_port_unlock_deref(port, flags);
return ret;
}
@@ -612,9 +613,9 @@ static int uart_chars_in_buffer(struct tty_struct *tty)
unsigned long flags;
int ret;
- port = uart_port_lock(state, flags);
+ port = uart_port_ref_lock(state, flags);
ret = uart_circ_chars_pending(&state->xmit);
- uart_port_unlock(port, flags);
+ uart_port_unlock_deref(port, flags);
return ret;
}
@@ -635,13 +636,13 @@ static void uart_flush_buffer(struct tty_struct *tty)
pr_debug("uart_flush_buffer(%d) called\n", tty->index);
- port = uart_port_lock(state, flags);
+ port = uart_port_ref_lock(state, flags);
if (!port)
return;
uart_circ_clear(&state->xmit);
if (port->ops->flush_buffer)
port->ops->flush_buffer(port);
- uart_port_unlock(port, flags);
+ uart_port_unlock_deref(port, flags);
tty_port_tty_wakeup(&state->port);
}
@@ -662,11 +663,11 @@ static void uart_send_xchar(struct tty_struct *tty, char ch)
if (port->ops->send_xchar)
port->ops->send_xchar(port, ch);
else {
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(&port->lock, flags);
port->x_char = ch;
if (ch)
port->ops->start_tx(port);
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(&port->lock, flags);
}
uart_port_deref(port);
}
@@ -1048,9 +1049,9 @@ static int uart_tiocmget(struct tty_struct *tty)
if (!tty_io_error(tty)) {
result = uport->mctrl;
- spin_lock_irq(&uport->lock);
+ uart_port_lock_irq(&uport->lock);
result |= uport->ops->get_mctrl(uport);
- spin_unlock_irq(&uport->lock);
+ uart_port_unlock_irq(&uport->lock);
}
out:
mutex_unlock(&port->mutex);
@@ -1186,16 +1187,16 @@ static int uart_wait_modem_status(struct uart_state *state, unsigned long arg)
uport = uart_port_ref(state);
if (!uport)
return -EIO;
- spin_lock_irq(&uport->lock);
+ uart_port_lock_irq(&uport->lock);
memcpy(&cprev, &uport->icount, sizeof(struct uart_icount));
uart_enable_ms(uport);
- spin_unlock_irq(&uport->lock);
+ uart_port_unlock_irq(&uport->lock);
add_wait_queue(&port->delta_msr_wait, &wait);
for (;;) {
- spin_lock_irq(&uport->lock);
+ uart_port_lock_irq(&uport->lock);
memcpy(&cnow, &uport->icount, sizeof(struct uart_icount));
- spin_unlock_irq(&uport->lock);
+ uart_port_unlock_irq(&uport->lock);
set_current_state(TASK_INTERRUPTIBLE);
@@ -1240,9 +1241,9 @@ static int uart_get_icount(struct tty_struct *tty,
uport = uart_port_ref(state);
if (!uport)
return -EIO;
- spin_lock_irq(&uport->lock);
+ uart_port_lock_irq(&uport->lock);
memcpy(&cnow, &uport->icount, sizeof(struct uart_icount));
- spin_unlock_irq(&uport->lock);
+ uart_port_unlock_irq(&uport->lock);
uart_port_deref(uport);
icount->cts = cnow.cts;
@@ -1266,9 +1267,9 @@ static int uart_get_rs485_config(struct uart_port *port,
unsigned long flags;
struct serial_rs485 aux;
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(&port->lock, flags);
aux = port->rs485;
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(&port->lock, flags);
if (copy_to_user(rs485, &aux, sizeof(aux)))
return -EFAULT;
@@ -1289,9 +1290,9 @@ static int uart_set_rs485_config(struct uart_port *port,
if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user)))
return -EFAULT;
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(&port->lock, flags);
ret = port->rs485_config(port, &rs485);
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(&port->lock, flags);
if (ret)
return ret;
@@ -1503,9 +1504,9 @@ static void uart_tty_port_shutdown(struct tty_port *port)
if (WARN(!uport, "detached port still initialized!\n"))
return;
- spin_lock_irq(&uport->lock);
+ uart_port_lock_irq(&uport->lock);
uport->ops->stop_rx(uport);
- spin_unlock_irq(&uport->lock);
+ uart_port_unlock_irq(&uport->lock);
uart_port_shutdown(port);
@@ -1659,10 +1660,10 @@ static int uart_carrier_raised(struct tty_port *port)
*/
if (WARN_ON(!uport))
return 1;
- spin_lock_irq(&uport->lock);
+ uart_port_lock_irq(&uport->lock);
uart_enable_ms(uport);
mctrl = uport->ops->get_mctrl(uport);
- spin_unlock_irq(&uport->lock);
+ uart_port_unlock_irq(&uport->lock);
uart_port_deref(uport);
if (mctrl & TIOCM_CAR)
return 1;
@@ -1770,9 +1771,9 @@ static void uart_line_info(struct seq_file *m, struct uart_driver *drv, int i)
pm_state = state->pm_state;
if (pm_state != UART_PM_STATE_ON)
uart_change_pm(state, UART_PM_STATE_ON);
- spin_lock_irq(&uport->lock);
+ uart_port_unlock_irq(&uport->lock);
status = uport->ops->get_mctrl(uport);
- spin_unlock_irq(&uport->lock);
+ uart_port_unlock_irq(&uport->lock);
if (pm_state != UART_PM_STATE_ON)
uart_change_pm(state, pm_state);
@@ -2099,11 +2100,11 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
tty_port_set_suspended(port, 1);
tty_port_set_initialized(port, 0);
- spin_lock_irq(&uport->lock);
+ uart_port_lock_irq(&uport->lock);
ops->stop_tx(uport);
ops->set_mctrl(uport, 0);
ops->stop_rx(uport);
- spin_unlock_irq(&uport->lock);
+ uart_port_unlock_irq(&uport->lock);
/*
* Wait for the transmitter to empty.
@@ -2179,9 +2180,9 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
int ret;
uart_change_pm(state, UART_PM_STATE_ON);
- spin_lock_irq(&uport->lock);
+ uart_port_lock_irq(&uport->lock);
ops->set_mctrl(uport, 0);
- spin_unlock_irq(&uport->lock);
+ uart_port_unlock_irq(&uport->lock);
if (console_suspend_enabled || !uart_console(uport)) {
/* Protected by port mutex for now */
struct tty_struct *tty = port->tty;
@@ -2189,10 +2190,10 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
if (ret == 0) {
if (tty)
uart_change_speed(tty, state, NULL);
- spin_lock_irq(&uport->lock);
+ uart_port_unlock_irq(&uport->lock);
ops->set_mctrl(uport, uport->mctrl);
ops->start_tx(uport);
- spin_unlock_irq(&uport->lock);
+ uart_port_unlock_irq(&uport->lock);
tty_port_set_initialized(port, 1);
} else {
/*
@@ -2286,9 +2287,9 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
* keep the DTR setting that is set in uart_set_options()
* We probably don't need a spinlock around this, but
*/
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(&port->lock, flags);
port->ops->set_mctrl(port, port->mctrl & TIOCM_DTR);
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(&port->lock, flags);
/*
* If this driver supports console, and it hasn't been
--
2.17.1
Do not directly lock/unlock uart_port->lock, but use uart_port_lock
helper macros.
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
drivers/tty/serial/8250/8250_core.c | 8 ++--
drivers/tty/serial/8250/8250_dma.c | 4 +-
drivers/tty/serial/8250/8250_port.c | 74 ++++++++++++++---------------
3 files changed, 43 insertions(+), 43 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 9342fc2ee7df..ce7aa601c809 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -272,7 +272,7 @@ static void serial8250_backup_timeout(struct timer_list *t)
unsigned int iir, ier = 0, lsr;
unsigned long flags;
- spin_lock_irqsave(&up->port.lock, flags);
+ uart_port_lock_irqsave(&up->port.lock, flags);
/*
* Must disable interrupts or else we risk racing with the interrupt
@@ -306,7 +306,7 @@ static void serial8250_backup_timeout(struct timer_list *t)
if (up->port.irq)
serial_out(up, UART_IER, ier);
- spin_unlock_irqrestore(&up->port.lock, flags);
+ uart_port_unlock_irqrestore(&up->port.lock, flags);
/* Standard timer interval plus 0.2s to keep the port running */
mod_timer(&up->timer,
@@ -1078,9 +1078,9 @@ void serial8250_unregister_port(int line)
if (uart->em485) {
unsigned long flags;
- spin_lock_irqsave(&uart->port.lock, flags);
+ uart_port_lock_irqsave(&uart->port.lock, flags);
serial8250_em485_destroy(uart);
- spin_unlock_irqrestore(&uart->port.lock, flags);
+ uart_port_unlock_irqrestore(&uart->port.lock, flags);
}
uart_remove_one_port(&serial8250_reg, &uart->port);
diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index bfa1a857f3ff..ec601779c32f 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -22,7 +22,7 @@ static void __dma_tx_complete(void *param)
dma_sync_single_for_cpu(dma->txchan->device->dev, dma->tx_addr,
UART_XMIT_SIZE, DMA_TO_DEVICE);
- spin_lock_irqsave(&p->port.lock, flags);
+ uart_port_lock_irqsave(&p->port.lock, flags);
dma->tx_running = 0;
@@ -39,7 +39,7 @@ static void __dma_tx_complete(void *param)
serial_port_out(&p->port, UART_IER, p->ier);
}
- spin_unlock_irqrestore(&p->port.lock, flags);
+ uart_port_unlock_irqrestore(&p->port.lock, flags);
}
static void __dma_rx_complete(void *param)
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index cf541aab2bd0..a1ee17870ebc 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -772,9 +772,9 @@ static void enable_rsa(struct uart_8250_port *up)
{
if (up->port.type == PORT_RSA) {
if (up->port.uartclk != SERIAL_RSA_BAUD_BASE * 16) {
- spin_lock_irq(&up->port.lock);
+ uart_port_lock_irq(&up->port.lock);
__enable_rsa(up);
- spin_unlock_irq(&up->port.lock);
+ uart_port_unlock_irq(&up->port.lock);
}
if (up->port.uartclk == SERIAL_RSA_BAUD_BASE * 16)
serial_out(up, UART_RSA_FRR, 0);
@@ -794,7 +794,7 @@ static void disable_rsa(struct uart_8250_port *up)
if (up->port.type == PORT_RSA &&
up->port.uartclk == SERIAL_RSA_BAUD_BASE * 16) {
- spin_lock_irq(&up->port.lock);
+ uart_port_lock_irq(&up->port.lock);
mode = serial_in(up, UART_RSA_MSR);
result = !(mode & UART_RSA_MSR_FIFO);
@@ -807,7 +807,7 @@ static void disable_rsa(struct uart_8250_port *up)
if (result)
up->port.uartclk = SERIAL_RSA_BAUD_BASE_LO * 16;
- spin_unlock_irq(&up->port.lock);
+ uart_port_unlock_irq(&up->port.lock);
}
}
#endif /* CONFIG_SERIAL_8250_RSA */
@@ -1218,7 +1218,7 @@ static void autoconfig(struct uart_8250_port *up)
* We really do need global IRQs disabled here - we're going to
* be frobbing the chips IRQ enable register to see if it exists.
*/
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(&port->lock, flags);
up->capabilities = 0;
up->bugs = 0;
@@ -1257,7 +1257,7 @@ static void autoconfig(struct uart_8250_port *up)
/*
* We failed; there's nothing here
*/
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(&port->lock, flags);
DEBUG_AUTOCONF("IER test failed (%02x, %02x) ",
scratch2, scratch3);
goto out;
@@ -1281,7 +1281,7 @@ static void autoconfig(struct uart_8250_port *up)
status1 = serial_in(up, UART_MSR) & 0xF0;
serial8250_out_MCR(up, save_mcr);
if (status1 != 0x90) {
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(&port->lock, flags);
DEBUG_AUTOCONF("LOOP test failed (%02x) ",
status1);
goto out;
@@ -1354,7 +1354,7 @@ static void autoconfig(struct uart_8250_port *up)
serial_out(up, UART_IER, 0);
out_lock:
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(&port->lock, flags);
/*
* Check if the device is a Fintek F81216A
@@ -1466,12 +1466,12 @@ static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t)
p = em485->port;
serial8250_rpm_get(p);
- spin_lock_irqsave(&p->port.lock, flags);
+ uart_port_lock_irqsave(&p->port.lock, flags);
if (em485->active_timer == &em485->stop_tx_timer) {
__do_stop_tx_rs485(p);
em485->active_timer = NULL;
}
- spin_unlock_irqrestore(&p->port.lock, flags);
+ uart_port_unlock_irqrestore(&p->port.lock, flags);
serial8250_rpm_put(p);
return HRTIMER_NORESTART;
}
@@ -1620,12 +1620,12 @@ static enum hrtimer_restart serial8250_em485_handle_start_tx(struct hrtimer *t)
em485 = container_of(t, struct uart_8250_em485, start_tx_timer);
p = em485->port;
- spin_lock_irqsave(&p->port.lock, flags);
+ uart_port_lock_irqsave(&p->port.lock, flags);
if (em485->active_timer == &em485->start_tx_timer) {
__start_tx(&p->port);
em485->active_timer = NULL;
}
- spin_unlock_irqrestore(&p->port.lock, flags);
+ uart_port_unlock_irqrestore(&p->port.lock, flags);
return HRTIMER_NORESTART;
}
@@ -1867,7 +1867,7 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
if (iir & UART_IIR_NO_INT)
return 0;
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(&port->lock, flags);
status = serial_port_in(port, UART_LSR);
@@ -1880,7 +1880,7 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
if ((!up->dma || up->dma->tx_err) && (status & UART_LSR_THRE))
serial8250_tx_chars(up);
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(&port->lock, flags);
return 1;
}
EXPORT_SYMBOL_GPL(serial8250_handle_irq);
@@ -1915,9 +1915,9 @@ static int serial8250_tx_threshold_handle_irq(struct uart_port *port)
if ((iir & UART_IIR_ID) == UART_IIR_THRI) {
struct uart_8250_port *up = up_to_u8250p(port);
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(&port->lock, flags);
serial8250_tx_chars(up);
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(&port->lock, flags);
}
iir = serial_port_in(port, UART_IIR);
@@ -1932,10 +1932,10 @@ static unsigned int serial8250_tx_empty(struct uart_port *port)
serial8250_rpm_get(up);
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(&port->lock, flags);
lsr = serial_port_in(port, UART_LSR);
up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(&port->lock, flags);
serial8250_rpm_put(up);
@@ -2008,13 +2008,13 @@ static void serial8250_break_ctl(struct uart_port *port, int break_state)
unsigned long flags;
serial8250_rpm_get(up);
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(&port->lock, flags);
if (break_state == -1)
up->lcr |= UART_LCR_SBC;
else
up->lcr &= ~UART_LCR_SBC;
serial_port_out(port, UART_LCR, up->lcr);
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(&port->lock, flags);
serial8250_rpm_put(up);
}
@@ -2266,7 +2266,7 @@ int serial8250_do_startup(struct uart_port *port)
* the interrupt is enabled. Delays are necessary to
* allow register changes to become visible.
*/
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(&port->lock, flags);
if (up->port.irqflags & IRQF_SHARED)
disable_irq_nosync(port->irq);
@@ -2282,7 +2282,7 @@ int serial8250_do_startup(struct uart_port *port)
if (port->irqflags & IRQF_SHARED)
enable_irq(port->irq);
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(&port->lock, flags);
/*
* If the interrupt is not reasserted, or we otherwise
@@ -2304,7 +2304,7 @@ int serial8250_do_startup(struct uart_port *port)
*/
serial_port_out(port, UART_LCR, UART_LCR_WLEN8);
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(&port->lock, flags);
if (up->port.flags & UPF_FOURPORT) {
if (!up->port.irq)
up->port.mctrl |= TIOCM_OUT1;
@@ -2351,7 +2351,7 @@ int serial8250_do_startup(struct uart_port *port)
}
dont_test_tx_en:
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(&port->lock, flags);
/*
* Clear the interrupt registers again for luck, and clear the
@@ -2418,17 +2418,17 @@ void serial8250_do_shutdown(struct uart_port *port)
/*
* Disable interrupts from this port
*/
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(&port->lock, flags);
up->ier = 0;
serial_port_out(port, UART_IER, 0);
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(&port->lock, flags);
synchronize_irq(port->irq);
if (up->dma)
serial8250_release_dma(up);
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(&port->lock, flags);
if (port->flags & UPF_FOURPORT) {
/* reset interrupts on the AST Fourport board */
inb((port->iobase & 0xfe0) | 0x1f);
@@ -2437,7 +2437,7 @@ void serial8250_do_shutdown(struct uart_port *port)
port->mctrl &= ~TIOCM_OUT2;
serial8250_set_mctrl(port, port->mctrl);
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(&port->lock, flags);
/*
* Disable break condition and FIFOs
@@ -2643,7 +2643,7 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
* interrupts disabled.
*/
serial8250_rpm_get(up);
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(&port->lock, flags);
up->lcr = cval; /* Save computed LCR */
@@ -2747,7 +2747,7 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
serial_port_out(port, UART_FCR, up->fcr); /* set fcr */
}
serial8250_set_mctrl(port, port->mctrl);
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(&port->lock, flags);
serial8250_rpm_put(up);
/* Don't rewrite B0 */
@@ -2770,15 +2770,15 @@ void serial8250_do_set_ldisc(struct uart_port *port, struct ktermios *termios)
{
if (termios->c_line == N_PPS) {
port->flags |= UPF_HARDPPS_CD;
- spin_lock_irq(&port->lock);
+ uart_port_lock_irq(&port->lock);
serial8250_enable_ms(port);
- spin_unlock_irq(&port->lock);
+ uart_port_unlock_irq(&port->lock);
} else {
port->flags &= ~UPF_HARDPPS_CD;
if (!UART_ENABLE_MS(port, termios->c_cflag)) {
- spin_lock_irq(&port->lock);
+ uart_port_lock_irq(&port->lock);
serial8250_disable_ms(port);
- spin_unlock_irq(&port->lock);
+ uart_port_unlock_irq(&port->lock);
}
}
}
@@ -3225,9 +3225,9 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
if (port->sysrq)
locked = 0;
else if (oops_in_progress)
- locked = spin_trylock_irqsave(&port->lock, flags);
+ locked = uart_port_trylock_irqsave(&port->lock, flags);
else
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(&port->lock, flags);
/*
* First save the IER then disable the interrupts
@@ -3265,7 +3265,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
serial8250_modem_status(up);
if (locked)
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(&port->lock, flags);
serial8250_rpm_put(up);
}
--
2.17.1
Do not directly lock/unlock tty_port->lock, but use tty_port_lock
helper macros.
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
drivers/tty/pty.c | 4 ++--
drivers/tty/serial/serial_core.c | 8 +++----
drivers/tty/tty_port.c | 38 ++++++++++++++++----------------
3 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index b0e2c4847a5d..dddeecc2289e 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -116,13 +116,13 @@ static int pty_write(struct tty_struct *tty, const unsigned char *buf, int c)
return 0;
if (c > 0) {
- spin_lock_irqsave(&to->port->lock, flags);
+ tty_port_lock_irqsave(&to->port->lock, flags);
/* Stuff the data into the input queue of the other end */
c = tty_insert_flip_string(to->port, buf, c);
/* And shovel */
if (c)
tty_flip_buffer_push(to->port);
- spin_unlock_irqrestore(&to->port->lock, flags);
+ tty_port_unlock_irqrestore(&to->port->lock, flags);
}
return c;
}
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 9c14a453f73c..48b3377d7f77 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1480,9 +1480,9 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
state = drv->state + tty->index;
port = &state->port;
- spin_lock_irq(&port->lock);
+ tty_port_lock_irq(&port->lock);
--port->count;
- spin_unlock_irq(&port->lock);
+ tty_port_unlock_irq(&port->lock);
return;
}
@@ -1603,9 +1603,9 @@ static void uart_hangup(struct tty_struct *tty)
if (tty_port_active(port)) {
uart_flush_buffer(tty);
uart_shutdown(tty, state);
- spin_lock_irqsave(&port->lock, flags);
+ tty_port_lock_irqsave(&port->lock, flags);
port->count = 0;
- spin_unlock_irqrestore(&port->lock, flags);
+ tty_port_unlock_irqrestore(&port->lock, flags);
tty_port_set_active(port, 0);
tty_port_tty_set(port, NULL);
if (uport && !uart_console(uport))
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 25d736880013..4237d282f89f 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -285,9 +285,9 @@ struct tty_struct *tty_port_tty_get(struct tty_port *port)
unsigned long flags;
struct tty_struct *tty;
- spin_lock_irqsave(&port->lock, flags);
+ tty_port_lock_irqsave(&port->lock, flags);
tty = tty_kref_get(port->tty);
- spin_unlock_irqrestore(&port->lock, flags);
+ tty_port_unlock_irqrestore(&port->lock, flags);
return tty;
}
EXPORT_SYMBOL(tty_port_tty_get);
@@ -305,10 +305,10 @@ void tty_port_tty_set(struct tty_port *port, struct tty_struct *tty)
{
unsigned long flags;
- spin_lock_irqsave(&port->lock, flags);
+ tty_port_lock_irqsave(&port->lock, flags);
tty_kref_put(port->tty);
port->tty = tty_kref_get(tty);
- spin_unlock_irqrestore(&port->lock, flags);
+ tty_port_unlock_irqrestore(&port->lock, flags);
}
EXPORT_SYMBOL(tty_port_tty_set);
@@ -349,13 +349,13 @@ void tty_port_hangup(struct tty_port *port)
struct tty_struct *tty;
unsigned long flags;
- spin_lock_irqsave(&port->lock, flags);
+ tty_port_lock_irqsave(&port->lock, flags);
port->count = 0;
tty = port->tty;
if (tty)
set_bit(TTY_IO_ERROR, &tty->flags);
port->tty = NULL;
- spin_unlock_irqrestore(&port->lock, flags);
+ tty_port_unlock_irqrestore(&port->lock, flags);
tty_port_set_active(port, 0);
tty_port_shutdown(port, tty);
tty_kref_put(tty);
@@ -496,10 +496,10 @@ int tty_port_block_til_ready(struct tty_port *port,
retval = 0;
/* The port lock protects the port counts */
- spin_lock_irqsave(&port->lock, flags);
+ tty_port_lock_irqsave(&port->lock, flags);
port->count--;
port->blocked_open++;
- spin_unlock_irqrestore(&port->lock, flags);
+ tty_port_unlock_irqrestore(&port->lock, flags);
while (1) {
/* Indicate we are open */
@@ -536,11 +536,11 @@ int tty_port_block_til_ready(struct tty_port *port,
/* Update counts. A parallel hangup will have set count to zero and
we must not mess that up further */
- spin_lock_irqsave(&port->lock, flags);
+ tty_port_lock_irqsave(&port->lock, flags);
if (!tty_hung_up_p(filp))
port->count++;
port->blocked_open--;
- spin_unlock_irqrestore(&port->lock, flags);
+ tty_port_unlock_irqrestore(&port->lock, flags);
if (retval == 0)
tty_port_set_active(port, 1);
return retval;
@@ -570,7 +570,7 @@ int tty_port_close_start(struct tty_port *port,
if (tty_hung_up_p(filp))
return 0;
- spin_lock_irqsave(&port->lock, flags);
+ tty_port_lock_irqsave(&port->lock, flags);
if (tty->count == 1 && port->count != 1) {
tty_warn(tty, "%s: tty->count = 1 port count = %d\n", __func__,
port->count);
@@ -583,10 +583,10 @@ int tty_port_close_start(struct tty_port *port,
}
if (port->count) {
- spin_unlock_irqrestore(&port->lock, flags);
+ tty_port_unlock_irqrestore(&port->lock, flags);
return 0;
}
- spin_unlock_irqrestore(&port->lock, flags);
+ tty_port_unlock_irqrestore(&port->lock, flags);
tty->closing = 1;
@@ -615,16 +615,16 @@ void tty_port_close_end(struct tty_port *port, struct tty_struct *tty)
tty_ldisc_flush(tty);
tty->closing = 0;
- spin_lock_irqsave(&port->lock, flags);
+ tty_port_lock_irqsave(&port->lock, flags);
if (port->blocked_open) {
- spin_unlock_irqrestore(&port->lock, flags);
+ tty_port_unlock_irqrestore(&port->lock, flags);
if (port->close_delay)
msleep_interruptible(jiffies_to_msecs(port->close_delay));
- spin_lock_irqsave(&port->lock, flags);
+ tty_port_lock_irqsave(&port->lock, flags);
wake_up_interruptible(&port->open_wait);
}
- spin_unlock_irqrestore(&port->lock, flags);
+ tty_port_unlock_irqrestore(&port->lock, flags);
tty_port_set_active(port, 0);
}
EXPORT_SYMBOL(tty_port_close_end);
@@ -675,9 +675,9 @@ EXPORT_SYMBOL_GPL(tty_port_install);
int tty_port_open(struct tty_port *port, struct tty_struct *tty,
struct file *filp)
{
- spin_lock_irq(&port->lock);
+ tty_port_lock_irq(&port->lock);
++port->count;
- spin_unlock_irq(&port->lock);
+ tty_port_unlock_irq(&port->lock);
tty_port_tty_set(port, tty);
/*
--
2.17.1
On (06/15/18 18:39), Sergey Senozhatsky wrote:
> Make printk_safe_enter_irqsave()/etc macros available to the
> rest of the kernel, so we can use them in TTY and UART code.
Sorry, this patch is incomplete. Since we are going to use printk_safe()
API in modules (serial console drivers) printk_safe() functions [enter and
exit] must be exported via EXPORT_SYMBOL_GPL().
-ss
> It doesn't come as a surprise that recursive printk() calls are not the
> only way for us to deadlock in printk() and we still have a whole bunch
> of other printk() deadlock scenarios. For instance, those that involve
> TTY port->lock spin_lock and UART port->lock spin_lock.
The tty layer code there is not re-entrant. Nor is it supposed to be
> So the idea of this patch set is to take tty_port->lock and
> uart_port->lock from printk_safe context and to eliminate some
> of non-recursive printk() deadlocks - the ones that don't start
> in printk(), but involve console related locks and thus eventually
> deadlock us in printk(). For this purpose the patch set introduces
> several helper macros:
I don't see how this helps - if you recurse into the uart code you are
still hitting the paths that are unsafe when re-entered. All you've done
is messed up a pile of locking code on critical performance paths.
As it stands I think it's a bad idea.
> Of course, TTY and UART port spin_locks are not the only locks that
> we can deadlock on. So this patch set does not address all deadlock
> scenarios, it just makes a small step forward.
>
> Any opinions?
The cure is worse than the disease.
The only case that's worth looking at is the direct polled console code
paths. The moment you touch the other layers you add essentially never
needed code to hot paths.
Given printk nowdays is already somewhat unreliable with all the perf
related changes, and we have other good debug tools I think it would be
far cleaner to have some kind of
if (spin_trylock(...)) {
console_defer(buffer);
return;
}
helper layer in the printk/console logic, at least for the non panic/oops
cases.
Alan
Thanks for taking a look!
On (06/18/18 14:38), Alan Cox wrote:
> > It doesn't come as a surprise that recursive printk() calls are not the
> > only way for us to deadlock in printk() and we still have a whole bunch
> > of other printk() deadlock scenarios. For instance, those that involve
> > TTY port->lock spin_lock and UART port->lock spin_lock.
>
> The tty layer code there is not re-entrant. Nor is it supposed to be
Could be.
But at least we have circular locking dependency in tty,
see [1] for more details:
tty_port->lock => uart_port->lock
CPU0
tty
spin_lock(&tty_port->lock)
printk()
call_console_drivers()
foo_console_write()
spin_lock(&uart_port->lock)
Whereas we normally have
uart_port->lock => tty_port->lock
CPU1
IRQ
foo_console_handle_IRQ()
spin_lock(&uart_port->lock)
tty
spin_lock(&tty_port->lock)
If we switch to printk_safe when we take tty_port->lock then we
remove the printk->uart_port chain from the picture.
> > So the idea of this patch set is to take tty_port->lock and
> > uart_port->lock from printk_safe context and to eliminate some
> > of non-recursive printk() deadlocks - the ones that don't start
> > in printk(), but involve console related locks and thus eventually
> > deadlock us in printk(). For this purpose the patch set introduces
> > several helper macros:
>
> I don't see how this helps - if you recurse into the uart code you are
> still hitting the paths that are unsafe when re-entered. All you've done
> is messed up a pile of locking code on critical performance paths.
>
> As it stands I think it's a bad idea.
The only new thing is that we inc/dec per-CPU printk context
variable when we lock/unlock tty/uart port lock:
printk_safe_enter() -> this_cpu_inc(printk_context);
printk_safe_exit() -> this_cpu_dec(printk_context);
How does this help? Suppose we have the following
IRQ
foo_console_handle_IRQ()
spin_lock(&uart_port->lock)
uart_write_wakeup()
tty_port_tty_wakeup()
tty_port_default_wakeup()
printk()
call_console_drivers()
foo_console_write()
spin_lock(&uart_port->lock) << deadlock
If we take uart_port lock from printk_safe context, we remove the
printk->call_console_drivers->foo_console_write->spin_lock
chain. Because printk() output will endup in a per-CPU buffer,
which will be flushed later from irq_work. So the whole thing
becomes:
IRQ
foo_console_handle_IRQ()
printk_safe_enter()
spin_lock(&uart_port->lock)
uart_write_wakeup()
tty_port_tty_wakeup()
tty_port_default_wakeup()
printk() << we don't re-enter foo_console_driver
<< from printk() anymore
printk_safe_log_store()
irq_work_queue
spin_unlock(&uart_port->lock)
printk_safe_exit()
iret
#flush per-CPU buffer
IRQ
printk_safe_flush_buffer()
vprintk_deferred()
> > Of course, TTY and UART port spin_locks are not the only locks that
> > we can deadlock on. So this patch set does not address all deadlock
> > scenarios, it just makes a small step forward.
> >
> > Any opinions?
>
> The cure is worse than the disease.
Because of this_cpu_inc(printk_context) / this_cpu_dec(printk_context)?
May be. That's why I put RFC :)
> The only case that's worth looking at is the direct polled console code
> paths. The moment you touch the other layers you add essentially never
> needed code to hot paths.
>
> Given printk nowdays is already somewhat unreliable with all the perf
> related changes, and we have other good debug tools I think it would be
> far cleaner to have some kind of
>
>
> if (spin_trylock(...)) {
> console_defer(buffer);
> return;
> }
>
> helper layer in the printk/console logic, at least for the non panic/oops
> cases.
spin_trylock() in every ->foo_console_write() callback?
This still will not address the reported deadlock [1].
[1] lkml.kernel.org/r/[email protected]
-ss
On Mon, Jun 18, 2018 at 02:38:18PM +0100, Alan Cox wrote:
> Given printk nowdays is already somewhat unreliable with all the perf
> related changes,
printk is a steaming pile of @#$#@, unreliable doesn't even begin to
cover it.
> and we have other good debug tools
What other good debug tools do you have? The only thing I have that
actually works is earlyser/8250_early -- and I route everything printk
into that.
On Tue 2018-06-19 09:53:08, Sergey Senozhatsky wrote:
> Thanks for taking a look!
>
> On (06/18/18 14:38), Alan Cox wrote:
> > > It doesn't come as a surprise that recursive printk() calls are not the
> > > only way for us to deadlock in printk() and we still have a whole bunch
> > > of other printk() deadlock scenarios. For instance, those that involve
> > > TTY port->lock spin_lock and UART port->lock spin_lock.
> >
> > The tty layer code there is not re-entrant. Nor is it supposed to be
It is re-entrant via printk(). I mean that any printk() called inside
the locked sections might cause recursion if the same lock is needed
also by con->write() callbacks.
> Could be.
>
> But at least we have circular locking dependency in tty,
> see [1] for more details:
>
> tty_port->lock => uart_port->lock
>
> CPU0
> tty
> spin_lock(&tty_port->lock)
> printk()
> call_console_drivers()
> foo_console_write()
> spin_lock(&uart_port->lock)
>
> Whereas we normally have
>
> uart_port->lock => tty_port->lock
>
> CPU1
> IRQ
> foo_console_handle_IRQ()
> spin_lock(&uart_port->lock)
> tty
> spin_lock(&tty_port->lock)
This is even more complicated situation because printk() allowed
an ABBA lock scenario.
> If we switch to printk_safe when we take tty_port->lock then we
> remove the printk->uart_port chain from the picture.
>
> > > So the idea of this patch set is to take tty_port->lock and
> > > uart_port->lock from printk_safe context and to eliminate some
> > > of non-recursive printk() deadlocks - the ones that don't start
> > > in printk(), but involve console related locks and thus eventually
> > > deadlock us in printk(). For this purpose the patch set introduces
> > > several helper macros:
> >
> > I don't see how this helps - if you recurse into the uart code you are
> > still hitting the paths that are unsafe when re-entered. All you've done
> > is messed up a pile of locking code on critical performance paths.
> >
> > As it stands I think it's a bad idea.
>
> The only new thing is that we inc/dec per-CPU printk context
> variable when we lock/unlock tty/uart port lock:
>
> printk_safe_enter() -> this_cpu_inc(printk_context);
> printk_safe_exit() -> this_cpu_dec(printk_context);
To make it clear. This patch set adds yet another spin_lock API.
It behaves exactly as spin_lock_irqsafe()/spin_unlock_irqrestore()
but in addition it sets printk_context.
Where printk_context defines what printk implementation is safe. We
basically have four possibilities:
1. normal (store in logbuf, try to handle consoles)
2. deferred (store in logbuf, deffer consoles)
3. safe (store in per-CPU buffer, deffer everything)
4. safe_nmi (store in another per-CPU buffer, deffer everything)
This patchset forces safe context around TTY and UART locks.
In fact, the deferred context would be enough to prevent
all the mentioned deadlocks.
IMHO, the only question is if people might get familiar with
yet another spin_lock API.
Best Regards,
Petr
On (06/19/18 10:30), Petr Mladek wrote:
> It is re-entrant via printk(). I mean that any printk() called inside
> the locked sections might cause recursion if the same lock is needed
> also by con->write() callbacks.
Perhaps Alan meant that we cannot return back to tty once we passed
the control from tty to printk -> uart serial console. So tty is
probably (but I didn't check) not re-entrant, but uart definitely is
re-entrant: IRQ -> uart console -> tty -> printk -> uart console.
The patch set attempts to address several issues, and re-entrant uart
is just one of them.
[..]
> This patchset forces safe context around TTY and UART locks.
Right.
> In fact, the deferred context would be enough to prevent
> all the mentioned deadlocks.
Agree.
But we can leave it as a printk_safe implementation detail and
change it later, outside of this patch, to avoid further confusion.
> IMHO, the only question is if people might get familiar with
> yet another spin_lock API.
Right. That's why I thought about renaming uart_port and tty_port
->lock to ->____lock. So the naming will suggest [hopefully] that
those locks are not meant to be used directly [anymore] and instead
people should use uart_port_lock/tty_port_lock API.
-ss
On Tue, Jun 19, 2018 at 5:30 PM Petr Mladek <[email protected]> wrote:
>
> To make it clear. This patch set adds yet another spin_lock API.
> It behaves exactly as spin_lock_irqsafe()/spin_unlock_irqrestore()
> but in addition it sets printk_context.
>
> This patchset forces safe context around TTY and UART locks.
> In fact, the deferred context would be enough to prevent
> all the mentioned deadlocks.
Please stop this garbage.
The rule is simple: DO NOT DO THAT THEN.
Don't make recursive locks. Don't make random complexity. Just stop
doing the thing that hurts.
There is no valid reason why an UART driver should do a printk() of
any sort inside the critical region where the console is locked.
Just remove those printk's, don't add new crazy locking.
If you had a spinlock that deadlocked because it was inside an already
spinlocked region, you'd say "that's buggy".
This is the exact same issue. We don't work around buggy garbage. We
fix the bug - by removing the problematic printk.
Linus
On Wed, 20 Jun 2018 11:01:49 +0900
Linus Torvalds <[email protected]> wrote:
> There is no valid reason why an UART driver should do a printk() of
> any sort inside the critical region where the console is locked.
>
> Just remove those printk's, don't add new crazy locking.
Perhaps we should do an audit of the console drivers and remove all
printk, pr_* , WARN*, BUG* from them.
-- Steve
On Wed, Jun 20, 2018 at 11:34 AM Steven Rostedt <[email protected]> wrote:
>
> Perhaps we should do an audit of the console drivers and remove all
> printk, pr_* , WARN*, BUG* from them.
Only the actual _printing_ parts.
Just randomly, look at drivers/tty/vt/vt.c that does a lot of
printing, and there's a lot of valid printing. Things like
pr_warn("Unable to create device for %s; errno = %ld\n", ..
is fine - it's done at console registration time if things go sideways.
But there are a few commented-out calls to "printk()" that are
obviously bogus, because they are in the printing path.
And they damn well should be commented out. The existence of something
like that SHOULD ABSOLUTELY NOT be seen as a "hey, let's make up some
crazy garbage locking ruls that would allow printing there".
Just don't do it. It's that simple.
Linus
On (06/20/18 11:01), Linus Torvalds wrote:
> On Tue, Jun 19, 2018 at 5:30 PM Petr Mladek <[email protected]> wrote:
> >
> > To make it clear. This patch set adds yet another spin_lock API.
> > It behaves exactly as spin_lock_irqsafe()/spin_unlock_irqrestore()
> > but in addition it sets printk_context.
> >
> > This patchset forces safe context around TTY and UART locks.
> > In fact, the deferred context would be enough to prevent
> > all the mentioned deadlocks.
>
> Please stop this garbage.
I'm guessing the message was addressed to me, not to Petr.
Let me explain myself.
> The rule is simple: DO NOT DO THAT THEN.
>
> Don't make recursive locks. Don't make random complexity. Just stop
> doing the thing that hurts.
We didn't add any of those recursive printk()-s. Those are the things
like kmalloc()/scheduler/etc which tty/etc invoke that hurt.
> There is no valid reason why an UART driver should do a printk() of
> any sort inside the critical region where the console is locked.
It's not UART on its own that immediately calls into printk(), that would
be trivial to fix, it's all those subsystems that serial console driver
can call into.
For instance, kernel/workqueue.c - it may WARN_ON/printk in various cases.
And those WARNs/printks are OK. Except for one thing: workqueue can be
called from a serial console driver, which suddenly will turn those
WARNs/printks into illegal ones, due to possible deadlocks. And serial
consoles can call into WQ. Not directly, but via tty code.
A random example:
s3c24xx_serial_rx_chars_dma()
spin_lock_irqsave(&port->lock, flags);
tty_flip_buffer_push()
__queue_work()
spin_unlock_irqrestore(&port->lock, flags);
Should for some reason WQ warn/printk, we are done, because we will
end up taking the same port->lock:
__queue_work()
printk()
call_console_drivers()
spin_lock_irqsave(&port->lock, flags);
IOW, there is this tricky "we were called from a serial driver" context,
which is hard to track, but printk_safe can help us in those cases. There
are other examples. WQ is not the only thing serial drivers can call into.
So that's why I sent the patch set.
-ss
On (06/19/18 22:34), Steven Rostedt wrote:
>
> > There is no valid reason why an UART driver should do a printk() of
> > any sort inside the critical region where the console is locked.
> >
> > Just remove those printk's, don't add new crazy locking.
>
> Perhaps we should do an audit of the console drivers and remove all
> printk, pr_* , WARN*, BUG* from them.
I think I did a terrible job explaining my motivation.
Sorry for that!
What I tried to address with my patch set was not a direct uart->printk,
but mostly all those
uart-> tty / core kernel / who knows what else -> printk
cases. When are in that special context "called from uart driver" which
can backfire on us.
-ss
On Wed, Jun 20, 2018 at 11:50 AM Sergey Senozhatsky
<[email protected]> wrote:
>
> It's not UART on its own that immediately calls into printk(), that would
> be trivial to fix, it's all those subsystems that serial console driver
> can call into.
We already have the whole PRINTK_SAFE_CONTEXT_MASK model that only
adds it to a secondary buffer if you get recursion. Why isn't that
triggering? That's the whole point of it.
I absolutely do *not* want to see any crazy changes to tty drivers. No no no.
Linus
On (06/20/18 12:38), Linus Torvalds wrote:
> On Wed, Jun 20, 2018 at 11:50 AM Sergey Senozhatsky
> <[email protected]> wrote:
> >
> > It's not UART on its own that immediately calls into printk(), that would
> > be trivial to fix, it's all those subsystems that serial console driver
> > can call into.
>
> We already have the whole PRINTK_SAFE_CONTEXT_MASK model that only
> adds it to a secondary buffer if you get recursion. Why isn't that
> triggering? That's the whole point of it.
This is exactly what I'm doing in my patch set.
PRINTK_SAFE_CONTEXT_MASK so far worked *one* way only: when we start
from printk.c
IOW:
printk -> printk_safe_mask -> vsprinf -> printk
But we also can have printk-related deadlocks the *other* way
around. For instance:
uart -> printk -> uart
printk_safe_mask is not triggering there because we don't use
printk_safe in uart / tty yet. And this is what I do in my
patch set - extend printk_safe usage.
The patch set does not add any _new_ locks or locking rules.
It just replaces the existing
spin_lock(a)
with
prinkt_safe_enter();
spin_lock(a)
and
spin_unlock(a)
with
spin_unlock(a)
printk_safe_exit();
and that's it.
So now we use printk_safe mechanism to avoid another bunch of
deadlock scenarious: which don't start from printk, but from
parts of the kernel which printk eventually calls.
-ss
On Wed, 20 Jun 2018 11:44:13 +0900
Linus Torvalds <[email protected]> wrote:
> On Wed, Jun 20, 2018 at 11:34 AM Steven Rostedt <[email protected]> wrote:
> >
> > Perhaps we should do an audit of the console drivers and remove all
> > printk, pr_* , WARN*, BUG* from them.
>
> Only the actual _printing_ parts.
No because they are normally rather useful because that port isn't the
console. If you trylock it as I suggested then at least you'd just drop
the recursive cases and get messages otherwise.
Really that's all that you need - log the message to whichever console
targets you can currently safely do so. If it's none well there was
always the proposed morse code keyboard light driver 8)
Alan
On Fri, Jun 22, 2018 at 05:21:00PM +0100, Alan Cox wrote:
> Really that's all that you need - log the message to whichever console
> targets you can currently safely do so. If it's none well there was
> always the proposed morse code keyboard light driver 8)
Only if your keyboard still has blinky lights on.. (mine doesn't)
On (06/22/18 17:21), Alan Cox wrote:
> On Wed, 20 Jun 2018 11:44:13 +0900
> Linus Torvalds <[email protected]> wrote:
>
> > On Wed, Jun 20, 2018 at 11:34 AM Steven Rostedt <[email protected]> wrote:
> > >
> > > Perhaps we should do an audit of the console drivers and remove all
> > > printk, pr_* , WARN*, BUG* from them.
> >
> > Only the actual _printing_ parts.
>
> No because they are normally rather useful because that port isn't the
> console. If you trylock
trylock is boring, me wants printk_safe_mask everywhere :)
> Really that's all that you need - log the message to whichever console
> targets you can currently safely do so. If it's none well there was
> always the proposed morse code keyboard light driver 8)
Hm, just discard messages? With printk_safe_mask we keep everything
in a lockless per-CPU buffer, which we flush [per-CPU buffer -> printk logbuf]
from irq_work, so we can print it later.
-ss
On (06/20/18 12:38), Linus Torvalds wrote:
> On Wed, Jun 20, 2018 at 11:50 AM Sergey Senozhatsky
> <[email protected]> wrote:
> >
> > It's not UART on its own that immediately calls into printk(), that would
> > be trivial to fix, it's all those subsystems that serial console driver
> > can call into.
>
> We already have the whole PRINTK_SAFE_CONTEXT_MASK model that only
> adds it to a secondary buffer if you get recursion. Why isn't that
> triggering? That's the whole point of it.
Linus, Alan, Steven,
are you on board with the patch set?
What shall I do to improve it?
PRINTK_SAFE_CONTEXT_MASK is what we answer nowadays when someone says
"printk causes deadlocks". We really can't remove all printk-s that can
cause uart->...->printk->uart recursion, and the only other option is to
use spin_trylock on uart_port->lock in every driver and discard con->write()
if we see that we have re-entered uart. I'd rather use per-CPU printk_safe
buffer in this case.
-ss