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

[PATCH net-next v7 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 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

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 | 239 ++++++++++++++++++++++++++++++-
 drivers/net/wwan/wwan_core.c     |  61 +++++++-
 include/linux/wwan.h             |   3 +
 3 files changed, 301 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH net-next v7 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 full TIOCM bitmask so they can assert
or de-assert DTR and RTS independently. The wwan core tracks the full TIOCM
bitmask in port->at_data.mdmbits and calls ->dtr_rts when it 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 mirrors TTY
semantics 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.

at_data.mdmbits is protected by data_lock. In the ioctl path the ->dtr_rts
call is deferred until ops_lock is held, where mdmbits is re-read under
data_lock, so the value passed to the driver always reflects the committed
bitmask under ops_lock and is serialised against concurrent ioctls and
against port removal (which nulls port->ops under ops_lock).

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. Both drop paths share a helper and pass the driver the resulting
bitmask.

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

diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
index ffbcf11e4e68..eba4b0582b5a 100644
--- a/drivers/net/wwan/wwan_core.c
+++ b/drivers/net/wwan/wwan_core.c
@@ -655,6 +655,10 @@ struct wwan_port *wwan_create_port(struct device *parent,
 	init_waitqueue_head(&port->waitqueue);
 	mutex_init(&port->data_lock);
 
+	/* AT ports hang up on last close by default, as a TTY does */
+	if (type == WWAN_PORT_AT)
+		port->at_data.termios.c_cflag = HUPCL;
+
 	port->dev.parent = &wwandev->dev;
 	port->dev.type = &wwan_port_dev_type;
 	dev_set_drvdata(&port->dev, drvdata);
@@ -679,12 +683,34 @@ struct wwan_port *wwan_create_port(struct device *parent,
 }
 EXPORT_SYMBOL_GPL(wwan_create_port);
 
+/* 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, on port
+ * removal they are always dropped.
+ */
+static void wwan_port_drop_dtr_rts(struct wwan_port *port, bool hupcl_only)
+{
+	unsigned int bits;
+
+	mutex_lock(&port->data_lock);
+	if (hupcl_only && !(port->at_data.termios.c_cflag & HUPCL)) {
+		mutex_unlock(&port->data_lock);
+		return;
+	}
+	port->at_data.mdmbits &= ~(TIOCM_DTR | TIOCM_RTS);
+	bits = port->at_data.mdmbits;
+	mutex_unlock(&port->data_lock);
+
+	port->ops->dtr_rts(port, bits);
+}
+
 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) {
+		if (port->type == WWAN_PORT_AT && port->ops->dtr_rts)
+			wwan_port_drop_dtr_rts(port, false);
 		port->ops->stop(port);
 		port->start_count = 0;
 	}
@@ -759,8 +785,20 @@ 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) {
+			unsigned int bits;
+
+			mutex_lock(&port->data_lock);
+			port->at_data.mdmbits |= TIOCM_DTR | TIOCM_RTS;
+			bits = port->at_data.mdmbits;
+			mutex_unlock(&port->data_lock);
+			port->ops->dtr_rts(port, bits);
+		}
+	}
 
 out_unlock:
 	mutex_unlock(&port->ops_lock);
@@ -773,6 +811,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 HUPCL is set
+		 */
+		if (port->ops && port->type == WWAN_PORT_AT && port->ops->dtr_rts)
+			wwan_port_drop_dtr_rts(port, true);
 		if (port->ops)
 			port->ops->stop(port);
 		skb_queue_purge(&port->rxq);
@@ -980,6 +1023,7 @@ static long wwan_port_fops_at_ioctl(struct wwan_port *port, unsigned int cmd,
 				    unsigned long arg)
 {
 	int ret = 0;
+	bool call_dtr_rts = false;
 
 	mutex_lock(&port->data_lock);
 
@@ -1036,6 +1080,8 @@ 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->type == WWAN_PORT_AT)
+			call_dtr_rts = true;
 		break;
 	}
 
