public inbox for linux-serial@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH AUTOSEL 7.0-6.12] tty: serial: samsung_tty: avoid dev_dbg deadlock
       [not found] <20260428104133.2858589-1-sashal@kernel.org>
@ 2026-04-28 10:40 ` Sasha Levin
  2026-04-28 10:40 ` [PATCH AUTOSEL 7.0-5.10] tty: serial: imx: keep dma request disabled before dma transfer setup Sasha Levin
  2026-04-28 10:40 ` [PATCH AUTOSEL 7.0-5.10] serial: qcom-geni: Fix RTS behavior with flow control Sasha Levin
  2 siblings, 0 replies; 3+ messages in thread
From: Sasha Levin @ 2026-04-28 10:40 UTC (permalink / raw)
  To: patches, stable
  Cc: Alyssa Milburn, Greg Kroah-Hartman, Sasha Levin,
	krzysztof.kozlowski, jirislaby, linux-arm-kernel,
	linux-samsung-soc, linux-kernel, linux-serial

From: Alyssa Milburn <amilburn@zall.org>

[ Upstream commit 43c2b86ff633c34831c8430925ba73d7c20da1ad ]

commit a05025d0ce72 ("tty: serial: samsung_tty: use standard
debugging macros") changed the debug prints to dev_dbg, which can
result in deadlocks:

s3c24xx_serial_set_termios can be called with the port lock, and then
calls dev_dbg, which needs the console mutex. At the same time,
s3c24xx_serial_console_write can be called with the console lock
(e.g., inside console_unlock), and needs the port lock.

To avoid this, move one dev_dbg call and just delete the other.

Signed-off-by: Alyssa Milburn <amilburn@zall.org>
Link: https://patch.msgid.link/aXny9km6N1v9eoXU@zall.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

Now I have all the information needed. Let me write up the full
analysis:

## Complete 9-Phase Analysis

### PHASE 1: COMMIT MESSAGE FORENSICS

**Step 1.1 - Subject line parsing:**
Record: Subsystem `tty: serial: samsung_tty:`, action verb `avoid`,
summary: avoid dev_dbg deadlock between port lock and console lock.

**Step 1.2 - Commit message tags:**
Record:
- `Signed-off-by: Alyssa Milburn <amilburn@zall.org>` (author)
- `Link: https://patch.msgid.link/aXny9km6N1v9eoXU@zall.org` (original
  submission)
- `Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>` (TTY
  subsystem maintainer applied)
- No explicit `Fixes:` tag, but the commit body names `a05025d0ce72` as
  introducing the issue
- No `Cc: stable@vger.kernel.org` (expected, as the prompt notes)
- No Reviewed-by/Acked-by/Tested-by tags
- No syzbot/Reported-by

**Step 1.3 - Commit body:**
Record: Bug description: `s3c24xx_serial_set_termios` acquires port lock
then calls `dev_dbg`, which goes through printk and can require the
console mutex. Meanwhile `s3c24xx_serial_console_write` runs with the
console lock held (e.g., from `console_unlock`) and acquires the port
lock. Classic AB-BA deadlock (port_lock ↔ console_lock). Fix: move one
`dev_dbg` before the port lock acquisition and remove the second (post-
write register-dump `dev_dbg`).

**Step 1.4 - Hidden bug fix?** Not hidden - explicit "avoid deadlock".

### PHASE 2: DIFF ANALYSIS

**Step 2.1 - Inventory:**
Record: 1 file (`drivers/tty/serial/samsung_tty.c`), +2/-8 lines total
(6 net deletion). One function modified: `s3c24xx_serial_set_termios()`.
Scope: single-file, surgical.

**Step 2.2 - Code flow change:**
Record:
- Before: `uart_port_lock_irqsave(...)` → `dev_dbg("setting ulcon ...")`
  → reg writes → `dev_dbg("uart: ulcon = ... ")` (reading registers
  back)
- After: `dev_dbg("setting ulcon ...")` → `uart_port_lock_irqsave(...)`
  → reg writes → (second dev_dbg removed)

The first `dev_dbg` only prints local variables (`ulcon, quot,
udivslot`) so moving it before the lock is safe. The second `dev_dbg`
re-read registers to report actual written values - removing it loses
diagnostic info but avoids the deadlock (moving it outside the lock
would read values that could race with other writes).

**Step 2.3 - Bug mechanism:**
Record: Synchronization fix (category b): removes an AB-BA deadlock
potential between the UART port spinlock (`port->lock`) and the
printk/console locking chain.

Verified dead-lock mechanism:
- `set_termios` holds `port->lock` (uart_port_lock_irqsave)
- `dev_dbg()` expands (when dynamic debug is enabled) to a printk call
  that can enter `console_unlock()` → `console->write()` →
  `s3c24xx_serial_console_write()` → which calls
  `uart_port_lock_irqsave(cons_uart, &flags)` = same port lock. On the
  same CPU that would self-deadlock; across CPUs it forms the AB-BA
  pattern with another printk path.
- Confirmed `s3c24xx_serial_console_write` does
  `uart_port_lock_irqsave(cons_uart, &flags)` (samsung_tty.c lines
  2280-2283 in HEAD).

**Step 2.4 - Fix quality:**
Record: Fix is obvious - debug prints are diagnostic-only; moving one
out and removing the other cannot change functional behavior. Zero risk
of regression from the fix itself. Only downside: loss of one diagnostic
print.

### PHASE 3: GIT HISTORY INVESTIGATION

**Step 3.1 - Blame/introduction:**
Record: The problematic `dev_dbg` calls were introduced by `a05025d0ce72
"tty: serial: samsung_tty: use standard debugging macros"` (Greg KH, Dec
2019). `git describe --contains a05025d0ce72` → `v5.6-rc1~139^2~130`,
i.e. first released in v5.6 (March 2020).

**Step 3.2 - Fixes target:**
Record: Named `a05025d0ce72` is present in all current LTS stable trees
(5.10.y, 5.15.y, 6.1.y, 6.6.y, 6.12.y, and 7.0.y). Before
`a05025d0ce72`, the samsung_tty driver used a custom `dbg()` macro that
routed to `printascii()` (low-level ARM debug UART write, no locking) -
that's why the original code wasn't a deadlock. After `a05025d0ce72`,
the conversion to `dev_dbg()` introduced the deadlock potential.

**Step 3.3 - File history:**
Record: Recent changes to `drivers/tty/serial/samsung_tty.c` are a small
trickle (Exynos850 earlycon, 18 port support, cast cleanup, etc.). No
churn in the set_termios area since a05025d0ce72. Standalone patch (not
part of a series) - confirmed via `b4 dig -a` showing only v1.

**Step 3.4 - Author context:**
Record: Alyssa Milburn has submitted other kernel patches (media
subsystem from previous years). Not the samsung TTY maintainer, but the
patch was applied by Greg KH (TTY maintainer) after CC'ing the proper
subsystem maintainers.

**Step 3.5 - Dependencies:**
Record: None. Fix is self-contained and touches only local code within
one function.

### PHASE 4: MAILING LIST RESEARCH

