Commit d6e1935819db ("serial: core: Allow processing sysrq at port unlock time") worked around a circular locking dependency by adding helpers used to defer sysrq processing to when the port lock was released.
A later commit unfortunately converted these inline helpers to exported functions despite the fact that the unlock helper was restoring irq flags, something which needs to be done in the same function that saved them (e.g. on SPARC).
Fixes: 8e20fc391711 ("serial_core: Move sysrq functions from header file") Cc: stable stable@vger.kernel.org # 5.6 Cc: Dmitry Safonov 0x7f454c46@gmail.com Signed-off-by: Johan Hovold johan@kernel.org --- drivers/tty/serial/serial_core.c | 19 ------------------- include/linux/serial_core.h | 21 +++++++++++++++++++-- 2 files changed, 19 insertions(+), 21 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c index edfb7bc14bbf..f6cf9cc4ce69 100644 --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c @@ -3239,25 +3239,6 @@ int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch) } EXPORT_SYMBOL_GPL(uart_prepare_sysrq_char);
-void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags) -{ - int sysrq_ch; - - if (!port->has_sysrq) { - spin_unlock_irqrestore(&port->lock, irqflags); - return; - } - - sysrq_ch = port->sysrq_ch; - port->sysrq_ch = 0; - - spin_unlock_irqrestore(&port->lock, irqflags); - - if (sysrq_ch) - handle_sysrq(sysrq_ch); -} -EXPORT_SYMBOL_GPL(uart_unlock_and_check_sysrq); - /* * We do the SysRQ and SAK checking like this... */ diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h index 1f4443db5474..858c5dd926ad 100644 --- a/include/linux/serial_core.h +++ b/include/linux/serial_core.h @@ -462,8 +462,25 @@ extern void uart_insert_char(struct uart_port *port, unsigned int status,
extern int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch); extern int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch); -extern void uart_unlock_and_check_sysrq(struct uart_port *port, - unsigned long irqflags); + +static inline void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags) +{ + int sysrq_ch; + + if (!port->has_sysrq) { + spin_unlock_irqrestore(&port->lock, irqflags); + return; + } + + sysrq_ch = port->sysrq_ch; + port->sysrq_ch = 0; + + spin_unlock_irqrestore(&port->lock, irqflags); + + if (sysrq_ch) + handle_sysrq(sysrq_ch); +} + extern int uart_handle_break(struct uart_port *port);
/*
On Tue, Jun 2, 2020 at 5:03 PM Johan Hovold johan@kernel.org wrote:
Commit d6e1935819db ("serial: core: Allow processing sysrq at port unlock time") worked around a circular locking dependency by adding helpers used to defer sysrq processing to when the port lock was released.
A later commit unfortunately converted these inline helpers to exported functions despite the fact that the unlock helper was restoring irq flags, something which needs to be done in the same function that saved them (e.g. on SPARC).
I'm not familiar with sparc, can you elaborate a bit what is ABI / architecture lock implementation background?
On 6/2/20 3:48 PM, Andy Shevchenko wrote:
On Tue, Jun 2, 2020 at 5:03 PM Johan Hovold johan@kernel.org wrote:
Commit d6e1935819db ("serial: core: Allow processing sysrq at port unlock time") worked around a circular locking dependency by adding helpers used to defer sysrq processing to when the port lock was released.
A later commit unfortunately converted these inline helpers to exported functions despite the fact that the unlock helper was restoring irq flags, something which needs to be done in the same function that saved them (e.g. on SPARC).
I'm not familiar with sparc, can you elaborate a bit what is ABI / architecture lock implementation background?
I remember that was a limitation a while ago to save/restore flags from the same function. Though, I vaguely remember the reason. I don't see this limitation in Documentation/*
Google suggests that it's related to storage location: https://stackoverflow.com/a/34279032
Which is definitely non-issue with tty drivers: they call spin_lock_irqsave() with local flags and pass them to uart_unlock_and_check_sysrq().
Looking into arch/sparc I also can't catch if it's still a limitation.
Also, looking around, xa_unlock_irqrestore() is called not from the same function. Maybe this issue is in history?
Johan, is it a theoretical problem or something you observe? Also, some comments would be nice near functions in the header.
Thanks, Dmitry
On Tue, Jun 02, 2020 at 04:34:16PM +0100, Dmitry Safonov wrote:
On 6/2/20 3:48 PM, Andy Shevchenko wrote:
On Tue, Jun 2, 2020 at 5:03 PM Johan Hovold johan@kernel.org wrote:
Commit d6e1935819db ("serial: core: Allow processing sysrq at port unlock time") worked around a circular locking dependency by adding helpers used to defer sysrq processing to when the port lock was released.
A later commit unfortunately converted these inline helpers to exported functions despite the fact that the unlock helper was restoring irq flags, something which needs to be done in the same function that saved them (e.g. on SPARC).
I'm not familiar with sparc, can you elaborate a bit what is ABI / architecture lock implementation background?
I remember that was a limitation a while ago to save/restore flags from the same function. Though, I vaguely remember the reason. I don't see this limitation in Documentation/*
It's described in both LDD3 and LKD, which is possibly where I first picked it up too (admittedly a long time ago).
Google suggests that it's related to storage location: https://stackoverflow.com/a/34279032
No, that was never the issue.
SPARC includes the current register window in those flags, which at least had to be restored in the same stack frame.
Looking into arch/sparc I also can't catch if it's still a limitation.
Yeah, looking closer at the current implementation it seems this is no longer an issue on SPARC.
Also, looking around, xa_unlock_irqrestore() is called not from the same function. Maybe this issue is in history?
xa_unlock_irqrestore() is just a macro for spin_unlock_irqsave() it seems, so not a counter example.
Also, some comments would be nice near functions in the header.
Agreed. Let me respin this and either merge this with the next patch or at least amend the commit message.
Johan
On Wed, Jun 03, 2020 at 10:40:51AM +0200, Johan Hovold wrote:
On Tue, Jun 02, 2020 at 04:34:16PM +0100, Dmitry Safonov wrote:
On 6/2/20 3:48 PM, Andy Shevchenko wrote:
On Tue, Jun 2, 2020 at 5:03 PM Johan Hovold johan@kernel.org wrote:
Commit d6e1935819db ("serial: core: Allow processing sysrq at port unlock time") worked around a circular locking dependency by adding helpers used to defer sysrq processing to when the port lock was released.
A later commit unfortunately converted these inline helpers to exported functions despite the fact that the unlock helper was restoring irq flags, something which needs to be done in the same function that saved them (e.g. on SPARC).
I'm not familiar with sparc, can you elaborate a bit what is ABI / architecture lock implementation background?
I remember that was a limitation a while ago to save/restore flags from the same function. Though, I vaguely remember the reason. I don't see this limitation in Documentation/*
It's described in both LDD3 and LKD, which is possibly where I first picked it up too (admittedly a long time ago).
Google suggests that it's related to storage location: https://stackoverflow.com/a/34279032
No, that was never the issue.
SPARC includes the current register window in those flags, which at least had to be restored in the same stack frame.
Looking into arch/sparc I also can't catch if it's still a limitation.
Yeah, looking closer at the current implementation it seems this is no longer an issue on SPARC.
Also, looking around, xa_unlock_irqrestore() is called not from the same function. Maybe this issue is in history?
xa_unlock_irqrestore() is just a macro for spin_unlock_irqsave() it seems, so not a counter example.
Also, some comments would be nice near functions in the header.
Agreed. Let me respin this and either merge this with the next patch or at least amend the commit message.
I stand corrected; this appears to longer be an issue (on any arch) as we these days have other common code passing the flags argument around like this.
I'll respin.
Johan
linux-stable-mirror@lists.linaro.org
-
Andy Shevchenko -
Dmitry Safonov -
Johan Hovold