@@ -1061,6 +1107,19 @@ static long wwan_port_fops_at_ioctl(struct wwan_port *port, unsigned int cmd,
 
 	mutex_unlock(&port->data_lock);
 
+	if (call_dtr_rts) {
+		unsigned int bits;
+
+		mutex_lock(&port->ops_lock);
+		if (port->ops && port->ops->dtr_rts) {
+			mutex_lock(&port->data_lock);
+			bits = port->at_data.mdmbits;
+			mutex_unlock(&port->data_lock);
+			port->ops->dtr_rts(port, bits);
+		}
+		mutex_unlock(&port->ops_lock);
+	}
+
 	return ret;
 }
 
diff --git a/include/linux/wwan.h b/include/linux/wwan.h
index 1e0e2cb53579..57406139304e 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 updates the modem control lines to match
+ *           @mdmbits (a TIOCM_* bitmask). Only called for WWAN_PORT_AT ports.
  *
  * 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, unsigned int mdmbits);
 };
 
 /** struct wwan_port_caps - The WWAN port capbilities
-- 
2.43.0

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

[Loic Poulain]

Hi Peter,

On Tue, Oct 6, 2026 at 6:05 AM 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 full TIOCM bitmask so they can assert
> or de-assert DTR and RTS independently. The wwan core tracks the full TIOCM
> bitmask in port->at_data.mdmbits and calls ->dtr_rts when it 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 mirrors TTY
> semantics 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.
>
> at_data.mdmbits is protected by data_lock. In the ioctl path the ->dtr_rts
> call is deferred until ops_lock is held, where mdmbits is re-read under
> data_lock, so the value passed to the driver always reflects the committed
> bitmask under ops_lock and is serialised against concurrent ioctls and
> against port removal (which nulls port->ops under ops_lock).
>
> 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. Both drop paths share a helper and pass the driver the resulting
> bitmask.
>
> Signed-off-by: Peter Hunt <peter.hunt@opengear.com>
> ---
> 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 | 61 +++++++++++++++++++++++++++++++++++-
>  include/linux/wwan.h         |  3 ++
>  2 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> index ffbcf11e4e68..eba4b0582b5a 100644
> --- a/drivers/net/wwan/wwan_core.c
> +++ b/drivers/net/wwan/wwan_core.c
> @@ -655,6 +655,10 @@ struct wwan_port *wwan_create_port(struct device *parent,
>         init_waitqueue_head(&port->waitqueue);
>         mutex_init(&port->data_lock);
>
> +       /* AT ports hang up on last close by default, as a TTY does */
> +       if (type == WWAN_PORT_AT)
> +               port->at_data.termios.c_cflag = HUPCL;
> +
>         port->dev.parent = &wwandev->dev;
>         port->dev.type = &wwan_port_dev_type;
>         dev_set_drvdata(&port->dev, drvdata);
> @@ -679,12 +683,34 @@ struct wwan_port *wwan_create_port(struct device *parent,
>  }
>  EXPORT_SYMBOL_GPL(wwan_create_port);
>
> +/* 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, on port
> + * removal they are always dropped.
> + */
> +static void wwan_port_drop_dtr_rts(struct wwan_port *port, bool hupcl_only)
> +{
> +       unsigned int bits;
> +
> +       mutex_lock(&port->data_lock);
> +       if (hupcl_only && !(port->at_data.termios.c_cflag & HUPCL)) {
> +               mutex_unlock(&port->data_lock);

Using (scoped) guard version would be simpler:
scoped_guard(mutex, &port->data_lock) {

> +               return;
> +       }
> +       port->at_data.mdmbits &= ~(TIOCM_DTR | TIOCM_RTS);
> +       bits = port->at_data.mdmbits;
> +       mutex_unlock(&port->data_lock);
> +
> +       port->ops->dtr_rts(port, bits);
> +}
> +
>  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) {
> +               if (port->type == WWAN_PORT_AT && port->ops->dtr_rts)

I would recommend checking for the dtr_rts callback in
wwan_port_drop_dtr_rts() rather than here.

> +                       wwan_port_drop_dtr_rts(port, false);
>                 port->ops->stop(port);
>                 port->start_count = 0;
>         }
> @@ -759,8 +785,20 @@ 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) {
> +                       unsigned int bits;
> +
> +                       mutex_lock(&port->data_lock);
> +                       port->at_data.mdmbits |= TIOCM_DTR | TIOCM_RTS;
> +                       bits = port->at_data.mdmbits;
> +                       mutex_unlock(&port->data_lock);
> +                       port->ops->dtr_rts(port, bits);

Could we add a wwan_port_raise_dtr_rts() counterpart to
wwan_port_drop_dtr_rts() and move this AT/TTY-specific handling there?

> +               }
> +       }
>
>  out_unlock:
>         mutex_unlock(&port->ops_lock);
> @@ -773,6 +811,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 HUPCL is set
> +                */
> +               if (port->ops && port->type == WWAN_PORT_AT && port->ops->dtr_rts)
> +                       wwan_port_drop_dtr_rts(port, true);
>                 if (port->ops)
>                         port->ops->stop(port);
>                 skb_queue_purge(&port->rxq);
> @@ -980,6 +1023,7 @@ static long wwan_port_fops_at_ioctl(struct wwan_port *port, unsigned int cmd,
>                                     unsigned long arg)
>  {
>         int ret = 0;
> +       bool call_dtr_rts = false;
>
>         mutex_lock(&port->data_lock);
>
> @@ -1036,6 +1080,8 @@ 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->type == WWAN_PORT_AT)
> +                       call_dtr_rts = true;
>                 break;
>         }
>
> @@ -1061,6 +1107,19 @@ static long wwan_port_fops_at_ioctl(struct wwan_port *port, unsigned int cmd,
>
>         mutex_unlock(&port->data_lock);
>
> +       if (call_dtr_rts) {
> +               unsigned int bits;
> +
> +               mutex_lock(&port->ops_lock);
> +               if (port->ops && port->ops->dtr_rts) {
> +                       mutex_lock(&port->data_lock);
> +                       bits = port->at_data.mdmbits;
> +                       mutex_unlock(&port->data_lock);
> +                       port->ops->dtr_rts(port, bits);
> +               }
> +               mutex_unlock(&port->ops_lock);
> +       }
> +
>         return ret;
>  }
>
> diff --git a/include/linux/wwan.h b/include/linux/wwan.h
> index 1e0e2cb53579..57406139304e 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 updates the modem control lines to match
> + *           @mdmbits (a TIOCM_* bitmask). Only called for WWAN_PORT_AT ports.
>   *
>   * 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, unsigned int mdmbits);
>  };
>
>  /** struct wwan_port_caps - The WWAN port capbilities
> --
> 2.43.0
>

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] [Baseline: false positive, kept only because
  requires_validation=true] The concern was that the new optional op…
- [Low] wwan_port_fops_at_ioctl() handles TIOCMSET/TIOCMBIS/TIOCMBIC by
  copying a raw user-supplied int with copy_from_user().
- [Low] A failed TCSETS/TCSETSW/TCSETSF/TCSETS2/TCSETSW2/TCSETSF2 ioctl
  can turn off the new hang-up-on-close behaviour.
- [Low] On every WWAN_PORT_AT port, TIOCMSET/TIOCMBIC/TIOCMBIS now take
  port->ops_lock with an uninterruptible mutex_lock().

--

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

This patch adds an optional ->dtr_rts() operation to struct wwan_port_ops.
The core calls it from the TIOCMSET/TIOCMBIC/TIOCMBIS ioctl path for AT
ports. It also raises DTR/RTS on first open, drops them on last close if
HUPCL is set (HUPCL now defaults to on for AT ports), and always drops them
when an open port is removed.

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

[ ... ]

> @@ -679,12 +683,34 @@ struct wwan_port *wwan_create_port(struct device *parent,
>  }
>  EXPORT_SYMBOL_GPL(wwan_create_port);
>  
> +/* 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, on port
> + * removal they are always dropped.
> + */
> +static void wwan_port_drop_dtr_rts(struct wwan_port *port, bool hupcl_only)
> +{
> +	unsigned int bits;
> +
> +	mutex_lock(&port->data_lock);
> +	if (hupcl_only && !(port->at_data.termios.c_cflag & HUPCL)) {
> +		mutex_unlock(&port->data_lock);
> +		return;
> +	}

[Severity: Low]
Can a failed TCSETS ioctl quietly turn off the hang-up on last close?

wwan_port_fops_at_ioctl() copies directly into the live termios:

	case TCSETS:
	case TCSETSW:
	case TCSETSF:
		if (copy_from_user(&port->at_data.termios, (void __user *)arg,
				   sizeof(struct termios)))
			ret = -EFAULT;
		break;

On a fault, copy_from_user() zero-fills the bytes it could not copy. With a
bad pointer this clears c_cflag, and so HUPCL, and the ioctl returns -EFAULT
without restoring the old settings. The TCSETS2/TCSETSW2/TCSETSF2 cases
behave the same way.

On last close, wwan_port_op_stop()->wwan_port_drop_dtr_rts(port, true) then
sees HUPCL clear and returns early. DTR/RTS stay asserted even though no
ioctl ever cleared HUPCL successfully.

The direct copy is older than this patch, but this patch is the first to
let c_cflag control the modem lines. Could the termios ioctls copy into a
temporary ktermios and commit it only on success?

> +	port->at_data.mdmbits &= ~(TIOCM_DTR | TIOCM_RTS);
> +	bits = port->at_data.mdmbits;
> +	mutex_unlock(&port->data_lock);
> +
> +	port->ops->dtr_rts(port, bits);
> +}

