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

[PATCH net-next v3 0/3] 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
exposed AT ports but never signalled DTR, so URCs never reached userspace.

Patch 1 extends the wwan core with an optional ->dtr_rts(port, on) port
op, mirroring tty_port_operations.dtr_rts.  The TIOCM bitmask state is
tracked entirely in the wwan core; drivers receive only a simple boolean
assert/de-assert signal.  The core also raises DTR/RTS on first open of
any AT port whose driver implements ->dtr_rts, and drops them on last
close, mirroring TTY semantics without requiring each driver to duplicate
that logic.

Patch 2 enables the IP_CTRL MHI channel in the Sierra PCI table so that
the IP_CTRL driver (patch 3) is actually bound on those controllers.

Patch 3 adds a second mhi_driver that binds the IP_CTRL channel and
registers a ->dtr_rts op so that the wwan core's open/close DTR raise/drop
and userspace TIOCMSET calls both reach the modem.  The existing AT/QMI/MBIM
data path is untouched.

v2: https://lore.kernel.org/netdev/20260806155253.3378294-1-peter.hunt@opengear.com/

v3:
- Replace ->tiocmget/->tiocmset port ops with a single ->dtr_rts(port, on)
  callback modelled on tty_port_operations.dtr_rts; TIOCM bitmask state
  stays in the wwan core (Loic Poulain)
- Move DTR/RTS raise on AT port open and drop on last close into
  wwan_port_op_start/wwan_port_op_stop in the wwan core (Loic Poulain)
- Remove is_at_port from struct mhi_wwan_dev; rely on wwan core instead
  (Loic Poulain)

Peter Hunt (3):
  net: wwan: core: propagate modem control signals to port drivers
  bus: mhi: host: pci_generic: enumerate IP_CTRL channel for Sierra
    EM919x/EM929x
  net: wwan: mhi_wwan_ctrl: drive DTR/RTS via the IP_CTRL channel

 drivers/bus/mhi/host/pci_generic.c |   2 +
 drivers/net/wwan/mhi_wwan_ctrl.c   | 172 ++++++++++++++++++++++++++++-
 drivers/net/wwan/wwan_core.c       |  18 ++-
 include/linux/wwan.h               |   3 +
 4 files changed, 193 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH net-next v3 1/3] 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, on) operation to struct wwan_port_ops,
mirroring tty_port_operations.dtr_rts. Drivers that implement it receive
a simple assert/de-assert signal while the TIOCM bitmask state is tracked
by the wwan core. TIOCMSET/TIOCMBIC/TIOCMBIS resolve the new bitmask and
call ->dtr_rts when the driver implements it.

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. This mirrors TTY semantics (DTR is asserted on open) and means
individual drivers do not need to implement this themselves.

Signed-off-by: Peter Hunt <peter.hunt@opengear.com>
---
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 | 18 +++++++++++++++++-
 include/linux/wwan.h         |  3 +++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
index ffbcf11e4e68..f8ca9cda3c9a 100644
--- a/drivers/net/wwan/wwan_core.c
+++ b/drivers/net/wwan/wwan_core.c
@@ -759,8 +759,15 @@ static int wwan_port_op_start(struct wwan_port *port)
 	if (!port->start_count)
 		ret = port->ops->start(port);
 
-	if (!ret)
+	if (!ret) {
 		port->start_count++;
+		/* Mirror TTY semantics: raise DTR/RTS on first open of an AT port */
+		if (port->start_count == 1 && port->type == WWAN_PORT_AT &&
+		    port->ops->dtr_rts) {
+			port->at_data.mdmbits |= TIOCM_DTR | TIOCM_RTS;
+			port->ops->dtr_rts(port, true);
+		}
+	}
 
 out_unlock:
 	mutex_unlock(&port->ops_lock);
