[PATCH net-next v10 0/2] net: wwan: support DTR/RTS on AT ports via MHI IP_CTRL

[PATCH net-next v10 0/2] net: wwan: support DTR/RTS on AT ports via MHI IP_CTRL

[Peter Hunt]

Qualcomm/Sierra SDX55/SDX65 modems (e.g. EM9291) withhold unsolicited AT
result codes until the host asserts DTR. The in-tree mhi_wwan_ctrl driver
exposes AT ports but never signals DTR, so URCs never reach userspace.

Patch 1 extends the wwan core with an optional ->dtr_rts(port, mdmbits)
port op. The TIOCM bitmask state is tracked in the wwan core, which raises
DTR/RTS on first open of an AT port whose driver implements ->dtr_rts,
drops them on last close (if HUPCL is set, which is the default for AT
ports) and on port removal, and passes the resolved bitmask to the
driver on TIOCMSET/TIOCMBIC/TIOCMBIS.

Patch 2 adds a second mhi_driver to mhi_wwan_ctrl that binds the IP_CTRL
channel and implements ->dtr_rts by sending the host serial state to the
modem over that channel. The existing AT/QMI/MBIM data path is untouched.

This is a two-patch series. The pci_generic patch from v5 that
enumerates the IP_CTRL channel for the Sierra EM919x/EM929x has been
applied to mhi-next by Mani as commit 83c29a55b89e ("bus: mhi: host:
pci_generic: Add IP_CTRL channel for Sierra EM919x/EM929x"). There is no
build dependency between the two, without that commit the IP_CTRL driver
simply never binds and ->dtr_rts is a no-op.

Note on the ->dtr_rts signature (Loic):

In v3 I replaced ->tiocmget/->tiocmset with ->dtr_rts(port, bool on)
modelled on tty_port_operations, and moved the TIOCM handling into the
wwan core, as you suggested on v2. Review of v3 and v4 then pointed out
that a single bool cannot represent the two lines independently. With
TIOCMBIC/TIOCMBIS on one line while the other is in the opposite state,
the line state sent to the modem no longer matches what TIOCMGET reports
(e.g. RTS re-asserted after TIOCMBIC(TIOCM_RTS)).

Since v5 the op keeps the dtr_rts name and the core still owns all TIOCM
handling, but it is passed the resolved TIOCM bitmask instead of a bool,
so the driver can drive DTR and RTS independently. I kept the dtr_rts
name deliberately, following your v2 preference over ->tiocmset, even
though the signature now differs from tty_port_operations.dtr_rts. The
open/close paths still behave as tty_port dtr_rts does. Is this
acceptable to you, or would you prefer a different shape, for example a
bool ->dtr_rts for open/close plus a separate op for the ioctl path?

Changes in v10 (Sashiko review of v9):
- Patch 1: the AT/QCDM ioctl handler only holds ops_lock around the
  termios/TIOCM state, never across a user-memory copy, returns
  -ERESTARTSYS when interrupted, and no longer takes ops_lock for
  ioctls that don't touch that state (TCFLSH, TIOCEXCL and friends,
  and the FIONREAD fallback). Still ops_lock only, as agreed with Loic
- Patch 1: word the TTY comparisons and the ->dtr_rts kernel-doc more
  precisely (the baud rate is not used, removal only drops the lines
  while the port is open, open only once ->start() has succeeded)
- Patch 2: send DTR/RTS changes from a per-port work item that
  coalesces bursts and retries a send that fails because the IP_CTRL UL
  ring is full or memory is short, so the modem always ends up with the
  final state. Stress testing showed rapid toggling could fill the ring
  and drop updates. Loic, I've dropped your Reviewed-by on this patch as
  the send path changed

Changes in v9:
- Patch 1: drop data_lock, so the termios/TIOCM state and ->dtr_rts
  calls are all serialised by ops_lock, which the AT/QCDM ioctl handler
  now takes interruptibly (Loic Poulain)
- Patch 1: raise DTR/RTS inside the existing first-open branch of
  wwan_port_op_start() (Loic Poulain)
- Patch 1: mask TIOCMSET/TIOCMBIS/TIOCMBIC to the output bits, only call
  ->dtr_rts when DTR or RTS changes, and commit TCSETS/TCSETS2 only on a
  successful copy (Sashiko review of v7)
- Patch 2: requeue the IP_CTRL DL sink buffer only from the refill work,
  and disable it with disable_delayed_work_sync() before
  mhi_unprepare_from_transfer() in remove() (Loic Poulain, Sashiko
  review of v7 and v8)
- Patch 2: leave unpreparing to mhi_probe() when probe fails, and name
  the wwan core commit in the commit message (Sashiko review of v7)
- DTR/RTS stays limited to AT ports, as agreed with Loic and Daniele,
  and the ->dtr_rts kernel-doc now says why

Changes in v8 (Loic Poulain's review of v7, no functional change):
- Patch 1: add wwan_port_raise_dtr_rts() as the counterpart of
  wwan_port_drop_dtr_rts() and move the open-path handling into it
- Patch 1: do the AT port and ->dtr_rts checks inside both helpers
- Patch 1: use scoped_guard() for data_lock in the helpers

Changes in v7 (all from the Sashiko review of v6):
- Patch 1: drop DTR/RTS on last close only if HUPCL is set, as a TTY
  does, and default HUPCL on for AT ports so close behaviour is
  unchanged unless userspace clears it with TCSETS
- Patch 1: wwan_remove_port() passes the masked mdmbits rather than 0,
  via a helper shared with the close path
- Patch 2: retry a failed DL sink buffer requeue from a delayed work
  item on -ENOMEM, and log other failures
- Patch 2: note in the commit message that the IP_CTRL channel entry
  for the EM919x/EM929x is 83c29a55b89e in the MHI tree

Changes in v6:
- Rebased onto net-next, dropped the pci_generic patch (now in mhi-next)
- Patch 1: snapshot mdmbits under data_lock before calling ->dtr_rts from
  the open/close paths
- Patch 2: allocate the IP_CTRL DL sink buffer separately instead of
  embedding it in struct mhi_wwan_dtr (DMA safety on non-coherent
  platforms), and do not requeue it when the DL transfer completes with an
  error such as -ENOTCONN during channel teardown

v9: https://lore.kernel.org/netdev/20261012172219.3668624-1-peter.hunt@opengear.com/
v8: https://lore.kernel.org/netdev/20261007182238.1154851-1-peter.hunt@opengear.com/
v7: https://lore.kernel.org/netdev/20261006040422.3208888-1-peter.hunt@opengear.com/
v6: https://lore.kernel.org/netdev/20261001204614.3481089-1-peter.hunt@opengear.com/
v5: https://lore.kernel.org/netdev/20260819224927.2274790-1-peter.hunt@opengear.com/
v4: https://lore.kernel.org/netdev/20260816121705.858013-1-peter.hunt@opengear.com/
v3: https://lore.kernel.org/netdev/20260807215042.2714442-1-peter.hunt@opengear.com/
v2: https://lore.kernel.org/netdev/20260806155253.3378294-1-peter.hunt@opengear.com/
v1: https://lore.kernel.org/netdev/20260804233411.1953445-1-peter.hunt@opengear.com/

Peter Hunt (2):
  net: wwan: core: propagate modem control signals to port drivers
  net: wwan: mhi_wwan_ctrl: drive DTR/RTS via the IP_CTRL channel

 drivers/net/wwan/mhi_wwan_ctrl.c | 262 ++++++++++++++++++++++++++++++-
 drivers/net/wwan/wwan_core.c     | 172 +++++++++++++-------
 include/linux/wwan.h             |   9 ++
 3 files changed, 385 insertions(+), 58 deletions(-)

-- 
2.43.0

[PATCH net-next v10 1/2] net: wwan: core: propagate modem control signals to port drivers

[Peter Hunt]

The WWAN character device emulates the TTY modem-control ioctls
(TIOCMGET/TIOCMSET/TIOCMBIC/TIOCMBIS) for AT and QCDM ports, but the
result is only stored in port->at_data.mdmbits and never reaches the port
driver. A driver therefore cannot act on the host raising or dropping
DTR/RTS, even though some modems depend on it (e.g. they withhold
unsolicited AT result codes until the host asserts DTR).

Add an optional ->dtr_rts(port, mdmbits) operation to struct wwan_port_ops.
Drivers that implement it receive the TIOCM output bitmask so they can
assert or de-assert DTR and RTS independently. As in tty_tiocmset(), the
wwan core keeps only the output bits (DTR, RTS, OUT1, OUT2, LOOP) of the
TIOCMSET/TIOCMBIS/TIOCMBIC value in port->at_data.mdmbits, and calls
->dtr_rts when DTR or RTS actually changes, gated on WWAN_PORT_AT to match
the open/close raise/drop behaviour.

Also raise DTR/RTS in wwan_port_op_start on first open of an AT port
when the driver implements ->dtr_rts, and drop them in wwan_port_op_stop
on last close if HUPCL is set in the port's termios. This follows TTY
open/close semantics, although the termios baud rate is not used, and
means individual drivers do not need to implement this themselves. As on
a TTY, HUPCL defaults to on for AT ports, and userspace can clear it
with TCSETS to keep DTR asserted across close, for example so that
closing the port does not end a call on a modem set to AT&D1/AT&D2.
TCSETS and TCSETS2 now only commit the new termios when the copy from
userspace succeeds, so a faulting call cannot silently clear HUPCL.

->dtr_rts must be called under ops_lock, which serialises port ops and
protects against port removal (removal clears port->ops under it). So
that updates to the TIOCM state and the ->dtr_rts calls are serialised
together, the termios/TIOCM state now also lives under ops_lock and the
separate data_lock is removed. The AT/QCDM ioctl handler only holds
ops_lock while it reads or updates that state, never across a
user-memory copy, and takes it interruptibly since a driver's blocking
write holds it while it waits. Ioctls that do not touch that state, such
as TCFLSH, TIOCEXCL and the FIONREAD fallback, do not take it.

wwan_remove_port() also drops DTR/RTS before ->stop() when a port is
removed while still open, regardless of HUPCL since the device is going
away. The raise and drop paths go through two small helpers, which do
the AT port and ->dtr_rts checks and pass the driver the resulting
bitmask.

Signed-off-by: Peter Hunt <peter.hunt@opengear.com>
---
v10: Only hold ops_lock in the AT ioctl handler around the termios/TIOCM
    state, with user copies done outside it, return -ERESTARTSYS when
    interrupted, and leave ops_lock alone for ioctls that do not touch
    that state. Word the TTY comparisons and the ->dtr_rts kernel-doc
    more precisely (the baud rate is not used, removal only drops the
    lines while the port is open, open only once ->start() has
    succeeded). Both from the Sashiko review of v9, still using ops_lock
    only as agreed with Loic Poulain
v9: Raise DTR/RTS inside the existing first-open branch of
    wwan_port_op_start(), and drop data_lock so the termios/TIOCM state
    and ->dtr_rts calls are all serialised by ops_lock, taken
    interruptibly in the ioctl handler (Loic Poulain). Mask
    TIOCMSET/TIOCMBIS/TIOCMBIC to the output bits and only call
    ->dtr_rts when DTR or RTS changes, commit TCSETS/TCSETS2 only on a
    successful copy, and describe the calls in the ->dtr_rts kernel-doc
    (Sashiko review of v7). Document in the ->dtr_rts kernel-doc why it
    is AT-only (Loic Poulain, Daniele Palmas)
v8: Add wwan_port_raise_dtr_rts() as the counterpart of
    wwan_port_drop_dtr_rts() and move the open-path handling there, move
    the AT port and ->dtr_rts checks into both helpers, use
    scoped_guard() for data_lock (Loic Poulain)
v7: Drop DTR/RTS on last close only if HUPCL is set, and default HUPCL on
    for AT ports. Pass the masked mdmbits rather than 0 from
    wwan_remove_port(), via a helper shared with the close path. Both
    from the Sashiko review of v6
v6: Rebase onto net-next. Snapshot mdmbits under data_lock before calling
    ->dtr_rts from wwan_port_op_start()/wwan_port_op_stop(), rather than
    reading it after data_lock has been released
v5: Change ->dtr_rts signature from bool to unsigned int mdmbits so DTR
    and RTS can be driven independently; re-read mdmbits inside ops_lock
    in the ioctl path to close a concurrent-ioctl ordering race; add
    de-assert call to wwan_remove_port() for the hot-unplug case; update
    kernel-doc to note the op is AT-only and describe the mdmbits argument
v4: Protect at_data.mdmbits in wwan_port_op_start/stop under data_lock;
    release data_lock and acquire ops_lock with a NULL check before calling
    ->dtr_rts from the ioctl path; gate ioctl ->dtr_rts on WWAN_PORT_AT to
    match open/close behaviour; reduce boolean to TIOCM_DTR only
v3: Replace ->tiocmget/->tiocmset with ->dtr_rts(port, bool on) modelled
    on tty_port_operations.dtr_rts; raise/drop DTR/RTS in
    wwan_port_op_start/stop rather than in the driver (Loic Poulain)
---
 drivers/net/wwan/wwan_core.c | 172 +++++++++++++++++++++++------------
 include/linux/wwan.h         |   9 ++
 2 files changed, 124 insertions(+), 57 deletions(-)

diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
index ffbcf11e4e68..7097b58dafde 100644
--- a/drivers/net/wwan/wwan_core.c
+++ b/drivers/net/wwan/wwan_core.c
@@ -25,6 +25,10 @@
 #include <net/rtnetlink.h>
 #include <uapi/linux/wwan.h>
 
+/* Modem control bits userspace may set on an AT port, as for tty_tiocmset() */
+#define WWAN_TIOCM_OUTPUTS	(TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 | \
+				 TIOCM_OUT2 | TIOCM_LOOP)
+
 /* Maximum number of minors in use */
 #define WWAN_MAX_MINORS		(1 << MINORBITS)
 