[ ... ]

> @@ -1036,6 +1080,8 @@ 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->type == WWAN_PORT_AT)
> +			call_dtr_rts = true;
>  		break;
>  	}

[Severity: Low]
Should the user value be masked before it is stored and passed to
->dtr_rts()?

mdmbits comes straight from copy_from_user(). It is merged into
port->at_data.mdmbits unchanged and later passed as-is to
port->ops->dtr_rts(). That means TIOCM_LOOP, the input-only bits
(TIOCM_CAR, TIOCM_CTS, TIOCM_DSR, TIOCM_RNG) and undefined bits all reach
the driver. So does the sign bit after the int to unsigned int conversion.

By comparison, tty_tiocmset() in drivers/tty/tty_io.c masks the value
before calling the driver:

	set &= TIOCM_DTR|TIOCM_RTS|TIOCM_OUT1|TIOCM_OUT2|TIOCM_LOOP;
	clear &= TIOCM_DTR|TIOCM_RTS|TIOCM_OUT1|TIOCM_OUT2|TIOCM_LOOP;

Also, call_dtr_rts is set on every TIOCMSET/TIOCMBIS/TIOCMBIC, even when
DTR/RTS do not change. The commit message says:

  The wwan core tracks the full TIOCM bitmask in port->at_data.mdmbits
  and calls ->dtr_rts when it changes

