Re: [PATCH net] net: wwan: mhi_wwan_ctrl: keep DTR drops and resend state when IP_CTRL binds

[Loic Poulain]

Hi Peter,

On Fri, Oct 23, 2026 at 7:41 PM Peter Hunt <peter.hunt@xxxxxxxxxxxx> wrote:
>
> Two corner cases in the DTR/RTS coalescing, found by the Sashiko review
> of the series that added it.
>
> If the IP_CTRL channel binds after an AT port has already signalled
> DTR/RTS, for example after a sysfs unbind and rebind of mhi_wwan_dtr or
> a failed IP_CTRL probe that is retried, the state requested while it was
> unbound never reaches the modem, and the wwan core does not send it
> again unless DTR or RTS changes. Keep a list of the driver's ports and,
> when IP_CTRL binds, resend the current state of every port on that
> controller that has signalled DTR/RTS.
>
> Coalescing could also let a DTR drop be overtaken by a later raise, for
> example on a quick close and reopen while a retry was pending, so the
> modem never saw DTR go low. A drop matters on its own, a modem set to
> AT&D2 hangs up on it, so remember a pending drop and send it before the
> latest state. Also run the work straight away on a new request instead
> of waiting for a pending retry.
>
> Fixes: d509edf90bc6 ("net: wwan: mhi_wwan_ctrl: drive DTR/RTS via the IP_CTRL channel")
> Signed-off-by: Peter Hunt <peter.hunt@xxxxxxxxxxxx>
> ---
> Follow-up to the v10 review, as Jakub asked:
> https://lore.kernel.org/all/20261020114405.38a06826@xxxxxxxxxx/
>
> Tested on a Sierra EM9291 (CM8100): IP_CTRL unbind/rebind with DTR
> cleared and set while unbound, back-to-back DTR drop+raise pairs, bursts
> of 10,000 DTR changes, plus the HUPCL, teardown and lock stress tests
> used for v10.

This looks a bit over-engineered for the reported Sashiko corner
cases, and adds quite a bit of complexity. You'll also need to handle
the lifetime of dtr_work and ensure it cannot run after/during device
removal.

I don't have a better solution to propose at the moment, but this
problem suggests that we should enforce some level of ordering between
the MHI devices (channels), or introduce an explicit dependency/link
between them.

Regards,
Loic

