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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>

Reviewed-by: Loic Poulain <loic.poulain@xxxxxxxxxxxxxxxx>

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