Is that accurate? With the mhi_wwan_ctrl user added later in the series,
each of these ioctls allocates and queues a new IP_CTRL message.

The kernel-doc in include/linux/wwan.h only describes @mdmbits as "a
TIOCM_* bitmask". Could the core filter out the extra bits and the
unchanged-state calls? If not, could the kernel-doc say which bits drivers
may see and that calls can be redundant?

[ ... ]

> @@ -1061,6 +1107,19 @@ static long wwan_port_fops_at_ioctl(struct wwan_port *port, unsigned int cmd,
>  
>  	mutex_unlock(&port->data_lock);
>  
> +	if (call_dtr_rts) {
> +		unsigned int bits;
> +
> +		mutex_lock(&port->ops_lock);
> +		if (port->ops && port->ops->dtr_rts) {

[Severity: Low]
Could this make TIOCMSET/TIOCMBIC/TIOCMBIS sleep uninterruptibly on AT
ports whose driver does not implement ->dtr_rts?

ops_lock is taken before port->ops->dtr_rts is checked, and
wwan_port_op_tx() holds ops_lock across the driver's blocking 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);

rpmsg_wwan_ctrl registers AT ports and implements .tx_blocking with
rpmsg_send(), which sleeps until the remote has TX space. It does not
implement ->dtr_rts:

thread A
write()
  wwan_port_op_tx()
    mutex_lock(&port->ops_lock)
    rpmsg_wwan_ctrl_tx_blocking()
      rpmsg_send()    <- waiting on the remote

thread B
ioctl(TIOCMBIS)
  wwan_port_fops_at_ioctl()
    mutex_lock(&port->ops_lock)    <- blocks until A is done
    port->ops->dtr_rts is NULL, nothing to call

Before this patch, these ioctls only took data_lock and returned at once.
Could ops_lock be skipped when no ->dtr_rts can apply, or taken with
mutex_lock_interruptible()?

> +			mutex_lock(&port->data_lock);
> +			bits = port->at_data.mdmbits;
> +			mutex_unlock(&port->data_lock);
> +			port->ops->dtr_rts(port, bits);
> +		}
> +		mutex_unlock(&port->ops_lock);
> +	}
> +
>  	return ret;
>  }
>  
> diff --git a/include/linux/wwan.h b/include/linux/wwan.h
> index 1e0e2cb535799..57406139304ea 100644
> --- a/include/linux/wwan.h
> +++ b/include/linux/wwan.h

