Re: [PATCH net-next v6 2/2] net: wwan: mhi_wwan_ctrl: drive DTR/RTS via the IP_CTRL channel

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 · Low: 0

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] The patch does not reach its stated goal on any in-tree
  hardware.
- [Medium] mhi_wwan_dtr_dl_xfer_cb()
  (drivers/net/wwan/mhi_wwan_ctrl.c:351-364) discards the return value…

--

net: wwan: mhi_wwan_ctrl: drive DTR/RTS via the IP_CTRL channel

This patch adds a second mhi_driver to mhi_wwan_ctrl. The new driver
binds the IP_CTRL MHI channel and records it per controller in a small
registry. AT ports implement ->dtr_rts by sending a host serial state
message over their own controller's IP_CTRL channel. A single recycled
DL sink buffer stays queued on IP_CTRL so the modem's transmit path does
not stall.

> diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
> index a31d8540fbb85..b94484a1657d7 100644
> --- a/drivers/net/wwan/mhi_wwan_ctrl.c
> +++ b/drivers/net/wwan/mhi_wwan_ctrl.c

[ ... ]

> @@ -255,6 +340,86 @@ static void mhi_wwan_ctrl_remove(struct mhi_device *mhi_dev)

[ ... ]

> +static void mhi_wwan_dtr_dl_xfer_cb(struct mhi_device *mhi_dev,
> +				    struct mhi_result *mhi_result)
> +{
> +	struct mhi_wwan_dtr *dtr = dev_get_drvdata(&mhi_dev->dev);
> +
> +	/* Channel is being torn down (e.g. -ENOTCONN), do not requeue */
> +	if (mhi_result->transaction_status &&
> +	    mhi_result->transaction_status != -EOVERFLOW)
> +		return;
> +
> +	/* Modem serial state not needed, requeue the sink buffer to keep DL ring live */
> +	mhi_queue_buf(mhi_dev, DMA_FROM_DEVICE, dtr->rx_buf,
> +		      sizeof(*dtr->rx_buf), MHI_EOT);

[Severity: Medium]
What happens if this mhi_queue_buf() fails? The return value is ignored,
and dtr->rx_buf is the only DL buffer. It is queued once in
mhi_wwan_dtr_probe() and requeued only from here.

parse_xfer_event() has already consumed the ring element before this
callback runs. A failed requeue therefore leaves the IP_CTRL DL ring
empty, and nothing retries until the device is unbound and probed again.

mhi_queue() can fail here in three ways:

- -EIO in an MHI PM error state
- -ENODEV from mhi_gen_tre() when the channel is not enabled
- -ENOMEM from map_single

The map_single call in mhi_gen_tre() looks like this:

mhi_gen_tre()
    if (!info->pre_mapped) {
        ret = mhi_cntrl->map_single(mhi_cntrl, buf_info);
        if (ret)
            goto out;
    }

mhi_map_single_no_bb() returns -ENOMEM on dma_mapping_error().
mhi_map_single_use_bb() calls dma_alloc_coherent(..., GFP_ATOMIC) from
this completion path.

Couldn't one transient mapping or allocation failure here cause the
IP_CTRL transmit stall that the sink buffer is meant to prevent? Should
the error at least be logged, and the refill retried later, for example
from a work item?

> +}

[ ... ]

> @@ -278,7 +443,45 @@ static struct mhi_driver mhi_wwan_ctrl_driver = {
>  	},
>  };
>  
> -module_mhi_driver(mhi_wwan_ctrl_driver);
> +static const struct mhi_device_id mhi_wwan_dtr_match_table[] = {
> +	{ .chan = "IP_CTRL" },
> +	{},
> +};

[Severity: High]
Does any in-tree MHI controller declare an "IP_CTRL" channel? This match
table is the only place the name appears as a channel.

MHI client devices are only created for channels in the controller's
static config, because mhi_create_devices() walks mhi_cntrl->mhi_chan.
The SDX55/SDX65 tables in drivers/bus/mhi/host/pci_generic.c don't list
IP_CTRL. For example:

drivers/bus/mhi/host/pci_generic.c:
static const struct mhi_channel_config mhi_sierra_em919x_channels[] = {
	...
	MHI_CHANNEL_CONFIG_UL(32, "DUN", 32, 0),
	MHI_CHANNEL_CONFIG_DL(33, "DUN", 32, 0),
	MHI_CHANNEL_CONFIG_HW_UL(100, "IP_HW0", 512, 1),
	MHI_CHANNEL_CONFIG_HW_DL(101, "IP_HW0", 512, 2),
};

The same is true of modem_qcom_v1 and the Foxconn, Quectel and Telit
tables.

If so, mhi_wwan_dtr_probe() never runs and mhi_wwan_dtr_list stays empty.
Every ->dtr_rts call then ends up in this branch of
mhi_wwan_ctrl_send_dtr(), whether it comes from wwan core open, close,
remove or TIOCMSET/TIOCMBIS/TIOCMBIC:

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

TIOCMGET would then report DTR as asserted, but the modem never receives
it. The URCs described in the commit message would still be withheld.

Neither this patch nor "net: wwan: core: propagate modem control signals
to port drivers" adds IP_CTRL UL/DL entries to pci_generic.c. Is a
controller-side change needed with this patch? If not, could the commit
message at least note that dependency?

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