Re: [PATCH net-next v6 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 2 potential
issue(s) to consider.

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

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] wwan_port_op_stop() always clears TIOCM_DTR|TIOCM_RTS and
  calls ->dtr_rts() on the last close of an AT port.
- [Low] wwan: inconsistent dtr_rts() bitmask during wwan_remove_port()

--

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 the TIOCM bits of an AT port change through the
ioctl path. The core also raises DTR/RTS on first open and drops them on
last close or port removal, which the commit message describes as
mirroring TTY semantics.

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

[ ... ]

> @@ -773,6 +791,16 @@ 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) {
> +			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);
> +		}

[Severity: Low]
In wwan_remove_port(), port->at_data.mdmbits is updated by clearing
only TIOCM_DTR and TIOCM_RTS. However, ->dtr_rts() is then called with
a literal 0 rather than the resulting bitmask.

The new kernel-doc says the driver receives @mdmbits as a TIOCM_*
bitmask that the modem control lines should match.
wwan_port_op_stop() follows that contract: it snapshots the masked
value under data_lock and passes it on.

Here the driver is told that every bit is clear. Meanwhile the core's
copy still carries whatever else userspace set through TIOCMSET or
TIOCMBIS.

The commit message describes this as mirroring the last-close
de-assert path, but the two paths now give the driver different
values for the same event. The practical impact is small, since the
port is being removed. Still, a driver that derives its state from
the bitmask, as the kernel-doc invites it to, will see one value on
hot-unplug and another on close.

Should this pass the masked port->at_data.mdmbits, snapshotted under
data_lock, as wwan_port_op_stop() does? A small shared helper for the
drop path might keep the two from drifting apart.

[Severity: Medium]
The comment and commit message say this mirrors TTY semantics. Does that
hold for the close path?

In the TTY core, tty_port_shutdown() lowers DTR/RTS only when HUPCL is
set:

drivers/tty/tty_port.c:tty_port_shutdown() {
    ...
	if (tty && C_HUPCL(tty))
		tty_port_lower_dtr_rts(port);
    ...
}

The wwan core already saves termios in port->at_data.termios through
TCSETS/TCSETS2. However, wwan_port_op_stop() never checks c_cflag for
HUPCL. As a result, DTR is dropped on every last close of an AT port, and
userspace cannot prevent this by clearing HUPCL.

Before this series, closing a WWAN AT port never changed the modem's DTR
state. With the follow-up mhi_wwan_ctrl patch, the drop reaches Qualcomm
SDX55/SDX65 modems as an IP_CTRL host-state message with DTR=0.

Take a DUN port with a PPP or circuit-switched call up, on a modem
configured with AT&D1 or AT&D2. Could closing the last fd then put the
modem into command mode or hang up the call, even after HUPCL was cleared
with TCSETS?

Should this path check HUPCL in port->at_data.termios.c_cflag before it
clears TIOCM_DTR and TIOCM_RTS and calls ->dtr_rts()?

>  		if (port->ops)
>  			port->ops->stop(port);
>  		skb_queue_purge(&port->rxq);

[ ... ]

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