Re: [PATCH v9 3/6] net: wwan: t9xx: Add control DMA interface

["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:

> Can a device overrun a single BD skb here?
[...]
> Should recv_len also be clamped to req->frag_size?
>
> BD mode is not configured today, because Q_MTU_3_5K == Q_FRAG_3_5K keeps
> nr_bds at 0. This path becomes reachable as soon as a queue uses
> mtu > frag_size.

Will fix in v10: allow_len is bounded by the host-known buffer size
before it is used, with
min_t(u32, le16_to_cpu(...data_allow_len), req->frag_size), and the
comment is corrected to say the per-BD bound is that minimum.

> What brings this slot back after the goto out?
[...]
> If no further completion arrives at all, doesn't RX stay stalled even
> after memory pressure clears? Nothing appears to implement the "clean
> stall until modem reset recovery" in the comment.

Will fix in v10.  Both halves are right, and we traced the first one:
the slot is left with skb set, HWO clear and data_recv_len 0, which the
next pass reads as a completed descriptor, so a zero-length skb goes up
and the port layer drops it with a warning that blames the modem for a
host allocation failure.

v10 frees the un-mappable skb and leaves req->skb NULL so the slot is
unambiguously empty, and turns the existing !req->skb dev_err branch
into a reload-and-rearm retry, so the queue refills itself on any later
completion.  The comment is replaced with one that describes that,
rather than a modem reset recovery that does not exist.

> Is the need_exit check atomic with respect to mtk_cldma_rxq_free()?
[...]
> mtk_cldma_rxq_free() does not stop the queue again after flush_work(). Can
> the engine then be running on GPDs with HWO set while their buffers and
> descriptors are being unmapped and freed?

Will fix in v10, exactly as your last sentence suggests: mtk_cldma_rxq_free()
issues mtk_cldma_stop_queue(DIR_RX) a second time after synchronize_irq()
and flush_work(), with the existing -ETIMEDOUT path (leave the ring
allocated rather than free it under a live engine) covering a device
that does not acknowledge.  flush_work() proves the worker finished, not
that it did not resume the engine on its way out, so the stop has to be
the thing that terminates it; need_exit stays only as an early-out.

> What happens if free_idx names a GPD the hardware has already completed
> (HWO=0) but mtk_cldma_tx_done_work() has not reclaimed yet?
[...]
> That never moves the hardware cursor. Could the pending slots between the
> old free_idx and wr_idx then stay unprocessed, draining req_budget to zero
> and stalling TX? This depends on how the hardware handles RESUME at an
> HWO=0 descriptor.

Will fix in v10.  free_idx is the reclaim cursor and recovery runs
exactly when the done-work is behind, so this is the normal case rather
than a rare one.  mtk_cldma_rearm_queues() now only programs the start
address and issues START when that descriptor still has HWO set;
otherwise it clears tx_started and lets the next mtk_cldma_start_xfer()
do a fresh START from the then-current free_idx.  Interrupts are
unmasked either way.

We did not need to settle the RESUME question - the start address is
wrong under both readings.

> Is it safe to flush here without stopping the TX queue first?
[...]
> Should this path also stop the queue and clear tx_started before calling
> mtk_cldma_txq_flush()?

Will fix in v10: this path is brought in line with the err_work you
quote - stop the queue, keep the requests and bail out if the stop
fails, clear tx_started under ring_lock, then flush.  A leaked request
is recoverable; unmapping a buffer the device is still writing is not.

We are not adding err_work's is_stopping flag here: it excludes a
concurrent producer, and in this path the caller is the only producer.

> Are RST0_SET and RST0_CLR write-1-to-set and write-1-to-clear registers?
[...]
> If these registers read back the current reset status, the
> read-modify-write on RST0_CLR would release every other block in bank 0
> that is currently held in reset. Would writing just the CLDMA bit be
> safer?
>
> Separately, udelay(1) follows a posted write with no read-back. Is the
> reset assert width actually guaranteed?

No change in v10 on either point.

On the second, the premise does not hold.  mtk_pci_read32() is
ioread32(), and a PCIe read is non-posted, so the read of RST0_CLR
between the udelay(1) and the CLR write already flushes the posted SET
write.  The assert width is the udelay plus one read round trip, not an
unbounded race.

On the first, the read-modify-write and the single-bit write produce the
same value here.  These two lines are the only writes to this bank in
the driver; the only bits they ever assert are the two CLDMA bits, and
each is released a microsecond later.  The callers are serialised - one
TRB service thread drives the close path, and teardown runs under
submit_lock - so the two instances are never in reset at the same time.
Whichever way the registers read back, the value written is the CLDMA
bit alone.

> Should usr_cnt be rolled back when the ENABLE is rejected with -EINVAL?
[...]
> This path leaves the increment in place. One leaked increment keeps
> usr_cnt at 1 or more. The real owner's DISABLE would then never reach
> mtk_cldma_close(), and later ENABLEs would keep failing until
> mtk_pcie_hif_init() resets the count.

No change here, because the increment is not leaked - it is consumed by
the caller, and rolling it back as well would be the bug.

usr_cnt is not balanced inside mtk_ch_status_check(); it is balanced
across the ENABLE/DISABLE pair, and the port layer always issues the
matching DISABLE.  Both call sites look like this (mtk_port_io.c, in
"net: wwan: t9xx: Add control port"):

    ret = mtk_port_ch_enable(port);
    if (ret && ret != -EBUSY) {
            mtk_port_ch_disable(port);
            return;
    }

-EBUSY is the "another user already owns this queue" success case and is
deliberately not unwound.  Every other failure, including the -EINVAL
from mtk_cldma_check_ch_cfg() this comment is about, issues a DISABLE,
and the DISABLE path decrements the count.

If mtk_ch_status_check() decremented too, one failed ENABLE would
decrement twice.  With two users that takes usr_cnt from 2 to 0, and the
next DISABLE - from the other user, whose channel is still open - would
reach mtk_cldma_close() and tear down rings that are in use.  That
failure is worse and much harder to see than a stuck count.

mtk_cldma_open() is the opposite case rather than a contradiction: it
runs only after mtk_ch_status_check() has already returned success, so
the caller is no longer treating the ENABLE as failed and
mtk_cldma_open() has to unwind its own increment itself.

What we will add is a comment above the increment saying it is balanced
by the caller's DISABLE and not by this function.  The two halves of the
pair live in different files and different patches, and this is the
third review round in which they have been read apart - the pairing is
genuinely not visible from here, which is the part of your comment we
agree with.

> Once mtk_cldma_submit_tx() has published the skb to the ring,
> mtk_cldma_tx_done_work() can complete it through trb_complete(). The
> handler then still uses the skb for skb_queue_is_last(), __skb_unlink()
> and mtk_cldma_trb_process(trans->dev, skb). What keeps it alive?
[...]
> Would it be clearer to take that reference here, where the code is
> introduced?

Will fix in v10, but by moving the member later rather than the
kref_get() earlier.  A get at this commit would reference a kref that
nothing has initialised and nothing releases - kref_init() and the
kref_put() that frees the skb both arrive with the port patch - so it
would be a leak rather than a protection, on a path you have already
shown is unreachable here.

The real oddity is the other way round: struct trb carries its kref
member in this patch while nothing touches it until the next one.  v10
moves the member to the port patch, so the field, the init, the get in
mtk_ctrl_trb_handler() and the put all appear in one commit.

> What happens to queued TRBs if kthread_stop() arrives before this thread
> has run for the first time?
[...]
> Would flushing in mtk_ctrl_trb_srv_exit() after kthread_stop() cover this?

Will fix in v10, that way: mtk_ctrl_chs_flush() moves out of the thread
and into mtk_ctrl_trb_srv_exit(), after kthread_stop() and before
kfree(srv).  The thread is gone at that point and available was already
cleared, so there is no concurrent producer or consumer and no new
locking is needed; on the normal path it finds an empty list.

The error unwind in mtk_pcie_hif_init() is the likelier way in - a
failure right after kthread_run() stops the thread before it has run -
and the same move covers it.

> Can this free struct trb_srv while the CLDMA completion paths still
> dereference it?
[...]
> The comment "The service may already have been torn down" treats the NULL
> check as sufficient, but nothing orders that check against the free.
>
> Would it be safer to quiesce the CLDMA IRQ and workers before freeing the
> service?

Will fix in v10, and yes - the quiesce is the answer, which is worth
spelling out because we got this wrong once already.  You reported the
same defect against v8; we fixed it by swapping the two calls, and had
to put the order back in v9 because the other order lets the TRB threads
dereference trans->dev after mtk_cldma_exit() has freed it.  Both
orderings are broken, so there is no ordering that fixes it.

v10 hoists the quiesce prologue that mtk_cldma_dev_exit() already
performs - mask the queue interrupts, unregister and synchronize_irq(),
flush the workqueue - into a mtk_cldma_quiesce() called before
mtk_ctrl_trb_srv_exit().  No worker can then load trb_srv[], and
trans->dev still outlives every thread that reads it.

That change lands in "net: wwan: t9xx: Add FSM thread", which is the
patch that adds the IRQ and the workqueues; at this commit there is
nothing yet to quiesce, so the hunk you quote is correct for the code
that exists here.

> Does this walk hold up the promise in the comment when an ENABLE sits
> behind a TX entry?
[...]
> Should the DISABLE go after the last pending ENABLE for this queue
> instead?

Will fix in v10, that way: the walk now records the last ENABLE in the
list and uses __skb_queue_after() on it, falling back to
__skb_queue_head() when there is none.  Stopping at the first
non-ENABLE implements a weaker rule than the comment states, and your
TX, ENABLE example shows the inversion.

The fallback changes from tail to head deliberately: with no pending
ENABLE there is nothing for the DISABLE to stay behind, and queueing it
at the tail makes teardown wait for the TX backlog, which the same
comment says it must not.

One qualification: the -EBUSY-against-usr_cnt-0 outcome you describe is
not reachable from the port layer today, since a DISABLE only comes from
an owner whose ENABLE has completed.  We are fixing it anyway - the
comment states an invariant the loop does not provide, and we would
rather not have its correctness depend on a calling pattern in another
patch.

> Is it safe to call radix_tree_delete() from inside
> radix_tree_for_each_slot() in mtk_ctrl_remove_radix_tree()?
[...]
> radix_tree_iter_delete() exists for deleting at the iterator
> position, and it moves the iterator past a freed node. Would that
> work here? Or, since this is new code, would an xarray with
> xa_for_each() and xa_erase() be simpler?

Will fix in v10 with the xarray, for the reason in your last sentence:
this is new code, so there is nothing to keep us on the older interface,
and xa_for_each() plus xa_erase() is correct by construction instead of
by picking the right delete helper.  trans->queue_tbl becomes a struct
xarray, with xa_init()/xa_insert()/xa_load() at the eight call sites in
mtk_trans_ctrl.c and mtk_cldma.c.

Your reachability note is right and we had missed it: 0x1000 and 0x2000
are in different leaf nodes, so the first delete frees the node the
iterator is standing in on every teardown and every failed init, not
just in a corner case.

Thanks,
Jack