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