[PATCH v8 5/6] net: wwan: t9xx: Add FSM thread

[Wu. JackBB (GSM)]

On Sun, 20 Sep 2026 07:54:00 +0800 Jakub Kicinski wrote:

> Can this record of hif_err ever be cleared again?

Will fix in v9.  hif_err becomes one slot per producer (HIF init, CLDMA0,
CLDMA1), each recorded unconditionally so a successful init writes 0 over
its own previous failure, and the READY check walks the array.

> The same applies to mtk_cldma_fsm_state_listener(), which records a CLDMA
> init failure on an HS1 transition; if a later HS1 retry initializes CLDMA
> successfully, hif_err stays set.

This half is the reachable one, and it is what the fix is for.  The HS1
retry exists: mtk_fsm_startup_act() accepts an event while already in
FSM_STATE_BOOTUP, and its hs_err path re-arms both handshake channels
precisely so the device can send HS1 again.

> Because the transitions run in the FSM kthread, mtk_pci_dev_start() and
> probe still return success, so the driver stays bound with a device parked
> in FSM_STATE_BOOTUP forever. Should a successful (re-)init clear hif_err,
> or should the failure be surfaced to probe?

A successful re-init clearing it, not surfacing to probe.  For the
FSM_STATE_ON listener you quote first, though, the current behaviour is
already right: mtk_fsm_dev_add_act() refuses any state but OFF or
INVALID, and it is the only path into FSM_STATE_ON, so that listener can
never run against a stale hif_err and is never retried without another
DEV_ADD - which zeroes it again.  A failure there is a real one and
refusing READY is the intended outcome.  Only the CLDMA producer can
latch a stale value, and the per-producer slots fix that without
changing the ON case.

> Is this read guaranteed to be aligned?

Will fix in v9 by marking struct runtime_feature_entry __packed, which
covers the two reads and the writes in mtk_fsm_append_rtft_entries() in
one annotation.

> offset advances by sizeof(*rtft_entry) + data_len, and data_len comes
> from the device, so an odd first payload length leaves the next entry's
> data_len member at an odd address. Would get_unaligned_le32() (or copying
> each wire entry into aligned storage) be better here for
> strict-alignment architectures?

Both work; we picked __packed because it is one annotation on a wire
structure whose sibling is already __packed, rather than a rule each new
access site has to remember.  We checked that it does not move anything:
feature_id + support_info + reserved[2] + data_len is 8 bytes with no
padding either way, so sizeof() stays 8 and the two bounds checks
written in terms of sizeof(*rtft_entry) are unaffected.

> This adds a userspace-visible interface that the changelog does not
> mention.

Will fix in v9.  The commit message will give the id, the exact payload
format and that it is informational only.

> Once merged, udev rules and modem managers can parse that format, so it
> becomes ABI that cannot be changed later. Should this be described and
> justified in the commit message, and could the state be reported through
> the wwan framework instead?

Described and justified, yes.  Reported through the wwan framework, no -
there is nothing there to report it through.  The framework gives a
driver two things: ports, via wwan_create_port(), and netdevs, via
wwan_register_ops().  Neither carries a device lifecycle state, and
there is no wwan-level notification channel a driver can raise one on.
Adding one would be a core change proposed from inside a driver
submission, which we did not think was ours to make.

If netdev would rather have that core addition than a raw uevent, we
will follow that - it is a better end state than every modem driver
inventing its own string.  We are asking rather than assuming because
the uevent is what the existing userspace for this device consumes
today.

> The new enum in mtk_utility.h also pre-declares values nothing emits:

Will fix in v9 by removing MTK_UEVENT_MINIDUMP and MTK_UEVENT_LOWPOWER.
They belong with the crash-dump and low-power series that emit them.

> Does this early return leave the boot-flow channel masked forever?

Will fix in v9.  The -ENXIO return becomes a goto exit, keeping the
dev_err, so the channel is re-armed on this path like every other.

> Was the asymmetry intentional, or should the invalid-state case also
> goto exit?

It should, and it was not intentional - the comment above the exit:
label already states the invariant this path breaks, that the device
notifies once per boot stage.  last_dev_state stays unlatched, so the
next notification is not filtered out, which is the same rule the IDLE
path follows.

> Could the commit message cover the scope of these CLDMA changes?

