Re: [PATCH v2 6.12.y 00/10] serial: 8250_dw: backport BUSY deassert series

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

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

On Wed, 13 May 2026, Ionut Nechita (Wind River) wrote:

> From: Ionut Nechita <ionut.nechita@windriver.com>
> 
> Hi Greg, Ilpo,
> 
> This is v2 of the 8250_dw BUSY deassert backport to 6.12.y,
> addressing Ilpo's review feedback on v1.

FYI, this came up yesterday related to guard()s vs unlock variants:

https://lore.kernel.org/linux-serial/cover.1778592805.git.jnilo@free.fr/

> v1 thread:
>   https://lore.kernel.org/linux-serial/20260510134011.618215-1-ionut.nechita@windriver.com/
> 
> v1 review (Ilpo, 2026-05-11):
>   https://lore.kernel.org/linux-serial/d4b4e4db-505f-4400-c1b9-fe589786456e@linux.intel.com/
> 
> Changes in v2:
> 
>   - Keep guard()/scoped_guard() from upstream verbatim in patches
>     5, 7, 8 and 9 (no more explicit uart_port_lock_irqsave/
>     unlock_irqrestore + goto-out adaptations), per Ilpo's review:
> 
>       "By doing this you had to add some gotos which makes code
>        control flow more complicated than it was in the original
>        changes. From reviewer's point of view, the code is more
>        complicated without guard() than with them."
> 
>     To make that work on 6.12.y, two small additional prerequisites
>     are now part of the series (see new patches 3/10 and 4/10
>     below).
> 
>   - Stop replacing guard()s with explicit lock/unlock pairs; v2
>     diff against upstream is now significantly smaller.
> 
> Series structure (10 patches):
> 
>   Prerequisites (per Ilpo's original guidance, plus one implicit
>   guard-defs prereq):
>     1/10 serial: 8250: use serial_port_in/out() helpers         [dbd26a886e94]
>     2/10 serial: 8250_dw: Comment possible corner cases ...     [bd8cad85561b]
>     3/10 serial: introduce uart_port_lock() guard()s            [0fd60b689b0d]      (NEW in v2)
>     4/10 serial: 8250: convert serial8250_do_shutdown()
>          to scoped_guard()                                      [partial b339809edda1] (NEW in v2)
> 
>   BUSY deassert series:
>     5/10 serial: 8250: Protect LCR write in shutdown            [59a33d83bbe6]
>     6/10 serial: 8250_dw: Avoid unnecessary LCR writes          [8002d6d6d0d8]
>     7/10 serial: 8250: Add serial8250_handle_irq_locked()       [8324a54f604d]
>     8/10 serial: 8250_dw: Rework dw8250_handle_irq() ...        [883c5a2bc934]
>     9/10 serial: 8250_dw: Rework IIR_NO_INT handling ...        [73a4ed8f9efa]
>    10/10 serial: 8250_dw: Ensure BUSY is deasserted             [a7b9ce39fbe4]
> 
> Notes on the new prerequisites (3/10 and 4/10):
> 
>  - Patch 3/10 cherry-picks commit 0fd60b689b0d ("serial: introduce
>    uart_port_lock() guard()s") unmodified (13 additive lines in
>    include/linux/serial_core.h). It introduces DEFINE_GUARD() /
>    DEFINE_LOCK_GUARD_1() invocations for uart_port_lock,
>    uart_port_lock_irq and uart_port_lock_irqsave, which are
>    required so that scoped_guard(uart_port_lock_irqsave, port) and
>    guard(uart_port_lock_irqsave)(port) compile in 6.12.y. The
>    underlying cleanup.h infrastructure (DEFINE_GUARD,
>    DEFINE_LOCK_GUARD_1) already exists in 6.12.y; only this small
>    serial-specific wiring is missing.
> 
>  - Patch 4/10 is a *partial* backport of commit b339809edda1
>    ("serial: 8250: use guard()s"), per Ilpo's original request:
> 
>      "b339809edda1 ('serial: 8250: use guard()s')
>       (8250_port.c shutdown hunks)"
> 
>    The commit is authored by Jiri Slaby (SUSE) and carries the
>    upstream Link/SoB chain, plus an explicit [Ionut: partial
>    backport - only the serial8250_do_shutdown() hunks from
>    b339809edda1] note. The remaining (much larger) refactor of
>    8250_port.c and 8250_core.c is not needed for the BUSY deassert
>    series and is intentionally not backported.
> 
> Notes on the BUSY series itself:
> 
>  - Patch 6/7 of the original mainline BUSY series,
>    commit e0a368ae7953 ("serial: 8250: Add late synchronize_irq()
>    to shutdown to handle DW UART BUSY"), is *already* in 6.12.y as
>    commit 0bae1c670aa8 (Ilpo, 2026-02-03), so it is not re-sent
>    here. Functionally this means patches 5-10 above land on top of
>    the existing late-synchronize_irq() fix.
> 
>  - Ilpo's other suggested-but-not-required prerequisites remain
>    *not* included:
>      * commit fc9ceb501e38 ("serial: 8250: sanitize
>        uart_port::serial_{in,out}() types") only causes a trivial
>        conflict in patch 10 in code that the BUSY change removes
>        anyway (per Ilpo's note);
>      * commit c213375e3283 ("serial: 8250_dw: Call dw8250_quirks()
>        conditionally") only causes a conflict in patch 9 around
>        the dw8250_setup_dma_filter() helper and the conditional
>        p->handle_irq assignment, neither of which exist in 6.12.y
>        and neither of which is needed for the BUSY fix.
> 
>    Both of those conflicts are resolved trivially in patches 9 and
>    10 with explicit [Ionut: adapt to 6.12.y - ...] notes.
> 
>  - Namespace export syntax: in 6.12.y both EXPORT_SYMBOL_NS_GPL()
>    and MODULE_IMPORT_NS() apply __stringify(ns) to the namespace
>    argument, so it must be a bare identifier. Mainline (where the
>    upstream patches were written) accepts a string literal. Patches
>    7 and 10 here use the bare-identifier form (SERIAL_8250) instead
>    of the upstream string form ("SERIAL_8250"); without this fix
>    the .vmlinux.export.c link step fails with "expected ':' or ')'
>    before 'SERIAL_8250'". This is noted in the [Ionut: ...] block
>    of the affected patches.
> 
>    Only patches 4, 7, 9 and 10 carry "[Ionut: adapt to 6.12.y - ...]"
>    notes; the rest are clean cherry-picks of the upstream commits
>    using the standard "commit <hash> upstream." stable convention.
> 
> Build:
>  - HEAD of the series builds to a complete vmlinux on 6.12.87 with
>    CONFIG_SERIAL_8250=y, CONFIG_SERIAL_8250_DW=m on x86_64. The
>    .vmlinux.export.c link step (which v1 originally tripped on
>    during development) passes cleanly.
> 
> Testing plan:
>  - Same as v1: test on Intel platforms with DW APB UART
>    (snps,dw-apb-uart) running 6.12.x-rt (PREEMPT_RT) to confirm
>    the original symptom (LCR writes silently ignored under Rx
>    load -> baud / framing mismatch after set_termios) is gone.
>    Will report Tested-by once cycles complete.
> 
> Based on: linux-6.12.y at v6.12.87 (8bf2f55ef536).
> 
> 
> Andy Shevchenko (1):
>   serial: 8250_dw: Comment possible corner cases in serial_out()
>     implementation
> 
> Ilpo Järvinen (6):
>   serial: 8250: Protect LCR write in shutdown
>   serial: 8250_dw: Avoid unnecessary LCR writes
>   serial: 8250: Add serial8250_handle_irq_locked()
>   serial: 8250_dw: Rework dw8250_handle_irq() locking and IIR handling
>   serial: 8250_dw: Rework IIR_NO_INT handling to stop interrupt storm
>   serial: 8250_dw: Ensure BUSY is deasserted
> 
> Jiri Slaby (SUSE) (3):
>   serial: 8250: use serial_port_in/out() helpers
>   serial: introduce uart_port_lock() guard()s
>   serial: 8250: convert serial8250_do_shutdown() to scoped_guard()
> 
>  drivers/tty/serial/8250/8250.h      |  25 +++
>  drivers/tty/serial/8250/8250_dw.c   | 296 +++++++++++++++++++++++-----
>  drivers/tty/serial/8250/8250_fsl.c  |   8 +-
>  drivers/tty/serial/8250/8250_omap.c |   2 +-
>  drivers/tty/serial/8250/8250_port.c |  90 +++++----
>  include/linux/serial_8250.h         |   1 +
>  include/linux/serial_core.h         |  13 ++
>  7 files changed, 338 insertions(+), 97 deletions(-)
> 
> 

-- 
 i.