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

[netdev-bot+sashiko@kernel.org]

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

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

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

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

--

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

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

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

[ ... ]

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

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

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

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

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

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

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

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

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

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

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

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

[ ... ]

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