Re: [PATCH v9 6/6] net: wwan: t9xx: Add AT & MBIM WWAN ports

["Wu. JackBB (GSM)" <JackBB_Wu@compal.com>]

Thanks for the review. 

On Sun, 4 Oct 2026 17:12:00 +0800 netdev-bot+sashiko@kernel.org wrote:

> Does this make large writes quadratic in the number of fragments?
[...]
> The loop doesn't check for signals and has no size cap. It also runs under
> the core's ops_lock in wwan_port_op_tx(), so other users of the port and
> wwan_remove_port() are blocked while it runs.

Will fix in v10 by capping the message rather than rewriting the copy:
one write() is limited to SKB_LIST_MAX_LEN (16) CCCI packets, i.e.
16 * 3440 bytes, and anything larger returns -EMSGSIZE.  With the
fragment count bounded at 16 the walk costs at most 120 node visits,
the loop runs at most 16 times, and the ops_lock hold time and the
missing signal check both stop mattering.  The same cap is the fix for
the force_send item below, which is where the 16 comes from.

> Can a later packet fail here for some reason other than the channel going
> away?
[...]
> The comment above the function says only failures that mean the channel
> itself is going away can happen after the first submit. Doesn't a
> transient DMA mapping failure, with the channel still enabled afterwards,
> contradict that?

Yes, it does, and the comment is wrong.  Will fix in v10: it is rewritten
to say that once any packet has been submitted the write is not atomic,
that the core has no way to report a short count, and that a transient
HIF failure - the dma_map_single() one you quote being the concrete
case - therefore surfaces as a whole-write error with a prefix already
on the wire.  A ratelimited log naming the port, the bytes already
submitted and the errno goes with it, so a duplicated prefix at the
modem can be matched to a host event.  The cap above bounds the exposure
to 15 packets.

What we cannot do from here is make it atomic, and it is worth saying why
we are not attempting it.  Retrying the failed packet is not possible:
on a submit failure mtk_port_send_data() drops both krefs, and in the
blocking case the completion drops the other one, so the skb is already
freed - a retry would have to rebuild it, which re-introduces the
allocation failure the two-phase build removed.  Submitting the whole
message under one submit_lock acquisition would make the submit step
all-or-nothing but not the mapping, which happens later in the trb_srv
thread, which is exactly your case.  And the tx op returns a plain int
error, so a short count has nowhere to go.  So v10 fixes the claim, not
the property.