Will fix in v9.  The message will name the ISR and that it is what first
dispatches the done workers, the QUEUE_ERROR worker and its -EPIPE
completion policy, the ring_leaked rule, and the teardown changes.

> It also removes an unrelated register define:

Will fix in v9, but in patch 3/6: that define is added there and has no
reader anywhere, so it is dropped at the point it is introduced and the
removal hunk disappears from this patch.  Removing in 5/6 what 3/6 adds
is a cross-patch edit we should not have left in.

> Would splitting the CLDMA interrupt/error-recovery work into its own
> patch make these decisions easier to review and bisect?

No change planned, because neither half of the split builds a working
commit.

mtk_cldma_isr() is registered by mtk_cldma_dev_init(), and
mtk_cldma_dev_init() is called only from mtk_cldma_fsm_state_listener()
on the FSM_STATE_BOOTUP transition.  Both the listener and the
transition arrive with this patch.  So an earlier patch carrying the
interrupt and error-recovery work would add an ISR, a worker and a
recovery policy that nothing can reach - dead code in that commit - and
a later one would leave this patch introducing an FSM that drives a
CLDMA bring-up which does not yet handle its own interrupts.  Either
way one commit in the series is not functional on its own, which is the
property the split is meant to improve.

The scope problem itself is real and we are not arguing with it; the
commit message above is where we think it gets fixed.

> Can err_work and the per-queue done worker run concurrently for the same
> queue and issue contradictory commands to it?

Yes.  Will fix in v9 by giving each direction one owner: on TX the error
worker sets is_stopping under ring_lock for the whole stop-through-flush
and start_xfer honours it under the same lock; on RX the error worker
stops issuing queue commands at all and hands the queue to
rx_done_work, which is the only code that may program it.

That RX half is also the fix for the two findings below, so all three
are one change.

> Should the error worker record a persistent stopping state (is_stopping /
> need_exit) and hold ring_lock across stop-through-flush, and should the RX
> cldma_stop_queue() return value be checked rather than discarded?

is_stopping and the ring_lock, yes, and the return value will be checked
like the TX branch already does.  not need_exit, though: it means "this
queue is going away" and rx_done_work passes it to rxq->rx_done() as the
drop flag, so reusing it for a recoverable error would silently drop
packets.  You are right that is_stopping is not doing anything today -
it is written once in txq_alloc and read once in mtk_cldma_tx(), and
nothing ever sets it true.

> Is 4-byte alignment sufficient for GPDs? Every address from this pool is
> programmed into CLDMA UL/SO start-address and next-GPD registers, and the
> existing in-tree CLDMA driver for the same descriptor layout uses 16:

Will fix in v9: both pools go to 16.

> Does the T9xx DMA engine tolerate GPD addresses that are only 4- or
> 8-byte aligned?

We have no document that says it does, so we are not going to defend 4.
Worth recording that the condition is real rather than latent: sizeof
is 24 for both descriptor types, which is not a multiple of 16, so with
align 4 consecutive blocks land on 16n and 16n+8 alternately and the
pool really does hand out 8-byte-aligned GPDs today.

> Should this publication use smp_store_release()?

Will fix in v9: smp_store_release() at the publish site and
smp_load_acquire() at the four readers, with the same "pairs with"
comments the queue arrays already carry.

> On a weakly ordered machine, can a reader see a non-NULL
> cldma_drv_info[hif_id] while drv_ops, hw_regs, base_addr or wq are still
> stale?

Yes - and the part that makes this worth fixing rather than arguing is
the one you point at: the same file already uses release/acquire for the
queue arrays and spells the pairing out in the ISR comments, so this is
one file disagreeing with itself.

> This isn't a bug, but the removal side is a plain store while the new
> mtk_cldma_isr() and mtk_cldma_err_work() read the same slots with
> smp_load_acquire(), so the annotated pairing is one-sided. The same
> applies to mtk_cldma_rxq_free() and the txq_alloc error path. Would
> WRITE_ONCE() or smp_store_release() here make the pairing consistent?

Will fix in v9 with WRITE_ONCE() at the three removal sites and at the
cldma_drv_info[] clear.  A release is not needed on that side - there is
no prior store a reader has to see - but the load must not be refetched,
which is what the annotation is for.

