Re: [PATCH v6 5/7] net: wwan: t9xx: Add FSM thread

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

On Mon, 17 Aug 2026 at 22:36, Jakub Kicinski <kuba@kernel.org> wrote:
> [Severity: Low]
> Does the code match this description?  enum mtk_fsm_state in the new
> mtk_fsm.h defines five states:
...
> There is also no sub-state anywhere: struct mtk_md_fsm and
...
> only ever assigns fsm->state.  Should the count and the sub-state
> sentence be updated?

Will fix in v7. Will update commit message to say "Five FSM states"
and remove the sub-state sentence.

> [Severity: Low]
> The changelog only mentions the FSM thread, but the patch carries several
> other functional changes.  Would it help reviewers to split them out or at
> least mention them?

Will fix in v7. Will expand the commit message to mention the HS1/HS2/HS3
handshake protocol, CLDMA device init/exit wiring, and CLDMA4 removal.

> [Severity: High]
> Can a failing ops->init() here lead to a NULL dereference on removal?
...
> and mtk_ctrl_trb_srv_exit() does:
...
> with no NULL check.

Will fix in v7. Will check ops->init() return value in
mtk_ctrl_trans_fsm_state_handler() and add NULL check for srv in
mtk_ctrl_trb_srv_exit() as defensive hardening.

> [Severity: High]
> Can two HS2 messages race on the single hs_info->rt_data slot?
...
> skb1 is leaked, skb2 is freed early, and event B then reaches:
...
> on a NULL pointer.

Will fix in v7. Will add a guard in the ctrl_msg_handlers to reject
a duplicate HS2 if hs_info->rt_data is already set, dropping the
duplicate with dev_warn.

> [Severity: High]
> What protects these two list walks against concurrent unregistration?
...
> kfree() from other threads.  There is no mutex, no _rcu list variant, no
> synchronize_rcu() before the kfree, and no barrier.

Will fix in v7. Will add a mutex (fsm->notifier_lock) to protect the
notifier lists in switch_state, register, unregister, and cleanup.

> [Severity: High]
> Can fsm->fsm_handler be NULL at this wake_up_process()?
...
> On the probe error path (mtk_pci_dev_init() -> goto free_fsm
> -> mtk_fsm_exit()) no DEV_RM is ever submitted, so the gate is open, and
...
> mtk_fsm_exit() also never clears mdev->fsm, so the torn-down (devm-freed)
> fsm stays reachable from those callbacks.

Will fix in v7. Will move wake_up_process() inside the evtq_lock
critical section with a NULL check, set EVT_TF_GATECLOSED in
mtk_fsm_exit() before NULLing fsm_handler, and clear mdev->fsm.

> [Severity: Medium]
> Can the FSM reach FSM_STATE_READY when HS3 was never delivered?
...
> hs_err exit does not clear the already-committed FSM_F_*_HS2_DONE bit.

Will fix in v7. Will move mtk_fsm_switch_state() (which commits
the HS2_DONE flag) to after mtk_fsm_send_hs3_msg() succeeds.

> [Severity: Medium]
> Is there a way back if the queued startup action later fails?
...
> path only logs; the channel stays masked and no retry is scheduled, so the
> FSM stays in FSM_STATE_BOOTUP.

Will fix in v7. Will unmask the handshake channels in the hs_err
path so the modem can re-trigger HS1.

> [Severity: Medium]
> What happens to the QUEUE_ERROR bits that get masked but never handled?
...
> So the first queue error permanently disarms error reporting, leaves the
> bit latched in L2TISAR0/L2RISAR0, and leaves the halted queue with no log,
> no queue reset and no GPD/HWO recovery.

Will fix in v7. Will extract and handle QUEUE_ERROR bits in
mtk_cldma_isr(): log dev_err, clear the error bits, and re-unmask.
Full queue recovery will be a follow-up if needed.