@@ -773,6 +780,11 @@ static void wwan_port_op_stop(struct wwan_port *port)
 	mutex_lock(&port->ops_lock);
 	port->start_count--;
 	if (!port->start_count) {
+		/* Mirror TTY semantics: drop DTR/RTS on last close of an AT port */
+		if (port->ops && port->type == WWAN_PORT_AT && port->ops->dtr_rts) {
+			port->at_data.mdmbits &= ~(TIOCM_DTR | TIOCM_RTS);
+			port->ops->dtr_rts(port, false);
+		}
 		if (port->ops)
 			port->ops->stop(port);
 		skb_queue_purge(&port->rxq);
@@ -1036,6 +1048,10 @@ static long wwan_port_fops_at_ioctl(struct wwan_port *port, unsigned int cmd,
 			port->at_data.mdmbits |= mdmbits;
 		else
 			port->at_data.mdmbits = mdmbits;
+		if (port->ops->dtr_rts)
+			port->ops->dtr_rts(port,
+					   !!(port->at_data.mdmbits &
+					      (TIOCM_DTR | TIOCM_RTS)));
 		break;
 	}
 
diff --git a/include/linux/wwan.h b/include/linux/wwan.h
index 1e0e2cb53579..a7684950d73b 100644
--- a/include/linux/wwan.h
+++ b/include/linux/wwan.h
@@ -57,6 +57,8 @@ 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 asserts (on=true) or de-asserts (on=false)
+ *           the DTR and RTS modem control lines.
  *
  * 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 +72,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, bool on);
 };
 
 /** struct wwan_port_caps - The WWAN port capbilities
-- 
2.43.0

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

[Peter Hunt]

Qualcomm/Sierra SDX55/SDX65 modems (e.g. Sierra EM9291) 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 wwan core (patch 1) raises DTR/RTS on first open and drops them on
last close 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 is not needed and is ignored.

Signed-off-by: Peter Hunt <peter.hunt@opengear.com>
---
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 | 172 ++++++++++++++++++++++++++++++-
 1 file changed, 171 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
index a31d8540fbb8..3aef90ec2176 100644
--- a/drivers/net/wwan/mhi_wwan_ctrl.c
+++ b/drivers/net/wwan/mhi_wwan_ctrl.c
@@ -1,8 +1,12 @@
 // 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/wwan.h>
 
 /* MHI wwan flags */
@@ -14,6 +18,30 @@ 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;
+};
+
+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;
@@ -103,6 +131,56 @@ 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)
+		return 0; /* IP_CTRL not enumerated; best-effort, not an error */
+
+	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;
+}
+
+static void mhi_wwan_ctrl_dtr_rts(struct wwan_port *port, bool on)
+{
+	mhi_wwan_ctrl_send_dtr(wwan_port_get_drvdata(port),
+			       on ? TIOCM_DTR | TIOCM_RTS : 0);
+}
+
 static int mhi_wwan_ctrl_start(struct wwan_port *port)
 {
 	struct mhi_wwan_dev *mhiwwan = wwan_port_get_drvdata(port);
@@ -163,6 +241,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,
@@ -255,6 +334,59 @@ static void mhi_wwan_ctrl_remove(struct mhi_device *mhi_dev)
 	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);
+}
+
+static void mhi_wwan_dtr_dl_xfer_cb(struct mhi_device *mhi_dev,
+				    struct mhi_result *mhi_result)
+{
+	/* Modem serial state is not needed, drop it */
+}
+
+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;
+
+	ret = mhi_prepare_for_transfer(mhi_dev);
+	if (ret) {
+		kfree(dtr);
+		return ret;
+	}
+
+	dtr->cntrl = mhi_dev->mhi_cntrl;
+	dtr->mhi_dev = mhi_dev;
+	dev_set_drvdata(&mhi_dev->dev, dtr);
+
+	mutex_lock(&mhi_wwan_dtr_lock);
+	list_add(&dtr->node, &mhi_wwan_dtr_list);
+	mutex_unlock(&mhi_wwan_dtr_lock);
+
+	return 0;
+}
+
+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);
+
+	mhi_unprepare_from_transfer(mhi_dev);
+	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 +410,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 v3 2/3] bus: mhi: host: pci_generic: enumerate IP_CTRL channel for Sierra EM919x/EM929x

[Loic Poulain]

