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