> [Severity: High]
> Is the initial mask plus synchronize_irq() enough to keep the ISR away
> from the freed drv_info?
...
> so an ISR that was already running when the vector was masked unmasks it
> again before returning, and synchronize_irq() only waits for that ISR to
> finish.

Will fix in v7. Will reorder mtk_cldma_dev_exit() to unregister the
IRQ callback first, then mask + synchronize_irq, then free resources.
Will also add a NULL callback check in mtk_pci_irq_handler().

> [Severity: High]
> Are the TRB service threads guaranteed to be stopped before this runs?
...
> In mtk_ctrl_trans_fsm_state_handler(), the FSM_STATE_OFF arm calls
> fsm_indication() (which lands here and tears down every CLDMA hif)
> before ops->exit(), and it is ops->exit() -> mtk_pcie_hif_exit() that
> clears trans->available and stops the TRB kthreads.

Will fix in v7. Will swap the ordering in FSM_STATE_OFF: call
ops->exit() (stop TRB threads) before fsm_indication() (free CLDMA).
Will also swap kfree/NULL assignment order in mtk_cldma_dev_exit().

> [Severity: High]
> What happens here when the blocking removal event does not complete?
...
> the result is discarded.  In that case the FSM_STATE_OFF notifier chain
> never runs, so ctrl_blk->ops->exit() = mtk_pcie_hif_exit() is never
> called: the CLDMA IRQ callback stays registered, the CLDMA workqueue and
> DMA pools stay alive, the TRB service kthreads keep running and
> trans->available stays 1.

Will fix in v7. Will check DEV_RM return value in mtk_pci_dev_exit().
On failure (kzalloc fail or timeout), fall back to direct cleanup
of HIF resources before proceeding with teardown.

> [Severity: Medium]
> Should these two return values be propagated?  mtk_pci_dev_start() is
...
> but both calls are discarded and 0 is returned unconditionally.

Will fix in v7. Will propagate return values of mtk_fsm_evt_submit()
and mtk_fsm_start() from mtk_pci_dev_start().

> [Severity: Medium]
> Should these registrations be checked?  mtk_dev_register_dev_evt() reaches
...
> Here the return value is dropped, and the two per-handshake-channel
> registrations inside the void mtk_fsm_hs_info_init() cannot report failure
> at all.

Will fix in v7. Will check return values of mtk_dev_register_dev_evt()
in mtk_fsm_init() and make mtk_fsm_hs_info_init() return int so it
can propagate registration failures.

> [Severity: Low]
> Should this loop key off rtft_entry->feature_id instead of the loop index?
...
> Here the received entry's feature_id is never read, so the parser assumes
> the device returns exactly FEATURE_CNT entries in dense ascending order.

The loop intentionally uses the index rather than rtft_entry->feature_id
for dispatch. The protocol contract requires the device to return exactly
FEATURE_CNT entries in dense ascending order, enforced by the check after
the loop:

  if (ft_id != FEATURE_CNT) {
      dev_err(..., "Unable to handle mistake hs2 msg, ft_id=%d\n", ft_id);
      return -EPROTO;
  }

The reason this check cannot be moved before the loop is that
runtime_feature_entry is variable-length — each entry carries a data_len
field followed by an inline payload, so the number of entries can only be
determined by traversing the buffer. If the device omits or reorders
entries, ft_id will fall short of FEATURE_CNT and the entire HS2 is
rejected as a hard protocol error. There is no partial-match path. The
feature_id field exists in the wire struct as a marker but is not used as
a dispatch key by design, to prevent a misbehaving device from
redirecting its response to an unintended handler.

> [Severity: Medium]
> Is a minimum length check missing here?  Only an upper bound on data_len
> is applied, so data_len == 0 is accepted and passed to
> mtk_port_status_update(), which dereferences the 12-byte
> struct mtk_port_enum_msg header before validating the length:

Will fix in v7. Will add a data_len == 0 check before calling the
handler, and move the sizeof(*msg) length check in
mtk_port_status_update() before the header dereference.

Thanks.
--
Jack