On Fri, Aug 7, 2026 at 11:51 PM Peter Hunt <peter.hunt@opengear.com> wrote:
>
> IP_CTRL is a standard Qualcomm MHI control channel (channels 18/19) that
> carries the host serial control lines (DTR/RTS) to the modem; it is not
> Sierra-specific. It was not enumerated for any device in pci_generic, so
> the host had no way to drive those signals.
>
> Enumerate it in the Sierra em919x channel config (shared by the EM919x
> and EM929x) using the control event ring. This is the device the change
> was tested against; other Qualcomm modem configs that expose IP_CTRL can
> enable it the same way, since the mhi_wwan_ctrl DTR support that consumes
> it is device-agnostic and binds purely by channel name.
>
> Signed-off-by: Peter Hunt <peter.hunt@opengear.com>

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

> ---
>  drivers/bus/mhi/host/pci_generic.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
> index 0d0d9c7ffa4b..ba47dd153837 100644
> --- a/drivers/bus/mhi/host/pci_generic.c
> +++ b/drivers/bus/mhi/host/pci_generic.c
> @@ -728,6 +728,8 @@ static const struct mhi_channel_config mhi_sierra_em919x_channels[] = {
>         MHI_CHANNEL_CONFIG_DL(13, "MBIM", 128, 0),
>         MHI_CHANNEL_CONFIG_UL(14, "QMI", 32, 0),
>         MHI_CHANNEL_CONFIG_DL(15, "QMI", 32, 0),
> +       MHI_CHANNEL_CONFIG_UL(18, "IP_CTRL", 32, 0),
> +       MHI_CHANNEL_CONFIG_DL(19, "IP_CTRL", 32, 0),
>         MHI_CHANNEL_CONFIG_UL(32, "DUN", 32, 0),
>         MHI_CHANNEL_CONFIG_DL(33, "DUN", 32, 0),
>         MHI_CHANNEL_CONFIG_HW_UL(100, "IP_HW0", 512, 1),
> --
> 2.43.0
>

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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: wwan: core: propagate modem control signals to port drivers

Adds an optional ->dtr_rts(port, on) callback to struct wwan_port_ops so
port drivers learn when the host raises or drops DTR/RTS. The wwan core
calls it from the TIOCMSET/TIOCMBIC/TIOCMBIS emulation and also raises the
lines on first open and drops them on last close of an AT port.

A few questions about the new call sites below.

> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> index ffbcf11e4e68..f8ca9cda3c9a 100644
> --- a/drivers/net/wwan/wwan_core.c
> +++ b/drivers/net/wwan/wwan_core.c
> @@ -759,8 +759,15 @@ static int wwan_port_op_start(struct wwan_port *port)
>  	if (!port->start_count)
>  		ret = port->ops->start(port);
>  
> -	if (!ret)
> +	if (!ret) {
>  		port->start_count++;
> +		/* Mirror TTY semantics: raise DTR/RTS on first open of an AT port */
> +		if (port->start_count == 1 && port->type == WWAN_PORT_AT &&
> +		    port->ops->dtr_rts) {
> +			port->at_data.mdmbits |= TIOCM_DTR | TIOCM_RTS;
> +			port->ops->dtr_rts(port, true);
> +		}
> +	}

[Severity: Medium]
Which lock owns port->at_data.mdmbits now?

wwan_port_op_start() and wwan_port_op_stop() do the read-modify-write while
holding port->ops_lock only, but wwan_port_fops_at_ioctl() reads and updates
the same field under port->data_lock:

	mutex_lock(&port->data_lock);
	...
		if (cmd == TIOCMBIC)
			port->at_data.mdmbits &= ~mdmbits;
		else if (cmd == TIOCMBIS)
			port->at_data.mdmbits |= mdmbits;

The struct comment says that field is covered by data_lock:

	struct mutex data_lock;	/* Port specific data access serialization */

Since a wwan port allows multiple concurrent opens (start_count is a
counter and exclusivity is opt-in via TIOCEXCL), can a close on one fd and
a TIOCMBIS on another fd run at the same time and lose one of the two
updates, leaving TIOCMGET reporting a bitmask that does not match what the
driver was last told?

Along the same lines, can the two paths deliver the assert and de-assert to
->dtr_rts in an order inverted relative to the order in which mdmbits was
committed, for example an ioctl-driven dtr_rts(true) landing after the
last-close dtr_rts(false)/->stop() sequence has already started?

>  
>  out_unlock:
>  	mutex_unlock(&port->ops_lock);
> @@ -773,6 +780,11 @@ static void wwan_port_op_stop(struct wwan_port *port)
>  	mutex_lock(&port->ops_lock);
>  	port->start_count--;
>  	if (!port->start_count) {
> +		/* Mirror TTY semantics: drop DTR/RTS on last close of an AT port */
> +		if (port->ops && port->type == WWAN_PORT_AT && port->ops->dtr_rts) {
> +			port->at_data.mdmbits &= ~(TIOCM_DTR | TIOCM_RTS);
> +			port->ops->dtr_rts(port, false);
> +		}

[Severity: Medium]
Should the WWAN_PORT_AT filter here also apply to the ioctl call site?

wwan_port_fops_ioctl() dispatches into wwan_port_fops_at_ioctl() for both
port types:

	if (port->type == WWAN_PORT_AT || port->type == WWAN_PORT_QCDM) {
		/* AT and QCDM port specific IOCTLs */
		res = wwan_port_fops_at_ioctl(port, cmd, arg);

The new ioctl handling calls ->dtr_rts() with no type gate, while both the
raise in wwan_port_op_start() and this drop are gated on WWAN_PORT_AT.

For a QCDM port, is this sequence possible?

	open("/dev/wwan0qcdm0")
	ioctl(TIOCMSET, &(int){TIOCM_DTR | TIOCM_RTS})  ->  dtr_rts(port, true)
	close(fd)
	  wwan_port_op_stop()
	    /* WWAN_PORT_AT test fails, drop is skipped */
	    port->ops->stop(port);

That leaves the channel torn down with the lines still asserted and no path
that ever calls ->dtr_rts(port, false). The MHI driver added later in this
series has such a port:

	{ .chan = "DIAG", .driver_data = WWAN_PORT_QCDM },

Also, since port->at_data.mdmbits lives in the long-lived wwan_port, does a
re-open of that QCDM port then report a stale TIOCM_DTR|TIOCM_RTS through
TIOCMGET even though the core never re-drove ->dtr_rts, because the
open-time raise is AT-only?

>  		if (port->ops)
>  			port->ops->stop(port);
>  		skb_queue_purge(&port->rxq);
> @@ -1036,6 +1048,10 @@ static long wwan_port_fops_at_ioctl(struct wwan_port *port, unsigned int cmd,
>  			port->at_data.mdmbits |= mdmbits;
>  		else
>  			port->at_data.mdmbits = mdmbits;
> +		if (port->ops->dtr_rts)

[Severity: High]
Can port->ops be NULL here?

wwan_remove_port() clears it under ops_lock while file descriptors are
still open:

	mutex_lock(&port->ops_lock);
	if (port->start_count) {
		port->ops->stop(port);
		port->start_count = 0;
	}
	port->ops = NULL; /* Prevent any new port operations (e.g. from fops) */
	mutex_unlock(&port->ops_lock);

The wwan_port itself survives removal because wwan_port_fops_open() ->
wwan_port_get_by_minor() -> class_find_device() holds a device reference
that is only dropped in wwan_port_fops_release(). So a process that keeps
an fd open on an AT or QCDM chardev across a modem unplug or a driver
unbind, and then issues TIOCMSET/TIOCMBIC/TIOCMBIS, reaches this
dereference with ops == NULL.

Every other ops access in the file is guarded, including the new hunk in
wwan_port_op_stop() above:

	wwan_port_op_start():  if (!port->ops) { ret = -ENODEV; ... }
	wwan_port_fops_poll(): if (port->ops && port->ops->tx_poll)
	wwan_port_op_stop():   if (port->ops && port->type == WWAN_PORT_AT && ...

Separately, this call site holds only data_lock, not ops_lock, whose comment
reads:

	struct mutex ops_lock; /* Serialize ops + protect against removal */

Can ->dtr_rts() therefore run concurrently with, or after, ->stop() and
driver teardown? With the driver added later in this series,
wwan_port_get_drvdata() returns NULL once wwan_port_unregister_wwan() has
done dev_set_drvdata(&port->dev, NULL), so mhi_wwan_ctrl_send_dtr() would
dereference a NULL mhiwwan as well.

> +			port->ops->dtr_rts(port,
> +					   !!(port->at_data.mdmbits &
> +					      (TIOCM_DTR | TIOCM_RTS)));

[Severity: Medium]
Does this reduction lose the independent state of the two lines?

After first open the core sets mdmbits = TIOCM_DTR|TIOCM_RTS. Consider the
usual software hangup:

	ioctl(TIOCMBIC, &(int){TIOCM_DTR})
	  port->at_data.mdmbits &= ~TIOCM_DTR;      /* leaves TIOCM_RTS */
	  !!(mdmbits & (TIOCM_DTR | TIOCM_RTS))     /* still true */
	  port->ops->dtr_rts(port, true);

DTR stays asserted at the driver while TIOCMGET reports it clear. In the
other direction, a TIOCMBIS with TIOCM_RTS alone also yields on = true, and
the consumer added later in this series maps that to both lines:

	mhi_wwan_ctrl_dtr_rts():  on ? TIOCM_DTR | TIOCM_RTS : 0

so DTR gets asserted without userspace asking for it. TTY routes these
ioctls to tty_operations->tiocmset(tty, set, clear) with separate masks for
that reason. Would a set/clear mask pair in the op signature be preferable
here, given that widening it later is an ops-struct change?

Also, ->dtr_rts is invoked on every TIOCM* command even when the resolved
boolean is unchanged. Is the redundant driver-side signalling intended?

>  		break;
>  	}
>  
> diff --git a/include/linux/wwan.h b/include/linux/wwan.h
> index 1e0e2cb53579..a7684950d73b 100644
> --- a/include/linux/wwan.h
> +++ b/include/linux/wwan.h
> @@ -57,6 +57,8 @@ 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 asserts (on=true) or de-asserts (on=false)
> + *           the DTR and RTS modem control lines.

Related to the previous comment: this text describes both lines moving
together, but the ioctls permit mixed states, and the caller passes true
whenever either bit is set. Can the header text and the caller's reduction
both be correct?

>   *
>   * 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 +72,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, bool on);
>  };

[ ... ]
-- 
pw-bot: cr

Re: [PATCH net-next v3 2/3] bus: mhi: host: pci_generic: enumerate IP_CTRL channel for Sierra EM919x/EM929x

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
bus: mhi: host: pci_generic: enumerate IP_CTRL channel for Sierra EM919x/EM929x

This adds the standard Qualcomm MHI control channels 18/19 ("IP_CTRL") to
the Sierra em919x channel configuration, using the control event ring.
The stated purpose is to let the host drive DTR/RTS towards the modem.

> Enumerate it in the Sierra em919x channel config (shared by the EM919x
> and EM929x) using the control event ring. This is the device the change
> was tested against; other Qualcomm modem configs that expose IP_CTRL can
> enable it the same way, since the mhi_wwan_ctrl DTR support that consumes
> it is device-agnostic and binds purely by channel name.

[Severity: Low]
This isn't a bug, but the phrase "the mhi_wwan_ctrl DTR support that
consumes it is device-agnostic and binds purely by channel name" is written
in the present tense, while that consumer only appears later in the same
series.

At this commit, drivers/net/wwan/mhi_wwan_ctrl.c has no IP_CTRL entry in its
mhi_device_id match table and no ->dtr_rts implementation in wwan_pops; both
arrive with "net: wwan: mhi_wwan_ctrl: drive DTR/RTS via the IP_CTRL
channel".

Would it be clearer to phrase this as support added by a following patch in
the series?

For the record, the intermediate state looks harmless: the newly enumerated
channels only create mhi_device instances with no bound driver, and such a
channel is never started because mhi_prepare_for_transfer() is only called
by a client. pci_generic already enumerates channels with no in-tree
consumer for a given config, for example "IPCR" and "IP_SW0".

> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
> index 0d0d9c7ffa4b..ba47dd153837 100644
> --- a/drivers/bus/mhi/host/pci_generic.c
> +++ b/drivers/bus/mhi/host/pci_generic.c
> @@ -728,6 +728,8 @@ static const struct mhi_channel_config mhi_sierra_em919x_channels[] = {
>  	MHI_CHANNEL_CONFIG_DL(13, "MBIM", 128, 0),
>  	MHI_CHANNEL_CONFIG_UL(14, "QMI", 32, 0),
>  	MHI_CHANNEL_CONFIG_DL(15, "QMI", 32, 0),
> +	MHI_CHANNEL_CONFIG_UL(18, "IP_CTRL", 32, 0),
> +	MHI_CHANNEL_CONFIG_DL(19, "IP_CTRL", 32, 0),
>  	MHI_CHANNEL_CONFIG_UL(32, "DUN", 32, 0),
>  	MHI_CHANNEL_CONFIG_DL(33, "DUN", 32, 0),
>  	MHI_CHANNEL_CONFIG_HW_UL(100, "IP_HW0", 512, 1),

Cross-instance finding from sashiko-gemini (7294b6d42c500c3db0e0cd5f3fe50b00b9c09cbd890618c9d32841fb863f7603):
[Severity: High]
Unchecked dereference of `port->ops` in `wwan_port_fops_at_ioctl` can cause a kernel panic during device hot-unplug.

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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: wwan: mhi_wwan_ctrl: drive DTR/RTS via the IP_CTRL channel

This adds a second mhi_driver to mhi_wwan_ctrl that binds the IP_CTRL
channel and keeps a per-controller registry of those devices, so an AT
port can push the host DTR/RTS state to its own modem.  A new
wwan_port_ops->dtr_rts implementation builds a struct mhi_dtr_ctrl_msg
and queues it on that channel.

A few questions below about the new ->dtr_rts path.

> diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
> index a31d8540fbb8..3aef90ec2176 100644
> --- a/drivers/net/wwan/mhi_wwan_ctrl.c
> +++ b/drivers/net/wwan/mhi_wwan_ctrl.c

[ ... ]

> @@ -14,6 +18,30 @@ 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

[ ... ]

> @@ -103,6 +131,56 @@ 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)
> +		return 0; /* IP_CTRL not enumerated; best-effort, not an error */

[Severity: Medium]
Is it intended that this path is completely silent?

For any MHI modem configuration that does not declare an IP_CTRL channel
(the Quectel EM1xx, Foxconn SDX55/SDX61 and Telit FN990/FN920c04 DUN
entries in pci_generic.c, for example) the lookup never finds an entry, so
every DTR request is accepted and discarded without so much as a dev_dbg().
The core has already committed port->at_data.mdmbits by then, so TIOCMGET
reports DTR/RTS asserted while nothing was sent to the modem.

Would a dev_dbg() or a one-time dev_warn() here make the "URCs still not
arriving" case diagnosable?

> +
> +	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;
> +}
> +
> +static void mhi_wwan_ctrl_dtr_rts(struct wwan_port *port, bool on)
> +{
> +	mhi_wwan_ctrl_send_dtr(wwan_port_get_drvdata(port),
> +			       on ? TIOCM_DTR | TIOCM_RTS : 0);
> +}

[Severity: Medium]
Continuing the previous question: the return value of
mhi_wwan_ctrl_send_dtr() is dropped here, including the -ENOMEM path and
every mhi_queue_buf() error.  Those are reachable failures rather than dead
code:

drivers/bus/mhi/host/main.c:mhi_queue() {
	...
	if (unlikely(MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)))
		return -EIO;

	ret = mhi_is_ring_full(mhi_cntrl, tre_ring);
	if (unlikely(ret))
		return -EAGAIN;
	...
}

