Re: [REPORT] serial: 8250: BREAK + SysRq dispatch silently broken since 8324a54f604d

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 6161 bytes --]

On Tue, 12 May 2026, Jacques Nilo wrote:

> Hi,
> 
> We hit what looks like a silent SysRq-over-serial regression on a 6.18
> build of the 8250 driver. Posting as a report rather than a patch because
> there are at least two reasonable fixes and I'd like a maintainer call
> before sending one.
> 
> Symptom
> =======
> 
> CONFIG_MAGIC_SYSRQ=y, CONFIG_MAGIC_SYSRQ_SERIAL=y,
> CONFIG_SERIAL_8250_CONSOLE=y.
> 
> A BREAK followed by a SysRq key on the console UART is consumed by the
> kernel (BREAK counter in /proc/tty/driver/serial increments correctly)
> but is never dispatched to handle_sysrq(). dmesg shows no "sysrq: ..."
> line.
> 
> `echo h > /proc/sysrq-trigger` still works, isolating the regression to
> the serial input path. Verified end-to-end on an RTL8196E MIPS board
> running 6.18.24; the affected code is in the generic 8250 core, so the
> issue is not platform-specific.
> 
> Path
> ====
> 
>   serial8250_default_handle_irq()
>     -> serial8250_handle_irq() [8250_port.c:1835]
>          guard(uart_port_lock_irqsave)(port);  [8250_port.c:1840]
>          serial8250_handle_irq_locked()
>            -> serial8250_rx_chars()
>               -> serial8250_read_char()
>                  -> uart_handle_break()                 -- arms port->sysrq
>                  -> uart_prepare_sysrq_char(port, ch)   -- captures sysrq_ch
>          /* guard scope ends -> port unlock */
> 
> The captured port->sysrq_ch is dispatched to handle_sysrq() at unlock
> time -- but only by uart_unlock_and_check_sysrq[_irqrestore]() (see
> include/linux/serial_core.h:1239). The scope guard's destructor at
> serial_core.h:797 is plain uart_port_unlock_irqrestore(), which skips
> the dispatch:
> 
>   DEFINE_LOCK_GUARD_1(uart_port_lock_irqsave, struct uart_port,
>                       uart_port_lock_irqsave(_T->lock, &_T->flags),
>                       uart_port_unlock_irqrestore(_T->lock, _T->flags),
>                       unsigned long flags);
> 
> So sysrq_ch stays in the struct until the next BREAK clears it.
> 
> Bisection
> =========
> 
>   commit 8324a54f604d ("serial: 8250: Add serial8250_handle_irq_locked()")
> 
> Pre-split serial8250_handle_irq() used explicit uart_port_lock_irqsave()
> + uart_unlock_and_check_sysrq_irqrestore(). The split moved the body into
> _locked() and replaced the explicit lock pair with
> guard(uart_port_lock_irqsave), losing the sysrq-aware unlock.
>
> This was the very condition Johan Hovold's 853a9ae29e978 ("serial: 8250:
> fix handle_irq locking", 2021) introduced
> uart_unlock_and_check_sysrq_irqrestore() to address -- the new helper was
> deliberately the sysrq-aware variant. The guard() conversion undoes that
> intent.

I seem to have come blind to the (unlock function) names. I'm sorry about 
breaking this.
 
> Reproducer
> ==========
> 
> On any 8250-driven console with CONFIG_MAGIC_SYSRQ_SERIAL=y:
> 
>   # On the host side:
>   python3 -c 'import os,fcntl,termios,time
>   fd=os.open("/dev/ttyUSB0",os.O_RDWR|os.O_NOCTTY)
>   fcntl.ioctl(fd,0x5427); time.sleep(0.3); fcntl.ioctl(fd,0x5428)
>   time.sleep(0.05); os.write(fd,b"h"); time.sleep(0.3)'
> 
>   # On the gateway:
>   grep brk /proc/tty/driver/serial      # counter increments
>   dmesg | grep sysrq:                   # empty -- no dispatch
> 
> Two ways to fix
> ===============
> 
> Option A -- surgical, only fix serial8250_handle_irq():
> 
>   int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
>   {
>           unsigned long flags;
> 
>           if (iir & UART_IIR_NO_INT)
>                   return 0;
> 
>           uart_port_lock_irqsave(port, &flags);
>           serial8250_handle_irq_locked(port, iir);
>           uart_unlock_and_check_sysrq_irqrestore(port, flags);
> 
>           return 1;
>   }
> 
> Restores the pre-split behaviour. Doesn't touch the guard infrastructure.
> Drawback: leaves uart_port_lock_irqsave() as a generic primitive that
> silently swallows pending sysrq_ch in any other call site that processes
> RX under the guard. There are no such sites today in 8250_port.c
> (uart_prepare_sysrq_char is only reachable through serial8250_handle_irq),
> but the trap remains.

8250_dw's handle_irq also uses guard() which was the reason for this 
change in the first place so it should be fixed as well.

> Option B -- fix the guard destructor in serial_core.h:
> 
>   DEFINE_LOCK_GUARD_1(uart_port_lock_irqsave, struct uart_port,
>                       uart_port_lock_irqsave(_T->lock, &_T->flags),
> uart_unlock_and_check_sysrq_irqrestore(_T->lock,
>  _T->flags),
>                       unsigned long flags);
> 
> uart_unlock_and_check_sysrq_irqrestore() short-circuits to plain unlock
> when !port->has_sysrq, so no overhead on non-sysrq ports. Fixes all
> current and future guard(uart_port_lock_irqsave) users in one place.
> Drawback: changes the semantics of a shared serial primitive. Some
> callers in 8250_port.c run under that guard from non-RX contexts
> (serial8250_set_mctrl, wait_for_xmitr, etc.); the only observable effect
> there would be a one-time handle_sysrq() call if a previous BREAK left
> sysrq_ch set -- functionally desirable, but a behaviour change worth
> documenting.

There will be many such sites so this doesn't sound a great idea.

I wonder why we couldn't instead do another DEFINE_GUARD() for the sysrq 
case?

I suppose thought the lockdep assert in serial8250_handle_irq_locked() 
cannot discern that the correct one of them is being used by the caller. 
But at least it's context comment should mention that the correct 
guard/unlock variant should be used.

> I have a tested Option A patch against 6.18.24 (verified the dispatch
> fires and produces the SysRq help dump). Happy to send it formally, or
> to retarget to Option B if that's the preferred direction.
> 
> Thanks,
> 
> Jacques
> 

-- 
 i.