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

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

Subject: Re: [PATCH v9 5/6] net: wwan: t9xx: Add FSM thread

Thanks for the review.

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

> Should a version mismatch here fail the handshake when the host has
> marked the feature mandatory?
>
> mtk_fsm_hs_info_init_by_hsid() configures MD_PORT_ENUM and SAP_PORT_ENUM
> as RTFT_TYPE_MUST_SUPPORT, version 0. Suppose the device replies
> OPTIONAL_SUPPORT with a different version. This code sets cur_ft_spt to
> RTFT_TYPE_NOT_SUPPORT and returns 0. BACKWARD_COMPAT with a lower version
> gives NOT_EXIST in the same way.
>
> mtk_fsm_parse_hs2_msg() then skips the port enumeration action:
>
>     if (cur_ft_spt == RTFT_TYPE_MUST_SUPPORT && query_rtft_action[ft_id]) {
>
> HS3 still goes out, and the FSM can reach READY without the mandatory
> enumeration ever running. That seems to contradict the comment in the
> NOT_SUPPORT case above: "The device refusing a feature the host declared
> mandatory must fail the handshake".

No change in v10, because the two arms are not saying the same thing.
NOT_EXIST/NOT_SUPPORT is the device refusing the feature outright, which
is what the comment you quote is about.  OPTIONAL_SUPPORT with a
different version is the device answering with a version the host never
offered: HS1 sends the host's whole query_ft_set[] to the device, and
the host only ever declares version 0, so such a reply is outside the
query it is answering.  That is the non-conforming device case the
guidance for this series puts out of scope - driver-upstreaming-
prompts.md lines 119-120, quoted in full in the patch 1 reply.

The consequence is bounded as well.  The only action behind that gate is
mtk_port_status_update(), so a device that declines the enumeration
leaves you with a modem that has no control ports whether we fail the
handshake or not; nothing is left half-initialised.  Returning -EPROTO
would report it sooner, which I agree reads better, but it is a
diagnostics change rather than a correctness one, and I would rather not
restructure the match function for it in the same series that is already
reworking the handshake paths around it.

> This case only compares versions and never looks at the host's
> configured type. For a feature the host never offered, query_ft_set[] is
> zero (NOT_EXIST, version 0). The >= check then always passes and
> cur_ft_spt becomes MUST_SUPPORT.
>
> mtk_fsm_parse_hs2_msg() will then run query_rtft_action[ft_id] on data
> supplied by the device. For example, during the MD handshake the device
> could trigger the SAP_PORT_ENUM action (mtk_port_status_update()).
>
> Should this case reject or downgrade when the host config is NOT_EXIST
> or NOT_SUPPORT, as the MUST_SUPPORT and OPTIONAL_SUPPORT cases do?

No change in v10 either, for the reason above and one more.  Reaching
this needs the device to answer for a feature the host never declared,
and HS1 told it the full set, so it is the same out-of-scope case.

It also does not get the device anything.  The action behind the gate is
mtk_port_status_update(), the same function the declared enumeration
calls, on device-supplied data either way - and it validates its own
input: version, head and tail patterns, data_len against port_cnt, and
each channel id looked up in the port table, with unknown ids skipped.
A device that wants to feed us bad port data can do it through the query
it was actually given, so this arm is not a way around anything.

> Can this retry ever arrive?
>
> mtk_mhccif_isr_work() acks MHCCIF_EP2RC_SW_INT_ACK before it dispatches
> the callbacks, and it ignores their return values. mtk_mhccif_init()
> documents EP2RC notifications as one-shot: the device never re-sends
> them.
>
> The callback runs under spin_lock_bh, so mtk_fsm_evt_submit() allocates
> with GFP_ATOMIC here and can fail under memory pressure. When it fails,
> this function returns before it unmasks the HS channels. In addition,
> mtk_fsm_early_bootup_handler() leaves last_dev_state unlatched so that
> the repeated notification is not filtered out.
[...]
> The status has already been acked, and DEV_STAGE_IDLE is the last stage,
> so no second notification seems to come. The FSM would then stay in ON
> with the HS channels masked, and the device would never reach READY.
>
> The callbacks also ack a second time through mtk_pci_clear_ext_evt().
> That contradicts the dispatcher's ack-first ordering, and it can erase
> an event that was re-asserted in the meantime.

No, it cannot arrive. Will fix in v10 by unmasking the handshake
channels unconditionally and only logging a failed submit, instead of
returning early, and by deleting the "waiting for retry" comments in
both handlers. mtk_fsm_startup_act() already copes with a bare HS1
arriving without a preceding STARTUP, so an open channel is enough. The
mtk_pci_clear_ext_evt() calls in the FSM callbacks are dropped too - the
dispatcher's ack is the only one needed and a second write can only lose
a re-asserted bit.

> Can the mask here undo the FSM thread's re-arm?
>
> mtk_fsm_evt_submit() queues the event and wakes the FSM thread before
> this callback masks and clears the channel. Meanwhile the FSM thread on
> another CPU can process the STARTUP event and fail. Examples are the
> -EPROTO state check, a mtk_fsm_ctrl_ch_start() failure, or a failed HS1
> send. On failure it unmasks the HS channels in mtk_fsm_startup_act() so
> that a retry can come in:
[...]
> This callback then masks the channel again, and later HS notifications
> stay masked.
>
> The FSM side does not take mhccif_lock, so the lock held here does not
> order the two paths. On PREEMPT_RT the spin_lock_bh section can also be
> preempted, which makes the window wider.

It can. Will fix in v10 by masking the channel before the submit and
unmasking it again in the callback if the submit fails, so exactly one
side owns the mask at a time: the callback until the event is queued,
the FSM thread afterwards. The clear goes away with the previous item.

> Does this unconditional unmask defeat the mask in mtk_cldma_dev_exit()?
>
> mtk_cldma_dev_exit() relies on this sequence:
>
>     mtk_pci_mask_irq(mdev, drv_info->pci_ext_irq_id);
>     synchronize_irq(virq_id);
>     mtk_pci_unregister_irq(mdev, drv_info->pci_ext_irq_id);
>
> Its comment says "mask first so no new interrupt can fire". A handler
> that is already running would unmask L1 again here before
> synchronize_irq() returns.
[...]
> A new interrupt could then call mtk_cldma_isr() with NULL data. It could
> also run against a drv_info whose workqueue is being destroyed and which
> is freed later.
>
> In the queue-timeout path, mtk_cldma_rearm_queues() and
> mtk_cldma_drv_init() can also rewrite int_mask and unmask L2 sources
> after the handler has been removed.

It does. Will fix in v10 by writing LINK_ERROR_VAL to
reg_cldma_int_mask at the top of mtk_cldma_dev_exit(), before the
host-side mask, so the source is silent rather than merely masked, and
by masking the vector once more after the reset to cover the
rearm_queues path you point out.

> Is err_work flushed too early here? synchronize_irq() and
> flush_work(&drv_info->err_work) both run before QUEUE_ERROR is masked
> for this queue.
>
> If a QUEUE_ERROR interrupt lands in between, mtk_cldma_isr() sets the
> tx_err_qs bit, unmasks QUEUE_ERROR again and queues err_work after the
> flush has finished.
>
> A later OPEN could reallocate the same queue number before the stale
> err_work runs. err_work would then stop the new queue and complete its
> pending TX requests with -EPIPE.
>
> mtk_cldma_rxq_free() uses the same ordering. On RX, a stale bit would
> stop and restart a healthy new queue, which is the case the new comment
> in rxq_free says the flush prevents.

Yes. Will fix in v10 by masking XFER_DONE and QUEUE_ERROR
first, then synchronize_irq(), then masking QUEUE_ERROR again because a
handler retired in between can have unmasked it, then clearing the
latched bit with atomic_andnot() before the flushes. Same change in both
txq_free() and rxq_free().

> Is this flush ordering enough to keep rxq alive?
>
> mtk_cldma_err_work() can load rxq with
> smp_load_acquire(&drv_info->rxq[i]) before rxq_free() unpublishes the
> slot. It then spends time in mtk_cldma_stop_queue(DIR_RX), and finally
> does:
>
>     atomic_set(&rxq->need_restart, 1);
>     queue_work(drv_info->wq, &rxq->rx_done_work);
>
> If that queue_work() runs after flush_work(&rxq->rx_done_work) has
> returned, flush_work(&drv_info->err_work) waits only for err_work
> itself. rxq_free() then goes on to kfree(rxq) while rx_done_work is
> still pending, and mtk_cldma_rx_done_work() later runs on freed memory.
>
> need_exit does not help, because the work_struct itself is inside the
> freed rxq.
>
> Should err_work be flushed before rx_done_work? This path can be reached
> from both mtk_cldma_close() and mtk_cldma_dev_exit().

It is not enough, and yes - err_work is the producer, so it will be
flushed first in v10, with the same order applied to txq_free(). That
alone is not sufficient, so it goes in together with the masking change
above (so the ISR cannot queue a fresh err_work after the flush) and
with moving the RX stop into the worker (below), after which err_work's
only effect on an RX queue is retired before the per-queue flush runs.

> Which of these two comments is correct? The reset above is said to
> quiesce the IP "so it cannot touch the rings again". This branch then
> skips dma_pool_destroy() because "the device may still master DMA into
> them".
>
> ring_leaked is only set after txq_free() or rxq_free() has already reset
> the instance and the stop still timed out. So the code already assumes
> that a reset is not enough in that case.
>
> Either the pool leak is unnecessary, or the "cannot touch the rings
> again" claim is wrong. The leak policy in the commit message does not
> say which guarantee it relies on.

The reset comment is the wrong one. Will fix in v10 by rewording it as
best-effort and stating the guarantee the leak actually relies on, in
both the comment and the commit message: dma_pool_destroy() releases the
backing pages even with blocks still outstanding, so after a stop that
timed out, returning the pools would hand memory the device may still be
mastering back to the allocator.

> Does this comment match the code? It says "Unpublish before teardown".
> The store actually runs after the IRQ unregister, the queue frees,
> destroy_workqueue(), the reset and dma_pool_destroy().
>
> The comment looks copied from txq_free() and rxq_free(), where the order
> really is the other way round. The protection today comes from
> mtk_cldma_exit() clearing trans->dev first and from the trb threads
> being joined.

It does not, and the comment is indeed copied. Will fix in v10 by moving
the store to the top of mtk_cldma_dev_exit() rather than editing the
comment. Every reader loads the slot and treats NULL as "gone", which is
the right answer for all of dev_exit and not just its last line.

> Can this -EPIPE return lead to unmapping TX buffers that the device
> still owns?
>
> mtk_cldma_tx() treats any error from mtk_cldma_start_xfer() as a reason
> to flush the ring:
[...]
> mtk_cldma_txq_flush() clears CLDMA_GPD_FLAG_HWO, calls
> dma_unmap_single() and completes every pending skb.
>
> mtk_cldma_err_work() sets is_stopping before it polls
> mtk_cldma_stop_queue(). If the stop fails, it leaves is_stopping set on
> purpose and skips the flush, because the device may still be walking the
> ring. A TRB_CMD_TX that arrives during the stop poll, or any time after
> a failed stop, would flush exactly the requests err_work is trying to
> keep.
>
> mtk_cldma_submit_tx() also never checks is_stopping, although the new
> comment in struct txq says the producer reads it under ring_lock. So new
> HWO descriptors keep being published into the ring.
>
> Nothing clears is_stopping after a failed stop. Wouldn't every later TX
> on that queue take the flush path?

It can, on all three counts. Will fix in v10: mtk_cldma_tx() returns
-EPIPE straight up without calling mtk_cldma_txq_flush(),
mtk_cldma_submit_tx() checks is_stopping under ring_lock and refuses the
request, so the struct txq comment becomes true.

is_stopping staying set is deliberate and will be documented as such
rather than cleared: after a stop that timed out we do not know what the
device owns, so the queue refuses new requests and keeps the ones it has
until TRB_CMD_DISABLE tears it down through the ring_leaked path.
Writers get -EPIPE, which is the honest answer for that queue.

> Is the locking precondition in this comment accurate? The err_cldma_exit
> path in mtk_pcie_hif_init() is taken when mtk_ctrl_trb_srv_init() fails,
> and it calls mtk_cldma_exit() without holding submit_lock.
>
> That is harmless today because trans->available is still 0 on that
> path. Should the comment cover this case as well?

It is not accurate, and yes. Will fix in v10 by rewording it to say what
makes the latch safe for each caller: the teardown caller holds
submit_lock, and the mtk_pcie_hif_init() unwind does not need it because
trans->available is still 0 there and no submitter can be in flight. No
check is added - the path is correct as it stands.

> Does this continue, and the one after the !txq check above, also skip
> RX error handling for the same queue index?
>
> Both masks are consumed up front with atomic_xchg(). When the TX branch
> hits either continue, the "if (rx_err & BIT(i))" branch for the same i
> never runs.
>
> The ISR has already cleared and unmasked the RX QUEUE_ERROR source. As
> the comment in the RX branch notes, a stopped RX queue raises no further
> interrupt. Wouldn't that RX queue then stay stalled until it is closed
> and reopened?

They do, and it would - the bit is already gone from rx_err_qs and
nothing re-raises it. Will fix in v10 by factoring the TX and RX bodies
into two small static helpers whose early exits are return rather than
continue, and calling both unconditionally for each index.

> Is this stop serialized with rxq->rx_done_work? drv_info->wq is
> allocated with WQ_UNBOUND, so err_work and rx_done_work can run at the
> same time on different CPUs.
>
> Consider an rx_done_work near its end. It can read need_restart == 0 and
> call mtk_cldma_resume_queue(DIR_RX) right after this stop. err_work then
> sets need_restart and queues rx_done_work again. That call runs
> mtk_cldma_rxq_restart() and reprograms the start address on a queue that
> may already be running again.
>
> The comment below says the queue is handed back to its worker instead
> of being programmed here. The stop command is also queue programming,
> though, and it is not serialized with the owner's resume or restart.

It is not serialized, and you are right that the stop is queue
programming too. Will fix in v10 by moving the RX stop out of err_work
into mtk_cldma_rxq_restart(), along with its timeout handling, so
rx_done_work performs every programming write on an RX queue. A work
item cannot run concurrently with itself, so no new lock is needed and
err_work is left with need_restart plus queue_work().

> Can the RX engine be restarted after mtk_cldma_rxq_free() has stopped
> it?
>
> The end of mtk_cldma_rx_done_work() checks need_exit and then acts on
> it, with no lock:
[...]
> rxq_free() sets need_exit and calls mtk_cldma_stop_queue(DIR_RX), then
> synchronize_irq() and flush_work(). A worker that read need_exit == 0
> before that can still call mtk_cldma_rxq_restart() after the stop has
> completed. flush_work() only waits for the restart to finish.
>
> The stop returned 0, so ring_leaked stays false. rxq_free() then unmaps
> and frees the RX skbs and returns the GPDs, while the SO engine may be
> running on HWO descriptors.
>
> Setting need_restart here on every RX QUEUE_ERROR makes this
> interleaving concrete. The added flush_work(&drv_info->err_work) in
> rxq_free() does not stop the hardware again.

It can, and the flush indeed does not stop the hardware. This is the
same defect you raised against patch 3, and since rxq_free() and
rx_done_work are introduced there, the stop-again-after-flush_work()
fix is amended into patch 3, feeding its existing -ETIMEDOUT and
ring_leaked path so nothing is unmapped.

The producer you identify is this patch's, and it is fixed here: err_work
is flushed before rx_done_work, the error latch is cleared before either
flush, and rx_done_work becomes the only context that programs an RX
queue, so there is no restart left to race the free-side stop.

> Does this ordering leave a window where trb_srv is used after it has
> been freed?
>
> mtk_ctrl_trb_srv_exit() calls kthread_stop() and kfree(srv), and only
> then sets trans->trb_srv[i] = NULL. The CLDMA IRQ is masked, and
> drv_info->wq drained, only later in
> mtk_cldma_exit()->mtk_cldma_dev_exit().
>
> This patch registers mtk_cldma_isr(), so mtk_cldma_tx_done_work() and
> mtk_cldma_err_work()->mtk_cldma_txq_flush() can run inside that window.
> Both do this without a lock:
[...]
> srv can already be freed at that point. It is even still non-NULL
> between the kfree() and the NULL store.
>
> The commit message says "mtk_pcie_hif_exit() joins the trb service
> threads before mtk_cldma_exit() frees what they dereference". However,
> the IRQ and work producers that also dereference trb_srv[] are not
> quiesced before srv is freed.
>
> A queue can still be open at this point. That happens, for example, when
> the DEV_RM submit fails and removal forces cleanup, or when
> mtk_port_ch_disable() times out.

It does, and the NULL guard there does not help for the reason you give.
Will fix in v10 by hoisting mtk_cldma_dev_exit()'s quiesce prologue -
silence reg_cldma_int_mask, mask the vector, synchronize_irq(),
unregister the handler, flush_workqueue() - into a new
mtk_cldma_quiesce(trans) called from mtk_pcie_hif_exit() before
mtk_ctrl_trb_srv_exit(). dev_exit keeps its own prologue, which then
runs as an idempotent second call and still covers the per-HIF teardown
that does not go through hif_exit. The commit message paragraph is
reworded to say that the interrupt and the workqueue are quiesced, not
just that the kthreads are joined.

Thanks,
Jack