Re: [PATCH net-next v9 2/2] net: wwan: mhi_wwan_ctrl: drive DTR/RTS via the IP_CTRL channel

[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] When a DTR/RTS message fails to send, nothing retries it, and
  the wwan core's change-only filter stops userspace from re-sending it.
- [Low] (Baseline verification: the main trigger is disproven; only a
  root-only sysfs corner case remains.) The concern says an AT port can…

--

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

This patch makes mhi_wwan_ctrl also bind the IP_CTRL MHI channel and uses
it to send the host DTR/RTS state to the modem, so SDX55/SDX65 modems
deliver URCs on the AT ports. A per-controller registry maps each AT port
to the IP_CTRL device of its own modem, and a new ->dtr_rts op sends the
line state, while a recycled DL sink buffer keeps the IP_CTRL DL ring
live.

> diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
> index a31d8540fbb85..1cbbfcb3b920a 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;
> +	}
> +
> +	dtr_msg = kzalloc_obj(*dtr_msg);
> +	if (!dtr_msg)
> +		return -ENOMEM;

[ ... ]

> +	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: Medium]
What happens to the modem's line state when this send fails?

mhi_wwan_ctrl_send_dtr() can fail with -ENOMEM from kzalloc_obj(). It can
also fail from mhi_queue_buf() with -EAGAIN (UL ring full), -EIO (MHI
error state), -ENODEV, or a map_single error. In each case the message is
freed, and mhi_wwan_ctrl_dtr_rts() ignores the return value.

Nothing retries the send. The DL sink path in this same patch does retry
-ENOMEM.

The wwan core has already stored the new value in port->at_data.mdmbits
before it calls ->dtr_rts:

drivers/net/wwan/wwan_core.c:wwan_port_raise_dtr_rts() {
    ...
	port->at_data.mdmbits |= TIOCM_DTR | TIOCM_RTS;
	port->ops->dtr_rts(port, port->at_data.mdmbits);
}

The TIOCMSET/TIOCMBIC/TIOCMBIS handler only calls the driver when the
bits change:

drivers/net/wwan/wwan_core.c:wwan_port_fops_at_ioctl() {
    ...
		/* 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);
    ...
}

So after a lost DTR raise, TIOCMGET reports DTR as asserted, but the modem
still sees it de-asserted and keeps withholding +CREG/+DMI. A repeated
TIOCMBIS(TIOCM_DTR), or a TIOCMSET with the same bits, never reaches the
driver.

Only clearing and then setting DTR, or a full last-close/first-open cycle,
brings the modem back in step. A long-lived opener such as ModemManager
rarely does either.

This also seems to conflict with the commit message:

    the next ioctl or open/close cycle will resynchronise the modem
    state.

That only holds for an ioctl that actually changes DTR or RTS. The
companion wwan core commit also says that the TIOCM state and the line
state sent to the driver cannot get out of step.

Could a failed send be retried from a work item, as the DL refill is? Or
could the driver track the last state the modem accepted, so that a later
ioctl can resend it?

[ ... ]

> +	mutex_lock(&mhi_wwan_dtr_lock);
> +	list_add(&dtr->node, &mhi_wwan_dtr_list);
> +	mutex_unlock(&mhi_wwan_dtr_lock);
> +
> +	return 0;

[ ... ]

> +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);
> +
> +	/* Stop the refill work, and stop a late DL callback re-arming it, before
> +	 * the channel is torn down so no requeue can race with the teardown
> +	 */
> +	disable_delayed_work_sync(&dtr->rx_refill);
> +	mhi_unprepare_from_transfer(mhi_dev);

[Severity: Low]
This is a narrow corner case, but can the IP_CTRL device and the open AT
ports get out of step across a sysfs unbind and rebind of mhi_wwan_dtr?

The normal boot and hot-plug order is fine. mhi_create_devices() probes
channels synchronously in channel-number order, so IP_CTRL (18/19) binds
before DUN (32/33). mhi_wwan_ctrl_init() also registers
mhi_wwan_dtr_driver first.

Suppose root unbinds mhi_wwan_dtr while a DUN port is open. Then
mhi_wwan_dtr_remove() tears down the channel without first sending DTR/RTS
low. The drop on last close finds no ctrl_dev in mhi_wwan_ctrl_send_dtr()
and returns 0 without sending anything.

On a later rebind, mhi_wwan_dtr_probe() adds the registry entry but does
not replay the stored mdmbits of AT ports that are already open. The core
filters ioctls that do not change the bits, so TIOCMBIS(TIOCM_DTR) cannot
recover this either.

The ordering assumption above also does not hold when the async_probe
module parameter is used.

During mhi_destroy_devices(), IP_CTRL is also removed before DUN, so the
final drop does nothing. The modem is being reset or powered off at that
point anyway.

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