and mhi_gen_tre() returns -ENODEV when the channel is not
MHI_CH_STATE_ENABLED.

So after a modem SYS_ERR, or with a full UL ring, the host-state message is
dropped with no retry and no message, and the symptom the patch is fixing
comes back silently.  Since the op returns void the error cannot be
propagated, but could it at least be logged the way
mhi_wwan_ctrl_refill_work() logs its queue failure?

[Severity: Medium]
Can DTR and RTS still be driven independently after this?

mhi_wwan_ctrl_send_dtr() maps each line separately (TIOCM_DTR -> BIT(0),
TIOCM_RTS -> BIT(1)) and struct mhi_dtr_ctrl_msg carries both bits, but its
only caller collapses everything into one boolean, so msg can only ever be
DTR|RTS or 0.  The core side does the same collapse:

drivers/net/wwan/wwan_core.c:wwan_port_fops_at_ioctl() {
	...
		if (cmd == TIOCMBIC)
			port->at_data.mdmbits &= ~mdmbits;
	...
		if (port->ops->dtr_rts)
			port->ops->dtr_rts(port,
					   !!(port->at_data.mdmbits &
					      (TIOCM_DTR | TIOCM_RTS)));
	...
}

With RTS asserted, ioctl(fd, TIOCMBIC, &(int){TIOCM_DTR}) - the usual way to
drop DTR so an AT&D1/AT&D2 modem leaves data mode or hangs up - leaves "on"
true, so DTR is re-asserted on the wire while TIOCMGET reports it cleared.
Asserting only RTS likewise raises DTR.