[ ... ]

> @@ -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, unsigned int mdmbits);
>  };

[Severity: Low]
This isn't a bug, but this patch adds ->dtr_rts with no in-tree
implementer. On its own it does not fix the problem the commit message
describes, where modems withhold unsolicited result codes until DTR is
asserted.

The series covers this. The next patch, "net: wwan: mhi_wwan_ctrl: drive
DTR/RTS via the IP_CTRL channel", adds .dtr_rts = mhi_wwan_ctrl_dtr_rts to
wwan_pops. mhi_wwan_ctrl_stop() only tears down the DUN data channel and
sends nothing on IP_CTRL. So with HUPCL cleared, DTR also stays asserted
across close for that driver, as the commit message says.

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

[PATCH net-next v7 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 (patch 1) 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. Delivery is best-effort: at_data.mdmbits always reflects the committed
userspace intent for TIOCMGET regardless of whether the IP_CTRL message was
queued, and the next ioctl or open/close cycle will resynchronise the modem
state. If requeueing the DL sink buffer fails with a transient -ENOMEM it
is retried from a work item, so the DL ring does not stay empty.

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

diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
index a31d8540fbb8..feb9c03b6392 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;
@@ -103,6 +134,61 @@ 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) {
+		dev_dbg(&mhiwwan->mhi_dev->dev,
+			"failed to queue DTR/RTS signal: %d\n", ret);
+		kfree(dtr_msg);
+	}
+
+	return ret;
+}
+
+static void mhi_wwan_ctrl_dtr_rts(struct wwan_port *port, unsigned int mdmbits)
+{
+	mhi_wwan_ctrl_send_dtr(wwan_port_get_drvdata(port), mdmbits);
+}
+
 static int mhi_wwan_ctrl_start(struct wwan_port *port)
 {
 	struct mhi_wwan_dev *mhiwwan = wwan_port_get_drvdata(port);
@@ -163,6 +249,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 +342,118 @@ 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);
+}
+
+/* Requeue the single DL sink buffer so the IP_CTRL DL ring never runs empty.
+ * A transient mapping failure is retried from a work item, other errors mean
+ * the channel is going away and the next probe queues a fresh buffer.
+ */
+static void mhi_wwan_dtr_queue_rx(struct mhi_wwan_dtr *dtr)
+{
+	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_refill_work(struct work_struct *work)
+{
+	struct mhi_wwan_dtr *dtr = container_of(to_delayed_work(work),
+						struct mhi_wwan_dtr, rx_refill);
+
+	mhi_wwan_dtr_queue_rx(dtr);
+}
+
+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 */
+	mhi_wwan_dtr_queue_rx(dtr);
+}
+
+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);
+
+	ret = mhi_queue_buf(mhi_dev, DMA_FROM_DEVICE, dtr->rx_buf,
+			    sizeof(*dtr->rx_buf), MHI_EOT);
+	if (ret)
+		goto err_unprepare;
+
+	mutex_lock(&mhi_wwan_dtr_lock);
+	list_add(&dtr->node, &mhi_wwan_dtr_list);
+	mutex_unlock(&mhi_wwan_dtr_lock);
+
+	return 0;
+
+err_unprepare:
+	mhi_unprepare_from_transfer(mhi_dev);
+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);
+
+	mhi_unprepare_from_transfer(mhi_dev);
+	/* No DL callbacks after unprepare, and a pending retry now fails on
+	 * the disabled channel without rescheduling
+	 */
+	cancel_delayed_work_sync(&dtr->rx_refill);
+	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 +477,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 v7 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 4 potential
issue(s) to consider.

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

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

