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

[Jakub Kicinski <kuba@kernel.org>]

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