Re: [PATCH net-next v10 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] A DTR/RTS request made while the IP_CTRL channel is not bound
  is silently lost, and nothing sends it again when IP_CTRL binds later.
- [Low] The DTR/RTS drop on last close (HUPCL) or on port removal is now
  sent asynchronously and coalesced with later state, so the modem may…

--

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, using a
separate mhi_driver. It implements the wwan ->dtr_rts op for AT ports.
Each port records its DTR/RTS state, and a delayed work item sends it over
the IP_CTRL channel of the same controller. The work coalesces bursts and
retries on -EAGAIN/-ENOMEM.

> diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
> index a31d8540fbb85..e3c2075889565 100644
> --- a/drivers/net/wwan/mhi_wwan_ctrl.c
> +++ b/drivers/net/wwan/mhi_wwan_ctrl.c

[ ... ]

> @@ -103,6 +140,80 @@ 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)
> +{

[ ... ]

> +	guard(mutex)(&mhi_wwan_dtr_lock);
> +
> +	list_for_each_entry(dtr, &mhi_wwan_dtr_list, node) {
> +		if (dtr->cntrl == cntrl) {
> +			ctrl_dev = dtr->mhi_dev;
> +			break;
> +		}
> +	}
> +	if (!ctrl_dev) {
> +		dev_dbg(&mhiwwan->mhi_dev->dev,
> +			"IP_CTRL not enumerated; DTR/RTS not signalled to modem\n");
> +		return 0;
> +	}

[Severity: Medium]
If IP_CTRL is not bound when a DTR/RTS request comes in, is that request
lost for good?

mhi_wwan_ctrl_dtr_work() only reschedules on -EAGAIN or -ENOMEM, so it
treats this return 0 as a successful send:

	ret = mhi_wwan_ctrl_send_dtr(mhiwwan, READ_ONCE(mhiwwan->dtr_mdmbits));
	if (ret == -EAGAIN || ret == -ENOMEM)
		schedule_delayed_work(&mhiwwan->dtr_work, msecs_to_jiffies(10));

Later, mhi_wwan_dtr_probe() adds the IP_CTRL entry to mhi_wwan_dtr_list.
Nothing at that point sends the current dtr_mdmbits for the AT ports that
are already open on that controller.

The wwan core won't resend it either. wwan_port_raise_dtr_rts() only runs
on first open from wwan_port_op_start(). The TIOCMSET/TIOCMBIS/TIOCMBIC
handler only calls ->dtr_rts when the cached bits change:

	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 if userspace asks for TIOCM_DTR again, the driver never sees it.

The modem would then keep DTR deasserted and hold back URCs, while
TIOCMGET reports DTR|RTS as asserted.

Some ways to get into this state:

Unbinding and rebinding mhi_wwan_dtr through sysfs while a DUN port is
open. Any request made between mhi_wwan_dtr_remove() and the next probe
is dropped.

An IP_CTRL probe that fails and is retried later.

A controller config that puts IP_CTRL at a higher channel index than DUN.
mhi_create_devices() probes devices in channel-index order.

The commit message says:

    If a send fails because the ring is full or memory is short, the
    work retries shortly with whatever state is latest by then, so the modem
    ends up with the final state that was requested.

As far as I can tell, that only holds for -EAGAIN/-ENOMEM. It does not
cover an IP_CTRL channel that binds late.

Could mhi_wwan_dtr_probe() resync the open AT ports on its controller, or
could this case be handled so the state is sent again once IP_CTRL binds?

> +
> +	dtr_msg = kzalloc_obj(*dtr_msg);
> +	if (!dtr_msg)
> +		return -ENOMEM;

[ ... ]

> +static void mhi_wwan_ctrl_dtr_rts(struct wwan_port *port, unsigned int mdmbits)
> +{
> +	struct mhi_wwan_dev *mhiwwan = wwan_port_get_drvdata(port);
> +
> +	WRITE_ONCE(mhiwwan->dtr_mdmbits, mdmbits);
> +	schedule_delayed_work(&mhiwwan->dtr_work, 0);
> +}

[Severity: Low]
Can the HUPCL drop on last close get lost here?

wwan_port_op_stop() drops DTR/RTS and then stops the port:

	wwan_port_drop_dtr_rts(port, true);
	port->ops->stop(port);

This function only records the bits and schedules the work. The message
is therefore queued later, possibly after mhi_wwan_ctrl_stop() has already
called mhi_unprepare_from_transfer() on the DUN channel. The drop from
wwan_remove_port() is also sent only after ->stop() has run.

Suppose dtr_work is still pending when the port is reopened.
wwan_port_raise_dtr_rts() overwrites dtr_mdmbits with DTR|RTS, and
mhi_wwan_ctrl_dtr_work() only reads the raised value. The drop is never
sent.

The most likely way for the work to be pending is a 10 ms retry armed
after -EAGAIN or -ENOMEM. schedule_delayed_work(..., 0) does nothing to a
delayed work that is already pending, and it does not shorten the delay.

On a modem configured with AT&D2, could a quick close and reopen then
fail to hang up an active data call?

[ ... ]

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