@@ -73,11 +77,10 @@ struct wwan_device {
  * @start_count: Port start counter
  * @flags: Store port state and capabilities
  * @ops: Pointer to WWAN port operations
- * @ops_lock: Protect port ops
+ * @ops_lock: Protect port ops, and the AT/QCDM termios and TIOCM state
  * @dev: Underlying device
  * @rxq: Buffer inbound queue
  * @waitqueue: The waitqueue for port fops (read/write/poll)
- * @data_lock: Port specific data access serialization
  * @headroom_len: SKB reserved headroom size
  * @frag_len: Length to fragment packet
  * @at_data: AT port specific data
@@ -88,11 +91,10 @@ struct wwan_port {
 	unsigned int start_count;
 	unsigned long flags;
 	const struct wwan_port_ops *ops;
-	struct mutex ops_lock; /* Serialize ops + protect against removal */
+	struct mutex ops_lock; /* Serialize ops + at_data, protect against removal */
 	struct device dev;
 	struct sk_buff_head rxq;
 	wait_queue_head_t waitqueue;
-	struct mutex data_lock;	/* Port specific data access serialization */
 	size_t headroom_len;
 	size_t frag_len;
 	union {
@@ -374,7 +376,6 @@ static void wwan_port_destroy(struct device *dev)
 
 	if (dev->class == &wwan_class)
 		ida_free(&minors, MINOR(dev->devt));
-	mutex_destroy(&port->data_lock);
 	mutex_destroy(&port->ops_lock);
 	kfree(port);
 }
@@ -653,7 +654,10 @@ struct wwan_port *wwan_create_port(struct device *parent,
 	mutex_init(&port->ops_lock);
 	skb_queue_head_init(&port->rxq);
 	init_waitqueue_head(&port->waitqueue);
-	mutex_init(&port->data_lock);
+
+	/* AT ports default to HUPCL, so DTR/RTS are dropped on last close */
+	if (type == WWAN_PORT_AT)
+		port->at_data.termios.c_cflag = HUPCL;
 
 	port->dev.parent = &wwandev->dev;
 	port->dev.type = &wwan_port_dev_type;
@@ -679,12 +683,44 @@ struct wwan_port *wwan_create_port(struct device *parent,
 }
 EXPORT_SYMBOL_GPL(wwan_create_port);
 
+/* Raise DTR/RTS when an AT port is first opened, as a TTY does on open.
+ * Unlike a TTY the termios baud rate is not used. Called with ops_lock held.
+ */
+static void wwan_port_raise_dtr_rts(struct wwan_port *port)
+{
+	lockdep_assert_held(&port->ops_lock);
+
+	if (port->type != WWAN_PORT_AT || !port->ops->dtr_rts)
+		return;
+
+	port->at_data.mdmbits |= TIOCM_DTR | TIOCM_RTS;
+	port->ops->dtr_rts(port, port->at_data.mdmbits);
+}
+
+/* Drop DTR/RTS on an AT port. Called with ops_lock held. On last close
+ * the lines are only dropped if HUPCL is set, as for a TTY. When a port is
+ * removed while still open they are always dropped.
+ */
+static void wwan_port_drop_dtr_rts(struct wwan_port *port, bool hupcl_only)
+{
+	lockdep_assert_held(&port->ops_lock);
+
+	if (port->type != WWAN_PORT_AT || !port->ops->dtr_rts)
+		return;
+	if (hupcl_only && !(port->at_data.termios.c_cflag & HUPCL))
+		return;
+
+	port->at_data.mdmbits &= ~(TIOCM_DTR | TIOCM_RTS);
+	port->ops->dtr_rts(port, port->at_data.mdmbits);
+}
+
 void wwan_remove_port(struct wwan_port *port)
 {
 	struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
 
 	mutex_lock(&port->ops_lock);
 	if (port->start_count) {
+		wwan_port_drop_dtr_rts(port, false);
 		port->ops->stop(port);
 		port->start_count = 0;
 	}
@@ -756,8 +792,11 @@ static int wwan_port_op_start(struct wwan_port *port)
 	}
 
 	/* If port is already started, don't start again */
-	if (!port->start_count)
+	if (!port->start_count) {
 		ret = port->ops->start(port);
+		if (!ret)
+			wwan_port_raise_dtr_rts(port);
+	}
 
 	if (!ret)
 		port->start_count++;
@@ -773,8 +812,10 @@ static void wwan_port_op_stop(struct wwan_port *port)
 	mutex_lock(&port->ops_lock);
 	port->start_count--;
 	if (!port->start_count) {
-		if (port->ops)
+		if (port->ops) {
+			wwan_port_drop_dtr_rts(port, true);
 			port->ops->stop(port);
+		}
 		skb_queue_purge(&port->rxq);
 		clear_bit(WWAN_PORT_EXCLUSIVE, &port->flags);
 	}
@@ -976,92 +1017,109 @@ static __poll_t wwan_port_fops_poll(struct file *filp, poll_table *wait)
 }
 
 /* Implements minimalistic stub terminal IOCTLs support */
-static long wwan_port_fops_at_ioctl(struct wwan_port *port, unsigned int cmd,
-				    unsigned long arg)
+/* TCGETS/TCSETS and their termios2 variants. @size is the size of the user
+ * structure, a prefix of struct ktermios. As for the TIOCM ioctls, user
+ * memory is only accessed outside ops_lock.
+ */
+static long wwan_port_termios_ioctl(struct wwan_port *port, bool set,
+				    void __user *uarg, size_t size)
 {
-	int ret = 0;
+	struct ktermios termios;
 
-	mutex_lock(&port->data_lock);
+	if (!set) {
+		scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &port->ops_lock)
+			termios = port->at_data.termios;
+		return copy_to_user(uarg, &termios, size) ? -EFAULT : 0;
+	}
+
+	/* Commit only on success, a partial copy must not clear HUPCL */
+	if (copy_from_user(&termios, uarg, size))
+		return -EFAULT;
+	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &port->ops_lock)
+		memcpy(&port->at_data.termios, &termios, size);
+	return 0;
+}
 
+static long wwan_port_fops_at_ioctl(struct wwan_port *port, unsigned int cmd,
+				    unsigned long arg)
+{
+	/* ops_lock serialises the termios/TIOCM state with port ops and
+	 * removal. It is only held while that state is read or updated, never
+	 * across a user copy, and is taken interruptibly as a blocking write
+	 * holds it too.
+	 */
 	switch (cmd) {
 	case TCFLSH:
-		break;
+		return 0;
 
 	case TCGETS:
-		if (copy_to_user((void __user *)arg, &port->at_data.termios,
-				 sizeof(struct termios)))
-			ret = -EFAULT;
-		break;
-
 	case TCSETS:
 	case TCSETSW:
 	case TCSETSF:
-		if (copy_from_user(&port->at_data.termios, (void __user *)arg,
-				   sizeof(struct termios)))
-			ret = -EFAULT;
-		break;
+		return wwan_port_termios_ioctl(port, cmd != TCGETS,
+					       (void __user *)arg,
+					       sizeof(struct termios));
 
 #ifdef TCGETS2
 	case TCGETS2:
-		if (copy_to_user((void __user *)arg, &port->at_data.termios,
-				 sizeof(struct termios2)))
-			ret = -EFAULT;
-		break;
-
 	case TCSETS2:
 	case TCSETSW2:
 	case TCSETSF2:
-		if (copy_from_user(&port->at_data.termios, (void __user *)arg,
-				   sizeof(struct termios2)))
-			ret = -EFAULT;
-		break;
+		return wwan_port_termios_ioctl(port, cmd != TCGETS2,
+					       (void __user *)arg,
+					       sizeof(struct termios2));
 #endif
 
-	case TIOCMGET:
-		ret = put_user(port->at_data.mdmbits, (int __user *)arg);
-		break;
+	case TIOCMGET: {
+		int mdmbits;
+
+		scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &port->ops_lock)
+			mdmbits = port->at_data.mdmbits;
+		return put_user(mdmbits, (int __user *)arg);
+	}
 
 	case TIOCMSET:
 	case TIOCMBIC:
 	case TIOCMBIS: {
 		int mdmbits;
 
-		if (copy_from_user(&mdmbits, (int __user *)arg, sizeof(int))) {
-			ret = -EFAULT;
-			break;
+		if (get_user(mdmbits, (int __user *)arg))
+			return -EFAULT;
+		mdmbits &= WWAN_TIOCM_OUTPUTS;
+
+		scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &port->ops_lock) {
+			int old = port->at_data.mdmbits;
+
+			if (cmd == TIOCMBIC)
+				port->at_data.mdmbits &= ~mdmbits;
+			else if (cmd == TIOCMBIS)
+				port->at_data.mdmbits |= mdmbits;
+			else
+				port->at_data.mdmbits = mdmbits;
+			/* Only signal the driver when DTR or RTS actually changes */
+			if (port->type == WWAN_PORT_AT && port->ops &&
+			    port->ops->dtr_rts &&
+			    (old ^ port->at_data.mdmbits) & (TIOCM_DTR | TIOCM_RTS))
+				port->ops->dtr_rts(port, port->at_data.mdmbits);
 		}
-		if (cmd == TIOCMBIC)
-			port->at_data.mdmbits &= ~mdmbits;
-		else if (cmd == TIOCMBIS)
-			port->at_data.mdmbits |= mdmbits;
-		else
-			port->at_data.mdmbits = mdmbits;
-		break;
+		return 0;
 	}
 
 	case TIOCEXCL:
 		set_bit(WWAN_PORT_EXCLUSIVE, &port->flags);
-		break;
+		return 0;
 
 	case TIOCNXCL:
 		clear_bit(WWAN_PORT_EXCLUSIVE, &port->flags);
-		break;
+		return 0;
 
 	case TIOCGEXCL:
-	{
-		int excl = test_bit(WWAN_PORT_EXCLUSIVE, &port->flags);
-
-		ret = put_user(excl, (int __user *)arg);
-		break;
-	}
+		return put_user(test_bit(WWAN_PORT_EXCLUSIVE, &port->flags),
+				(int __user *)arg);
 
 	default:
-		ret = -ENOIOCTLCMD;
+		return -ENOIOCTLCMD;
 	}
