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

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

[Jacques Nilo]

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.

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.

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.

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

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

[Ilpo Järvinen]

[-- 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.

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

[Jacques Nilo]

On Tue, 12 May 2026, Ilpo Järvinen wrote:

 > I seem to have come blind to the (unlock function) names. I'm sorry
 > about breaking this.

No problem at all -- the asymmetry between the lock and unlock helper
names is exactly the kind of thing that's easy to miss in a refactor.

 > 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.

Confirmed -- dw8250_handle_irq() at drivers/tty/serial/8250/8250_dw.c:421
does the same guard(uart_port_lock_irqsave)(p) before
serial8250_handle_irq_locked(). Same bug.

 > > Option B -- fix the guard destructor in serial_core.h:
 >
 > 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?

Agreed, that's cleaner. The lock side is identical -- only the unlock
needs the sysrq-aware variant -- so a second lock-guard macro keyed off
the unlock destructor fits well:

   DEFINE_LOCK_GUARD_1(uart_port_lock_irqsave_sysrq, struct uart_port,
                       uart_port_lock_irqsave(_T->lock, &_T->flags),
uart_unlock_and_check_sysrq_irqrestore(_T->lock,
  _T->flags),
                       unsigned long flags);

Callers that may capture sysrq_ch (currently serial8250_handle_irq and
dw8250_handle_irq) opt in by using guard(uart_port_lock_irqsave_sysrq);
the existing guard(uart_port_lock_irqsave) keeps its current plain-unlock
semantics for everyone else.

Naming-wise I'm not attached to "_sysrq" -- if you'd prefer something
shorter (e.g. uart_port_rx_lock_irqsave) or aligned with another
convention in the tree, happy to take direction.

 > 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.

Right -- both guards take port->lock so lockdep can't distinguish them.
I'll update the Context: line on serial8250_handle_irq_locked() to spell
out the requirement, e.g.:

   /*
    * Context: port's lock must be held by the caller, and must be released
    * via guard(uart_port_lock_irqsave_sysrq) or
    * uart_unlock_and_check_sysrq_irqrestore() so a SysRq character
    * captured by serial8250_read_char() is dispatched on unlock.
    */

Plan, then, for a v1 patch series against tty-next:

   1. include/linux/serial_core.h: add the new lock-guard macro.
   2. drivers/tty/serial/8250/8250_port.c: switch serial8250_handle_irq()
      to guard(uart_port_lock_irqsave_sysrq); update the Context comment
      on serial8250_handle_irq_locked().
   3. drivers/tty/serial/8250/8250_dw.c: switch dw8250_handle_irq() to
      the same guard.

I'll mark it with Fixes: 8324a54f604d and Cc: stable for the kernels
that carry the regression. Let me know if the naming or the Context
wording wants adjusting before I send.

Thanks for the quick read,

Jacques

Le 12/05/2026 à 14:58, Ilpo Järvinen a écrit :
> 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
>>

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

[Ilpo Järvinen]

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

On Tue, 12 May 2026, Jacques Nilo wrote:

> On Tue, 12 May 2026, Ilpo Järvinen wrote:
> 
> > I seem to have come blind to the (unlock function) names. I'm sorry
> > about breaking this.
> 
> No problem at all -- the asymmetry between the lock and unlock helper
> names is exactly the kind of thing that's easy to miss in a refactor.
> 
> > 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.
> 
> Confirmed -- dw8250_handle_irq() at drivers/tty/serial/8250/8250_dw.c:421
> does the same guard(uart_port_lock_irqsave)(p) before
> serial8250_handle_irq_locked(). Same bug.
> 
> > > Option B -- fix the guard destructor in serial_core.h:
> >
> > 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?
> 
> Agreed, that's cleaner. The lock side is identical -- only the unlock
> needs the sysrq-aware variant -- so a second lock-guard macro keyed off
> the unlock destructor fits well:
> 
>   DEFINE_LOCK_GUARD_1(uart_port_lock_irqsave_sysrq, struct uart_port,
>                       uart_port_lock_irqsave(_T->lock, &_T->flags),
> uart_unlock_and_check_sysrq_irqrestore(_T->lock,
>  _T->flags),
>                       unsigned long flags);
> 
> Callers that may capture sysrq_ch (currently serial8250_handle_irq and
> dw8250_handle_irq) opt in by using guard(uart_port_lock_irqsave_sysrq);
> the existing guard(uart_port_lock_irqsave) keeps its current plain-unlock
> semantics for everyone else.
> 
> Naming-wise I'm not attached to "_sysrq" -- if you'd prefer something
> shorter (e.g. uart_port_rx_lock_irqsave) or aligned with another
> convention in the tree, happy to take direction.

I don't have strong preferences (I'm usually not good with names
anyway :-)). Somebody might object not placing _irqsave as last in the 
name (and reversing the word order would be a bit inconsistent compared 
with the unlock name).

> > 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.
> 
> Right -- both guards take port->lock so lockdep can't distinguish them.
> I'll update the Context: line on serial8250_handle_irq_locked() to spell
> out the requirement, e.g.:
> 
>   /*
>    * Context: port's lock must be held by the caller, and must be released
>    * via guard(uart_port_lock_irqsave_sysrq) or
>    * uart_unlock_and_check_sysrq_irqrestore()

> so a SysRq character
>    * captured by serial8250_read_char() is dispatched on unlock.

This would be less words to the same effect:

which captures SysRq character on unlock.

?

>    */
> 
> Plan, then, for a v1 patch series against tty-next:
> 
>   1. include/linux/serial_core.h: add the new lock-guard macro.
>   2. drivers/tty/serial/8250/8250_port.c: switch serial8250_handle_irq()
>      to guard(uart_port_lock_irqsave_sysrq); update the Context comment
>      on serial8250_handle_irq_locked().
>   3. drivers/tty/serial/8250/8250_dw.c: switch dw8250_handle_irq() to
>      the same guard.
> 
> I'll mark it with Fixes: 8324a54f604d and Cc: stable for the kernels
> that carry the regression. Let me know if the naming or the Context
> wording wants adjusting before I send.

+

Fixes: 883c5a2bc934 ("serial: 8250_dw: Rework dw8250_handle_irq() locking and IIR handling")


-- 
 i.

[PATCH 0/3] serial: 8250: fix BREAK+SysRq dispatch on guard()-locked IRQ handlers

[Jacques Nilo]

This series fixes a silent regression where a SysRq character entered as
BREAK + key on the serial console is consumed by the kernel but never
dispatched to handle_sysrq().

The root cause and two fix candidates were discussed in [1]. Following
Ilpo's suggestion, this series adds a dedicated lock-guard variant whose
destructor is the sysrq-aware unlock helper, and switches the two
affected IRQ handlers (serial8250_handle_irq and dw8250_handle_irq) to
use it. The plain guard(uart_port_lock_irqsave) keeps its current
semantics for the many callers that do not process RX.

Patch 1 introduces guard(uart_port_lock_sysrq_irqsave) in serial_core.h.
Patch 2 switches serial8250_handle_irq() and updates the Context comment
        on serial8250_handle_irq_locked() so future HW-specific 8250
        wrappers know which unlock variant is required.
Patch 3 switches dw8250_handle_irq() to the same guard.

Verified on RTL8196E with CONFIG_MAGIC_SYSRQ_SERIAL=y: BREAK + 'h' on
the console UART now produces the SysRq help dump in dmesg; the brk
counter in /proc/tty/driver/serial increments per BREAK as expected.
Build tested on tty-next (base 16e95bfb79b5).

[1] https://lore.kernel.org/linux-serial/5efe9e03-4d86-43a0-9ec2-e610ff31095d@free.fr/

Jacques Nilo (3):
  serial: core: introduce guard(uart_port_lock_sysrq_irqsave)
  serial: 8250: dispatch SysRq character in serial8250_handle_irq()
  serial: 8250_dw: dispatch SysRq character in dw8250_handle_irq()

 drivers/tty/serial/8250/8250_dw.c   |  2 +-
 drivers/tty/serial/8250/8250_port.c |  7 +++++--
 include/linux/serial_core.h         | 13 +++++++++++++
 3 files changed, 19 insertions(+), 3 deletions(-)


base-commit: 16e95bfb79b5d9d01dc7651d98caf3c2ace331cd
-- 
2.43.0

[PATCH 1/3] serial: core: introduce guard(uart_port_lock_sysrq_irqsave)

[Jacques Nilo]

uart_handle_break() and uart_prepare_sysrq_char() (in
include/linux/serial_core.h) capture a SysRq character into
port->sysrq_ch while the port lock is held and rely on the unlock
helper -- uart_unlock_and_check_sysrq_irqrestore() -- to dispatch the
captured character to handle_sysrq() on scope exit.

The existing guard(uart_port_lock_irqsave) cannot be used by IRQ
handlers that process RX, because its destructor calls plain
uart_port_unlock_irqrestore() and silently drops port->sysrq_ch.

Add a dedicated guard variant whose destructor is the sysrq-aware
unlock helper. Callers that may capture SysRq characters opt in by
using guard(uart_port_lock_sysrq_irqsave); the existing
uart_port_lock_irqsave guard keeps its current plain-unlock semantics
for the many callers that do not process RX.

The lock side is identical to uart_port_lock_irqsave -- only the
unlock-time behaviour differs. Naming mirrors
uart_unlock_and_check_sysrq_irqrestore() (sysrq before irqsave/irqrestore).

The new macro is placed after the CONFIG_MAGIC_SYSRQ_SERIAL block so
both definitions of uart_unlock_and_check_sysrq_irqrestore() (sysrq
enabled and disabled) are visible at expansion time. When
CONFIG_MAGIC_SYSRQ_SERIAL=n the destructor degenerates to plain
uart_port_unlock_irqrestore(), so there is no overhead.

No functional change on its own; users are converted in the following
patches.

Signed-off-by: Jacques Nilo <jnilo@free.fr>
---
 include/linux/serial_core.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 4f7bbdd90..4fa079dc9 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -1286,6 +1286,19 @@ static inline void uart_unlock_and_check_sysrq_irqrestore(struct uart_port *port
 }
 #endif	/* CONFIG_MAGIC_SYSRQ_SERIAL */
 
+/*
+ * Variant of guard(uart_port_lock_irqsave) for IRQ handlers that may capture
+ * a SysRq character via uart_prepare_sysrq_char(). The destructor uses the
+ * sysrq-aware unlock helper so that a captured port->sysrq_ch is dispatched
+ * to handle_sysrq() on scope exit. The plain guard variant silently drops
+ * sysrq_ch and must not be used by callers that process RX.
+ */
+DEFINE_LOCK_GUARD_1(uart_port_lock_sysrq_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);
+
 /*
  * We do the SysRQ and SAK checking like this...
  */
-- 
2.43.0

[PATCH 2/3] serial: 8250: dispatch SysRq character in serial8250_handle_irq()

[Jacques Nilo]

serial8250_handle_irq() captures a SysRq character into port->sysrq_ch
inside serial8250_handle_irq_locked() via uart_prepare_sysrq_char()
(reached from serial8250_read_char()). Dispatch of that captured
character to handle_sysrq() is expected to happen at port-unlock time,
through uart_unlock_and_check_sysrq[_irqrestore]().

After commit 8324a54f604d ("serial: 8250: Add
serial8250_handle_irq_locked()") the function was reduced to a wrapper
that takes the port lock via guard(uart_port_lock_irqsave) whose
destructor is plain uart_port_unlock_irqrestore(). The sysrq-aware
unlock helper is no longer called, so port->sysrq_ch is captured but
never dispatched: BREAK + SysRq key is consumed silently.

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.

Switch to the new guard(uart_port_lock_sysrq_irqsave), whose destructor
is the sysrq-aware unlock helper, restoring the pre-split behaviour.
Update the Context: comment on serial8250_handle_irq_locked() so future
HW-specific 8250 wrappers know to use the same guard or the explicit
sysrq-aware unlock.

Verified on RTL8196E with CONFIG_MAGIC_SYSRQ_SERIAL=y: BREAK + 'h' on
the console UART produces the SysRq help dump in dmesg and the brk
counter in /proc/tty/driver/serial increments correctly.

Fixes: 8324a54f604d ("serial: 8250: Add serial8250_handle_irq_locked()")
Cc: stable@vger.kernel.org
Signed-off-by: Jacques Nilo <jnilo@free.fr>
---
 drivers/tty/serial/8250/8250_port.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index e4e6a53eb..64f3487e8 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1786,7 +1786,10 @@ static bool handle_rx_dma(struct uart_8250_port *up, unsigned int iir)
 }
 
 /*
- * Context: port's lock must be held by the caller.
+ * Context: port's lock must be held by the caller. The caller must
+ * release it via guard(uart_port_lock_sysrq_irqsave) or
+ * uart_unlock_and_check_sysrq_irqrestore(), which captures SysRq
+ * character on unlock.
  */
 void serial8250_handle_irq_locked(struct uart_port *port, unsigned int iir)
 {
@@ -1839,7 +1842,7 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
 	if (iir & UART_IIR_NO_INT)
 		return 0;
 
-	guard(uart_port_lock_irqsave)(port);
+	guard(uart_port_lock_sysrq_irqsave)(port);
 	serial8250_handle_irq_locked(port, iir);
 
 	return 1;
-- 
2.43.0

[PATCH 3/3] serial: 8250_dw: dispatch SysRq character in dw8250_handle_irq()

[Jacques Nilo]

dw8250_handle_irq() calls serial8250_handle_irq_locked() with the port
lock held via guard(uart_port_lock_irqsave). The guard destructor is
plain uart_port_unlock_irqrestore(), so a SysRq character captured into
port->sysrq_ch by uart_prepare_sysrq_char() is dropped without ever
being dispatched to handle_sysrq().

This is the same regression pattern as in serial8250_handle_irq(),
introduced when 883c5a2bc934 ("serial: 8250_dw: Rework
dw8250_handle_irq() locking and IIR handling") moved the function to
the guard()-based locking scheme without using the sysrq-aware unlock
helper.

Switch to guard(uart_port_lock_sysrq_irqsave) so that captured
sysrq_ch is dispatched on scope exit, matching the fix in
serial8250_handle_irq().

Fixes: 883c5a2bc934 ("serial: 8250_dw: Rework dw8250_handle_irq() locking and IIR handling")
Cc: stable@vger.kernel.org
Signed-off-by: Jacques Nilo <jnilo@free.fr>
---
 drivers/tty/serial/8250/8250_dw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 55e40c10f..237543fa7 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -416,7 +416,7 @@ static int dw8250_handle_irq(struct uart_port *p)
 	unsigned int quirks = d->pdata->quirks;
 	unsigned int status;
 
-	guard(uart_port_lock_irqsave)(p);
+	guard(uart_port_lock_sysrq_irqsave)(p);
 
 	switch (FIELD_GET(DW_UART_IIR_IID, iir)) {
 	case UART_IIR_NO_INT:
-- 
2.43.0