[PATCH net-next] net: wwan: mhi_wwan_ctrl: say the DTR drop flush is best effort

[PATCH net-next] net: wwan: mhi_wwan_ctrl: say the DTR drop flush is best effort

[Peter Hunt]

Three comments around the DTR/RTS work don't match the code since
commit 10f51f4fb99d ("net: wwan: mhi_wwan_ctrl: send a DTR drop before
it can be coalesced"):

- the ->dtr_rts() comment says the drop is sent now, but a drop that
  hits a full UL ring is retried with whatever state is latest by then,
- the dtr_mdmbits comment only describes coalescing,
- the remove() comment says the removal drop gets one attempt, but the
  drop and stop() paths now flush the work as well.

Update them. No functional change.

Link: https://lore.kernel.org/all/179340002701.3131.15535297427661129303@kernel.org/
Signed-off-by: Peter Hunt <peter.hunt@opengear.com>
---
 drivers/net/wwan/mhi_wwan_ctrl.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
index 772319927867..3610613e442e 100644
--- a/drivers/net/wwan/mhi_wwan_ctrl.c
+++ b/drivers/net/wwan/mhi_wwan_ctrl.c
@@ -68,7 +68,8 @@ struct mhi_wwan_dev {
 	unsigned int rx_budget;
 
 	/* 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
+	 * changes is coalesced and only the final state has to get through.
+	 * A DTR drop is flushed out on its own, see mhi_wwan_ctrl_dtr_rts().
 	 */
 	unsigned int dtr_mdmbits;
 	struct delayed_work dtr_work;
@@ -216,7 +217,9 @@ static void mhi_wwan_ctrl_dtr_rts(struct wwan_port *port, unsigned int 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
+	 * so try to send it now rather than let a following raise coalesce it
+	 * away. This is best effort, a drop that hits a full UL ring is retried
+	 * with whatever state is latest by then.
 	 */
 	if (drop)
 		flush_delayed_work(&mhiwwan->dtr_work);
@@ -376,7 +379,7 @@ 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);
-	/* Give the DTR/RTS drop from removal one attempt, then stop the work */
+	/* Give any pending DTR/RTS state a last attempt, then stop the work */
 	flush_delayed_work(&mhiwwan->dtr_work);
 	disable_delayed_work_sync(&mhiwwan->dtr_work);
 	kfree(mhiwwan);
-- 
2.43.0

Re: [PATCH net-next] net: wwan: mhi_wwan_ctrl: say the DTR drop flush is best effort

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: wwan: mhi_wwan_ctrl: say the DTR drop flush is best effort

This commit updates three comments around the DTR/RTS delayed work so they
match the retry behaviour added by commit 10f51f4fb99d ("net: wwan:
mhi_wwan_ctrl: send a DTR drop before it can be coalesced"). It changes
no code.

Link: https://lore.kernel.org/all/179340002701.3131.15535297427661129303@kernel.org/

> diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
> index 7723199278671..3610613e442e4 100644
> --- a/drivers/net/wwan/mhi_wwan_ctrl.c
> +++ b/drivers/net/wwan/mhi_wwan_ctrl.c

[ ... ]

> @@ -216,7 +217,9 @@ static void mhi_wwan_ctrl_dtr_rts(struct wwan_port *port, unsigned int 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
> +	 * so try to send it now rather than let a following raise coalesce it
> +	 * away. This is best effort, a drop that hits a full UL ring is retried
> +	 * with whatever state is latest by then.
>  	 */
>  	if (drop)
>  		flush_delayed_work(&mhiwwan->dtr_work);

[Severity: Low]
Should the comment in mhi_wwan_ctrl_stop() get the same treatment? The
patch leaves it as is, and it uses the same flush_delayed_work() pattern:

drivers/net/wwan/mhi_wwan_ctrl.c:mhi_wwan_ctrl_stop() {
    ...
	/* Send any pending DTR/RTS state before the channel stops */
	flush_delayed_work(&mhiwwan->dtr_work);

	mhi_unprepare_from_transfer(mhiwwan->mhi_dev);
}

flush_delayed_work() runs mhi_wwan_ctrl_dtr_work() only once. If
mhi_wwan_ctrl_send_dtr() returns -EAGAIN or -ENOMEM, the work re-arms
itself:

drivers/net/wwan/mhi_wwan_ctrl.c:mhi_wwan_ctrl_dtr_work() {
    ...
	if (ret == -EAGAIN || ret == -ENOMEM)
		schedule_delayed_work(&mhiwwan->dtr_work, msecs_to_jiffies(10));
    ...
}

The flush does not wait for that retry, so mhi_unprepare_from_transfer()
can run before the state goes out. That breaks the "before the channel
stops" promise in the same way the old dtr_rts() comment overstated
things.

The commit message also says "the drop and stop() paths now flush the
work as well", so the stop() path was considered, but its comment was not
updated.

A smaller point: the new comment here gives only a full UL ring as the
retry cause. -ENOMEM from kzalloc_obj() in mhi_wwan_ctrl_send_dtr() is
retried the same way. Would it help to mention both, as the comment above
mhi_wwan_ctrl_dtr_work() already does?

[ ... ]
-- 
pw-bot: cr