-
-	mutex_unlock(&port->data_lock);
-
-	return ret;
 }
 
 static long wwan_port_fops_ioctl(struct file *filp, unsigned int cmd,
diff --git a/include/linux/wwan.h b/include/linux/wwan.h
index 1e0e2cb53579..23a8a8ee473d 100644
--- a/include/linux/wwan.h
+++ b/include/linux/wwan.h
@@ -57,6 +57,14 @@ struct wwan_port;
  * @tx_blocking: Optional blocking routine that sends WWAN port protocol data
  *               to the device.
  * @tx_poll: Optional routine that sets additional TX poll flags.
+ * @dtr_rts: Optional routine that updates the modem control lines to match
+ *           @mdmbits, the TIOCM_* output bits (DTR, RTS, OUT1, OUT2, LOOP)
+ *           currently set for the port. Called when the port is first opened
+ *           and ->start() has succeeded, on last close if HUPCL is set, when
+ *           the port is removed while still open, and when TIOCMSET, TIOCMBIS
+ *           or TIOCMBIC changes DTR or RTS. Only called for WWAN_PORT_AT ports,
+ *           as only the AT (DUN) port carries the modem's virtual RS-232
+ *           control signals.
  *
  * The wwan_port_ops structure contains a list of low-level operations
  * that control a WWAN port device. All functions are mandatory unless specified.
@@ -70,6 +78,7 @@ struct wwan_port_ops {
 	int (*tx_blocking)(struct wwan_port *port, struct sk_buff *skb);
 	__poll_t (*tx_poll)(struct wwan_port *port, struct file *filp,
 			    poll_table *wait);
+	void (*dtr_rts)(struct wwan_port *port, unsigned int mdmbits);
 };
 
 /** struct wwan_port_caps - The WWAN port capbilities
-- 
2.43.0

Re: [PATCH net-next v10 1/2] net: wwan: core: propagate modem control signals to port drivers

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch moves the AT/QCDM termios and TIOCM state from the
  private data_lock to ops_lock, but wwan_port_op_tx() holds ops_lock…

--

net: wwan: core: propagate modem control signals to port drivers

This adds an optional ->dtr_rts() operation to struct wwan_port_ops. The
wwan core calls it when an AT port is first opened, on last close if
HUPCL is set, on port removal and when TIOCMSET/TIOCMBIS/TIOCMBIC changes
DTR or RTS. The termios and TIOCM state is moved from data_lock to
ops_lock, and data_lock is removed.

> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> index ffbcf11e4e681..7097b58dafdea 100644
> --- a/drivers/net/wwan/wwan_core.c
> +++ b/drivers/net/wwan/wwan_core.c

[ ... ]

> @@ -976,92 +1017,109 @@ static __poll_t wwan_port_fops_poll(struct file *filp, poll_table *wait)
>  }
>  
>  /* Implements minimalistic stub terminal IOCTLs support */
> -static long wwan_port_fops_at_ioctl(struct wwan_port *port, unsigned int cmd,
> -				    unsigned long arg)
> +/* TCGETS/TCSETS and their termios2 variants. @size is the size of the user
> + * structure, a prefix of struct ktermios. As for the TIOCM ioctls, user
> + * memory is only accessed outside ops_lock.
> + */
> +static long wwan_port_termios_ioctl(struct wwan_port *port, bool set,
> +				    void __user *uarg, size_t size)
>  {
> -	int ret = 0;
> +	struct ktermios termios;
>  
> -	mutex_lock(&port->data_lock);
> +	if (!set) {
> +		scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &port->ops_lock)
> +			termios = port->at_data.termios;

[Severity: Medium]
Can TCGETS and the other termios and TIOCM ioctls now block indefinitely
behind a blocking write() on the same AT port?

wwan_port_op_tx() holds ops_lock for the whole of ->tx_blocking():

drivers/net/wwan/wwan_core.c:wwan_port_op_tx() {
	mutex_lock(&port->ops_lock);
	...
	if (nonblock || !port->ops->tx_blocking)
		ret = port->ops->tx(port, skb);
	else
		ret = port->ops->tx_blocking(port, skb);
	...
}

Before this patch, these ioctls only took data_lock, and no TX path takes
data_lock. After this patch, TCGETS/TCSETS*/TCGETS2/TCSETS*2 in
wwan_port_termios_ioctl() and TIOCMGET/TIOCMSET/TIOCMBIS/TIOCMBIC in
wwan_port_fops_at_ioctl() all take ops_lock. They would sleep until an
in-flight blocking write finishes, or return -ERESTARTSYS if a signal
arrives first.

This seems reachable with in-tree drivers. rpmsg_wwan_ctrl registers
DATA4 and DATA1 as WWAN_PORT_AT, and its .tx_blocking calls rpmsg_send().
On SMD that ends up in __qcom_smd_send(), which has an untimed wait:

wwan_port_op_tx()
  mutex_lock(&port->ops_lock)
  rpmsg_wwan_ctrl_tx_blocking()
    rpmsg_send()
      __qcom_smd_send()
        wait_event_interruptible(channel->fblockread_event,
                qcom_smd_get_tx_avail(channel) >= tlen ||
                channel->state != SMD_CHANNEL_OPENED);

If the remote side never drains the FIFO, a tcgetattr(), tcsetattr() or
TIOCMGET from another thread or process could stay stuck until it gets a
signal. On a real TTY, TCGETS and tty_tiocmget()/tty_tiocmset() do not
take the tty write lock.

The commit message says the ioctls wait behind a blocking write and that
the lock is taken interruptibly. That makes the wait interruptible, but
the wait itself is still unbounded.

A driver that implements both .tx_blocking and .dtr_rts could also end
up in a circular wait, where the write only completes once the host
raises DTR, but TIOCMBIS(DTR) is stuck on ops_lock. No in-tree driver
does this right now: rpmsg_wwan_ctrl has no .dtr_rts and mhi_wwan_ctrl
has no .tx_blocking.

Could the termios/TIOCM state be kept under a lock the TX path does not
hold? If ->dtr_rts() needs to be serialised against removal, only that
call could take ops_lock.

> +		return copy_to_user(uarg, &termios, size) ? -EFAULT : 0;
> +	}
> +
> +	/* Commit only on success, a partial copy must not clear HUPCL */
> +	if (copy_from_user(&termios, uarg, size))
> +		return -EFAULT;
> +	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &port->ops_lock)
> +		memcpy(&port->at_data.termios, &termios, size);
> +	return 0;
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013183500.261229-1-peter.hunt%40opengear.com

Re: [PATCH net-next v10 1/2] net: wwan: core: propagate modem control signals to port drivers

[Peter Hunt]

Thanks for the review.

[Medium] termios/TIOCM ioctls waiting behind a blocking write: this is
a trade-off discussed on v8, where Loic chose a single ops_lock for now,
with finer-grained locking possibly later. In v10 the lock is only held
around the termios/TIOCM state, never across a user copy, the wait is
interruptible, and ioctls that don't touch that state don't take it.

No in-tree driver implements both ->tx_blocking and ->dtr_rts, so the
circular wait you describe can't happen today. If the maintainers would
rather keep the termios/TIOCM state under a lock the TX path doesn't
hold, I'm happy to do that as a follow-up.

Re: [PATCH net-next v10 1/2] net: wwan: core: propagate modem control signals to port drivers

[Loic Poulain]

On Tue, Oct 13, 2026 at 8:37 PM Peter Hunt <peter.hunt@opengear.com> wrote:
>
> The WWAN character device emulates the TTY modem-control ioctls
> (TIOCMGET/TIOCMSET/TIOCMBIC/TIOCMBIS) for AT and QCDM ports, but the
> result is only stored in port->at_data.mdmbits and never reaches the port
> driver. A driver therefore cannot act on the host raising or dropping
> DTR/RTS, even though some modems depend on it (e.g. they withhold
> unsolicited AT result codes until the host asserts DTR).
>
> Add an optional ->dtr_rts(port, mdmbits) operation to struct wwan_port_ops.
> Drivers that implement it receive the TIOCM output bitmask so they can
> assert or de-assert DTR and RTS independently. As in tty_tiocmset(), the
> wwan core keeps only the output bits (DTR, RTS, OUT1, OUT2, LOOP) of the
> TIOCMSET/TIOCMBIS/TIOCMBIC value in port->at_data.mdmbits, and calls
> ->dtr_rts when DTR or RTS actually changes, gated on WWAN_PORT_AT to match
> the open/close raise/drop behaviour.
>
> Also raise DTR/RTS in wwan_port_op_start on first open of an AT port
> when the driver implements ->dtr_rts, and drop them in wwan_port_op_stop
> on last close if HUPCL is set in the port's termios. This follows TTY
> open/close semantics, although the termios baud rate is not used, and
> means individual drivers do not need to implement this themselves. As on
> a TTY, HUPCL defaults to on for AT ports, and userspace can clear it
> with TCSETS to keep DTR asserted across close, for example so that
> closing the port does not end a call on a modem set to AT&D1/AT&D2.
> TCSETS and TCSETS2 now only commit the new termios when the copy from
> userspace succeeds, so a faulting call cannot silently clear HUPCL.
>
> ->dtr_rts must be called under ops_lock, which serialises port ops and
> protects against port removal (removal clears port->ops under it). So
> that updates to the TIOCM state and the ->dtr_rts calls are serialised
> together, the termios/TIOCM state now also lives under ops_lock and the
> separate data_lock is removed. The AT/QCDM ioctl handler only holds
> ops_lock while it reads or updates that state, never across a
> user-memory copy, and takes it interruptibly since a driver's blocking
> write holds it while it waits. Ioctls that do not touch that state, such
> as TCFLSH, TIOCEXCL and the FIONREAD fallback, do not take it.
>
> wwan_remove_port() also drops DTR/RTS before ->stop() when a port is
> removed while still open, regardless of HUPCL since the device is going
> away. The raise and drop paths go through two small helpers, which do
> the AT port and ->dtr_rts checks and pass the driver the resulting
> bitmask.
>
> Signed-off-by: Peter Hunt <peter.hunt@opengear.com>

Reviewed-by: Loic Poulain <loic.poulain@oss.qualcomm.com>