- [High] Race in mhi_wwan_dtr_remove() (drivers/net/wwan/mhi_wwan_ctrl.c)
  between the rx_refill retry worker and channel teardown.
- [Low] mhi_wwan_dtr_probe() unprepares the channels twice on its
  err_unprepare path.
- [Low] The commit message names its prerequisite only as "The wwan core
  (patch 1)".
- [Low] Concern: mhi_wwan_ctrl_dtr_rts() drops the result of
  mhi_wwan_ctrl_send_dtr(), and failed sends are not retried.

--

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

This commit adds a second mhi_driver to mhi_wwan_ctrl. It binds the IP_CTRL
MHI channel and uses it to send DTR/RTS host state messages to the modem.
AT ports implement the new ->dtr_rts wwan op by finding their own
controller's IP_CTRL device in a small registry. A single recycled DL sink
buffer keeps the IP_CTRL DL ring live, and a delayed work item retries the
requeue if it fails.

> The wwan core (patch 1) 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.

[Severity: Low]
This isn't a bug, but "patch 1" only means something inside the posted
series.

Could this name the wwan core commit by subject, the same way the MHI tree
dependency is cited as commit 83c29a55b89e? The ->dtr_rts op and the
open/close/HUPCL handling seem to come from "net: wwan: core: propagate
modem control signals to port drivers".

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

[ ... ]

> @@ -103,6 +134,61 @@ 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)
> +{

[ ... ]

> +	if (!ctrl_dev) {
> +		dev_dbg(&mhiwwan->mhi_dev->dev,
> +			"IP_CTRL not enumerated; DTR/RTS not signalled to modem\n");
> +		return 0;
> +	}

[ ... ]

> +	ret = mhi_queue_buf(ctrl_dev, DMA_TO_DEVICE, dtr_msg, sizeof(*dtr_msg),
> +			    MHI_EOT);
> +	if (ret) {
> +		dev_dbg(&mhiwwan->mhi_dev->dev,
> +			"failed to queue DTR/RTS signal: %d\n", ret);
> +		kfree(dtr_msg);
> +	}
> +
> +	return ret;
> +}
> +
> +static void mhi_wwan_ctrl_dtr_rts(struct wwan_port *port, unsigned int mdmbits)
> +{
> +	mhi_wwan_ctrl_send_dtr(wwan_port_get_drvdata(port), mdmbits);
> +}

[Severity: Low]
mhi_wwan_ctrl_dtr_rts() throws away the return value of
mhi_wwan_ctrl_send_dtr(). Nothing retries a failed send, whether it is a
kzalloc failure or -EAGAIN/-EIO/-ENODEV/-ENOMEM from mhi_queue_buf().

mhi_wwan_ctrl_send_dtr() also returns 0 when IP_CTRL is not bound yet.
mhi_wwan_dtr_probe() does not replay the DTR/RTS state of AT ports that are
already open.

In these cases, can TIOCMGET (at_data.mdmbits) show DTR as asserted when the
modem never got the message? URCs would then stay withheld until the next
TIOCM ioctl or reopen.