> Can this free memory the trb service kthreads are still using?

Will fix in v9.  mtk_ctrl_trb_srv_exit() moves above mtk_cldma_exit() so
the consumers are joined before their memory is freed.

> Latching trans->dev = NULL does not stop a consumer that already loaded
> the pointer.

Agreed, and that is the part of the comment that was wrong.  The
kthreads read trans->dev with no lock and never take submit_lock, so
neither the lock nor the latch was ever excluding them.

> Should mtk_ctrl_trb_srv_exit() run before mtk_cldma_exit()?

Yes, but that reorder alone turns the use-after-free into a NULL
dereference, so v9 does both halves.  mtk_ctrl_trb_srv_exit() sets
trans->trb_srv[i] = NULL, and the two wake_up() sites in mtk_cldma.c
dereference that array unguarded:

        wake_up(&trans->trb_srv[trans->srv_cfg[hif_id][txqno]]->trb_waitq);

One of them is on the txq_flush path you describe, which runs during
this very teardown.  With the srv_exit moved first, both would run
against NULL slots on every removal that has a starved queue.  So both
sites will load the service pointer once and skip the wake when it is
NULL - after srv_exit there is nobody left to wake, and completing the
TRB is still correct.

> Once this stops an errored RX queue, what re-arms it?

Nothing today.  Will fix in v9: on a successful stop the error worker
sets need_restart and queues rx_done_work, which is the change described
above that also removes the RX race.

> Separately, the cldma_stop_queue(DIR_RX) return value is discarded here,
> so an -ETIMEDOUT queue that never quiesced looks identical to one that
> did. Should the RX branch mirror the TX recovery?

It should, and it will: checked return value, restart only on success,
and on failure the queue is logged and left stopped - the device may
still be walking the ring, which is the same reason the TX branch keeps
its requests.

> Related to the above: can reset recovery restart RX queues at all?

Not as written.  Will fix in v9 by consuming need_restart before
honouring the -ENXIO from mtk_cldma_check_rx_req(), so a queue with no
start address programmed is the case the restart handles rather than the
case it skips.

> Since the queue was never started, no further completion interrupt can
> retry the path. Should mtk_cldma_rearm_queues() program the RX queue
> directly instead of relying on the worker?

We would rather keep it in the worker, for the reason the comment there
gives: free_idx belongs to rx_done_work, which may be halfway through a
packet, and programming the start address from rearm_queues() would race
it.  Your diagnosis is right - a zero curr_addr is exactly the state a
freshly reset instance is in - but the bail-out is the bug, not the
deferral.  Reordering the -ENXIO check after the need_restart handling
keeps one owner for free_idx and makes reset recovery reach
mtk_cldma_rxq_restart(), which programs the address and starts the
queue; cldma_resume_queue() stays skipped in that case, which is what
the bail-out was protecting.

> On the receive completion path, mtk_cldma_rx_skb_adjust() bounds the put
> length with a descriptor field the device itself writes:

Will fix in v9 by clamping against req->mtu instead, which is host-only
state the device cannot reach.

> For the in-tree queue config rx_mtu == rx_frag_size == Q_MTU_3_5K, so
> mtk_cldma_rxq_alloc() leaves nr_bds at 0 and this unclamped path is the
> one taken:

Confirmed, including the detail that makes it so - the guard is
rx_mtu > rx_frag_size, and with the two equal nr_bds stays 0.

One correction that does not change the fix: data_allow_len is not
device-written.  The host programs it once in mtk_cldma_rxq_alloc() as
cpu_to_le16(req->mtu) and never rewrites it, so it holds host state that
happens to live in a buffer the device can also write - which is the
real reason it is the wrong thing to trust, and why req->mtu is the
right replacement.  In the good case the two are the same value by
construction, so only the malformed case changes behaviour.

> The skb was allocated with __dev_alloc_skb(req->mtu), i.e. 3584 bytes, so
> a device writing data_recv_len = data_allow_len = 0xFFFF would push
> skb->tail past skb->end and hit skb_over_panic() -> BUG(). Should the
> bound come from host-only state (req->mtu / the known allocation size)
> rather than from descriptor memory the device can write?

Yes.  The -EPROTO the clamp already returns makes rx_done_work drop the
packet and recycle the buffer, so no new policy comes with the fix.