The limitation comes from the void dtr_rts(struct wwan_port *, bool)
signature added in patch 1 rather than from the hardware.  The tty layer
keeps these separate (ops->tiocmset gets the full bitmask, dtr_rts is only
used for open/close/hangup) - would passing the mdmbits mask through be
workable here?

[Severity: Critical]
Is there anything keeping the port drvdata alive across this callback?

wwan_remove_port() clears port->ops under ops_lock:

drivers/net/wwan/wwan_core.c:wwan_remove_port() {
	...
	port->ops = NULL; /* Prevent any new port operations (e.g. from fops) */
	mutex_unlock(&port->ops_lock);
	...
}

but the new TIOCMSET/TIOCMBIC/TIOCMBIS branch in
wwan_port_fops_at_ioctl() dereferences port->ops holding only data_lock,
without ops_lock and without the "if (!port->ops) return -ENODEV;" check the
other fops-reachable op calls (wwan_port_op_tx(), wwan_port_fops_poll()) use.

An fd opened on /dev/wwanXatY stays usable after the device is gone -
wwan_port_fops_open() takes a device reference via
wwan_port_get_by_minor() and only wwan_port_fops_release() drops it - so
after a modem firmware crash, a PCI hot-unplug or rmmod of the controller
driver, a TIOCMSET on that fd reads port->ops as NULL.

The second half is this driver's remove path:

drivers/net/wwan/mhi_wwan_ctrl.c:mhi_wwan_ctrl_remove() {
	wwan_remove_port(mhiwwan->wwan_port);
	kfree(mhiwwan);
}

If the ioctl thread observed port->ops non-NULL just before the store and is
inside mhi_wwan_ctrl_dtr_rts() -> mhi_wwan_ctrl_send_dtr(), the lockset
intersection with wwan_remove_port() is empty (data_lock vs ops_lock), so
mhiwwan is freed while that thread reads mhiwwan->mhi_dev->mhi_cntrl and
mhiwwan->mhi_dev->ul_chan_id and then queues a DMA buffer on behalf of the
removed device.  mhi_wwan_dtr_lock only covers the IP_CTRL registry, not the
AT port's mhi_wwan_dev.

This patch is the first ->dtr_rts implementer, so it is what makes the
use-after-free variant reachable; the unguarded call site itself came in with
patch 1.  Would calling the op under port->ops_lock with the usual NULL check
close both cases?  A bare NULL test alone would only narrow the window.