**Step 4.1 - Original thread:**
Record: `b4 dig -c 43c2b86ff633c` found match at
https://patch.msgid.link/aXny9km6N1v9eoXU@zall.org. Fetched via `b4 am`
to `/tmp/b4_samsung/`. No review replies found in the thread (the mbox
from b4 contained only the original patch - `b4 am` reported "Analyzing
0 code-review messages"). Single version - no v2/v3.

**Step 4.2 - Recipients (b4 dig -w):**
Record: Proper CC list: Krzysztof Kozlowski (samsung-soc maintainer),
Alim Akhtar (samsung platform maintainer), Greg Kroah-Hartman (TTY
maintainer), Jiri Slaby (TTY co-maintainer), Faraz Ata (samsung
contributor), linux-serial, linux-samsung-soc, linux-arm-kernel, linux-
kernel. Proper maintainer audience; Greg KH signed off, indicating the
TTY maintainer accepted it.

**Step 4.3 - Bug report:**
Record: No `Reported-by:`, no bug URL. The author appears to have
identified the issue through code review/lockdep analysis rather than
user report. Similar patterns have been found by lockdep in other serial
drivers.

**Step 4.4 - Related patches:**
Record: None (single patch).

**Step 4.5 - Stable list:**
Record: Not investigated further; lore.kernel.org fetching is blocked by
Anubis bot-protection. However, `b4` successfully retrieved the thread
without review feedback, so there was no stable-related discussion.

### PHASE 5: CODE SEMANTIC ANALYSIS

**Step 5.1 - Key functions:**
Record: `s3c24xx_serial_set_termios` is the only function modified.

**Step 5.2 - Callers of set_termios:**
Record: Called via `uport->ops->set_termios(...)` from serial_core.c at
three sites: (1) `uart_change_line_settings` (normal termios change),
(2) `uart_configure_port` (probe path), (3) `uart_resume_port` (resume).
All are triggered by common operations (open, `tcsetattr`, resume).
Confirmed `set_termios` is called WITHOUT the port_lock held by
serial_core - the driver itself acquires it internally.

**Step 5.3 - The console-write side:**
Record: `s3c24xx_serial_console_write` is registered as
`s3c24xx_serial_console.write`. Called from the printk/console subsystem
when the samsung serial is the configured console. Takes
`cons_uart->lock` via `uart_port_lock_irqsave(cons_uart, &flags)` (lines
2278-2283). This forms the second half of the AB-BA.

**Step 5.4 - Reachability:**
Record: Deadlock trigger requires: (a) dynamic debug enabled for these
specific `dev_dbg` statements in samsung_tty.c, AND (b) samsung_tty
serves as a console, AND (c) a termios change happens (user
`tcsetattr()`, getty open, resume) while another CPU/task is doing a
printk that's flushing via this console. Practical trigger is uncommon
in production (dynamic debug is off by default) but clearly reachable
from unprivileged userspace when enabled.

**Step 5.5 - Similar patterns:**
Record: Confirmed similar fixes in other serial drivers: `436c979360017
"serial: stm32: fix a deadlock in set_termios"` (same AB-BA between
`&port_lock_key` and `console_owner`), `7fd6f640f2dd1 "serial: 8250_dw:
Fix deadlock in LCR workaround"`, `62b2caef400c1 "drivers: tty: serial:
Fix deadlock in sa1100_set_termios()"`. Well-established bug-class with
accepted fixes.

### PHASE 6: STABLE TREE ANALYSIS

**Step 6.1 - Buggy code in stable:**
Record: The buggy structure (port_lock acquired before `dev_dbg`) is
present in all stable trees ≥ 5.6 (where a05025d0ce72 first appeared).
Verified in 7.0.y HEAD (stable/linux-7.0.y) - the current checkout
contains exactly the pre-patch code. Stable trees 5.10, 5.15, 6.1, 6.6,
6.12, 7.0 are affected.

**Step 6.2 - Backport difficulty:**
Record: Very low difficulty. The hunk context is stable identifiers
(`ulcon, quot, udivslot` variables, `wr_regl(port, S3C2410_ULCON,
ulcon)`). Only complication: older stable trees used
`spin_lock_irqsave(&port->lock, flags)` instead of
`uart_port_lock_irqsave(port, &flags)` (the port-lock wrappers were
introduced by 97d7a9aeba1d4 "serial: samsung_tty: Use port lock
wrappers" which went to various stable trees). Still trivial to backport
by substituting the correct lock call; git will likely auto-apply with
minor context fuzz in old trees.

**Step 6.3 - Already in stable?**
Record: No prior fix for this deadlock in stable trees (confirmed by
file history).

### PHASE 7: SUBSYSTEM CONTEXT

**Step 7.1 - Criticality:**
Record: Subsystem = tty/serial driver for Samsung SoCs (Exynos family,
Apple A7-A11, S3C). Criticality: PERIPHERAL (specific-hardware), but
used extensively on Samsung/Exynos-based embedded systems, Chromebooks,
Google Pixel/development boards, and similar ARM platforms. These
devices often use the samsung_tty as their primary console, which is
exactly the configuration where the deadlock matters.

**Step 7.2 - Activity:**
Record: Mature driver, low-churn subsystem. Active maintenance by
Krzysztof Kozlowski and Greg KH. 20 commits over the last ~2 years per
file history.

### PHASE 8: IMPACT AND RISK

**Step 8.1 - Affected users:**
Record: Users of samsung_tty on Exynos, S3C, Apple Silicon (A7-A11 via
this driver), etc. ARM/ARM64 platforms with Samsung or Apple early SoCs.
Limited to systems where dynamic debug is enabled for this file AND the
UART is a console.

**Step 8.2 - Trigger:**
Record: Requires unusual runtime config (dynamic debug on), so not
common in stock distros. However, fully reachable from userspace
(`tcsetattr()`) once dynamic debug is enabled. Developers debugging on
these platforms can hit it.

**Step 8.3 - Failure mode severity:**
Record: Hard hang (deadlock) - system becomes unresponsive. Severity if
triggered: CRITICAL (system hang). Probability: LOW in production,
HIGHER during debugging sessions.

**Step 8.4 - Benefit/risk:**
Record: BENEFIT = correctness fix for a recognized deadlock class (same
pattern fixed in stm32, 8250_dw, sa1100). RISK = essentially zero: no
functional behavior change, only debug-print placement. 2 insertions, 8
deletions, all in one function. Obviously correct on inspection.
Risk/benefit strongly favors backport.

### PHASE 9: SYNTHESIS

**Step 9.1 - Evidence:**
FOR backport:
- Fixes a real AB-BA deadlock between `port_lock` and the console
  subsystem locking
- Extremely small, contained change (+2/-8 lines in a single function)
- Zero risk of behavior regression (only debug prints affected)
- Applied by the TTY subsystem maintainer (Greg KH)
- Proper maintainers and lists were CC'd
- Same bug class has been fixed in other serial drivers (stm32, sa1100,
  8250_dw) and those fixes reached stable
- Buggy code has existed since v5.6 (2019), in every LTS tree

AGAINST:
- Trigger requires dynamic debug to be enabled for this file; not a
  default-config hang
- No reporter, no lockdep report in the commit message (identified by
  code inspection)
- Second `dev_dbg` is deleted rather than relocated (minor loss of
  diagnostic info)

**Step 9.2 - Stable rules check:**
1. Obviously correct? YES - inspection-verifiable, no behavioral change
   beyond debug placement
2. Fixes a real bug? YES - AB-BA deadlock (class well-documented)
3. Important issue? YES - deadlock/hang
4. Small and contained? YES - 10 lines, one function
5. No new features/APIs? YES - only relocates/removes debug prints
6. Applies to stable? YES - trivial with minor wrapper-name substitution
   for older trees

**Step 9.3 - Exception category:** Not an exception case; it's a true
bug fix.

**Step 9.4 - Decision:** YES.

## Verification

- [Phase 1] Parsed tags: Link to patch.msgid.link, SOB author Alyssa
  Milburn, SOB Greg KH as committer; no Fixes tag but body names
  a05025d0ce72 as introducing commit.
- [Phase 2] Diff: 2 insertions, 8 deletions, single function
  `s3c24xx_serial_set_termios`.
- [Phase 2] Verified `s3c24xx_serial_console_write` takes
  `uart_port_lock_irqsave(cons_uart, &flags)` (samsung_tty.c lines
  2278-2283), confirming the port-lock half of the AB-BA.
- [Phase 3] `git show a05025d0ce72` confirmed it replaced `dbg()` (a
  `printascii`-based macro) with `dev_dbg()` - verifying the regression
  introduction.
- [Phase 3] `git describe --contains a05025d0ce72` →
  `v5.6-rc1~139^2~130`, so bug first appeared in v5.6.
- [Phase 3] `git log -- drivers/tty/serial/samsung_tty.c` shows no
  existing fix in tree; checked the 20 recent commits.
- [Phase 3] Found related file-history commit `97d7a9aeba1d4 "serial:
  samsung_tty: Use port lock wrappers"` that changed `spin_lock_irqsave`
  → `uart_port_lock_irqsave`; relevant for backport to older trees.
- [Phase 4] `b4 dig -c 43c2b86ff633c` matched the patch by patch-id to
  lore thread `aXny9km6N1v9eoXU@zall.org`.
- [Phase 4] `b4 dig -c 43c2b86ff633c -a` confirmed single revision (v1
  only).
- [Phase 4] `b4 dig -c 43c2b86ff633c -w` confirmed proper recipients
  (Krzysztof Kozlowski, Alim Akhtar, Greg KH, Jiri Slaby, linux-serial,
  linux-samsung-soc).
- [Phase 4] `b4 am` fetched the thread; only the original patch, no
  review replies.
- [Phase 5] Traced `->set_termios` callers in serial_core.c: three sites
  (`uart_change_line_settings`, `uart_configure_port`,
  `uart_resume_port`), all reachable from common userspace operations;
  confirmed serial_core does NOT hold port lock when calling
  set_termios.
- [Phase 5] Found similar accepted fixes for the same deadlock class:
  `436c979360017` (stm32), `7fd6f640f2dd1` (8250_dw), `62b2caef400c1`
  (sa1100).
- [Phase 6] Verified current stable/linux-7.0.y HEAD contains the pre-
  patch buggy structure in `s3c24xx_serial_set_termios` (samsung_tty.c
  lines 1562-1595).
- [Phase 6] Upstream commit SHA in origin/master:
  `43c2b86ff633c34831c8430925ba73d7c20da1ad`.
- [Phase 8] Confirmed trigger conditions require dynamic debug enabled
  (`dev_dbg` is no-op otherwise) AND samsung_tty serving as console -
  partially gating severity.
- UNVERIFIED: Could not fetch lore.kernel.org directly (Anubis bot-
  protection). Had to rely on `b4` tooling, which reported zero review
  replies - so there's likely no stable-relevant discussion I missed,
  but I cannot 100% guarantee it.
- UNVERIFIED: Did not enumerate every LTS tree's exact file contents;
  relied on (a) the described introduction in 2019 and (b) no file-
  history fix. Minor lock-wrapper adjustments may be needed for < 6.6.y
  backports (older trees use `spin_lock_irqsave(&port->lock, ...)`
  pre-97d7a9aeba1d4).

## Summary

This is a small (+2/-8 lines), surgical fix for an AB-BA deadlock
between the UART port lock and the console locking chain, introduced in
v5.6 (2019) when the driver's local `dbg()` macro - which routed to a
lock-free `printascii()` - was converted to `dev_dbg()` that enters the
full printk path. The fix moves one `dev_dbg` out of the critical
section and removes another redundant one. It meets all stable-kernel
criteria: obviously correct, fixes a real deadlock, small/contained, no
new features, and applies with at most trivial context adjustment. The
same bug class has precedent backports in stm32, sa1100, and 8250_dw
serial drivers.

**YES**

 drivers/tty/serial/samsung_tty.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index c1fabad6ba1fa..e27806bf2cf3e 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -1562,12 +1562,12 @@ static void s3c24xx_serial_set_termios(struct uart_port *port,
 		ulcon |= S3C2410_LCON_PNONE;
 	}
 
-	uart_port_lock_irqsave(port, &flags);
-
 	dev_dbg(port->dev,
 		"setting ulcon to %08x, brddiv to %d, udivslot %08x\n",
 		ulcon, quot, udivslot);
 
+	uart_port_lock_irqsave(port, &flags);
+
 	wr_regl(port, S3C2410_ULCON, ulcon);
 	wr_regl(port, S3C2410_UBRDIV, quot);
 
@@ -1587,12 +1587,6 @@ static void s3c24xx_serial_set_termios(struct uart_port *port,
 	if (ourport->info->has_divslot)
 		wr_regl(port, S3C2443_DIVSLOT, udivslot);
 
-	dev_dbg(port->dev,
-		"uart: ulcon = 0x%08x, ucon = 0x%08x, ufcon = 0x%08x\n",
-		rd_regl(port, S3C2410_ULCON),
-		rd_regl(port, S3C2410_UCON),
-		rd_regl(port, S3C2410_UFCON));
-
 	/*
 	 * Update the per-port timeout.
 	 */
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* [PATCH AUTOSEL 7.0-5.10] tty: serial: imx: keep dma request disabled before dma transfer setup
       [not found] <20260428104133.2858589-1-sashal@kernel.org>
  2026-04-28 10:40 ` [PATCH AUTOSEL 7.0-6.12] tty: serial: samsung_tty: avoid dev_dbg deadlock Sasha Levin
@ 2026-04-28 10:40 ` Sasha Levin
  2026-04-28 10:40 ` [PATCH AUTOSEL 7.0-5.10] serial: qcom-geni: Fix RTS behavior with flow control Sasha Levin
  2 siblings, 0 replies; 3+ messages in thread
From: Sasha Levin @ 2026-04-28 10:40 UTC (permalink / raw)
  To: patches, stable
  Cc: Robin Gong, Sherry Sun, Greg Kroah-Hartman, Sasha Levin,
	jirislaby, shawnguo, linux-kernel, linux-serial, linux-arm-kernel

From: Robin Gong <yibin.gong@nxp.com>

[ Upstream commit 74e0c9f0528bcd597cb1299a027d7be27d1c27d9 ]

Since sdma hardware configure postpone to transfer phase, have to
disable dma request before dma transfer setup because there is a
hardware limitation on sdma event enable(ENBLn) as below.

Refer SDMA 2.6.28 Channel Enable RAM (SDMAARMx_CHNENBLn) section:
"It is thus essential for the Arm platform to program them before any
DMA request is triggered to the SDMA, otherwise an unpredictable
combination of channels may be started."

Signed-off-by: Robin Gong <yibin.gong@nxp.com>
Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
Link: https://patch.msgid.link/20260312094526.297348-1-sherry.sun@nxp.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

Now I have enough information to provide the full analysis.

## Analysis: tty: serial: imx: keep dma request disabled before dma
transfer setup

### PHASE 1: COMMIT MESSAGE FORENSICS

**Step 1.1: Subject line**
Record: `tty: serial: imx:` subsystem, action verb "keep" (implies
correcting improper state), one-line summary: "ensure UART DMA request
stays disabled until SDMA channel is fully configured."

**Step 1.2: Tags**
Record:
- Signed-off-by: Robin Gong (author, NXP), Sherry Sun (submitter, NXP),
  Greg Kroah-Hartman (tty maintainer)
- Link:
  https://patch.msgid.link/20260312094526.297348-1-sherry.sun@nxp.com
- NO Fixes: tag (expected)
- NO Cc: stable tag (expected)
- NO Reported-by/Tested-by/Reviewed-by

**Step 1.3: Commit body analysis**
Record: Commit explains a hardware limitation documented in the SDMA
reference manual (section 2.6.28, "Channel Enable RAM /
SDMAARMx_CHNENBLn"). When UART asserts a DMA request before the SDMA
channel script has been properly configured, "unpredictable combination
of channels may be started." No stack trace/reproducer, but cites an
authoritative NXP/Freescale hardware reference manual. Root cause: SDMA
configuration is postponed to the transfer phase, so starting the UART
DMA request before `dma_async_issue_pending()` on the corresponding
channel is a hardware-level ordering violation.

**Step 1.4: Hidden bug fix detection**
Record: Yes — "keep disabled before setup" is a classic disguised bug
fix. This corrects an improper ordering that leads to undefined hardware
behavior.

### PHASE 2: DIFF ANALYSIS

**Step 2.1: Inventory**
Record: 1 file (`drivers/tty/serial/imx.c`), 3 lines modified + 2
comment lines changed, 1 line added net. Two functions touched:
`imx_uart_enable_dma()` and `imx_uart_startup()`. Scope: single-file
surgical fix.

**Step 2.2: Code flow change**

```1438:1451:drivers/tty/serial/imx.c
static void imx_uart_enable_dma(struct imx_port *sport)
{
        u32 ucr1;

        imx_uart_setup_ufcr(sport, TXTL_DMA, RXTL_DMA);

        /* set UCR1 */
        ucr1 = imx_uart_readl(sport, UCR1);
        ucr1 |= UCR1_RXDMAEN | UCR1_TXDMAEN | UCR1_ATDMAEN;
        imx_uart_writel(sport, ucr1, UCR1);

        sport->dma_is_enabled = 1;
}
```

Record:
- Hunk 1 (`imx_uart_enable_dma`): BEFORE sets `UCR1_RXDMAEN |
  UCR1_TXDMAEN | UCR1_ATDMAEN` atomically; AFTER sets only `UCR1_RXDMAEN
  | UCR1_ATDMAEN` (TXDMAEN now enabled later in `imx_uart_dma_tx`).
- Hunk 2 (`imx_uart_startup`): BEFORE calls `imx_uart_enable_dma()` THEN
  `imx_uart_start_rx_dma()`; AFTER calls `imx_uart_start_rx_dma()` THEN
  `imx_uart_enable_dma()`. The RX DMA channel is configured/submitted
  BEFORE the UART starts asserting DMA requests.

**Step 2.3: Bug mechanism**
Record: Category (h) Hardware workaround + ordering/correctness fix. The
mechanism: UART asserting DMA requests on UCR1 before SDMA has a valid
descriptor/channel configuration can trigger an ill-defined SDMA
channel, leading to corrupted/misrouted transfers. Confirmed by
verifying that `imx_uart_dma_tx()` at line 662-664 already sets
`UCR1_TXDMAEN` just before
`dmaengine_submit()/dma_async_issue_pending()` — so removing it from
`imx_uart_enable_dma()` is safe (TXDMAEN will still be set when actually
needed).

**Step 2.4: Fix quality**
Record: Obviously correct. The fix preserves exact functionality
(TXDMAEN still ends up set before TX transfer, RX DMA still starts
before UART DMA requests flow). No regression risk in the fix itself —
just reorders two well-defined function calls and defers one register
bit. No locking changes, no API changes.

### PHASE 3: GIT HISTORY INVESTIGATION

**Step 3.1: Blame**
Record: `imx_uart_enable_dma` and UART DMA support originated from
commit `b4cdc8f61beb2` ("serial: imx: add DMA support for imx6q", July
2013). The `temp |= UCR1_RDMAEN | UCR1_TDMAEN | UCR1_ATDMAEN` line was
set together from day one — the buggy ordering has been present since
2013 (kernel v3.11). All active stable trees inherit it.

**Step 3.2: Fixes: tag** — Not present. The bug is a long-standing
hardware sequencing violation.

**Step 3.3: Related file changes**
Record: Recent changes to `drivers/tty/serial/imx.c` (wake event
reporting, hrtimer, nbcon, etc.) do not touch the DMA init/enable paths
— no conflicts expected.

**Step 3.4: Author's relationship**
Record: Robin Gong is an NXP engineer and has authored the equivalent
fix in the SDMA driver itself (commit `107d06441b709` in 2018) which
changed where `sdma_event_enable()` is called. He's an authority on SDMA
hardware semantics.

**Step 3.5: Dependencies**
Record: No prerequisite commits needed. The fix depends only on
`imx_uart_dma_tx()` already containing `ucr1 |= UCR1_TXDMAEN`, which I
verified exists in v5.4, v5.10, v5.15, v6.1, v6.6, v6.12.

### PHASE 4: MAILING LIST RESEARCH

**Step 4.1: Original submission**
Record: `b4 dig -c 74e0c9f0528bc` found
https://lore.kernel.org/all/20260312094526.297348-1-sherry.sun@nxp.com/.
Single-revision v1 patch. Thread contains only the patch submission — no
review replies, no NAKs, no stable nominations. Greg KH applied it
directly.

**Step 4.2: Recipients**
Record: Sent to gregkh, jirislaby (tty maintainers), Frank.Li@nxp.com,
s.hauer@pengutronix.de, kernel@pengutronix.de, festevam, tglx, mingo.
Appropriate maintainers CC'd.

**Step 4.3: Bug report** — No explicit report linked; the fix cites the
SoC reference manual.

**Step 4.4: Related series (CRITICAL)**
Record: The SAME hardware-sequencing fix was previously applied to the
SPI driver in commit `86d57d9c07d54` ("spi: imx: keep dma request
disabled before dma transfer setup", Oct 2025). That SPI fix has already
been backported to stable branches 5.10, 5.15, 6.1, 6.6, 6.12, and 6.17
(verified via `git branch --contains`). This establishes a clear
precedent that the stable maintainers consider this exact SDMA-ordering
issue worth backporting.

**Step 4.5: Stable ML** — The SPI equivalent already flowed into stable
via AUTOSEL.

### PHASE 5: CODE SEMANTIC ANALYSIS

**Step 5.1-5.4: Call chain**
Record: `imx_uart_enable_dma()` is called exclusively from
`imx_uart_startup()`. `imx_uart_startup()` is the `uart_ops::startup`
callback, invoked every time a UART port is opened. This is a common,
user-triggerable path — every process opening `/dev/ttymxcN` hits it. So
the buggy sequencing is exercised on every UART open with DMA enabled.

**Step 5.5: Similar patterns**
Record: The same bug pattern exists in `drivers/spi/spi-imx.c` and was
fixed by commit `86d57d9c07d54`, already backported broadly. The SDMA
driver itself carries a comment "Set ENBLn earlier to make sure dma
request triggered after that" (`drivers/dma/imx-sdma.c:1859`),
corroborating that this ordering requirement is well-established
hardware lore.

### PHASE 6: STABLE TREE ANALYSIS

**Step 6.1: Buggy code exists in stable?**
Record: YES. Verified `imx_uart_enable_dma()` and the buggy
`imx_uart_enable_dma(); imx_uart_start_rx_dma();` ordering is present in
v5.4, v5.10, v5.15, v6.1, v6.6, v6.12. `imx_uart_dma_tx()` also already
contains the `ucr1 |= UCR1_TXDMAEN` statement (the dependency for the
fix).

**Step 6.2: Backport complications**
Record: Patch applies CLEANLY (`git apply --check` succeeded with no
output) against v5.4, v5.10, v5.15, v6.1, v6.6, v6.12. No backport
adjustments needed.

**Step 6.3: Related fixes already in stable**
Record: No — this particular fix has not yet flowed to stable for the
UART driver. The sibling SPI fix is already in stable trees.

### PHASE 7: SUBSYSTEM CONTEXT

Record: `drivers/tty/serial/imx.c` — IMX UART driver. Used by millions
of embedded/industrial iMX-based systems. Criticality: IMPORTANT
(driver-specific but widely deployed). Active subsystem (recent
wakeup/RXTL fixes show ongoing maintenance).

### PHASE 8: IMPACT AND RISK ASSESSMENT

**Step 8.1: Affected users**
Record: iMX SoC users (iMX6, iMX7, iMX8, iMX9 families) using UART with
DMA (typical when hardware flow control is enabled for high-throughput
serial communication).

**Step 8.2: Triggering conditions**
Record: Every time a DMA-capable iMX UART port is opened (uart startup
path). Whether the race window actually causes misbehavior depends on:
whether the TX FIFO happens to fall below the watermark immediately
after TXDMAEN is set but before a descriptor is prepared; whether the RX
line has incoming data arriving before start_rx_dma finishes. Easily
reachable on active serial links.

**Step 8.3: Failure mode**
Record: "unpredictable combination of channels may be started" per SDMA
hardware docs. In practice this can manifest as: lost/corrupted UART
data, spurious channel activation that may corrupt memory (SDMA writing
to stale addresses), or flaky DMA behavior that's hard to debug.
Severity: HIGH (data integrity + potential silent memory corruption on a
DMA engine).

**Step 8.4: Risk-benefit**
Record: BENEFIT: prevents documented hardware misbehavior on a widely-
deployed SoC family; consistency with SPI driver fix already in stable.
RISK: very low — 3-line reordering, no new locks/APIs, dependency
(TXDMAEN in dma_tx) verified present in all stable trees, patch applies
cleanly, identical fix pattern already tested in production via SPI
backport.

### PHASE 9: FINAL SYNTHESIS

**Evidence FOR:** Fixes documented SoC hardware limitation; tiny
surgical change; author is SDMA subject-matter expert; same fix pattern
already accepted into stable (5.10→6.17) for spi-imx; code path is very
common (uart_startup); patch applies cleanly to all stable trees; falls
into the "Hardware Quirk/Workaround" exception category.

**Evidence AGAINST:** No explicit Reported-by/Tested-by (but the fix
cites authoritative SoC docs); no explicit stable nomination (but
absence is expected).

**Stable rules checklist:**
1. Obviously correct and tested? YES — TXDMAEN placement verified,
   reorder is semantically equivalent.
2. Fixes a real bug? YES — documented hardware sequencing violation.
3. Important issue? YES — data corruption / unpredictable DMA behavior.
4. Small and contained? YES — 3 lines of actual code change.
5. No new features? YES — pure correctness/reorder fix.
6. Applies to stable trees? YES — clean apply verified.

**Exception category:** Hardware workaround for SDMA ENBLn sequencing
requirement.

### Verification
- [Phase 1] Parsed tags: no Fixes/Cc-stable/Reported-by; Link to lore
  verified
- [Phase 2] Diff shows `imx_uart_enable_dma` drops TXDMAEN;
  `imx_uart_startup` swaps call order of `start_rx_dma` and `enable_dma`
- [Phase 2] Verified `imx_uart_dma_tx()` in current tree and all stable
  trees contains `ucr1 |= UCR1_TXDMAEN` at lines
  ~658/657/666/660/682/637 respectively — the fix's dependency is met
- [Phase 3] `git log -S"imx_enable_dma"` identified original DMA commit
  `b4cdc8f61beb2` (Jul 2013)
- [Phase 3] `git blame` on `sdma_event_enable` call in imx-sdma.c
  corroborates the hardware ordering requirement
- [Phase 4] `b4 dig -c 74e0c9f0528bc` found lore URL; `-a` shows only
  v1; mbox inspection shows no replies/NAKs
- [Phase 4] `b4 dig -c 86d57d9c07d54` for sibling SPI fix confirmed it
  went through review (Frank Li, Mark Brown)
- [Phase 4] `git branch --contains` for multiple SPI backport commits
  confirmed backport to 5.10/5.15/6.1/6.6/6.12/6.17
- [Phase 5] Confirmed `imx_uart_enable_dma` has a single caller
  (`imx_uart_startup`) via `Grep`
- [Phase 6] `git show vX:drivers/tty/serial/imx.c` for v5.4, v5.10,
  v5.15, v6.1, v6.6, v6.12 all show identical buggy ordering
- [Phase 6] `git apply --check /tmp/fix.patch` succeeded on v5.4, v5.10,
  v5.15, v6.1, v6.6, v6.12 (no output = success)
- [Phase 8] Failure mode per SoC reference manual — cited directly in
  commit message
- UNVERIFIED: No user bug report could be located; severity inferred
  from SoC documentation only. The exact race-window probability on a
  specific board is not quantified.

Small, well-contained hardware-sequencing fix; same issue already
accepted into stable trees for the sister SPI driver; dependency
verified; applies cleanly. Meets all stable criteria.

**YES**

 drivers/tty/serial/imx.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index c488e5d372ffd..251a50c8aa387 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1442,9 +1442,9 @@ static void imx_uart_enable_dma(struct imx_port *sport)
 
 	imx_uart_setup_ufcr(sport, TXTL_DMA, RXTL_DMA);
 
-	/* set UCR1 */
+	/* set UCR1 except TXDMAEN which would be enabled in imx_uart_dma_tx */
 	ucr1 = imx_uart_readl(sport, UCR1);
-	ucr1 |= UCR1_RXDMAEN | UCR1_TXDMAEN | UCR1_ATDMAEN;
+	ucr1 |= UCR1_RXDMAEN | UCR1_ATDMAEN;
 	imx_uart_writel(sport, ucr1, UCR1);
 
 	sport->dma_is_enabled = 1;
@@ -1567,8 +1567,9 @@ static int imx_uart_startup(struct uart_port *port)
 	imx_uart_enable_ms(&sport->port);
 
 	if (dma_is_inited) {
-		imx_uart_enable_dma(sport);
+		/* Note: enable dma request after transfer start! */
 		imx_uart_start_rx_dma(sport);
+		imx_uart_enable_dma(sport);
 	} else {
 		ucr1 = imx_uart_readl(sport, UCR1);
 		ucr1 |= UCR1_RRDYEN;
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* [PATCH AUTOSEL 7.0-5.10] serial: qcom-geni: Fix RTS behavior with flow control
       [not found] <20260428104133.2858589-1-sashal@kernel.org>
  2026-04-28 10:40 ` [PATCH AUTOSEL 7.0-6.12] tty: serial: samsung_tty: avoid dev_dbg deadlock Sasha Levin
  2026-04-28 10:40 ` [PATCH AUTOSEL 7.0-5.10] tty: serial: imx: keep dma request disabled before dma transfer setup Sasha Levin
@ 2026-04-28 10:40 ` Sasha Levin
  2 siblings, 0 replies; 3+ messages in thread
From: Sasha Levin @ 2026-04-28 10:40 UTC (permalink / raw)
  To: patches, stable
  Cc: Anup Kulkarni, Greg Kroah-Hartman, Sasha Levin, agross, andersson,
	konrad.dybcio, jirislaby, linux-arm-msm, linux-kernel,
	linux-serial

From: Anup Kulkarni <anup.kulkarni@oss.qualcomm.com>

[ Upstream commit 0b1837c04d2335ec50b9a55b0282dcde7bc12439 ]

When userspace enables flow control (CRTSCTS), the driver
deasserts RTS even when the receive buffer has space. This prevents the
peer device from transmitting, causing communication to stall.

The root cause is that the driver unconditionally uses manual RTS control
regardless of flow control mode. When CRTSCTS is set, the hardware should
automatically manage RTS based on buffer status, but the driver overrides
this by setting manual control.

Fix this by introducing port->manual_flow flag. In set_termios(), disable
manual flow when CRTSCTS is set. In set_mctrl(), only assert
SE_UART_MANUAL_RFR when manual_flow is active. Verified by enabling and
disabling hardware flow control with stty.

Signed-off-by: Anup Kulkarni <anup.kulkarni@oss.qualcomm.com>
Link: https://patch.msgid.link/20260310104155.339010-1-anup.kulkarni@oss.qualcomm.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

Now I have enough information to complete the full analysis.

## Analysis

### PHASE 1: COMMIT MESSAGE FORENSICS

**Step 1.1: Subject line parse**
- Record: [serial: qcom-geni:] [fix] [RTS behavior with hardware flow
  control (CRTSCTS)]

**Step 1.2: Tag inventory**
- Record: Signed-off-by: Anup Kulkarni (author, @oss.qualcomm.com),
  Signed-off-by: Greg KH (applied to tty tree). Link: patch.msgid.link
  URL. NO Fixes: tag, NO Cc: stable, NO Reported-by, NO Tested-by, NO
  Reviewed-by, NO Acked-by. Only v1 of patch, no reviewer replies on
  thread.

**Step 1.3: Commit body analysis**
- Record: Bug: "driver deasserts RTS even when the receive buffer has
  space" when CRTSCTS is enabled, causing "communication to stall". Root
  cause: driver unconditionally uses manual RTS control via
  `UART_MANUAL_RFR_EN`, which overrides hardware's auto-flow control. No
  stack trace or reproducer. Author verified fix "by enabling and
  disabling hardware flow control with stty".

**Step 1.4: Hidden bug fix detection**
- Record: Not hidden - commit message explicitly says "Fix RTS behavior"
  and describes the bug clearly.

### PHASE 2: DIFF ANALYSIS

**Step 2.1: Inventory**
- Record: Single file `drivers/tty/serial/qcom_geni_serial.c`. +15/-4
  lines. Modifies `qcom_geni_serial_set_mctrl()` (1 line guard added)
  and `qcom_geni_serial_set_termios()` (restructured CRTSCTS branch).
  Adds `bool manual_flow` to `struct qcom_geni_serial_port`. Scope:
  surgical single-driver fix.

**Step 2.2: Code flow change**
- Record:
  - set_mctrl BEFORE: `if (!(mctrl & TIOCM_RTS) && !uport->suspended)` →
    enable manual RFR (`UART_MANUAL_RFR_EN | UART_RFR_NOT_READY`) then
    unconditionally write.
  - set_mctrl AFTER: Only enables manual RFR when `port->manual_flow` is
    true AND RTS not set AND not suspended.
  - set_termios BEFORE: Only toggles `UART_CTS_MASK` bit in TX config.
  - set_termios AFTER: Also sets `port->manual_flow = false` when
    CRTSCTS set (HW manages), `true` otherwise.

**Step 2.3: Bug mechanism**
- Record: Category (g) Logic/correctness fix. The mechanism: when
  CRTSCTS is enabled, hardware should automatically drive RTS based on
  RX FIFO fullness. But any call to `set_mctrl` with `!TIOCM_RTS` (which
  happens during resume: `ops->set_mctrl(uport, 0)` at
  serial_core.c:2421, at B0 baud transitions serial_core.c:1685, or on
  ioctl TIOCMBIC) would cause the driver to write `UART_MANUAL_RFR_EN |
  UART_RFR_NOT_READY`, forcing manual RTS deassertion and preventing the
  peer from transmitting.

**Step 2.4: Fix quality**
- Record: Simple, easy to verify by reading. Logic is correct: when
  hardware flow control is enabled, never override with manual mode. The
  `manual_flow` flag is BSS-initialized to `false`, which matches
  "hardware controls RTS" default. Small regression risk: before the
  first `set_termios` call, any `set_mctrl(!TIOCM_RTS)` path now writes
  0 instead of enabling manual mode; this is arguably more correct but
  changes initial state. No public API changes, no lock changes.

### PHASE 3: GIT HISTORY INVESTIGATION

**Step 3.1: Blame the changed lines**
- Record: The manual RFR logic in `set_mctrl` was added in
  `8a8a66a1a18a1` (v4.19, July 2018) "tty: serial: qcom_geni_serial: Add
  support for flow control". The early-return guard
  `!uart_cts_enabled(uport)` was removed in `e8a6ca808c5ed` (v5.0) "tty:
  serial: qcom_geni_serial: Allow mctrl when flow control is disabled" —
  which actually made the bug more exposed (before that change, the
  manual-mode code path was unreachable when CRTSCTS was off, but still
  buggy when on).

**Step 3.2: Follow the Fixes: tag**
- Record: No Fixes: tag in this commit. Most likely should reference
  `8a8a66a1a18a1` (v4.19) as the origin — the bug has existed since flow
  control support was added to this driver.

**Step 3.3: File history for related changes**
- Record: Recent related fix `947cc4ecc06cb` "serial: qcom-geni: fix
  soft lockup on sw flow control and suspend" (July 2024) was tagged
  `Cc: stable # 4.17` — shows that flow-control-related bugs in this
  driver have been deemed stable-worthy. Also `23f5f5debcaac` "serial:
  qcom-geni: fix shutdown race" exists. No overlap/conflict with this
  fix.

**Step 3.4: Author commits**
- Record: `git log --author="Anup Kulkarni"` shows only 1 other commit
  (`4fcc287f3c692` "serial: qcom-geni: Enable support for half-duplex
  mode"). Newer contributor but from @oss.qualcomm.com - this is the
  vendor (Qualcomm) whose hardware this driver supports. Patch went
  through Greg KH's tty tree.

**Step 3.5: Dependencies**
- Record: No dependencies found. The fix is self-contained and
  references only symbols that exist since the original flow control
  support commit (v4.19).

### PHASE 4: MAILING LIST RESEARCH

**Step 4.1: b4 dig**
- Record: Single revision (v1) at https://patch.msgid.link/2026031010415
  5.339010-1-anup.kulkarni@oss.qualcomm.com. Thread mbox retrieved. Only
  one message in thread - just the patch itself, no replies, no NAK, no
  explicit stable nomination.

**Step 4.2: Who reviewed**
- Record: `b4 dig -w` shows to/cc: gregkh, jirislaby (tty maintainers),
  praveen.talari/viken.dadhaniya/zongjian/jseerapu (Qualcomm),
  bryan.odonoghue (linaro), krzk (Krzysztof Kozlowski), linux-serial,
  linux-arm-msm, linux-kernel. Appropriate maintainers were CC'd but no
  one publicly replied on lore before Greg applied it.

**Step 4.3: Bug report search**
- Record: No Reported-by or bug link in commit. Web search did not
  surface a specific user report for this stall.

**Step 4.4: Related patches/series**
- Record: `b4 dig -a` shows v1 only; standalone single-patch submission.

**Step 4.5: Stable list history**
- Record: Nothing found discussing this specific fix on stable@.

### PHASE 5: CODE SEMANTIC ANALYSIS

**Step 5.1: Functions modified**
- Record: `qcom_geni_serial_set_mctrl`, `qcom_geni_serial_set_termios`.

**Step 5.2: Callers**
- Record: Both are `uart_ops` callbacks registered in
  `qcom_geni_console_pops`/`qcom_geni_uart_pops`. Called indirectly
  through `port->ops->set_mctrl(...)` and `uport->ops->set_termios(...)`
  from `drivers/tty/serial/serial_core.c`. Key caller sites for
  `set_mctrl`: startup/shutdown, suspend/resume (lines 2333/2421), RS485
  disable path (1483), B0 transitions (1685/1692), throttle/unthrottle
  (with AUTORTS — not used here). This makes the bug reachable on every
  resume and on any baud change to/from B0 when CRTSCTS is active — very
  common paths.

**Step 5.3: Callees**
- Record: set_mctrl only calls `writel(...)` to SE_UART_MANUAL_RFR. No
  locking, no allocation. Minimal side effects.

**Step 5.4: Call chain reachability**
- Record: Reachable from any userspace UART open with CRTSCTS, stty
  changes, system suspend/resume, and B0 transitions. Definitely user-
  reachable, exercised on every device with hardware flow control
  enabled.

**Step 5.5: Similar patterns**
- Record: Verified driver does NOT advertise `UPSTAT_AUTORTS` (no hits
  for that flag) - so auto-RTS tty layer logic doesn't apply; the driver
  relies entirely on hardware register-level RFR management when CRTSCTS
  is on. This confirms the issue: the driver's set_mctrl was silently
  overriding hardware-managed RTS.

### PHASE 6: STABLE TREE ANALYSIS

**Step 6.1: Does buggy code exist in stable?**
- Record: Verified the identical buggy `set_mctrl` body exists in stable
  6.17.y, 6.12.y, 6.6.y, 6.1.y, and 5.15.y. The same CRTSCTS branch `if
  (termios->c_cflag & CRTSCTS) tx_trans_cfg &= ~UART_CTS_MASK; else
  tx_trans_cfg |= UART_CTS_MASK;` is present in all of them. Bug has
  existed since v4.19 → affects ALL currently supported stable trees.

**Step 6.2: Backport complications**
- Record: Low complexity backport. The struct has `bool cts_rts_swap` in
  every stable branch (verified). Both hunks context-match. Minor
  difference: 5.15 uses legacy `to_dev_port(uport, uport)` macro
  (irrelevant to the hunk). Expected: clean apply or minor context
  rewrap.

**Step 6.3: Related fixes already in stable**
- Record: `947cc4ecc06cb` (flow control soft lockup fix) is already in
  stable and addresses a different flow-control issue. No conflict with
  this fix.

### PHASE 7: SUBSYSTEM CONTEXT

**Step 7.1: Subsystem criticality**
- Record: Subsystem: `drivers/tty/serial/` — Qualcomm GENI serial
  driver. Criticality: PERIPHERAL (driver-specific) but IMPORTANT for
  the affected platforms (Qualcomm SoCs used in Chromebooks, embedded
  devices, Android phones, etc., where hardware flow control to
  Bluetooth/GPS/modem peripherals is critical).

**Step 7.2: Subsystem activity**
- Record: Driver is actively maintained, with regular fixes going in.
  This suggests real users.

### PHASE 8: IMPACT AND RISK ASSESSMENT

**Step 8.1: Who is affected**
- Record: Users of Qualcomm SoCs running Linux that use UART with
  `CRTSCTS` enabled — commonly Bluetooth HCI over UART, GPS modules,
  baseband modems. Affects Android devices, Chromebooks, embedded
  Qualcomm platforms.

**Step 8.2: Trigger conditions**
- Record: Trigger is any invocation of set_mctrl with RTS cleared while
  CRTSCTS is active. Concrete triggers:
  1. System suspend/resume cycle (very common on mobile/laptop)
  2. B0 baud transitions (modem hangup)
  3. Any direct ioctl(TIOCMBIC, &TIOCM_RTS)
  Unprivileged? Root access to the tty device is typical for the
trigger.

**Step 8.3: Failure mode severity**
- Record: Functional failure — communication stalls because RTS is stuck
  deasserted and peer stops transmitting. No kernel crash, no data
  corruption, no security hole. Severity: MEDIUM-HIGH — "communication
  stall" is a real stability issue for devices depending on this UART
  (e.g., Bluetooth dropouts, serial session lockups). Not a crash, but
  noticeable and disruptive.

**Step 8.4: Risk-benefit**
- Record:
  - Benefit: MEDIUM — fixes a real functional bug on actively-used
    hardware (Qualcomm platforms) with a clear reproduction path,
    especially after resume.
  - Risk: LOW — 15 lines, single file, trivial to audit, no
    locking/memory changes, no API changes. The `manual_flow` field
    defaults to `false` which aligns with hardware-managed mode (safe
    default).
  - Ratio: Favorable for backport. Very low risk of regression for
    benefit of fixing real-world stalls.

### PHASE 9: FINAL SYNTHESIS

**Step 9.1: Evidence**
- FOR: Real user-visible bug clearly described; present since v4.19 in
  all stable trees; small surgical fix; touches only a driver callback;
  correct logical direction (CRTSCTS means HW controls, manual mode
  should be off); fix verified by author with stty; applies cleanly; no
  API/feature additions; author is Qualcomm, i.e., the hardware vendor.
- AGAINST: No Fixes: tag; no Cc: stable; no reviewer Ack/Reviewed-by; no
  user bug report linked; single-revision patch with no lore discussion;
  reachable in common paths but severity is functional (not
  crash/security/corruption).
- UNRESOLVED: Could not directly observe the reproduction trace (no
  stack trace in commit).

**Step 9.2: Stable rules checklist**
1. Obviously correct? YES — straightforward logic; author tested.
2. Fixes a real bug? YES — stated reproducer via stty.
3. Important? MEDIUM — functional stall on Qualcomm platforms using flow
   control (BT/GPS/modem), not a crash/security/corruption.
4. Small and contained? YES — 15 lines, 1 file, 1 driver.
5. No new features/APIs? YES — internal bool field only.
6. Applies to stable? YES — verified code exists unchanged in 5.15.y
   through 6.17.y.

**Step 9.3: Exception categories**
- Not strictly an exception, but closely aligned with a hardware
  workaround in a mature driver.

**Step 9.4: Decision**
This is a real, reproducible bug fix in a driver used on widely-deployed
Qualcomm hardware (Chromebooks, Android, embedded). The fix is tiny,
purely internal, doesn't introduce APIs or affect other subsystems, and
the logic is clearly correct (hardware flow control should not be
overridden by manual RTS assertion). The bug is reachable in common
operational paths (resume, B0 transitions). Backport risk is minimal.

## Verification

- [Phase 1] Parsed tags from commit: only Signed-off-by (author + Greg
  KH) and Link. No Fixes:, no Cc: stable, no Reported-by, no Reviewed-
  by/Acked-by.
- [Phase 2] Diff analysis confirmed on the actual patch text: 1-line
  guard in set_mctrl, 2 branches expanded to set `manual_flow`, 1 new
  struct field.
- [Phase 3] `git log` on file showed history; `git show 8a8a66a1a18a1`
  and `git show e8a6ca808c5ed` confirmed the code origin (v4.19 via git
  describe) and the removal of the CTS-enabled guard (v5.0).
- [Phase 3] `git describe --contains 8a8a66a1a18a1` →
  v4.19-rc1~102^2~33; `e8a6ca808c5ed` → v5.0-rc4~20^2~1. Bug has been
  present since v4.19.
- [Phase 3] `git log --author="Anup Kulkarni"` → 2 commits total (this
  one plus half-duplex mode). Relatively new contributor, Qualcomm
  vendor author.
- [Phase 3] `git log --grep="serial.*qcom.*flow"` → confirmed
  `947cc4ecc06cb` (previous flow control fix, tagged `Cc: stable #
  4.17`) is a precedent.
- [Phase 4] `b4 dig -c 0b1837c04d233` → found single lore thread, v1
  only, single message, no replies.
- [Phase 4] `b4 dig -c 0b1837c04d233 -w` → recipients confirmed: gregkh,
  jirislaby, Qualcomm team, bryan.odonoghue, krzk, linux-serial, linux-
  arm-msm, linux-kernel.
- [Phase 4] `b4 dig -c 0b1837c04d233 -a` → only v1 exists; went direct
  to Greg's tree.
- [Phase 4] Saved thread to /tmp/qcom_geni_thread.mbox; read contents -
  confirmed no replies, no reviewer feedback.
- [Phase 5] Grepped `UPSTAT_AUTORTS|UPSTAT_AUTOCTS` in
  qcom_geni_serial.c → no matches. Driver does NOT use UPSTAT_AUTORTS,
  so throttle/unthrottle don't clear RTS through set_mctrl path.
- [Phase 5] Grepped `SE_UART_MANUAL_RFR|UART_MANUAL_RFR_EN` → only 2
  writers: `qcom_geni_set_rs485_mode()` (only when RS485 enabled) and
  `qcom_geni_serial_set_mctrl()` (the fixed function). No other paths
  touch this register.
- [Phase 5] Read `serial_core.c:2320-2451` → confirmed
  `ops->set_mctrl(uport, 0)` is called at resume (2421) with
  `uport->suspended` already 0, directly triggering the bug path when
  CRTSCTS is active.
- [Phase 5] Read `serial_core.c:1680-1694` → confirmed B0 transition
  logic can cause set_mctrl with !TIOCM_RTS while CRTSCTS is active.
- [Phase 6] `git show stable/linux-
  {6.17,6.12,6.6,6.1,5.15}.y:drivers/tty/serial/qcom_geni_serial.c` →
  verified identical buggy `set_mctrl` body and identical CRTSCTS
  handling in set_termios in all five stable trees. Struct has
  `cts_rts_swap` in all of them, so the `manual_flow` addition will fit
  cleanly.
- [Phase 8] Failure mode: communication stall (peer cannot transmit). No
  crash. Severity: MEDIUM-HIGH functional. Trigger: common (resume, B0).
- UNVERIFIED: Could not obtain a literal crash/stall reproducer from
  commit message; the author's claim of verification via stty is the
  only direct test evidence. Did not independently reproduce the bug.

The fix is small, narrowly scoped, logically sound, and addresses a real
functional bug that is triggered on common paths (resume, B0) in a
driver used on widely-deployed hardware. It meets the stable kernel
rules.

**YES**

 drivers/tty/serial/qcom_geni_serial.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index e6b0a55f0cfb2..9854bb2406e3f 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -146,6 +146,7 @@ struct qcom_geni_serial_port {
 	int wakeup_irq;
 	bool rx_tx_swap;
 	bool cts_rts_swap;
+	bool manual_flow;
 
 	struct qcom_geni_private_data private_data;
 	const struct qcom_geni_device_data *dev_data;
@@ -250,7 +251,7 @@ static void qcom_geni_serial_set_mctrl(struct uart_port *uport,
 	if (mctrl & TIOCM_LOOP)
 		port->loopback = RX_TX_CTS_RTS_SORTED;
 
-	if (!(mctrl & TIOCM_RTS) && !uport->suspended)
+	if (port->manual_flow && !(mctrl & TIOCM_RTS) && !uport->suspended)
 		uart_manual_rfr = UART_MANUAL_RFR_EN | UART_RFR_NOT_READY;
 	writel(uart_manual_rfr, uport->membase + SE_UART_MANUAL_RFR);
 }
@@ -1401,11 +1402,21 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
 	else
 		stop_bit_len = TX_STOP_BIT_LEN_1;
 
-	/* flow control, clear the CTS_MASK bit if using flow control. */
-	if (termios->c_cflag & CRTSCTS)
+	/* Configure flow control based on CRTSCTS flag.
+	 * When CRTSCTS is set, use HW/auto flow control mode, where HW
+	 * controls the RTS/CTS pin based FIFO state.
+	 * When CRTSCTS is clear, the CTS pin value is ignored for TX
+	 * path and RTS pin can be set/cleared using registers, for RX
+	 * path.
+	 */
+
+	if (termios->c_cflag & CRTSCTS) {
 		tx_trans_cfg &= ~UART_CTS_MASK;
-	else
+		port->manual_flow = false;
+	} else {
 		tx_trans_cfg |= UART_CTS_MASK;
+		port->manual_flow = true;
+	}
 
 	if (baud) {
 		uart_update_timeout(uport, termios->c_cflag, baud);
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-04-28 10:42 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20260428104133.2858589-1-sashal@kernel.org>
2026-04-28 10:40 ` [PATCH AUTOSEL 7.0-6.12] tty: serial: samsung_tty: avoid dev_dbg deadlock Sasha Levin
2026-04-28 10:40 ` [PATCH AUTOSEL 7.0-5.10] tty: serial: imx: keep dma request disabled before dma transfer setup Sasha Levin
2026-04-28 10:40 ` [PATCH AUTOSEL 7.0-5.10] serial: qcom-geni: Fix RTS behavior with flow control Sasha Levin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox