[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

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.

Critical: 0 · High: 3 · Medium: 2 · 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).

- [High] In drivers/net/wwan/mhi_wwan_ctrl.c, a DTR drop can still be
  hidden by a later raise when the retry that is pending is the retry of…
- [High] The patch queues the same dtr_work on two different workqueues.
- [High] mhi_wwan_ctrl_dtr_work() reads dtr_mdmbits twice.
- [Medium] When IP_CTRL is not bound, mhi_wwan_ctrl_send_dtr() returns 0
  at line 172 ('IP_CTRL not enumerated').
- [Medium] mhi_wwan_ctrl_probe() adds the port to mhi_wwan_port_list
  (line 394) only after wwan_create_port() (line 384) has registered the…
- [Low] Two existing comments in drivers/net/wwan/mhi_wwan_ctrl.c still
  describe latest-state-only delivery.

--

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

This commit keeps a list of mhi_wwan_ctrl ports. When the IP_CTRL channel
binds, it resends the DTR/RTS state of every port that has already
signalled. It also records a pending DTR drop so that a later raise cannot
hide it, and uses mod_delayed_work() so that a new request runs the work
straight away.

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

[ ... ]

> @@ -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;
>  };

[Severity: Low]
This isn't a bug, but the dtr_mdmbits comment still says "only the final
state has to get through". The comment above mhi_wwan_ctrl_dtr_work() also
still says:

/* Send the latest DTR/RTS state. A full UL ring or a failed allocation is
 * retried shortly, with whatever state is latest by then, so the modem ends
 * up with the final state. Other errors mean the channel is going away.
 */

The work now sends dtr_drop_mdmbits before the latest state. On a
transient error it re-arms the pending drop and returns without sending
the latest state.

Should both comments be updated to match?

> @@ -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) {

[Severity: High]
Can a raise that lands between the two reads of dtr_mdmbits in
mhi_wwan_ctrl_dtr_work() still hide the drop?

Whether the drop is sent depends on this READ_ONCE(). The value actually
sent comes from a second READ_ONCE() further down. If DTR is still low at
the first read, the drop branch is skipped and DROP has already been
cleared:

CPU0 (dtr_work)                         CPU1
test_and_clear_bit(DROP) -> true
READ_ONCE(dtr_mdmbits) -> 0, skip
                                        mhi_wwan_ctrl_dtr_rts(DTR|RTS)
                                          drop = false
                                          WRITE_ONCE(dtr_mdmbits, DTR|RTS)
                                          mod_delayed_work(...)
send_dtr(READ_ONCE(dtr_mdmbits))
  sends DTR|RTS
requeued run: DROP clear, sends DTR|RTS

mhi_wwan_ctrl_dtr_rts() computes drop == false because the previous
requested state was already low. Nothing sets DROP again, so the modem
never sees DTR go low.

The worker is preemptible, and dtr_rts() can be called from open, close or
ioctl on any CPU.

Would a single snapshot of dtr_mdmbits, used for both the check and the
send, close this window?

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

[Severity: High]
What happens to the pending drop when the low state itself fails to send
here?

Suppose DTR high has been delivered and the port is closed with HUPCL:

mhi_wwan_ctrl_dtr_rts(0)
  sets MHI_WWAN_DTR_DROP, runs the work
mhi_wwan_ctrl_dtr_work()
  test_and_clear_bit(MHI_WWAN_DTR_DROP) -> DROP now clear
  dtr_mdmbits has no TIOCM_DTR, drop branch skipped
  mhi_wwan_ctrl_send_dtr(0) -> -EAGAIN (UL ring full) or -ENOMEM
  schedule_delayed_work(..., 10ms), DROP still clear

With -EAGAIN, mhi_queue() fails before any transfer descriptor is built, so
the low state never reaches the hardware.

Then suppose the port is reopened within 10ms.
mhi_wwan_ctrl_dtr_rts(DTR|RTS) compares against the last requested state,
which is 0, so drop is false. mod_delayed_work() runs the work at once,
and the work sends only DTR|RTS.

Doesn't this leave the modem going from high to high without seeing DTR
low? That is the "quick close and reopen while a retry was pending" case
the commit message describes as fixed.

Would keeping DROP set until a low state has actually been queued, or
tracking the last delivered state instead of the last requested one,
cover this case?

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

[Severity: High]
Is system_wq intended here? The retry paths in mhi_wwan_ctrl_dtr_work()
still use schedule_delayed_work(), which queues on system_percpu_wq:

include/linux/workqueue.h:schedule_delayed_work() {
	return queue_delayed_work(system_percpu_wq, dwork, delay);
}

In this tree these are separate workqueues, and system_wq is marked
deprecated:

kernel/workqueue.c:workqueue_init_early() {
    ...
	system_wq = alloc_workqueue("events", WQ_PERCPU | __WQ_DEPRECATED, 0);
	system_percpu_wq = alloc_workqueue("events", WQ_PERCPU, 0);
    ...
}

__queue_work() only keeps a work item on the pool where it is already
running if the workqueue matches:

kernel/workqueue.c:__queue_work() {
    ...
		if (worker && worker->current_pwq->wq == wq) {
    ...
}

So if a retry instance of dtr_work is running from system_percpu_wq and
mhi_wwan_ctrl_dtr_rts() queues it on system_wq, can two instances run at
once on different CPUs?

One effect would be out-of-order delivery. An older instance that read a
stale dtr_mdmbits before blocking on mhi_wwan_dtr_lock could send after
the newer one, leaving the modem in the old state.

The other is in mhi_wwan_ctrl_remove():

mhi_wwan_ctrl_remove()
  wwan_remove_port()
    wwan_port_drop_dtr_rts()
      mhi_wwan_ctrl_dtr_rts()
        mod_delayed_work(system_wq, ...)
  flush_delayed_work(&mhiwwan->dtr_work);
  disable_delayed_work_sync(&mhiwwan->dtr_work);
  kfree(mhiwwan);

flush_delayed_work() and disable_delayed_work_sync() only wait on the last
pool recorded for the work. Could kfree(mhiwwan) run while the older retry
instance is still using mhiwwan->mhi_dev, ->flags and ->dtr_work?

Before this patch every call site used schedule_delayed_work(). Would
using system_percpu_wq here and in mhi_wwan_dtr_probe() avoid both
problems, and also the deprecated workqueue warning?

[ ... ]

> @@ -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;
>  };

[Severity: Medium]
Can the resend in mhi_wwan_dtr_probe() miss this port?

By this point wwan_create_port() has registered the character device, so
userspace can open the AT port before list_add() runs:

wwan_port_op_start()
  wwan_port_raise_dtr_rts()
    mhi_wwan_ctrl_dtr_rts()
      set_bit(MHI_WWAN_DTR_SET, ...)
      queues dtr_work
        mhi_wwan_ctrl_send_dtr() -> IP_CTRL not enumerated, returns 0

If mhi_wwan_dtr_probe() for the same controller runs its resend scan in
that window, the port is not yet on mhi_wwan_port_list. The state is then
never sent, and the wwan core does not signal again unless DTR or RTS
changes.

mhi_dev, flags and dtr_work are all set up before wwan_create_port().
Could the list_add() move ahead of wwan_create_port(), with a list_del()
on its error path?

[ ... ]

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

[Severity: Medium]
Can a drop requested while IP_CTRL was unbound be lost before this resend
runs?

While mhi_wwan_dtr is unbound, mhi_wwan_ctrl_send_dtr() returns 0 without
queueing anything:

	if (!ctrl_dev) {
		dev_dbg(&mhiwwan->mhi_dev->dev,
			"IP_CTRL not enumerated; DTR/RTS not signalled to modem\n");
		return 0;
	}

By then mhi_wwan_ctrl_dtr_work() has already cleared MHI_WWAN_DTR_DROP,
and it only sets the bit again for -EAGAIN or -ENOMEM. So a close (drop)
followed by a reopen (raise) while unbound leaves DROP clear.

After a rebind, this loop requeues the work, which sends only the latest
dtr_mdmbits. The modem sees just DTR high.

This resend appears to assume the modem keeps its last DTR state while
IP_CTRL is absent. If so, would it ever see the drop?

In that case the comment on MHI_WWAN_DTR_DROP ("a DTR drop is still to be
sent") would not be accurate either.

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

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

[Peter Hunt]

Thanks for the review. This version is superseded by v2:
https://lore.kernel.org/all/20261027171055.1796482-1-peter.hunt@opengear.com/

After Loic's review, v2 only flushes dtr_work when DTR goes low and in
stop(). It drops the port list, the drop flag and field, and the late
bind resend. Going through the findings against v2:

[High] Two reads of dtr_mdmbits in the work: gone, v2 leaves the work as
it was in v10, so it reads dtr_mdmbits once.

[High] Drop lost when the low state hits -EAGAIN/-ENOMEM and a raise
follows within the retry delay: this can still happen in v2. The flush
makes the drop go out before ->dtr_rts() returns in the normal case,
but a drop that hits a full UL ring is still retried with whatever
state is latest by then. The v2 commit message says so.

[High] system_wq vs system_percpu_wq: agreed, that was a real problem
in v1. v2 has no mod_delayed_work() calls, it only queues the work with
schedule_delayed_work() as v10 does, and adds flush_delayed_work().

[Medium] Port added to the list after wwan_create_port(), and [Medium]
drop lost while IP_CTRL is unbound: both are in the late bind resend,
which v2 drops. Loic agreed that case, only reachable by unbinding and
rebinding mhi_wwan_dtr through sysfs, can be ignored.

[Low] Stale comments: the comments describe v10's latest-state work,
which v2 keeps, and the drop flush has its own comment.

pw-bot: cr