>
>  drivers/net/wwan/mhi_wwan_ctrl.c | 50 +++++++++++++++++++++++++++++++-
>  1 file changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
> index e3c207588956..52b6d3677558 100644
> --- a/drivers/net/wwan/mhi_wwan_ctrl.c
> +++ b/drivers/net/wwan/mhi_wwan_ctrl.c
> @@ -15,6 +15,8 @@ enum mhi_wwan_flags {
>         MHI_WWAN_DL_CAP,
>         MHI_WWAN_UL_CAP,
>         MHI_WWAN_RX_REFILL,
> +       MHI_WWAN_DTR_SET,       /* ->dtr_rts has been called for this port */
> +       MHI_WWAN_DTR_DROP,      /* a DTR drop is still to be sent */
>  };
>
>  #define MHI_WWAN_MAX_MTU       0x8000
> @@ -43,6 +45,9 @@ struct mhi_wwan_dtr {
>  };
>
>  static LIST_HEAD(mhi_wwan_dtr_list);
> +/* All ports, so an IP_CTRL channel that binds late can resend their state */
> +static LIST_HEAD(mhi_wwan_port_list);
> +/* Protects mhi_wwan_dtr_list and mhi_wwan_port_list */
>  static DEFINE_MUTEX(mhi_wwan_dtr_lock);
>
>  struct mhi_wwan_dev {
> @@ -71,7 +76,10 @@ struct mhi_wwan_dev {
>          * changes is coalesced and only the final state has to get through
>          */
>         unsigned int dtr_mdmbits;
> +       /* State to send for a DTR drop that a later raise would otherwise hide */
> +       unsigned int dtr_drop_mdmbits;
>         struct delayed_work dtr_work;
> +       struct list_head port_node;
>  };
>
>  /* Increment RX budget and schedule RX refill if necessary */
> @@ -197,6 +205,21 @@ static void mhi_wwan_ctrl_dtr_work(struct work_struct *work)
>                                                     struct mhi_wwan_dev, dtr_work);
>         int ret;
>
> +       /* A DTR drop matters on its own, e.g. to hang up a modem set to AT&D2,
> +        * so send it before the latest state if DTR has been raised again
> +        */
> +       if (test_and_clear_bit(MHI_WWAN_DTR_DROP, &mhiwwan->flags) &&
> +           READ_ONCE(mhiwwan->dtr_mdmbits) & TIOCM_DTR) {
> +               ret = mhi_wwan_ctrl_send_dtr(mhiwwan,
> +                                            READ_ONCE(mhiwwan->dtr_drop_mdmbits));
> +               if (ret == -EAGAIN || ret == -ENOMEM) {
> +                       set_bit(MHI_WWAN_DTR_DROP, &mhiwwan->flags);
> +                       schedule_delayed_work(&mhiwwan->dtr_work,
> +                                             msecs_to_jiffies(10));
> +                       return;
> +               }
> +       }
> +
>         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));
> @@ -209,9 +232,19 @@ 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);
>
> +       if (drop)
> +               WRITE_ONCE(mhiwwan->dtr_drop_mdmbits, mdmbits);
>         WRITE_ONCE(mhiwwan->dtr_mdmbits, mdmbits);
> -       schedule_delayed_work(&mhiwwan->dtr_work, 0);
> +       /* Publish the state before the flags the work tests */
> +       smp_mb__before_atomic();
> +       if (drop)
> +               set_bit(MHI_WWAN_DTR_DROP, &mhiwwan->flags);
> +       set_bit(MHI_WWAN_DTR_SET, &mhiwwan->flags);
> +       /* Run now, even if a retry is already pending */
> +       mod_delayed_work(system_wq, &mhiwwan->dtr_work, 0);
>  }
>
>  static int mhi_wwan_ctrl_start(struct wwan_port *port)
> @@ -357,6 +390,10 @@ static int mhi_wwan_ctrl_probe(struct mhi_device *mhi_dev,
>
>         mhiwwan->wwan_port = port;
>
> +       mutex_lock(&mhi_wwan_dtr_lock);
> +       list_add(&mhiwwan->port_node, &mhi_wwan_port_list);
> +       mutex_unlock(&mhi_wwan_dtr_lock);
> +
>         return 0;
>  };
>
> @@ -365,6 +402,11 @@ static void mhi_wwan_ctrl_remove(struct mhi_device *mhi_dev)
>         struct mhi_wwan_dev *mhiwwan = dev_get_drvdata(&mhi_dev->dev);
>
>         wwan_remove_port(mhiwwan->wwan_port);
> +
> +       mutex_lock(&mhi_wwan_dtr_lock);
> +       list_del(&mhiwwan->port_node);
> +       mutex_unlock(&mhi_wwan_dtr_lock);
> +
>         /* Give the DTR/RTS drop from removal one attempt, then stop the work */
>         flush_delayed_work(&mhiwwan->dtr_work);
>         disable_delayed_work_sync(&mhiwwan->dtr_work);
> @@ -419,6 +461,7 @@ static void mhi_wwan_dtr_dl_xfer_cb(struct mhi_device *mhi_dev,
>  static int mhi_wwan_dtr_probe(struct mhi_device *mhi_dev,
>                               const struct mhi_device_id *id)
>  {
> +       struct mhi_wwan_dev *mhiwwan;
>         struct mhi_wwan_dtr *dtr;
>         int ret;
>
> @@ -449,6 +492,11 @@ static int mhi_wwan_dtr_probe(struct mhi_device *mhi_dev,
>
>         mutex_lock(&mhi_wwan_dtr_lock);
>         list_add(&dtr->node, &mhi_wwan_dtr_list);
> +       /* Resend the state of ports that signalled DTR/RTS before IP_CTRL bound */
> +       list_for_each_entry(mhiwwan, &mhi_wwan_port_list, port_node)
> +               if (mhiwwan->mhi_dev->mhi_cntrl == dtr->cntrl &&
> +                   test_bit(MHI_WWAN_DTR_SET, &mhiwwan->flags))
> +                       mod_delayed_work(system_wq, &mhiwwan->dtr_work, 0);
>         mutex_unlock(&mhi_wwan_dtr_lock);
>
>         return 0;
> --
> 2.43.0
>