> ---
> v10: Only hold ops_lock in the AT ioctl handler around the termios/TIOCM
>     state, with user copies done outside it, return -ERESTARTSYS when
>     interrupted, and leave ops_lock alone for ioctls that do not touch
>     that state. Word the TTY comparisons and the ->dtr_rts kernel-doc
>     more precisely (the baud rate is not used, removal only drops the
>     lines while the port is open, open only once ->start() has
>     succeeded). Both from the Sashiko review of v9, still using ops_lock
>     only as agreed with Loic Poulain
> v9: Raise DTR/RTS inside the existing first-open branch of
>     wwan_port_op_start(), and drop data_lock so the termios/TIOCM state
>     and ->dtr_rts calls are all serialised by ops_lock, taken
>     interruptibly in the ioctl handler (Loic Poulain). Mask
>     TIOCMSET/TIOCMBIS/TIOCMBIC to the output bits and only call
>     ->dtr_rts when DTR or RTS changes, commit TCSETS/TCSETS2 only on a
>     successful copy, and describe the calls in the ->dtr_rts kernel-doc
>     (Sashiko review of v7). Document in the ->dtr_rts kernel-doc why it
>     is AT-only (Loic Poulain, Daniele Palmas)
> v8: Add wwan_port_raise_dtr_rts() as the counterpart of
>     wwan_port_drop_dtr_rts() and move the open-path handling there, move
>     the AT port and ->dtr_rts checks into both helpers, use
>     scoped_guard() for data_lock (Loic Poulain)
> v7: Drop DTR/RTS on last close only if HUPCL is set, and default HUPCL on
>     for AT ports. Pass the masked mdmbits rather than 0 from
>     wwan_remove_port(), via a helper shared with the close path. Both
>     from the Sashiko review of v6
> v6: Rebase onto net-next. Snapshot mdmbits under data_lock before calling
>     ->dtr_rts from wwan_port_op_start()/wwan_port_op_stop(), rather than
>     reading it after data_lock has been released
> v5: Change ->dtr_rts signature from bool to unsigned int mdmbits so DTR
>     and RTS can be driven independently; re-read mdmbits inside ops_lock
>     in the ioctl path to close a concurrent-ioctl ordering race; add
>     de-assert call to wwan_remove_port() for the hot-unplug case; update
>     kernel-doc to note the op is AT-only and describe the mdmbits argument
> v4: Protect at_data.mdmbits in wwan_port_op_start/stop under data_lock;
>     release data_lock and acquire ops_lock with a NULL check before calling
>     ->dtr_rts from the ioctl path; gate ioctl ->dtr_rts on WWAN_PORT_AT to
>     match open/close behaviour; reduce boolean to TIOCM_DTR only
> v3: Replace ->tiocmget/->tiocmset with ->dtr_rts(port, bool on) modelled
>     on tty_port_operations.dtr_rts; raise/drop DTR/RTS in
>     wwan_port_op_start/stop rather than in the driver (Loic Poulain)
> ---
>  drivers/net/wwan/wwan_core.c | 172 +++++++++++++++++++++++------------
>  include/linux/wwan.h         |   9 ++
>  2 files changed, 124 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> index ffbcf11e4e68..7097b58dafde 100644
> --- a/drivers/net/wwan/wwan_core.c
> +++ b/drivers/net/wwan/wwan_core.c
> @@ -25,6 +25,10 @@
>  #include <net/rtnetlink.h>
>  #include <uapi/linux/wwan.h>
>
> +/* Modem control bits userspace may set on an AT port, as for tty_tiocmset() */
> +#define WWAN_TIOCM_OUTPUTS     (TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 | \
> +                                TIOCM_OUT2 | TIOCM_LOOP)
> +
>  /* Maximum number of minors in use */
>  #define WWAN_MAX_MINORS                (1 << MINORBITS)
>
> @@ -73,11 +77,10 @@ struct wwan_device {
>   * @start_count: Port start counter
>   * @flags: Store port state and capabilities
>   * @ops: Pointer to WWAN port operations
> - * @ops_lock: Protect port ops
> + * @ops_lock: Protect port ops, and the AT/QCDM termios and TIOCM state
>   * @dev: Underlying device
>   * @rxq: Buffer inbound queue
>   * @waitqueue: The waitqueue for port fops (read/write/poll)
> - * @data_lock: Port specific data access serialization
>   * @headroom_len: SKB reserved headroom size
>   * @frag_len: Length to fragment packet
>   * @at_data: AT port specific data
> @@ -88,11 +91,10 @@ struct wwan_port {
>         unsigned int start_count;
>         unsigned long flags;
>         const struct wwan_port_ops *ops;
> -       struct mutex ops_lock; /* Serialize ops + protect against removal */
> +       struct mutex ops_lock; /* Serialize ops + at_data, protect against removal */
>         struct device dev;
>         struct sk_buff_head rxq;
>         wait_queue_head_t waitqueue;
> -       struct mutex data_lock; /* Port specific data access serialization */
>         size_t headroom_len;
>         size_t frag_len;
>         union {
> @@ -374,7 +376,6 @@ static void wwan_port_destroy(struct device *dev)
>
>         if (dev->class == &wwan_class)
>                 ida_free(&minors, MINOR(dev->devt));
> -       mutex_destroy(&port->data_lock);
>         mutex_destroy(&port->ops_lock);
>         kfree(port);
>  }
> @@ -653,7 +654,10 @@ struct wwan_port *wwan_create_port(struct device *parent,
>         mutex_init(&port->ops_lock);
>         skb_queue_head_init(&port->rxq);
>         init_waitqueue_head(&port->waitqueue);
> -       mutex_init(&port->data_lock);
> +
> +       /* AT ports default to HUPCL, so DTR/RTS are dropped on last close */
> +       if (type == WWAN_PORT_AT)
> +               port->at_data.termios.c_cflag = HUPCL;
>
>         port->dev.parent = &wwandev->dev;
>         port->dev.type = &wwan_port_dev_type;
> @@ -679,12 +683,44 @@ struct wwan_port *wwan_create_port(struct device *parent,
>  }
>  EXPORT_SYMBOL_GPL(wwan_create_port);
>
> +/* Raise DTR/RTS when an AT port is first opened, as a TTY does on open.
> + * Unlike a TTY the termios baud rate is not used. Called with ops_lock held.
> + */
> +static void wwan_port_raise_dtr_rts(struct wwan_port *port)
> +{
> +       lockdep_assert_held(&port->ops_lock);
> +
> +       if (port->type != WWAN_PORT_AT || !port->ops->dtr_rts)
> +               return;
> +
> +       port->at_data.mdmbits |= TIOCM_DTR | TIOCM_RTS;
> +       port->ops->dtr_rts(port, port->at_data.mdmbits);
> +}
> +
> +/* Drop DTR/RTS on an AT port. Called with ops_lock held. On last close
> + * the lines are only dropped if HUPCL is set, as for a TTY. When a port is
> + * removed while still open they are always dropped.
> + */
> +static void wwan_port_drop_dtr_rts(struct wwan_port *port, bool hupcl_only)
> +{
> +       lockdep_assert_held(&port->ops_lock);
> +
> +       if (port->type != WWAN_PORT_AT || !port->ops->dtr_rts)
> +               return;
> +       if (hupcl_only && !(port->at_data.termios.c_cflag & HUPCL))
> +               return;
> +
> +       port->at_data.mdmbits &= ~(TIOCM_DTR | TIOCM_RTS);
> +       port->ops->dtr_rts(port, port->at_data.mdmbits);
> +}
> +
>  void wwan_remove_port(struct wwan_port *port)
>  {
>         struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
>
>         mutex_lock(&port->ops_lock);
>         if (port->start_count) {
> +               wwan_port_drop_dtr_rts(port, false);
>                 port->ops->stop(port);
>                 port->start_count = 0;
>         }
> @@ -756,8 +792,11 @@ static int wwan_port_op_start(struct wwan_port *port)
>         }
>
>         /* If port is already started, don't start again */
> -       if (!port->start_count)
> +       if (!port->start_count) {
>                 ret = port->ops->start(port);
> +               if (!ret)
> +                       wwan_port_raise_dtr_rts(port);
> +       }
>
>         if (!ret)
>                 port->start_count++;
> @@ -773,8 +812,10 @@ static void wwan_port_op_stop(struct wwan_port *port)
>         mutex_lock(&port->ops_lock);
>         port->start_count--;
>         if (!port->start_count) {
> -               if (port->ops)
> +               if (port->ops) {
> +                       wwan_port_drop_dtr_rts(port, true);
>                         port->ops->stop(port);
> +               }
>                 skb_queue_purge(&port->rxq);
>                 clear_bit(WWAN_PORT_EXCLUSIVE, &port->flags);
>         }
> @@ -976,92 +1017,109 @@ static __poll_t wwan_port_fops_poll(struct file *filp, poll_table *wait)
>  }
>
>  /* Implements minimalistic stub terminal IOCTLs support */
> -static long wwan_port_fops_at_ioctl(struct wwan_port *port, unsigned int cmd,
> -                                   unsigned long arg)
> +/* TCGETS/TCSETS and their termios2 variants. @size is the size of the user
> + * structure, a prefix of struct ktermios. As for the TIOCM ioctls, user
> + * memory is only accessed outside ops_lock.
> + */
> +static long wwan_port_termios_ioctl(struct wwan_port *port, bool set,
> +                                   void __user *uarg, size_t size)
>  {
> -       int ret = 0;
> +       struct ktermios termios;
>
> -       mutex_lock(&port->data_lock);
> +       if (!set) {
> +               scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &port->ops_lock)
> +                       termios = port->at_data.termios;
> +               return copy_to_user(uarg, &termios, size) ? -EFAULT : 0;
> +       }
> +
> +       /* Commit only on success, a partial copy must not clear HUPCL */
> +       if (copy_from_user(&termios, uarg, size))
> +               return -EFAULT;
> +       scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &port->ops_lock)
> +               memcpy(&port->at_data.termios, &termios, size);
> +       return 0;
> +}
>
> +static long wwan_port_fops_at_ioctl(struct wwan_port *port, unsigned int cmd,
> +                                   unsigned long arg)
> +{
> +       /* ops_lock serialises the termios/TIOCM state with port ops and
> +        * removal. It is only held while that state is read or updated, never
> +        * across a user copy, and is taken interruptibly as a blocking write
> +        * holds it too.
> +        */
>         switch (cmd) {
>         case TCFLSH:
> -               break;
> +               return 0;
>
>         case TCGETS:
> -               if (copy_to_user((void __user *)arg, &port->at_data.termios,
> -                                sizeof(struct termios)))
> -                       ret = -EFAULT;
> -               break;
> -
>         case TCSETS:
>         case TCSETSW:
>         case TCSETSF:
> -               if (copy_from_user(&port->at_data.termios, (void __user *)arg,
> -                                  sizeof(struct termios)))
> -                       ret = -EFAULT;
> -               break;
> +               return wwan_port_termios_ioctl(port, cmd != TCGETS,
> +                                              (void __user *)arg,
> +                                              sizeof(struct termios));
>
>  #ifdef TCGETS2
>         case TCGETS2:
> -               if (copy_to_user((void __user *)arg, &port->at_data.termios,
> -                                sizeof(struct termios2)))
> -                       ret = -EFAULT;
> -               break;
> -
>         case TCSETS2:
>         case TCSETSW2:
>         case TCSETSF2:
> -               if (copy_from_user(&port->at_data.termios, (void __user *)arg,
> -                                  sizeof(struct termios2)))
> -                       ret = -EFAULT;
> -               break;
> +               return wwan_port_termios_ioctl(port, cmd != TCGETS2,
> +                                              (void __user *)arg,
> +                                              sizeof(struct termios2));
>  #endif
>
> -       case TIOCMGET:
> -               ret = put_user(port->at_data.mdmbits, (int __user *)arg);
> -               break;
> +       case TIOCMGET: {
> +               int mdmbits;
> +
> +               scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &port->ops_lock)
> +                       mdmbits = port->at_data.mdmbits;
> +               return put_user(mdmbits, (int __user *)arg);
> +       }
>
>         case TIOCMSET:
>         case TIOCMBIC:
>         case TIOCMBIS: {
>                 int mdmbits;
>
> -               if (copy_from_user(&mdmbits, (int __user *)arg, sizeof(int))) {
> -                       ret = -EFAULT;
> -                       break;
> +               if (get_user(mdmbits, (int __user *)arg))
> +                       return -EFAULT;
> +               mdmbits &= WWAN_TIOCM_OUTPUTS;
> +
> +               scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &port->ops_lock) {
> +                       int old = port->at_data.mdmbits;
> +
> +                       if (cmd == TIOCMBIC)
> +                               port->at_data.mdmbits &= ~mdmbits;
> +                       else if (cmd == TIOCMBIS)
> +                               port->at_data.mdmbits |= mdmbits;
> +                       else
> +                               port->at_data.mdmbits = mdmbits;
> +                       /* Only signal the driver when DTR or RTS actually changes */
> +                       if (port->type == WWAN_PORT_AT && port->ops &&
> +                           port->ops->dtr_rts &&
> +                           (old ^ port->at_data.mdmbits) & (TIOCM_DTR | TIOCM_RTS))
> +                               port->ops->dtr_rts(port, port->at_data.mdmbits);
>                 }
> -               if (cmd == TIOCMBIC)
> -                       port->at_data.mdmbits &= ~mdmbits;
> -               else if (cmd == TIOCMBIS)
> -                       port->at_data.mdmbits |= mdmbits;
> -               else
> -                       port->at_data.mdmbits = mdmbits;
> -               break;
> +               return 0;
>         }
>
>         case TIOCEXCL:
>                 set_bit(WWAN_PORT_EXCLUSIVE, &port->flags);
> -               break;
> +               return 0;
>
>         case TIOCNXCL:
>                 clear_bit(WWAN_PORT_EXCLUSIVE, &port->flags);
> -               break;
> +               return 0;
>
>         case TIOCGEXCL:
> -       {
> -               int excl = test_bit(WWAN_PORT_EXCLUSIVE, &port->flags);
> -
> -               ret = put_user(excl, (int __user *)arg);
> -               break;
> -       }
> +               return put_user(test_bit(WWAN_PORT_EXCLUSIVE, &port->flags),
> +                               (int __user *)arg);
>
>         default:
> -               ret = -ENOIOCTLCMD;
> +               return -ENOIOCTLCMD;
>         }
> -
> -       mutex_unlock(&port->data_lock);
> -
> -       return ret;
>  }
>
>  static long wwan_port_fops_ioctl(struct file *filp, unsigned int cmd,
> diff --git a/include/linux/wwan.h b/include/linux/wwan.h
> index 1e0e2cb53579..23a8a8ee473d 100644
> --- a/include/linux/wwan.h
> +++ b/include/linux/wwan.h
> @@ -57,6 +57,14 @@ struct wwan_port;
>   * @tx_blocking: Optional blocking routine that sends WWAN port protocol data
>   *               to the device.
>   * @tx_poll: Optional routine that sets additional TX poll flags.
> + * @dtr_rts: Optional routine that updates the modem control lines to match
> + *           @mdmbits, the TIOCM_* output bits (DTR, RTS, OUT1, OUT2, LOOP)
> + *           currently set for the port. Called when the port is first opened
> + *           and ->start() has succeeded, on last close if HUPCL is set, when
> + *           the port is removed while still open, and when TIOCMSET, TIOCMBIS
> + *           or TIOCMBIC changes DTR or RTS. Only called for WWAN_PORT_AT ports,
> + *           as only the AT (DUN) port carries the modem's virtual RS-232
> + *           control signals.
>   *
>   * The wwan_port_ops structure contains a list of low-level operations
>   * that control a WWAN port device. All functions are mandatory unless specified.
> @@ -70,6 +78,7 @@ struct wwan_port_ops {
>         int (*tx_blocking)(struct wwan_port *port, struct sk_buff *skb);
>         __poll_t (*tx_poll)(struct wwan_port *port, struct file *filp,
>                             poll_table *wait);
> +       void (*dtr_rts)(struct wwan_port *port, unsigned int mdmbits);
>  };
>
>  /** struct wwan_port_caps - The WWAN port capbilities
> --
> 2.43.0
>

[PATCH net-next v10 2/2] net: wwan: mhi_wwan_ctrl: drive DTR/RTS via the IP_CTRL channel

[Peter Hunt]

Qualcomm/Sierra SDX55/SDX65 modems withhold unsolicited AT result codes
(URCs such as +CREG, and the +DMI OMA-DM/LwM2M session indications) on
an AT port until the host asserts DTR. mhi_wwan_ctrl exposed the AT
(DUN) ports but had no way to signal DTR, so URCs never reached
userspace.

Carry the host serial-control lines to the modem over the dedicated
IP_CTRL MHI channel, which this module now also binds. IP_CTRL uses a
separate mhi_driver with its own callbacks so the AT/QMI/MBIM data path
is untouched; the control-channel device for each MHI controller is
tracked in a small registry so an AT port drives the IP_CTRL channel of
its own modem (multiple modems are supported).

The IP_CTRL channel has to be declared in the controller's channel config
for this driver to bind. For the Sierra EM919x/EM929x this is done by
commit 83c29a55b89e ("bus: mhi: host: pci_generic: Add IP_CTRL channel
for Sierra EM919x/EM929x") in the MHI tree. On controllers that do not
declare IP_CTRL the driver does not bind and ->dtr_rts is a no-op.

The wwan core, with "net: wwan: core: propagate modem control signals to
port drivers", raises DTR/RTS on first open and drops them on last close
(if HUPCL is set) for any AT port whose driver implements ->dtr_rts, so
no open/close handling is needed here. The new ->dtr_rts op lets
userspace assert or de-assert them via TIOCMSET/TIOCMBIC/TIOCMBIS.
Received device->host serial state messages are silently discarded; a
single recycled sink buffer keeps the IP_CTRL DL ring live so the
modem's transmit path does not stall.

->dtr_rts is void, following the tty_port_operations model it is
modelled on. It records the requested state and schedules a per-port
work item that sends the latest state to the modem, so a burst of
changes is coalesced into one message and cannot fill the IP_CTRL UL
ring. If a send fails because the ring is full or memory is short, the
work retries shortly with whatever state is latest by then, so the modem
ends up with the final state that was requested. The DL sink buffer is
requeued from a work item, which retries a transient -ENOMEM so the DL
ring does not stay empty, and which remove() disables before tearing
down the channel.

Signed-off-by: Peter Hunt <peter.hunt@opengear.com>
---
v10: Send DTR/RTS changes from a per-port work item that coalesces
    bursts and retries a send that fails with -EAGAIN (UL ring full) or
    -ENOMEM, so the modem always ends up with the final state. Testing
    v9 showed that rapid toggling could fill the IP_CTRL UL ring and
    drop updates (Sashiko review of v9). Loic's Reviewed-by is dropped
    as the send path changed
v9: Requeue the DL sink buffer only from the refill work and disable it
    with disable_delayed_work_sync() before mhi_unprepare_from_transfer()
    in remove(), so no requeue can race with channel teardown (Loic
    Poulain, Sashiko review of v7). Leave unpreparing to mhi_probe() when
    probe fails. Name the wwan core commit instead of "patch 1"
v8: No changes
v7: Retry a failed DL sink buffer requeue from a delayed work item on
    -ENOMEM, and log other failures. Note the dependency on the IP_CTRL
    channel entry in pci_generic, now in the MHI tree. Both from the
    Sashiko review of v6
v6: Rebase onto net-next. Allocate the IP_CTRL DL sink buffer separately
    instead of embedding it in struct mhi_wwan_dtr, so DMA cache
    maintenance on non-coherent platforms cannot touch neighbouring fields.
    Do not requeue the sink buffer from the DL callback when the transfer
    completes with an error (e.g. -ENOTCONN during channel teardown)
v5: Implement ->dtr_rts(port, mdmbits) and drive DTR and RTS
    independently. Pre-queue an RX sink buffer in probe and requeue it in
    the DL callback so the IP_CTRL DL ring does not run empty and stall
    the modem's transmit path
v4: Add dev_dbg when IP_CTRL channel is not enumerated for this controller
    and when mhi_queue_buf fails, to aid diagnosis of the "URCs missing"
    case on controllers that do not declare IP_CTRL
v3: Remove is_at_port, implement ->dtr_rts instead of ->tiocmset,
    open/close DTR raise/drop handled by wwan core (Loic Poulain)
---
 drivers/net/wwan/mhi_wwan_ctrl.c | 262 ++++++++++++++++++++++++++++++-
 1 file changed, 261 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
index a31d8540fbb8..e3c207588956 100644
--- a/drivers/net/wwan/mhi_wwan_ctrl.c
+++ b/drivers/net/wwan/mhi_wwan_ctrl.c
@@ -1,8 +1,13 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright (c) 2021, Linaro Ltd <loic.poulain@linaro.org> */
 #include <linux/kernel.h>
+#include <linux/list.h>
 #include <linux/mhi.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/termios.h>
+#include <linux/workqueue.h>
 #include <linux/wwan.h>
 
 /* MHI wwan flags */
@@ -14,6 +19,32 @@ enum mhi_wwan_flags {
 
 #define MHI_WWAN_MAX_MTU	0x8000
 
+/* IP_CTRL channel message that sets the modem's DTR/RTS control lines */
+struct mhi_dtr_ctrl_msg {
+	__le32 preamble;
+	__le32 msg_id;
+	__le32 dest_id;
+	__le32 size;
+	__le32 msg;
+} __packed;
+
+#define MHI_DTR_CTRL_MAGIC	0x4C525443	/* 'CTRL' */
+#define MHI_DTR_MSG_DTR		BIT(0)
+#define MHI_DTR_MSG_RTS		BIT(1)
+#define MHI_DTR_HOST_STATE	0x10
+
+/* Per-controller IP_CTRL channel, used to signal DTR/RTS to that modem */
+struct mhi_wwan_dtr {
+	struct mhi_controller *cntrl;
+	struct mhi_device *mhi_dev;
+	struct list_head node;
+	struct mhi_dtr_ctrl_msg *rx_buf; /* DL sink, separate allocation for DMA safety */
+	struct delayed_work rx_refill; /* retries a failed sink buffer requeue */
+};
+
+static LIST_HEAD(mhi_wwan_dtr_list);
+static DEFINE_MUTEX(mhi_wwan_dtr_lock);
+
 struct mhi_wwan_dev {
 	/* Lower level is a mhi dev, upper level is a wwan port */
 	struct mhi_device *mhi_dev;
@@ -35,6 +66,12 @@ struct mhi_wwan_dev {
 	 * decremented on data queueing and incremented on data release.
 	 */
 	unsigned int rx_budget;
+
+	/* Latest DTR/RTS state for the modem, sent from dtr_work so a burst of
+	 * changes is coalesced and only the final state has to get through
+	 */
+	unsigned int dtr_mdmbits;
+	struct delayed_work dtr_work;
 };
 
 /* Increment RX budget and schedule RX refill if necessary */
@@ -103,6 +140,80 @@ static void mhi_wwan_ctrl_refill_work(struct work_struct *work)
 	}
 }
 
+/* Signal the modem's DTR/RTS lines over its own controller's IP_CTRL channel */
+static int mhi_wwan_ctrl_send_dtr(struct mhi_wwan_dev *mhiwwan, unsigned int mdmbits)
+{
+	struct mhi_controller *cntrl = mhiwwan->mhi_dev->mhi_cntrl;
+	struct mhi_device *ctrl_dev = NULL;
+	struct mhi_dtr_ctrl_msg *dtr_msg;
+	struct mhi_wwan_dtr *dtr;
+	u32 msg = 0;
+	int ret;
+
+	guard(mutex)(&mhi_wwan_dtr_lock);
+
+	list_for_each_entry(dtr, &mhi_wwan_dtr_list, node) {
+		if (dtr->cntrl == cntrl) {
+			ctrl_dev = dtr->mhi_dev;
+			break;
+		}
+	}
+	if (!ctrl_dev) {
+		dev_dbg(&mhiwwan->mhi_dev->dev,
+			"IP_CTRL not enumerated; DTR/RTS not signalled to modem\n");
+		return 0;
+	}
+
+	dtr_msg = kzalloc_obj(*dtr_msg);
+	if (!dtr_msg)
+		return -ENOMEM;
+
+	if (mdmbits & TIOCM_DTR)
+		msg |= MHI_DTR_MSG_DTR;
+	if (mdmbits & TIOCM_RTS)
+		msg |= MHI_DTR_MSG_RTS;
+
+	dtr_msg->preamble = cpu_to_le32(MHI_DTR_CTRL_MAGIC);
+	dtr_msg->msg_id = cpu_to_le32(MHI_DTR_HOST_STATE);
+	dtr_msg->dest_id = cpu_to_le32(mhiwwan->mhi_dev->ul_chan_id);
+	dtr_msg->size = cpu_to_le32(sizeof(__le32));
+	dtr_msg->msg = cpu_to_le32(msg);
+
+	ret = mhi_queue_buf(ctrl_dev, DMA_TO_DEVICE, dtr_msg, sizeof(*dtr_msg),
+			    MHI_EOT);
+	if (ret)
+		kfree(dtr_msg);
+
+	return ret;
+}
+
+/* Send the latest DTR/RTS state. A full UL ring or a failed allocation is
+ * retried shortly, with whatever state is latest by then, so the modem ends
+ * up with the final state. Other errors mean the channel is going away.
+ */
+static void mhi_wwan_ctrl_dtr_work(struct work_struct *work)
+{
+	struct mhi_wwan_dev *mhiwwan = container_of(to_delayed_work(work),
+						    struct mhi_wwan_dev, dtr_work);
+	int ret;
+
+	ret = mhi_wwan_ctrl_send_dtr(mhiwwan, READ_ONCE(mhiwwan->dtr_mdmbits));
+	if (ret == -EAGAIN || ret == -ENOMEM)
+		schedule_delayed_work(&mhiwwan->dtr_work, msecs_to_jiffies(10));
+	else if (ret)
+		dev_warn_ratelimited(&mhiwwan->mhi_dev->dev,
+				     "failed to send DTR/RTS state to modem: %d\n",
+				     ret);
+}
+
+static void mhi_wwan_ctrl_dtr_rts(struct wwan_port *port, unsigned int mdmbits)
+{
+	struct mhi_wwan_dev *mhiwwan = wwan_port_get_drvdata(port);
+
+	WRITE_ONCE(mhiwwan->dtr_mdmbits, mdmbits);
+	schedule_delayed_work(&mhiwwan->dtr_work, 0);
+}
+
 static int mhi_wwan_ctrl_start(struct wwan_port *port)
 {
 	struct mhi_wwan_dev *mhiwwan = wwan_port_get_drvdata(port);
@@ -163,6 +274,7 @@ static const struct wwan_port_ops wwan_pops = {
 	.start = mhi_wwan_ctrl_start,
 	.stop = mhi_wwan_ctrl_stop,
 	.tx = mhi_wwan_ctrl_tx,
+	.dtr_rts = mhi_wwan_ctrl_dtr_rts,
 };
 
 static void mhi_ul_xfer_cb(struct mhi_device *mhi_dev,
@@ -224,6 +336,7 @@ static int mhi_wwan_ctrl_probe(struct mhi_device *mhi_dev,
 	mhiwwan->mhi_dev = mhi_dev;
 	mhiwwan->mtu = MHI_WWAN_MAX_MTU;
 	INIT_WORK(&mhiwwan->rx_refill, mhi_wwan_ctrl_refill_work);
+	INIT_DELAYED_WORK(&mhiwwan->dtr_work, mhi_wwan_ctrl_dtr_work);
 	spin_lock_init(&mhiwwan->tx_lock);
 	spin_lock_init(&mhiwwan->rx_lock);
 
@@ -252,9 +365,118 @@ static void mhi_wwan_ctrl_remove(struct mhi_device *mhi_dev)
 	struct mhi_wwan_dev *mhiwwan = dev_get_drvdata(&mhi_dev->dev);
 
 	wwan_remove_port(mhiwwan->wwan_port);
+	/* Give the DTR/RTS drop from removal one attempt, then stop the work */
+	flush_delayed_work(&mhiwwan->dtr_work);
+	disable_delayed_work_sync(&mhiwwan->dtr_work);
 	kfree(mhiwwan);
 }
 
+/* IP_CTRL channel driver, bound separately so the data-port path is untouched */
+static void mhi_wwan_dtr_ul_xfer_cb(struct mhi_device *mhi_dev,
+				    struct mhi_result *mhi_result)
+{
+	/* MHI core has done with the buffer, release it */
+	kfree(mhi_result->buf_addr);
+}
+
+/* Requeue the single DL sink buffer so the IP_CTRL DL ring never runs empty.
+ * This only runs from the work item, so remove() can stop it before the
+ * channel is torn down. A transient mapping failure is retried, other errors
+ * mean the channel is going away and the next probe queues a fresh buffer.
+ */
+static void mhi_wwan_dtr_refill_work(struct work_struct *work)
+{
+	struct mhi_wwan_dtr *dtr = container_of(to_delayed_work(work),
+						struct mhi_wwan_dtr, rx_refill);
+	int ret;
+
+	ret = mhi_queue_buf(dtr->mhi_dev, DMA_FROM_DEVICE, dtr->rx_buf,
+			    sizeof(*dtr->rx_buf), MHI_EOT);
+	if (ret == -ENOMEM) {
+		dev_warn_ratelimited(&dtr->mhi_dev->dev,
+				     "failed to requeue IP_CTRL RX buffer, retrying\n");
+		schedule_delayed_work(&dtr->rx_refill, msecs_to_jiffies(100));
+	} else if (ret) {
+		dev_dbg(&dtr->mhi_dev->dev,
+			"failed to requeue IP_CTRL RX buffer: %d\n", ret);
+	}
+}
+
+static void mhi_wwan_dtr_dl_xfer_cb(struct mhi_device *mhi_dev,
+				    struct mhi_result *mhi_result)
+{
+	struct mhi_wwan_dtr *dtr = dev_get_drvdata(&mhi_dev->dev);
+
+	/* Channel is being torn down (e.g. -ENOTCONN), do not requeue */
+	if (mhi_result->transaction_status &&
+	    mhi_result->transaction_status != -EOVERFLOW)
+		return;
+
+	/* Modem serial state not needed, requeue the sink buffer to keep DL ring live */
+	schedule_delayed_work(&dtr->rx_refill, 0);
+}
+
+static int mhi_wwan_dtr_probe(struct mhi_device *mhi_dev,
+			      const struct mhi_device_id *id)
+{
+	struct mhi_wwan_dtr *dtr;
+	int ret;
+
+	dtr = kzalloc_obj(*dtr);
+	if (!dtr)
+		return -ENOMEM;
+
+	dtr->rx_buf = kmalloc_obj(*dtr->rx_buf);
+	if (!dtr->rx_buf) {
+		ret = -ENOMEM;
+		goto err_free_dtr;
+	}
+	INIT_DELAYED_WORK(&dtr->rx_refill, mhi_wwan_dtr_refill_work);
+
+	ret = mhi_prepare_for_transfer(mhi_dev);
+	if (ret)
+		goto err_free_buf;
+
+	dtr->cntrl = mhi_dev->mhi_cntrl;
+	dtr->mhi_dev = mhi_dev;
+	dev_set_drvdata(&mhi_dev->dev, dtr);
+
+	/* On failure mhi_probe() unprepares the channels itself */
+	ret = mhi_queue_buf(mhi_dev, DMA_FROM_DEVICE, dtr->rx_buf,
+			    sizeof(*dtr->rx_buf), MHI_EOT);
+	if (ret)
+		goto err_free_buf;
+
+	mutex_lock(&mhi_wwan_dtr_lock);
+	list_add(&dtr->node, &mhi_wwan_dtr_list);
+	mutex_unlock(&mhi_wwan_dtr_lock);
+
+	return 0;
+
+err_free_buf:
+	kfree(dtr->rx_buf);
+err_free_dtr:
+	kfree(dtr);
+	return ret;
+}
+
+static void mhi_wwan_dtr_remove(struct mhi_device *mhi_dev)
+{
+	struct mhi_wwan_dtr *dtr = dev_get_drvdata(&mhi_dev->dev);
+
+	mutex_lock(&mhi_wwan_dtr_lock);
+	list_del(&dtr->node);
+	mutex_unlock(&mhi_wwan_dtr_lock);
+
+	/* Stop the refill work, and stop a late DL callback re-arming it, before
+	 * the channel is torn down so no requeue can race with the teardown
+	 */
+	disable_delayed_work_sync(&dtr->rx_refill);
+	mhi_unprepare_from_transfer(mhi_dev);
+	kfree(dtr->rx_buf);
+	kfree(dtr);
+}
+
 static const struct mhi_device_id mhi_wwan_ctrl_match_table[] = {
 	{ .chan = "DUN", .driver_data = WWAN_PORT_AT },
 	{ .chan = "DUN2", .driver_data = WWAN_PORT_AT },
@@ -278,7 +500,45 @@ static struct mhi_driver mhi_wwan_ctrl_driver = {
 	},
 };
 
-module_mhi_driver(mhi_wwan_ctrl_driver);
+static const struct mhi_device_id mhi_wwan_dtr_match_table[] = {
+	{ .chan = "IP_CTRL" },
+	{},
+};
+MODULE_DEVICE_TABLE(mhi, mhi_wwan_dtr_match_table);
+
+static struct mhi_driver mhi_wwan_dtr_driver = {
+	.id_table = mhi_wwan_dtr_match_table,
+	.remove = mhi_wwan_dtr_remove,
+	.probe = mhi_wwan_dtr_probe,
+	.ul_xfer_cb = mhi_wwan_dtr_ul_xfer_cb,
+	.dl_xfer_cb = mhi_wwan_dtr_dl_xfer_cb,
+	.driver = {
+		.name = "mhi_wwan_dtr",
+	},
+};
+
+static int __init mhi_wwan_ctrl_init(void)
+{
+	int ret;
+
+	ret = mhi_driver_register(&mhi_wwan_dtr_driver);
+	if (ret)
+		return ret;
+
+	ret = mhi_driver_register(&mhi_wwan_ctrl_driver);
+	if (ret)
+		mhi_driver_unregister(&mhi_wwan_dtr_driver);
+
+	return ret;
+}
+module_init(mhi_wwan_ctrl_init);
+
+static void __exit mhi_wwan_ctrl_exit(void)
+{
+	mhi_driver_unregister(&mhi_wwan_ctrl_driver);
+	mhi_driver_unregister(&mhi_wwan_dtr_driver);
+}
+module_exit(mhi_wwan_ctrl_exit);
 
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("MHI WWAN CTRL Driver");
-- 
2.43.0

Re: [PATCH net-next v10 2/2] net: wwan: mhi_wwan_ctrl: drive DTR/RTS via the IP_CTRL channel

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] A DTR/RTS request made while the IP_CTRL channel is not bound
  is silently lost, and nothing sends it again when IP_CTRL binds later.
- [Low] The DTR/RTS drop on last close (HUPCL) or on port removal is now
  sent asynchronously and coalesced with later state, so the modem may…

--

net: wwan: mhi_wwan_ctrl: drive DTR/RTS via the IP_CTRL channel

This patch makes mhi_wwan_ctrl also bind the IP_CTRL MHI channel, using a
separate mhi_driver. It implements the wwan ->dtr_rts op for AT ports.
Each port records its DTR/RTS state, and a delayed work item sends it over
the IP_CTRL channel of the same controller. The work coalesces bursts and
retries on -EAGAIN/-ENOMEM.

> diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
> index a31d8540fbb85..e3c2075889565 100644
> --- a/drivers/net/wwan/mhi_wwan_ctrl.c
> +++ b/drivers/net/wwan/mhi_wwan_ctrl.c

[ ... ]

> @@ -103,6 +140,80 @@ static void mhi_wwan_ctrl_refill_work(struct work_struct *work)
>  	}
>  }
>  
> +/* Signal the modem's DTR/RTS lines over its own controller's IP_CTRL channel */
> +static int mhi_wwan_ctrl_send_dtr(struct mhi_wwan_dev *mhiwwan, unsigned int mdmbits)
> +{

[ ... ]

> +	guard(mutex)(&mhi_wwan_dtr_lock);
> +
> +	list_for_each_entry(dtr, &mhi_wwan_dtr_list, node) {
> +		if (dtr->cntrl == cntrl) {
> +			ctrl_dev = dtr->mhi_dev;
> +			break;
> +		}
> +	}
> +	if (!ctrl_dev) {
> +		dev_dbg(&mhiwwan->mhi_dev->dev,
> +			"IP_CTRL not enumerated; DTR/RTS not signalled to modem\n");
> +		return 0;
> +	}

[Severity: Medium]
If IP_CTRL is not bound when a DTR/RTS request comes in, is that request
lost for good?

mhi_wwan_ctrl_dtr_work() only reschedules on -EAGAIN or -ENOMEM, so it
treats this return 0 as a successful send:

	ret = mhi_wwan_ctrl_send_dtr(mhiwwan, READ_ONCE(mhiwwan->dtr_mdmbits));
	if (ret == -EAGAIN || ret == -ENOMEM)
		schedule_delayed_work(&mhiwwan->dtr_work, msecs_to_jiffies(10));

Later, mhi_wwan_dtr_probe() adds the IP_CTRL entry to mhi_wwan_dtr_list.
Nothing at that point sends the current dtr_mdmbits for the AT ports that
are already open on that controller.

The wwan core won't resend it either. wwan_port_raise_dtr_rts() only runs
on first open from wwan_port_op_start(). The TIOCMSET/TIOCMBIS/TIOCMBIC
handler only calls ->dtr_rts when the cached bits change:

	if (port->type == WWAN_PORT_AT && port->ops &&
	    port->ops->dtr_rts &&
	    (old ^ port->at_data.mdmbits) & (TIOCM_DTR | TIOCM_RTS))
		port->ops->dtr_rts(port, port->at_data.mdmbits);

So if userspace asks for TIOCM_DTR again, the driver never sees it.

The modem would then keep DTR deasserted and hold back URCs, while
TIOCMGET reports DTR|RTS as asserted.

Some ways to get into this state:

Unbinding and rebinding mhi_wwan_dtr through sysfs while a DUN port is
open. Any request made between mhi_wwan_dtr_remove() and the next probe
is dropped.

An IP_CTRL probe that fails and is retried later.

A controller config that puts IP_CTRL at a higher channel index than DUN.
mhi_create_devices() probes devices in channel-index order.

The commit message says:

    If a send fails because the ring is full or memory is short, the
    work retries shortly with whatever state is latest by then, so the modem
    ends up with the final state that was requested.

As far as I can tell, that only holds for -EAGAIN/-ENOMEM. It does not
cover an IP_CTRL channel that binds late.

Could mhi_wwan_dtr_probe() resync the open AT ports on its controller, or
could this case be handled so the state is sent again once IP_CTRL binds?

> +
> +	dtr_msg = kzalloc_obj(*dtr_msg);
> +	if (!dtr_msg)
> +		return -ENOMEM;

[ ... ]

> +static void mhi_wwan_ctrl_dtr_rts(struct wwan_port *port, unsigned int mdmbits)
> +{
> +	struct mhi_wwan_dev *mhiwwan = wwan_port_get_drvdata(port);
> +
> +	WRITE_ONCE(mhiwwan->dtr_mdmbits, mdmbits);
> +	schedule_delayed_work(&mhiwwan->dtr_work, 0);
> +}

[Severity: Low]
Can the HUPCL drop on last close get lost here?

wwan_port_op_stop() drops DTR/RTS and then stops the port:

	wwan_port_drop_dtr_rts(port, true);
	port->ops->stop(port);

This function only records the bits and schedules the work. The message
is therefore queued later, possibly after mhi_wwan_ctrl_stop() has already
called mhi_unprepare_from_transfer() on the DUN channel. The drop from
wwan_remove_port() is also sent only after ->stop() has run.

Suppose dtr_work is still pending when the port is reopened.
wwan_port_raise_dtr_rts() overwrites dtr_mdmbits with DTR|RTS, and
mhi_wwan_ctrl_dtr_work() only reads the raised value. The drop is never
sent.

The most likely way for the work to be pending is a 10 ms retry armed
after -EAGAIN or -ENOMEM. schedule_delayed_work(..., 0) does nothing to a
delayed work that is already pending, and it does not shorten the delay.

On a modem configured with AT&D2, could a quick close and reopen then
fail to hang up an active data call?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013183500.261229-1-peter.hunt%40opengear.com

Re: [PATCH net-next v10 2/2] net: wwan: mhi_wwan_ctrl: drive DTR/RTS via the IP_CTRL channel

[Peter Hunt]

Thanks for the review.

[Medium] a DTR/RTS request made while IP_CTRL is not bound is lost:
agreed. It needs IP_CTRL to bind after the AT port has been opened. On
the EM919x/EM929x, IP_CTRL (channels 18/19) is probed before DUN
(32/33), so outside async probing this only happens after a sysfs
unbind/rebind of mhi_wwan_dtr or a failed IP_CTRL probe. The fix is for
mhi_wwan_dtr_probe() to resend the current state of the AT ports on its
controller.

[Low] coalescing can swallow a DTR drop on a quick close and reopen:
agreed, a drop matters on its own for a modem set to AT&D2. The fix is
to never coalesce away a pending drop, and send it before the latest
state.

Both are corner cases outside normal operation, which I've tested on an
EM9291, including bursts of 10,000 DTR changes ending in either state.
I'd propose to address them in a small follow-up patch on top of this
series, but I'm happy to fold them into a v11 instead if the
maintainers prefer.

Re: [PATCH net-next v10 2/2] net: wwan: mhi_wwan_ctrl: drive DTR/RTS via the IP_CTRL channel

[Loic Poulain]

On Tue, Oct 13, 2026 at 8:36 PM Peter Hunt <peter.hunt@opengear.com> wrote:
>
> Qualcomm/Sierra SDX55/SDX65 modems withhold unsolicited AT result codes
> (URCs such as +CREG, and the +DMI OMA-DM/LwM2M session indications) on
> an AT port until the host asserts DTR. mhi_wwan_ctrl exposed the AT
> (DUN) ports but had no way to signal DTR, so URCs never reached
> userspace.
>
> Carry the host serial-control lines to the modem over the dedicated
> IP_CTRL MHI channel, which this module now also binds. IP_CTRL uses a
> separate mhi_driver with its own callbacks so the AT/QMI/MBIM data path
> is untouched; the control-channel device for each MHI controller is
> tracked in a small registry so an AT port drives the IP_CTRL channel of
> its own modem (multiple modems are supported).
>
> The IP_CTRL channel has to be declared in the controller's channel config
> for this driver to bind. For the Sierra EM919x/EM929x this is done by
> commit 83c29a55b89e ("bus: mhi: host: pci_generic: Add IP_CTRL channel
> for Sierra EM919x/EM929x") in the MHI tree. On controllers that do not
> declare IP_CTRL the driver does not bind and ->dtr_rts is a no-op.
>
> The wwan core, with "net: wwan: core: propagate modem control signals to
> port drivers", raises DTR/RTS on first open and drops them on last close
> (if HUPCL is set) for any AT port whose driver implements ->dtr_rts, so
> no open/close handling is needed here. The new ->dtr_rts op lets
> userspace assert or de-assert them via TIOCMSET/TIOCMBIC/TIOCMBIS.
> Received device->host serial state messages are silently discarded; a
> single recycled sink buffer keeps the IP_CTRL DL ring live so the
> modem's transmit path does not stall.
>
> ->dtr_rts is void, following the tty_port_operations model it is
> modelled on. It records the requested state and schedules a per-port
> work item that sends the latest state to the modem, so a burst of
> changes is coalesced into one message and cannot fill the IP_CTRL UL
> ring. If a send fails because the ring is full or memory is short, the
> work retries shortly with whatever state is latest by then, so the modem
> ends up with the final state that was requested. The DL sink buffer is
> requeued from a work item, which retries a transient -ENOMEM so the DL
> ring does not stay empty, and which remove() disables before tearing
> down the channel.
>
> Signed-off-by: Peter Hunt <peter.hunt@opengear.com>

Reviewed-by: Loic Poulain <loic.poulain@oss.qualcomm.com>

> ---
> v10: Send DTR/RTS changes from a per-port work item that coalesces
>     bursts and retries a send that fails with -EAGAIN (UL ring full) or
>     -ENOMEM, so the modem always ends up with the final state. Testing
>     v9 showed that rapid toggling could fill the IP_CTRL UL ring and
>     drop updates (Sashiko review of v9). Loic's Reviewed-by is dropped
>     as the send path changed
> v9: Requeue the DL sink buffer only from the refill work and disable it
>     with disable_delayed_work_sync() before mhi_unprepare_from_transfer()
>     in remove(), so no requeue can race with channel teardown (Loic
>     Poulain, Sashiko review of v7). Leave unpreparing to mhi_probe() when
>     probe fails. Name the wwan core commit instead of "patch 1"
> v8: No changes
> v7: Retry a failed DL sink buffer requeue from a delayed work item on
>     -ENOMEM, and log other failures. Note the dependency on the IP_CTRL
>     channel entry in pci_generic, now in the MHI tree. Both from the
>     Sashiko review of v6
> v6: Rebase onto net-next. Allocate the IP_CTRL DL sink buffer separately
>     instead of embedding it in struct mhi_wwan_dtr, so DMA cache
>     maintenance on non-coherent platforms cannot touch neighbouring fields.
>     Do not requeue the sink buffer from the DL callback when the transfer
>     completes with an error (e.g. -ENOTCONN during channel teardown)
> v5: Implement ->dtr_rts(port, mdmbits) and drive DTR and RTS
>     independently. Pre-queue an RX sink buffer in probe and requeue it in
>     the DL callback so the IP_CTRL DL ring does not run empty and stall
>     the modem's transmit path
> v4: Add dev_dbg when IP_CTRL channel is not enumerated for this controller
>     and when mhi_queue_buf fails, to aid diagnosis of the "URCs missing"
>     case on controllers that do not declare IP_CTRL
> v3: Remove is_at_port, implement ->dtr_rts instead of ->tiocmset,
>     open/close DTR raise/drop handled by wwan core (Loic Poulain)
> ---
>  drivers/net/wwan/mhi_wwan_ctrl.c | 262 ++++++++++++++++++++++++++++++-
>  1 file changed, 261 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
> index a31d8540fbb8..e3c207588956 100644
> --- a/drivers/net/wwan/mhi_wwan_ctrl.c
> +++ b/drivers/net/wwan/mhi_wwan_ctrl.c
> @@ -1,8 +1,13 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /* Copyright (c) 2021, Linaro Ltd <loic.poulain@linaro.org> */
>  #include <linux/kernel.h>
> +#include <linux/list.h>
>  #include <linux/mhi.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/termios.h>
> +#include <linux/workqueue.h>
>  #include <linux/wwan.h>
>
>  /* MHI wwan flags */
> @@ -14,6 +19,32 @@ enum mhi_wwan_flags {
>
>  #define MHI_WWAN_MAX_MTU       0x8000
>
> +/* IP_CTRL channel message that sets the modem's DTR/RTS control lines */
> +struct mhi_dtr_ctrl_msg {
> +       __le32 preamble;
> +       __le32 msg_id;
> +       __le32 dest_id;
> +       __le32 size;
> +       __le32 msg;
> +} __packed;
> +
> +#define MHI_DTR_CTRL_MAGIC     0x4C525443      /* 'CTRL' */
> +#define MHI_DTR_MSG_DTR                BIT(0)
> +#define MHI_DTR_MSG_RTS                BIT(1)
> +#define MHI_DTR_HOST_STATE     0x10
> +
> +/* Per-controller IP_CTRL channel, used to signal DTR/RTS to that modem */
> +struct mhi_wwan_dtr {
> +       struct mhi_controller *cntrl;
> +       struct mhi_device *mhi_dev;
> +       struct list_head node;
> +       struct mhi_dtr_ctrl_msg *rx_buf; /* DL sink, separate allocation for DMA safety */
> +       struct delayed_work rx_refill; /* retries a failed sink buffer requeue */
> +};
> +
> +static LIST_HEAD(mhi_wwan_dtr_list);
> +static DEFINE_MUTEX(mhi_wwan_dtr_lock);
> +
>  struct mhi_wwan_dev {
>         /* Lower level is a mhi dev, upper level is a wwan port */
>         struct mhi_device *mhi_dev;
> @@ -35,6 +66,12 @@ struct mhi_wwan_dev {
>          * decremented on data queueing and incremented on data release.
>          */
>         unsigned int rx_budget;
> +
> +       /* Latest DTR/RTS state for the modem, sent from dtr_work so a burst of
> +        * changes is coalesced and only the final state has to get through
> +        */
> +       unsigned int dtr_mdmbits;
> +       struct delayed_work dtr_work;
>  };
>
>  /* Increment RX budget and schedule RX refill if necessary */
> @@ -103,6 +140,80 @@ static void mhi_wwan_ctrl_refill_work(struct work_struct *work)
>         }
>  }
>
> +/* Signal the modem's DTR/RTS lines over its own controller's IP_CTRL channel */
> +static int mhi_wwan_ctrl_send_dtr(struct mhi_wwan_dev *mhiwwan, unsigned int mdmbits)
> +{
> +       struct mhi_controller *cntrl = mhiwwan->mhi_dev->mhi_cntrl;
> +       struct mhi_device *ctrl_dev = NULL;
> +       struct mhi_dtr_ctrl_msg *dtr_msg;
> +       struct mhi_wwan_dtr *dtr;
> +       u32 msg = 0;
> +       int ret;
> +
> +       guard(mutex)(&mhi_wwan_dtr_lock);
> +
> +       list_for_each_entry(dtr, &mhi_wwan_dtr_list, node) {
> +               if (dtr->cntrl == cntrl) {
> +                       ctrl_dev = dtr->mhi_dev;
> +                       break;
> +               }
> +       }
> +       if (!ctrl_dev) {
> +               dev_dbg(&mhiwwan->mhi_dev->dev,
> +                       "IP_CTRL not enumerated; DTR/RTS not signalled to modem\n");
> +               return 0;
> +       }
> +
> +       dtr_msg = kzalloc_obj(*dtr_msg);
> +       if (!dtr_msg)
> +               return -ENOMEM;
> +
> +       if (mdmbits & TIOCM_DTR)
> +               msg |= MHI_DTR_MSG_DTR;
> +       if (mdmbits & TIOCM_RTS)
> +               msg |= MHI_DTR_MSG_RTS;
> +
> +       dtr_msg->preamble = cpu_to_le32(MHI_DTR_CTRL_MAGIC);
> +       dtr_msg->msg_id = cpu_to_le32(MHI_DTR_HOST_STATE);
> +       dtr_msg->dest_id = cpu_to_le32(mhiwwan->mhi_dev->ul_chan_id);
> +       dtr_msg->size = cpu_to_le32(sizeof(__le32));
> +       dtr_msg->msg = cpu_to_le32(msg);
> +
> +       ret = mhi_queue_buf(ctrl_dev, DMA_TO_DEVICE, dtr_msg, sizeof(*dtr_msg),
> +                           MHI_EOT);
> +       if (ret)
> +               kfree(dtr_msg);
> +
> +       return ret;
> +}
> +
> +/* Send the latest DTR/RTS state. A full UL ring or a failed allocation is
> + * retried shortly, with whatever state is latest by then, so the modem ends
> + * up with the final state. Other errors mean the channel is going away.
> + */
> +static void mhi_wwan_ctrl_dtr_work(struct work_struct *work)
> +{
> +       struct mhi_wwan_dev *mhiwwan = container_of(to_delayed_work(work),
> +                                                   struct mhi_wwan_dev, dtr_work);
> +       int ret;
> +
> +       ret = mhi_wwan_ctrl_send_dtr(mhiwwan, READ_ONCE(mhiwwan->dtr_mdmbits));
> +       if (ret == -EAGAIN || ret == -ENOMEM)
> +               schedule_delayed_work(&mhiwwan->dtr_work, msecs_to_jiffies(10));
> +       else if (ret)
> +               dev_warn_ratelimited(&mhiwwan->mhi_dev->dev,
> +                                    "failed to send DTR/RTS state to modem: %d\n",
> +                                    ret);
> +}
> +
> +static void mhi_wwan_ctrl_dtr_rts(struct wwan_port *port, unsigned int mdmbits)
> +{
> +       struct mhi_wwan_dev *mhiwwan = wwan_port_get_drvdata(port);
> +
> +       WRITE_ONCE(mhiwwan->dtr_mdmbits, mdmbits);
> +       schedule_delayed_work(&mhiwwan->dtr_work, 0);
> +}
> +
>  static int mhi_wwan_ctrl_start(struct wwan_port *port)
>  {
>         struct mhi_wwan_dev *mhiwwan = wwan_port_get_drvdata(port);
> @@ -163,6 +274,7 @@ static const struct wwan_port_ops wwan_pops = {
>         .start = mhi_wwan_ctrl_start,
>         .stop = mhi_wwan_ctrl_stop,
>         .tx = mhi_wwan_ctrl_tx,
> +       .dtr_rts = mhi_wwan_ctrl_dtr_rts,
>  };
>
>  static void mhi_ul_xfer_cb(struct mhi_device *mhi_dev,
> @@ -224,6 +336,7 @@ static int mhi_wwan_ctrl_probe(struct mhi_device *mhi_dev,
>         mhiwwan->mhi_dev = mhi_dev;
>         mhiwwan->mtu = MHI_WWAN_MAX_MTU;
>         INIT_WORK(&mhiwwan->rx_refill, mhi_wwan_ctrl_refill_work);
> +       INIT_DELAYED_WORK(&mhiwwan->dtr_work, mhi_wwan_ctrl_dtr_work);
>         spin_lock_init(&mhiwwan->tx_lock);
>         spin_lock_init(&mhiwwan->rx_lock);
>
> @@ -252,9 +365,118 @@ static void mhi_wwan_ctrl_remove(struct mhi_device *mhi_dev)
>         struct mhi_wwan_dev *mhiwwan = dev_get_drvdata(&mhi_dev->dev);
>
>         wwan_remove_port(mhiwwan->wwan_port);
> +       /* Give the DTR/RTS drop from removal one attempt, then stop the work */
> +       flush_delayed_work(&mhiwwan->dtr_work);
> +       disable_delayed_work_sync(&mhiwwan->dtr_work);
>         kfree(mhiwwan);
>  }
>
> +/* IP_CTRL channel driver, bound separately so the data-port path is untouched */
> +static void mhi_wwan_dtr_ul_xfer_cb(struct mhi_device *mhi_dev,
> +                                   struct mhi_result *mhi_result)
> +{
> +       /* MHI core has done with the buffer, release it */
> +       kfree(mhi_result->buf_addr);
> +}
> +
> +/* Requeue the single DL sink buffer so the IP_CTRL DL ring never runs empty.
> + * This only runs from the work item, so remove() can stop it before the
> + * channel is torn down. A transient mapping failure is retried, other errors
> + * mean the channel is going away and the next probe queues a fresh buffer.
> + */
> +static void mhi_wwan_dtr_refill_work(struct work_struct *work)
> +{
> +       struct mhi_wwan_dtr *dtr = container_of(to_delayed_work(work),
> +                                               struct mhi_wwan_dtr, rx_refill);
> +       int ret;
> +
> +       ret = mhi_queue_buf(dtr->mhi_dev, DMA_FROM_DEVICE, dtr->rx_buf,
> +                           sizeof(*dtr->rx_buf), MHI_EOT);
> +       if (ret == -ENOMEM) {
> +               dev_warn_ratelimited(&dtr->mhi_dev->dev,
> +                                    "failed to requeue IP_CTRL RX buffer, retrying\n");
> +               schedule_delayed_work(&dtr->rx_refill, msecs_to_jiffies(100));
> +       } else if (ret) {
> +               dev_dbg(&dtr->mhi_dev->dev,
> +                       "failed to requeue IP_CTRL RX buffer: %d\n", ret);
> +       }
> +}
> +
> +static void mhi_wwan_dtr_dl_xfer_cb(struct mhi_device *mhi_dev,
> +                                   struct mhi_result *mhi_result)
> +{
> +       struct mhi_wwan_dtr *dtr = dev_get_drvdata(&mhi_dev->dev);
> +
> +       /* Channel is being torn down (e.g. -ENOTCONN), do not requeue */
> +       if (mhi_result->transaction_status &&
> +           mhi_result->transaction_status != -EOVERFLOW)
> +               return;
> +
> +       /* Modem serial state not needed, requeue the sink buffer to keep DL ring live */
> +       schedule_delayed_work(&dtr->rx_refill, 0);
> +}
> +
> +static int mhi_wwan_dtr_probe(struct mhi_device *mhi_dev,
> +                             const struct mhi_device_id *id)
> +{
> +       struct mhi_wwan_dtr *dtr;
> +       int ret;
> +
> +       dtr = kzalloc_obj(*dtr);
> +       if (!dtr)
> +               return -ENOMEM;
> +
> +       dtr->rx_buf = kmalloc_obj(*dtr->rx_buf);
> +       if (!dtr->rx_buf) {
> +               ret = -ENOMEM;
> +               goto err_free_dtr;
> +       }
> +       INIT_DELAYED_WORK(&dtr->rx_refill, mhi_wwan_dtr_refill_work);
> +
> +       ret = mhi_prepare_for_transfer(mhi_dev);
> +       if (ret)
> +               goto err_free_buf;
> +
> +       dtr->cntrl = mhi_dev->mhi_cntrl;
> +       dtr->mhi_dev = mhi_dev;
> +       dev_set_drvdata(&mhi_dev->dev, dtr);
> +
> +       /* On failure mhi_probe() unprepares the channels itself */
> +       ret = mhi_queue_buf(mhi_dev, DMA_FROM_DEVICE, dtr->rx_buf,
> +                           sizeof(*dtr->rx_buf), MHI_EOT);
> +       if (ret)
> +               goto err_free_buf;
> +
> +       mutex_lock(&mhi_wwan_dtr_lock);
> +       list_add(&dtr->node, &mhi_wwan_dtr_list);
> +       mutex_unlock(&mhi_wwan_dtr_lock);
> +
> +       return 0;
> +
> +err_free_buf:
> +       kfree(dtr->rx_buf);
> +err_free_dtr:
> +       kfree(dtr);
> +       return ret;
> +}
> +
> +static void mhi_wwan_dtr_remove(struct mhi_device *mhi_dev)
> +{
> +       struct mhi_wwan_dtr *dtr = dev_get_drvdata(&mhi_dev->dev);
> +
> +       mutex_lock(&mhi_wwan_dtr_lock);
> +       list_del(&dtr->node);
> +       mutex_unlock(&mhi_wwan_dtr_lock);
> +
> +       /* Stop the refill work, and stop a late DL callback re-arming it, before
> +        * the channel is torn down so no requeue can race with the teardown
> +        */
> +       disable_delayed_work_sync(&dtr->rx_refill);
> +       mhi_unprepare_from_transfer(mhi_dev);
> +       kfree(dtr->rx_buf);
> +       kfree(dtr);
> +}
> +
>  static const struct mhi_device_id mhi_wwan_ctrl_match_table[] = {
>         { .chan = "DUN", .driver_data = WWAN_PORT_AT },
>         { .chan = "DUN2", .driver_data = WWAN_PORT_AT },
> @@ -278,7 +500,45 @@ static struct mhi_driver mhi_wwan_ctrl_driver = {
>         },
>  };
>
> -module_mhi_driver(mhi_wwan_ctrl_driver);
> +static const struct mhi_device_id mhi_wwan_dtr_match_table[] = {
> +       { .chan = "IP_CTRL" },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(mhi, mhi_wwan_dtr_match_table);
> +
> +static struct mhi_driver mhi_wwan_dtr_driver = {
> +       .id_table = mhi_wwan_dtr_match_table,
> +       .remove = mhi_wwan_dtr_remove,
> +       .probe = mhi_wwan_dtr_probe,
> +       .ul_xfer_cb = mhi_wwan_dtr_ul_xfer_cb,
> +       .dl_xfer_cb = mhi_wwan_dtr_dl_xfer_cb,
> +       .driver = {
> +               .name = "mhi_wwan_dtr",
> +       },
> +};
> +
> +static int __init mhi_wwan_ctrl_init(void)
> +{
> +       int ret;
> +
> +       ret = mhi_driver_register(&mhi_wwan_dtr_driver);
> +       if (ret)
> +               return ret;
> +
> +       ret = mhi_driver_register(&mhi_wwan_ctrl_driver);
> +       if (ret)
> +               mhi_driver_unregister(&mhi_wwan_dtr_driver);
> +
> +       return ret;
> +}
> +module_init(mhi_wwan_ctrl_init);
> +
> +static void __exit mhi_wwan_ctrl_exit(void)
> +{
> +       mhi_driver_unregister(&mhi_wwan_ctrl_driver);
> +       mhi_driver_unregister(&mhi_wwan_dtr_driver);
> +}
> +module_exit(mhi_wwan_ctrl_exit);
>
>  MODULE_LICENSE("GPL v2");
>  MODULE_DESCRIPTION("MHI WWAN CTRL Driver");
> --
> 2.43.0
>