Re: [PATCH net v2] net: wwan: mhi_wwan_ctrl: send a DTR drop before it can be coalesced

[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] The fix still loses a DTR drop when the transport refuses it.
- [Low] Two existing comments in drivers/net/wwan/mhi_wwan_ctrl.c no
  longer match the code after this patch.

--

net: wwan: mhi_wwan_ctrl: send a DTR drop before it can be coalesced

This patch flushes the DTR/RTS work item in mhi_wwan_ctrl_dtr_rts() when
DTR goes from high to low. The aim is to keep a following raise from
coalescing the drop away. It also flushes the work in mhi_wwan_ctrl_stop()
so that pending state is sent before the channel is stopped.

> diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
> index e3c2075889565..7723199278671 100644
> --- a/drivers/net/wwan/mhi_wwan_ctrl.c
> +++ b/drivers/net/wwan/mhi_wwan_ctrl.c
> @@ -209,9 +209,17 @@ static void mhi_wwan_ctrl_dtr_work(struct work_struct *work)
>  static void mhi_wwan_ctrl_dtr_rts(struct wwan_port *port, unsigned int mdmbits)
>  {
>  	struct mhi_wwan_dev *mhiwwan = wwan_port_get_drvdata(port);
> +	bool drop = (READ_ONCE(mhiwwan->dtr_mdmbits) & TIOCM_DTR) &&
> +		    !(mdmbits & TIOCM_DTR);
>  
>  	WRITE_ONCE(mhiwwan->dtr_mdmbits, mdmbits);
>  	schedule_delayed_work(&mhiwwan->dtr_work, 0);
> +
> +	/* A DTR drop matters on its own, e.g. to hang up a modem set to AT&D2,
> +	 * so send it now rather than let a following raise coalesce it away
> +	 */
> +	if (drop)
> +		flush_delayed_work(&mhiwwan->dtr_work);
>  }

[Severity: Medium]
Can the drop still be lost if the first send attempt fails? On a transient
error, mhi_wwan_ctrl_dtr_work() re-arms itself:

    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));

mhi_queue() returns -EAGAIN when the IP_CTRL UL ring is full. -ENOMEM comes
from the dtr_msg allocation in mhi_wwan_ctrl_send_dtr().

flush_delayed_work() only waits for the instance it queued itself. Its
kernel-doc says it "only considers the last queueing instance". So it
returns after one failed attempt, and the 10ms retry timer is still armed.

>From userspace on the AT port, the sequence would be:

TIOCMBIC(DTR)
  mhi_wwan_ctrl_dtr_rts(port, 0)
    drop = true
    flush_delayed_work()
      mhi_wwan_ctrl_dtr_work() -> -EAGAIN, re-arms 10ms timer

TIOCMBIS(DTR)
  mhi_wwan_ctrl_dtr_rts(port, DTR|RTS)
    drop = false, since dtr_mdmbits is already 0
    WRITE_ONCE(mhiwwan->dtr_mdmbits, DTR|RTS)
    schedule_delayed_work(..., 0) is a no-op, work is already pending

10ms later
  mhi_wwan_ctrl_dtr_work() sends DTR|RTS

Wouldn't the modem then see only the raise? That is the case this patch is
meant to fix. drop is computed against the last requested state, not the
last state that was actually sent.

The commit message says a drop that hits a full UL ring "is still retried
with whatever state is latest by then". The new comment says the drop is
sent "now rather than let a following raise coalesce it away".

Would it be better to keep track of an unsent drop until the transport
accepts it? One option is a drop-pending flag that dtr_work sends before the
latest state. Otherwise, the comment could say this is best effort.

[ ... ]

> @@ -246,6 +254,9 @@ static void mhi_wwan_ctrl_stop(struct wwan_port *port)
>  
>  	cancel_work_sync(&mhiwwan->rx_refill);
>  
> +	/* Send any pending DTR/RTS state before the channel stops */
> +	flush_delayed_work(&mhiwwan->dtr_work);
> +
>  	mhi_unprepare_from_transfer(mhiwwan->mhi_dev);
>  }

The same limitation applies to this flush. After one failed attempt,
mhi_unprepare_from_transfer() runs while the 10ms retry is still armed.

On a plain last close nothing cancels that retry. disable_delayed_work_sync()
is only called from mhi_wwan_ctrl_remove().

Could the DTR message then be queued after the channel named in its dest_id
has been stopped? Could it race with a later reopen?

[Severity: Low]
This isn't a bug, but two existing comments in mhi_wwan_ctrl.c no longer
match the code after this change.

The struct mhi_wwan_dev comment still describes pure coalescing:

	/* Latest DTR/RTS state for the modem, sent from dtr_work so a burst of
	 * changes is coalesced and only the final state has to get through
	 */

mhi_wwan_ctrl_dtr_rts() now flushes each DTR drop so that it is not
coalesced.

mhi_wwan_ctrl_remove() has this comment:

	wwan_remove_port(mhiwwan->wwan_port);
	/* Give the DTR/RTS drop from removal one attempt, then stop the work */
	flush_delayed_work(&mhiwwan->dtr_work);

When start_count is set, wwan_remove_port() calls
wwan_port_drop_dtr_rts(port, false) and then port->ops->stop(). After this
patch both of those paths flush dtr_work, so a removal drop can be tried up
to three times.

Should these two comments be updated to match?

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