> Is there anything that limits how many packets one write() can queue here?
[...]
> At peak the kernel holds about twice the write size (the
> core's skbs plus these copies), and none of it is charged to a memcg.
[...]
> A blocking write can end up the same way. After a signal, each later
> wait_event_interruptible_timeout() in mtk_port_send_data() returns
> -ERESTARTSYS at once. The resulting -EINTR is treated as accepted, so the
> remaining packets are force-queued without waiting.

Will fix in v10 with the cap described above: the first packet still
honours the depth check, so the force_send burst can add at most
SKB_LIST_MAX_LEN - 1 entries, and peak memory becomes 2 * 55040 bytes.
Tying the constant to SKB_LIST_MAX_LEN rather than picking a number
keeps the bound and the limit it protects in one place.

The v8 changelog accepted that phase 1 holds the whole message twice on
the grounds that AT and MBIM control messages are kilobytes; what it
never did was make "kilobytes" an invariant, which is your point.

One thing stays as it is: -EINTR continuing to submit the remaining
packets.  The skb whose wait was interrupted was submitted, so stopping
there would truncate the message on the wire, which is the defect the
two-phase build was added to fix.  With the cap, the post-signal burst
is bounded, which is what was missing.

> Can this deadlock against mtk_pcie_hif_exit()?
[...]
> That is submit_lock -> w_lock, the reverse of the order used here.
[...]
> The comment added in mtk_port_tx_complete() says:
>
>     /* Runs in the trb_srv kthread, so the hook may sleep on a mutex. */
>
> However, the hook also runs under submit_lock during HIF teardown.
[...]
> In the normal FSM_STATE_OFF path the ports are disabled before the trans
> handler runs, so that path should not deadlock. It can still record the
> submit_lock -> w_lock edge whenever a WWAN channel disable fails, which
> would give a lockdep circular dependency report.

Will fix in v10: mtk_port_wwan_tx_pause() takes w_lock twice and does the
HIF query with no lock held, so w_lock is never an outer lock with
respect to submit_lock - txoff under w_lock, drop it, query, then txon
under w_lock if the queue drained.  Both of your reachability notes are
right, including the forced-cleanup path through mtk_pci_dev_exit().

Splitting the section lets a tx_complete land in the gap, so for the
record: if it does and the query then reports not-full we txon as well,
which is a no-op; if the query reports full we return without a second
txoff, leaving TX on while the queue is full, and the next write comes
straight back here with -EAGAIN.  A txoff is never issued after a
not-full observation, so TX cannot be left off with nothing to wake it.
The error falls toward "writable", which is the same direction the
existing comment already argues for.

The comment you quote gains the second half.  It named the context that
permits sleeping and omitted the one that constrains which locks may be
taken, and that constraint is now what the new tx_pause() shape depends
on, so it belongs next to the hook.

> Can a blocking write() get -EAGAIN here?
[...]
> The queue can already be full when a blocking write starts. An earlier
> non-blocking write (on another fd, or on the same fd before fcntl) or an
> interrupted blocking write can leave it that way, because every packet
> after the first skips the limit.
[...]
> Should the blocking path wait for space and retry instead?

Will fix in v10 as "bool force_send = blocking;", which also matches
mtk_port_internal_write(), the driver's other caller of
mtk_port_send_data(), which already passes (blocking, blocking).  In
blocking mode each packet's completion is waited for before the next is
submitted, so back-pressure comes from the wait rather than from
-EAGAIN, and a blocking writer adds at most one entry beyond the depth.

Not waiting and retrying, because there is nothing left to retry with: on
a submit failure mtk_port_send_data() drops both krefs and the skb is
freed, so a retry needs the packet rebuilt from a source the caller no
longer has a cursor into.  It would also mean a second waitqueue at the
port layer for queue occupancy, which the HIF already reports through
txon/tx_complete.  The only unbounded case left is the post-signal one
you describe above, and the cap bounds that.

> Should tx_seq and rx_seq be reset for each modem session?
[...]
> This depends on the modem firmware resetting its CCCI sequence numbers
> across the cycle. The initial rx_seq = -1 suggests it does, but the host
> code can't confirm it. The internal port enable path has the same gap.

Will fix in v10 by resetting both counters at the top of
mtk_port_ch_enable(), which is one site covering every port type and
every enable path - so the internal-port gap you name is covered too.
That puts the change in the control-port patch rather than this one,
since that is where mtk_port_ch_enable() and the sequence check are
introduced.

On the dependency: the fix does not actually depend on what the firmware
does, and that is the argument for making it.  rx_seq = -1 is a sentinel
rather than a guess - rx_seq is unsigned short so it is 0xFFFF, seq_num
comes from a 15-bit field, and for seq_num == 0 the check evaluates
(0 - 0xFFFF) & 0x7fff == 1, which passes.  Resetting to the sentinel
makes the host correct whichever choice the firmware makes.

For completeness on the blast radius: mtk_port_check_rx_seq() assigns
port->rx_seq = seq_num on the drop path, so a stale counter costs exactly
one frame and one dev_warn per session - but that frame can be the
session's first MBIM indication, and the warning blames the modem for a
host bookkeeping error.

> Is tx_mtu guaranteed to be visible at this point?
[...]
> A waiter
> that checks the condition while the kthread is between them can see status
> 0 with tx_mtu still 0. That can happen on the first check after being
> preempted after submit, or on a timeout recheck. On weakly ordered CPUs the
> two stores can also become visible out of order.
[...]
> On the first enable, this would log "Invalid tx_mtu", disable a channel
> that opened successfully, and never create the AT or MBIM device for that
> session.

Will fix in v10 in two halves.  mtk_port_ch_enable() copies the MTU
fields itself after the wait instead of mtk_port_open_trb_complete()
writing them, which makes it the single writer and also removes the late
write on the -ETIMEDOUT path; and mtk_cldma_open() publishes trb->status
with smp_store_release() against an smp_load_acquire() in the waiters,
which mtk_cldma.c already does for cldma_drv_info.  The skb outlives the
wait, so reading trb_open_priv there is safe - mtk_port_ch_enable() holds
its own kref across it.  Both halves land in earlier patches; the
if (!port->tx_mtu) guard here stays, since a genuinely zero MTU from the
open should still be rejected.

> Can a reply to a legitimate command be dropped in this window?
[...]
> The comment before wwan_create_port()
> covers unsolicited RX arriving in between. It does not cover a reply to a
> command sent after a successful open.
[...]
> The start() callback is passed the wwan_port. Could it publish w_port
> itself?

Will fix in v10 exactly that way: mtk_port_wwan_open() assigns
w_priv.w_port under w_lock in the same critical section that sets
PORT_S_OPEN, so mtk_port_wwan_recv() can never see an open port with a
NULL w_port.  mtk_port_common_open() takes no locks of its own, so
wrapping it adds no lock edge, and it mirrors mtk_port_wwan_close(),
which already holds w_lock across mtk_port_common_close().  The
enable-time assignment stays, because mtk_port_wwan_disable() needs
w_port for wwan_remove_port() when userspace never opened the node.

This is the third round on this window and the correction is ours to
make.  The v8 answer was that AT and MBIM are solicited protocols, so the
reply follows a write that follows the open - but a successful open is
already possible inside the window, so the write and the reply can both
happen inside it.  That was the premise of the bug, not a disproof of it.
Publishing from .start is better than either option weighed then: the
state bits still precede wwan_create_port(), so a legitimate open is
never rejected with -ENODEV, and w_port is non-NULL for every open that
succeeds.  The comment is narrowed to what remains true - unsolicited RX
arriving before any open is still dropped by design.

Thanks,
Jack