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

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

[Peter Hunt]

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@opengear.com>
---
Follow-up to the v10 review, as Jakub asked:
https://lore.kernel.org/all/20261020114405.38a06826@kernel.org/

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.

 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

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@opengear.com> 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@opengear.com>
> ---
> Follow-up to the v10 review, as Jakub asked:
> https://lore.kernel.org/all/20261020114405.38a06826@kernel.org/
>
> 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
>

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

[Peter Hunt]

Hi Loic,

On Sun, Oct 25, 2026 at 3:27 PM Loic Poulain <loic.poulain@oss.qualcomm.com> wrote:
> 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.

Thanks for looking at it. On the lifetime, I think it's covered, but
please tell me if I've missed a path. dtr_work can only be queued from:

- ->dtr_rts(), which the wwan core stops calling once wwan_remove_port()
  has cleared port->ops under ops_lock,
- dtr_work itself, re-arming on -EAGAIN/-ENOMEM,
- mhi_wwan_dtr_probe() (new in this patch), which only walks the port
  list under mhi_wwan_dtr_lock, and remove() takes the port off that
  list under the same lock first.

After that, remove() flushes the work once and calls
disable_delayed_work_sync(), which cancels a pending retry and stops it
re-arming, before the kfree(). On the IP_CTRL side, the work only finds
the channel through mhi_wwan_dtr_list under mhi_wwan_dtr_lock, and
mhi_wwan_dtr_remove() unlists it under that lock before unpreparing.

> 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.

On ordering, here's what I found. On the EM919x/EM929x IP_CTRL is
channels 18/19 and DUN is 32/33, mhi_create_devices() adds the devices
in channel order, and module init registers mhi_wwan_dtr before
mhi_wwan_ctrl. So whether the controller powers up with the module
loaded or the module loads afterwards, IP_CTRL already binds before the
AT port. As far as I can tell the late bind case is only reachable by
unbinding and rebinding mhi_wwan_dtr through sysfs. What I don't know is
whether that's worth an explicit device link, or whether other modems
would declare IP_CTRL after their AT channels.

The DTR drop case is different, it doesn't need any unusual setup. With
v10, a back-to-back drop and raise from userspace (TIOCMBIC then
TIOCMBIS, or a quick close and reopen) was mostly coalesced into just
the raise in my tests, so the modem never saw DTR go low.

So I see three options:

a) Drop this patch and leave both cases as known limitations.
b) A much smaller v2 that only handles the drop: when DTR goes low,
   ->dtr_rts() flushes dtr_work so the drop is sent before it returns
   and a following raise can't overtake it. No new list, flag or
   field, and the late bind case is left to the existing ordering.
c) Keep this patch as it is.

I'd lean towards b), but which would you prefer?

Thanks,
Peter

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

[Loic Poulain]

Hi Peter,

On Mon, Oct 26, 2026 at 4:43 PM Peter Hunt <peter.hunt@opengear.com> wrote:
>
> Hi Loic,
>
> On Sun, Oct 25, 2026 at 3:27 PM Loic Poulain <loic.poulain@oss.qualcomm.com> wrote:
> > 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.
>
> Thanks for looking at it. On the lifetime, I think it's covered, but
> please tell me if I've missed a path. dtr_work can only be queued from:
>
> - ->dtr_rts(), which the wwan core stops calling once wwan_remove_port()
>   has cleared port->ops under ops_lock,
> - dtr_work itself, re-arming on -EAGAIN/-ENOMEM,
> - mhi_wwan_dtr_probe() (new in this patch), which only walks the port
>   list under mhi_wwan_dtr_lock, and remove() takes the port off that
>   list under the same lock first.

Right, it's handled correctly. I overlooked the disable_work cleanup
in the remove path.

>
> After that, remove() flushes the work once and calls
> disable_delayed_work_sync(), which cancels a pending retry and stops it
> re-arming, before the kfree(). On the IP_CTRL side, the work only finds
> the channel through mhi_wwan_dtr_list under mhi_wwan_dtr_lock, and
> mhi_wwan_dtr_remove() unlists it under that lock before unpreparing.
>
> > 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.
>
> On ordering, here's what I found. On the EM919x/EM929x IP_CTRL is
> channels 18/19 and DUN is 32/33, mhi_create_devices() adds the devices
> in channel order, and module init registers mhi_wwan_dtr before
> mhi_wwan_ctrl. So whether the controller powers up with the module
> loaded or the module loads afterwards, IP_CTRL already binds before the
> AT port. As far as I can tell the late bind case is only reachable by
> unbinding and rebinding mhi_wwan_dtr through sysfs. What I don't know is
> whether that's worth an explicit device link, or whether other modems
> would declare IP_CTRL after their AT channels.

Ok thanks for confirming, for now I think we can safely ignore this
corner-case then.

> The DTR drop case is different, it doesn't need any unusual setup. With
> v10, a back-to-back drop and raise from userspace (TIOCMBIC then
> TIOCMBIS, or a quick close and reopen) was mostly coalesced into just
> the raise in my tests, so the modem never saw DTR go low.

Ack.

>
> So I see three options:
>
> a) Drop this patch and leave both cases as known limitations.
> b) A much smaller v2 that only handles the drop: when DTR goes low,
>    ->dtr_rts() flushes dtr_work so the drop is sent before it returns
>    and a following raise can't overtake it. No new list, flag or
>    field, and the late bind case is left to the existing ordering.
> c) Keep this patch as it is.
>
> I'd lean towards b), but which would you prefer?

Yes b) looks good to me, I guess you can also flush dtr work
unconditionally in mhi_wwan_ctrl_stop as well.

Thanks,
Loic