The commit message calls this best-effort, and acm_port_dtr_rts() in cdc-acm
also only logs a failed acm_set_control(). Since mhi_wwan_dtr_driver is
registered first, the probe replay case seems to need a sysfs
unbind/rebind. Is that the intended tradeoff?

[ ... ]

> +static int mhi_wwan_dtr_probe(struct mhi_device *mhi_dev,
> +			      const struct mhi_device_id *id)
> +{

[ ... ]

> +	ret = mhi_queue_buf(mhi_dev, DMA_FROM_DEVICE, dtr->rx_buf,
> +			    sizeof(*dtr->rx_buf), MHI_EOT);
> +	if (ret)
> +		goto err_unprepare;
> +
> +	mutex_lock(&mhi_wwan_dtr_lock);
> +	list_add(&dtr->node, &mhi_wwan_dtr_list);
> +	mutex_unlock(&mhi_wwan_dtr_lock);
> +
> +	return 0;
> +
> +err_unprepare:
> +	mhi_unprepare_from_transfer(mhi_dev);

[Severity: Low]
Does this path unprepare the IP_CTRL channels twice? mhi_probe() in
drivers/bus/mhi/host/init.c already unprepares them whenever the client
probe fails:

	ret = mhi_drv->probe(mhi_dev, mhi_dev->id);
	if (ret)
		goto exit_probe;
	...
exit_probe:
	mhi_unprepare_from_transfer(mhi_dev);

On the second pass the channels are already DISABLED. mhi_unprepare_channel()
then logs "Failed to reset channel, still resetting" for each direction, in
addition to the real queue failure.

There is no double free, because mhi_deinit_chan_ctxt() returns early when
rbase is 0. The cost is only misleading log output.

Could the error path skip mhi_unprepare_from_transfer() and leave it to the
core?

> +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);
> +
> +	mhi_unprepare_from_transfer(mhi_dev);
> +	/* No DL callbacks after unprepare, and a pending retry now fails on
> +	 * the disabled channel without rescheduling
> +	 */
> +	cancel_delayed_work_sync(&dtr->rx_refill);

[Severity: High]
Can the rx_refill worker race with the channel teardown here? The comment
covers a retry that starts after unprepare. What about a retry that is
already inside mhi_queue_buf()?

Suppose mhi_wwan_dtr_queue_rx() got -ENOMEM earlier and armed rx_refill. The
worker can then be inside mhi_queue(), between mhi_gen_tre() and the
doorbell:

mhi_wwan_dtr_refill_work()
  mhi_wwan_dtr_queue_rx()
    mhi_queue_buf()
      mhi_queue()
        mhi_gen_tre()        <- succeeds, drops mhi_chan->lock
        <preempted>

At the same time, mhi_wwan_dtr_remove() runs on another CPU:

mhi_unprepare_from_transfer()
  mhi_unprepare_channel()
    mhi_reset_chan()
    mhi_deinit_chan_ctxt()
      buf_ring->base = tre_ring->base = NULL;
      tre_ring->ctxt_wp = NULL;

When the worker resumes, MHI_DB_ACCESS_VALID() is still true because the
device is powered. mhi_ring_chan_db() then does:

	db = ring->iommu_base + (ring->wp - ring->base);
	...
	*ring->ctxt_wp = cpu_to_le64(db);

Wouldn't this write through a NULL pointer from the kworker?
mhi_unprepare_channel() takes mhi_chan->mutex, which mhi_queue() never takes,
so nothing seems to serialize the two.

mhi_wwan_ctrl_stop() in the same file does the steps in the opposite order:

	cancel_work_sync(&mhiwwan->rx_refill);

	mhi_unprepare_from_transfer(mhiwwan->mhi_dev);

Should the work be stopped first, for example with
disable_delayed_work_sync() before mhi_unprepare_from_transfer()? That would
also stop a late DL callback from re-arming it.

> +	kfree(dtr->rx_buf);
> +	kfree(dtr);
> +}

[ ... ]

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