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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: wwan: t9xx: Add FSM thread

This adds a finite-state-machine kthread that serializes asynchronous
events from the device and the OS, plus a notifier mechanism so other
modules can react before and after each state transition.  It also wires
the FSM into the PCI probe/remove path and into the control plane and
CLDMA transport.

No Link: tags are present in the commit header.

> net: wwan: t9xx: Add FSM thread
>
> The FSM (Finite-state Machine) thread is responsible for
> synchronizing the actions of different modules. The
> asynchronous events from the device or the OS will trigger
> a state transition.
>
> The FSM thread will append it to the event queue when an
> event arrives. It handles the events sequentially. After
> processing the event, the FSM thread notifies other modules
> before and after the state transition.
>
> Seven FSM states are defined. They can transition from one
> state to another, self-transition in some states, and
> transition in some sub-states.

[Severity: Low]
Does the code match this description?  enum mtk_fsm_state in the new
mtk_fsm.h defines five states:

	FSM_STATE_INVALID, FSM_STATE_OFF, FSM_STATE_ON,
	FSM_STATE_BOOTUP, FSM_STATE_READY

There is also no sub-state anywhere: struct mtk_md_fsm and
struct mtk_fsm_param have no sub-state member, and mtk_fsm_switch_state()
only ever assigns fsm->state.  Should the count and the sub-state
sentence be updated?

[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?

 - the HS1/HS2/HS3 handshake and runtime-feature protocol
   (mtk_fsm_send_hs1_msg(), mtk_fsm_parse_hs2_msg(),
   mtk_fsm_send_hs3_msg() and the two ctrl-msg handlers)

 - the whole operational CLDMA bring-up and teardown path
   (cldma_drv_info_tbl, mtk_cldma_get_drv_info(), mtk_cldma_isr(),
   mtk_cldma_dev_init(), mtk_cldma_dev_exit(), including DMA pools,
   a workqueue and MSI-X registration)

 - an skb leak fix in mtk_cldma_rxq_free(), adding
   dev_kfree_skb_any(req->skb) in the rxq->nr_bds branch to code added
   by the earlier CLDMA patch of this series, with no Fixes: tag

 - removal of all declared CLDMA4 support (CLDMA4 from enum mtk_hif_id,
   which shrinks NR_CLDMA from 3 to 2, plus CLDMA4_HW_ID,
   CLDMA4_BASE_ADDR, REG_CLDMA_INT_WF_MASK, the cldma4_base_addr and
   reg_cldma4_int_mask fields, and the CLDMA4 branch of
   mtk_cldma_drv_init_m9xx())

> diff --git a/drivers/net/wwan/t9xx/mtk_ctrl_plane.c b/drivers/net/wwan/t9xx/mtk_ctrl_plane.c
> index 6ed47d0745565..1d9dc53482405 100644
> --- a/drivers/net/wwan/t9xx/mtk_ctrl_plane.c
> +++ b/drivers/net/wwan/t9xx/mtk_ctrl_plane.c
> @@ -5,10 +5,46 @@
> +static void mtk_ctrl_trans_fsm_state_handler(struct mtk_fsm_param *param,
> +					     struct mtk_ctrl_blk *ctrl_blk)
> +{
> +	struct mtk_md_dev *mdev = ctrl_blk->mdev;
> +
> +	switch (param->to) {
> +	case FSM_STATE_OFF:
> +		ctrl_blk->ops->fsm_indication(mdev, param);
> +		ctrl_blk->ops->exit(mdev);
> +		break;
> +	case FSM_STATE_ON:
> +		ctrl_blk->ops->init(mdev);
> +		fallthrough;

[Severity: High]
Can a failing ops->init() here lead to a NULL dereference on removal?

ops->init is mtk_pcie_hif_init(), which has several failure exits (kmemdup
ENOMEM, queue-info validation, radix_tree_insert(), mtk_cldma_init(),
mtk_ctrl_trb_srv_init()).  In all of them trans->trb_srv[] is left
all-NULL, but the return value is discarded here and the FSM commits
FSM_STATE_ON anyway.

On removal the FSM_STATE_OFF arm above calls ops->exit()
unconditionally:

mtk_pci_dev_exit() -> FSM_EVT_DEV_RM -> mtk_fsm_dev_rm_act()
  -> mtk_fsm_enter_off_state() (guard passes, state == ON)
  -> mtk_fsm_switch_state(FSM_STATE_OFF)
  -> mtk_ctrl_trans_fsm_state_handler() case FSM_STATE_OFF
  -> mtk_pcie_hif_exit() -> mtk_ctrl_trb_srv_exit()

and mtk_ctrl_trb_srv_exit() does:

	for (i = 0; i < trans->trb_srv_num; i++) {
		srv = trans->trb_srv[i];
		kthread_stop(srv->trb_thread);

with no NULL check.  The same unchecked-return pattern repeats one level
down, where the void mtk_cldma_fsm_state_listener() discards
mtk_cldma_dev_init() errors.

> +	default:
> +		ctrl_blk->ops->fsm_indication(mdev, param);
> +		break;
> +	}
> +}
> +
> +static void mtk_ctrl_fsm_state_listener(struct mtk_fsm_param *param, void *data)
> +{
> +	struct mtk_ctrl_blk *ctrl_blk = data;
> +
> +	mtk_port_mngr_fsm_state_handler(param, ctrl_blk->port_mngr);
> +	mtk_ctrl_trans_fsm_state_handler(param, ctrl_blk);
> +	mtk_port_mngr_fsm_state_handler_late(param, ctrl_blk->port_mngr);
> +}

[ ... ]

> diff --git a/drivers/net/wwan/t9xx/mtk_fsm.c b/drivers/net/wwan/t9xx/mtk_fsm.c
> new file mode 100644
> index 0000000000000..8cf9b239caeaf
> --- /dev/null
> +++ b/drivers/net/wwan/t9xx/mtk_fsm.c

[ ... ]

> +static int mtk_fsm_parse_hs2_msg(struct fsm_hs_info *hs_info)
> +{
> +	struct mtk_md_fsm *fsm = container_of(hs_info, struct mtk_md_fsm, hs_info[hs_info->id]);
> +	char *rt_data = ((struct sk_buff *)hs_info->rt_data)->data;
> +	enum runtime_feature_support_type cur_ft_spt;
> +	struct runtime_feature_entry *rtft_entry;
> +	unsigned int ft_id, offset, data_len;
> +	int ret = 0;
> +
> +	offset = sizeof(struct feature_query);
> +	for (ft_id = 0; ft_id < FEATURE_CNT; ft_id++) {
> +		if (offset + sizeof(*rtft_entry) > hs_info->rt_data_len)
> +			break;
> +
> +		rtft_entry = (struct runtime_feature_entry *)(rt_data + offset);
> +		ret = mtk_fsm_feature_set_match(&cur_ft_spt,
> +						rtft_entry->support_info,
> +						hs_info->query_ft_set[ft_id]);

[Severity: Low]
Should this loop key off rtft_entry->feature_id instead of the loop index?

The wire structure carries an explicit id:

	struct runtime_feature_entry {
		u8 feature_id;
		struct runtime_feature_info support_info;

and the host's own emitter fills it in mtk_fsm_append_rtft_entries():

	rtft_entry->feature_id = ft_id;

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.
If entries are omitted or reordered, one feature's payload is handed to
another feature's handler, or mtk_fsm_feature_set_match() returns -EPROTO
for a feature the device did answer.

> +		if (ret < 0)
> +			break;
> +
> +		data_len = le32_to_cpu(rtft_entry->data_len);
> +		if (data_len > hs_info->rt_data_len - offset - sizeof(*rtft_entry))
> +			break;
> +
> +		if (cur_ft_spt == RTFT_TYPE_MUST_SUPPORT)
> +			if (query_rtft_action[ft_id])
> +				ret = query_rtft_action[ft_id](fsm->mdev,
> +							       rtft_entry->data,
> +							       data_len);

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

drivers/net/wwan/t9xx/mtk_port.c:mtk_port_status_update() {
	if (le16_to_cpu(msg->version) != MTK_PORT_ENUM_VER ||
	    le32_to_cpu(msg->head_pattern) != MTK_PORT_ENUM_HEAD_PATTERN ||
	    le32_to_cpu(msg->tail_pattern) != MTK_PORT_ENUM_TAIL_PATTERN)
		return -EPROTO;

	if (data_len < sizeof(*msg) + ...
}

With data_len == 0 for QUERY_RTFT_ID_MD_PORT_ENUM or
QUERY_RTFT_ID_SAP_PORT_ENUM (both declared MUST-support by the host, so
cur_ft_spt == RTFT_TYPE_MUST_SUPPORT is reachable), rtft_entry->data
equals rt_data + rt_data_len and the read goes past the received message
into skb tailroom / skb_shared_info.  Can the handler be given a
guaranteed-minimum length instead?

> +		if (ret < 0)
> +			break;
> +
> +		offset += sizeof(*rtft_entry) + data_len;
> +	}

[ ... ]

> +static int mtk_fsm_sap_ctrl_msg_handler(void *__fsm, struct sk_buff *skb)
> +{

[ ... ]

> +	hs_info->rt_data = skb;
> +	hs_info->rt_data_len = skb->len;
> +	ret = mtk_fsm_evt_submit(fsm->mdev, FSM_EVT_STARTUP,
> +				 hs_info->fsm_flag_hs2, hs_info, sizeof(*hs_info), 0);
> +	if (ret == FSM_EVT_RET_FAIL) {
> +		dev_kfree_skb(skb);
> +		hs_info->rt_data = NULL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mtk_fsm_md_ctrl_msg_handler(void *__fsm, struct sk_buff *skb)
> +{

[ ... ]

> +	switch (le32_to_cpu(ctrl_msg_h->id)) {
> +	case CTRL_MSG_HS2:
> +		skb_pull(skb, sizeof(*ctrl_msg_h));
> +		hs_info->rt_data = skb;
> +		hs_info->rt_data_len = skb->len;
> +		ret = mtk_fsm_evt_submit(fsm->mdev, FSM_EVT_STARTUP,
> +					 hs_info->fsm_flag_hs2, hs_info, sizeof(*hs_info), 0);
> +		if (ret != FSM_EVT_RET_FAIL)
> +			consumed_skb = true;
> +		break;

[Severity: High]
Can two HS2 messages race on the single hs_info->rt_data slot?

The queued event carries only the hs_info pointer (event->data = hs_info),
never the skb, so no event owns its buffer.  The writer here runs in the
CLDMA rx_done work while the reader/freer runs in the FSM kthread, and
neither takes a lock or checks whether a buffer is already pending.

If the modem sends a second CTRL_MSG_HS2 while the FSM thread is still in
mtk_fsm_startup_act() for the first one:

rx work					FSM kthread
hs_info->rt_data = skb2			mtk_fsm_parse_hs2_msg()  /* skb1 */
submit event B
					dev_kfree_skb(hs_info->rt_data);
					hs_info->rt_data = NULL;

skb1 is leaked, skb2 is freed early, and event B then reaches:

	char *rt_data = ((struct sk_buff *)hs_info->rt_data)->data;

on a NULL pointer.  In the failure branches above, dev_kfree_skb(skb) can
also free a buffer the FSM thread is parsing right now, and a torn
rt_data / rt_data_len pair makes the parser bound the wrong buffer.
Should the skb be handed to the event itself rather than stored in the
shared hs_info?

> +	default:
> +		dev_err(fsm->mdev->dev, "Invalid ctrl msg id\n");
> +	}

[ ... ]

> +static void mtk_fsm_switch_state(struct mtk_md_fsm *fsm,
> +				 enum mtk_fsm_state to_state, struct mtk_fsm_evt *event)
> +{

[ ... ]

> +	list_for_each_entry(nt, &fsm->pre_notifiers, entry)
> +		nt->cb(&param, nt->data);
> +
> +	fsm->state = to_state;
> +	fsm->fsm_flag |= event ? event->fsm_flag : FSM_F_DFLT;

[ ... ]

> +	list_for_each_entry(nt, &fsm->post_notifiers, entry)
> +		nt->cb(&param, nt->data);
> +}

[Severity: High]
What protects these two list walks against concurrent unregistration?

The traversal runs in the FSM kthread with plain list_for_each_entry() and
calls nt->cb(), while mtk_fsm_notifier_register() /
mtk_fsm_notifier_unregister() do list_add()/list_add_tail()/list_del() and
kfree() from other threads.  There is no mutex, no _rcu list variant, no
synchronize_rcu() before the kfree, and no barrier.

The removal path is ordered against the FSM thread only by the blocking
FSM_EVT_DEV_RM event, whose result mtk_pci_dev_exit() ignores and which
can also time out.  When that happens:

mtk_trans_ctrl_exit() -> mtk_ctrl_exit()
  -> mtk_fsm_notifier_unregister(mdev, MTK_USER_CTRL)   /* list_del + kfree */
  -> mtk_port_mngr_exit(ctrl_blk)                       /* frees port_mngr */

while the FSM thread may still be iterating post_notifiers and about to
call nt->cb() on the freed notifier with the freed port_mngr as data.
Since both register and unregister are exported, any future runtime
register/unregister races with every state transition too.

> +static int mtk_fsm_startup_act(struct mtk_md_fsm *fsm, struct mtk_fsm_evt *event)
> +{

[ ... ]

> +	} else if (event->fsm_flag & FSM_HS2_DONE_MASK) {
> +		ret = mtk_fsm_parse_hs2_msg(hs_info);
> +		if (!ret) {
> +			mtk_fsm_switch_state(fsm, to_state, event);
> +			ret = mtk_fsm_send_hs3_msg(hs_info);
> +		}
> +		dev_kfree_skb(hs_info->rt_data);
> +		hs_info->rt_data = NULL;
> +		if (ret)
> +			goto hs_err;
> +	}
> +
> +	if (((fsm->fsm_flag | event->fsm_flag) & fsm->hs_done_flag) == fsm->hs_done_flag) {
> +		to_state = FSM_STATE_READY;
> +		mtk_fsm_switch_state(fsm, to_state, NULL);
> +	}

[Severity: Medium]
Can the FSM reach FSM_STATE_READY when HS3 was never delivered?

mtk_fsm_switch_state() is called before mtk_fsm_send_hs3_msg(), and it
permanently ORs the event flag in:

	fsm->fsm_flag |= event ? event->fsm_flag : FSM_F_DFLT;

If mtk_fsm_send_hs3_msg() then fails (-ENOMEM from
__dev_alloc_skb(RTFT_DATA_SIZE), -EPROTO from
mtk_fsm_append_rtft_entries(), or a failed mtk_port_internal_write()), the
hs_err exit does not clear the already-committed FSM_F_*_HS2_DONE bit.
In the dual-handshake configuration the other side can then complete, and
the hs_done_flag test above sees every HS_START/HS2_DONE bit set and moves
to FSM_STATE_READY.

[ ... ]

> +static int mtk_fsm_hs1_handler(u32 status, void *__hs_info)
> +{
> +	struct fsm_hs_info *hs_info = __hs_info;
> +	struct mtk_md_dev *mdev;
> +	struct mtk_md_fsm *fsm;
> +
> +	fsm = container_of(hs_info, struct mtk_md_fsm, hs_info[hs_info->id]);
> +	mdev = fsm->mdev;
> +	mtk_fsm_evt_submit(mdev, FSM_EVT_STARTUP,
> +			   hs_info->fsm_flag_hs1, hs_info, sizeof(*hs_info), 0);
> +	mtk_dev_mask_dev_evt(mdev, hs_info->mhccif_ch);
> +	mtk_dev_clear_dev_evt(mdev, hs_info->mhccif_ch);
> +
> +	return 0;
> +}

[Severity: Medium]
Is there a way back if the queued startup action later fails?

The HS1 notification is masked and cleared here, before the asynchronous
event result is known.  In mtk_fsm_startup_act():

	ret = mtk_fsm_ctrl_ch_start(fsm, hs_info, O_NONBLOCK);
	if (!ret)
		ret = mtk_fsm_send_hs1_msg(hs_info);
	if (ret)
		goto hs_err;

mtk_fsm_ctrl_ch_start() returns -ENODEV when the control port is not
enabled (which is what happens when mtk_cldma_dev_init() failed, since
mtk_cldma_fsm_state_listener() drops that error), and
mtk_fsm_send_hs1_msg() can return -ENOMEM or a write error.  The hs_err
path only logs; the channel stays masked and no retry is scheduled, so the
FSM stays in FSM_STATE_BOOTUP.  The only mtk_dev_unmask_dev_evt() calls
for hs_info->mhccif_ch are in mtk_fsm_idle_evt_handler(), which is not
re-entered because the boot-flow-sync channel is masked as well.

[ ... ]

> +static void mtk_fsm_notifier_insert(struct mtk_fsm_notifier *notifier, struct list_head *head)
> +{
> +	struct mtk_fsm_notifier *nt;
> +
> +	list_for_each_entry(nt, head, entry) {
> +		if (notifier->prio > nt->prio) {
> +			list_add(&notifier->entry, nt->entry.prev);
> +			return;
> +		}
> +	}
> +	list_add_tail(&notifier->entry, head);
> +}

[ ... ]

> +	spin_lock_irqsave(&fsm->evtq_lock, flags);
> +	if (test_bit(EVT_TF_GATECLOSED, &fsm->t_flag)) {
> +		spin_unlock_irqrestore(&fsm->evtq_lock, flags);
> +		mtk_fsm_evt_put(event);
> +		dev_err(mdev->dev, "Failed to add event, fsm dev has been removed!\n");
> +		return FSM_EVT_RET_FAIL;
> +	}
> +
> +	kref_get(&event->kref);
> +	if (mode & EVT_MODE_TOHEAD)
> +		list_add(&event->entry, &fsm->evtq);
> +	else
> +		list_add_tail(&event->entry, &fsm->evtq);
> +	wake_up_process(fsm->fsm_handler);
> +	spin_unlock_irqrestore(&fsm->evtq_lock, flags);

[Severity: High]
Can fsm->fsm_handler be NULL at this wake_up_process()?

mtk_fsm_exit() clears it under the same lock:

	spin_lock_irqsave(&fsm->evtq_lock, flags);
	handler = fsm->fsm_handler;
	fsm->fsm_handler = NULL;
	spin_unlock_irqrestore(&fsm->evtq_lock, flags);

	if (handler)
		kthread_stop(handler);

and only unregisters mtk_fsm_early_bootup_handler() /
mtk_fsm_hs1_handler() at the very end, after a kthread_stop() that can
block while the FSM thread finishes an event.  The only thing stopping a
submit in that window is EVT_TF_GATECLOSED, which is set exclusively in
mtk_fsm_dev_rm_act(), i.e. only if a FSM_EVT_DEV_RM was queued and
processed.  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
on removal mtk_pci_dev_exit() ignores the DEV_RM result.

Any device event arriving then (mtk_mhccif_isr_work() ->
mtk_fsm_early_bootup_handler() -> mtk_fsm_idle_evt_handler() ->
mtk_fsm_evt_submit(), or mtk_fsm_hs1_handler()) passes a NULL task_struct
into try_to_wake_up().  mtk_fsm_start() already has the check this path is
missing:

	if (!fsm->fsm_handler)
		return -EFAULT;

mtk_fsm_exit() also never clears mdev->fsm, so the torn-down (devm-freed)
fsm stays reachable from those callbacks.

[ ... ]

> +int mtk_fsm_init(struct mtk_md_dev *mdev)
> +{

[ ... ]

> +	mtk_fsm_hs_info_init(fsm);
> +	mtk_dev_register_dev_evt(mdev, DEV_EVT_D2H_BOOT_FLOW_SYNC,
> +				 mtk_fsm_early_bootup_handler, fsm);
> +	mdev->fsm = fsm;
> +	return 0;
> +exit:
> +	return ret;
> +}

[Severity: Medium]
Should these registrations be checked?  mtk_dev_register_dev_evt() reaches
mtk_pci_register_ext_evt(), which returns -EINVAL for bad arguments,
-EFAULT when the channel bitmask intersects an existing registration, and
-ENOMEM from:

	cb = kzalloc(sizeof(*cb), GFP_ATOMIC);
	if (!cb) {
		ret = -ENOMEM;
		goto err_spin_unlock;
	}

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.  If the BOOT_FLOW_SYNC registration fails,
mtk_fsm_early_bootup_handler() never runs, so
mtk_fsm_idle_evt_handler() never sets fsm->hs_done_flag and never submits
the STARTUP event: probe succeeds, the FSM never leaves FSM_STATE_ON, no
CLDMA is initialized, no port is created, and nothing is logged.

[ ... ]

> diff --git a/drivers/net/wwan/t9xx/pcie/mtk_cldma.c b/drivers/net/wwan/t9xx/pcie/mtk_cldma.c
> index 097b20d8fbd38..1431c2797eb5d 100644
> --- a/drivers/net/wwan/t9xx/pcie/mtk_cldma.c
> +++ b/drivers/net/wwan/t9xx/pcie/mtk_cldma.c
> @@ -34,12 +34,172 @@

[ ... ]

> +static int mtk_cldma_isr(int irq_id, void *param)
> +{
> +	struct cldma_drv_info *drv_info = param;
> +	struct mtk_md_dev *mdev;
> +	u32 tx_done, rx_done;
> +	u32 tx_sta, rx_sta;
> +	struct txq *txq;
> +	struct rxq *rxq;
> +	int i;
> +
> +	mdev = drv_info->mdev;
> +	drv_info->drv_ops->cldma_get_intr_status(drv_info, &tx_sta, &rx_sta);
> +	tx_done = (tx_sta >> QUEUE_XFER_DONE) & 0xFF;
> +	rx_done = (rx_sta >> QUEUE_XFER_DONE) & 0xFF;

[Severity: Medium]
What happens to the QUEUE_ERROR bits that get masked but never handled?

mtk_cldma_get_intr_status() masks every asserted bit but clears only the
XFER_DONE byte:

drivers/net/wwan/t9xx/pcie/mtk_cldma_drv.c:mtk_cldma_get_intr_status() {
	if (*tx_sta) {
		/* TX XFER_DONE and QUEUE_ERROR mask */
		mtk_pci_write32(mdev, base + hw_regs->reg_cldma_l2timsr0, *tx_sta);
		/* TX XFER_DONE clear */
		mtk_pci_write32(mdev, base + hw_regs->reg_cldma_l2tisar0,
				(*tx_sta) & (0xFF << QUEUE_XFER_DONE));
	}
}

The two shifts above then drop QUEUE_ERROR and QUEUE_ACTIVE_START
entirely.  QUEUE_ERROR is unmasked only once, in mtk_cldma_txq_alloc() /
mtk_cldma_rxq_alloc(); the done workers re-unmask only QUEUE_XFER_DONE.
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.

> +
> +	if (tx_done) {
> +		for (i = 0; i < HW_QUEUE_NUM; i++) {
> +			txq = drv_info->txq[i];
> +			if (!(tx_done & BIT(i)) || !txq)
> +				continue;
> +			queue_work(drv_info->wq, &txq->tx_done_work);
> +		}
> +	}

[ ... ]

> +	mtk_pci_clear_irq(mdev, drv_info->pci_ext_irq_id);
> +	mtk_pci_unmask_irq(mdev, drv_info->pci_ext_irq_id);
> +
> +	return IRQ_HANDLED;
> +}

[ ... ]

> @@ -885,6 +1046,44 @@ static void mtk_cldma_rxq_free(struct cldma_drv_info *drv_info, u32 rxqno)
> +static int mtk_cldma_dev_exit(struct cldma_dev *cd, int hif_id)
> +{

[ ... ]

> +	virq_id = mtk_pci_get_virq_id(mdev, drv_info->pci_ext_irq_id);
> +	mtk_pci_mask_irq(mdev, drv_info->pci_ext_irq_id);
> +	synchronize_irq(virq_id);
> +	for (i = 0; i < HW_QUEUE_NUM; i++) {
> +		if (drv_info->txq[i])
> +			mtk_cldma_txq_free(drv_info, drv_info->txq[i]->txqno);
> +		if (drv_info->rxq[i])
> +			mtk_cldma_rxq_free(drv_info, drv_info->rxq[i]->rxqno);
> +	}
> +
> +	flush_workqueue(drv_info->wq);
> +	destroy_workqueue(drv_info->wq);
> +	dma_pool_destroy(drv_info->bd_dma_pool);
> +	dma_pool_destroy(drv_info->gpd_dma_pool);
> +	mtk_pci_unregister_irq(mdev, drv_info->pci_ext_irq_id);
> +
> +	kfree(drv_info);
> +	cd->cldma_drv_info[hif_id] = NULL;
> +
> +	return 0;
> +}

[Severity: High]
Is the initial mask plus synchronize_irq() enough to keep the ISR away
from the freed drv_info?

mtk_cldma_isr() re-arms its own vector on every exit:

	mtk_pci_clear_irq(mdev, drv_info->pci_ext_irq_id);
	mtk_pci_unmask_irq(mdev, drv_info->pci_ext_irq_id);

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.  QUEUE_ERROR stays armed in hardware, so a later interrupt can
call mtk_cldma_isr() after the workqueue is destroyed (queue_work() on
freed workqueue memory) or after kfree(drv_info)
(drv_info->drv_ops->cldma_get_intr_status(), drv_info->txq[]).

mtk_pci_unregister_irq() only clears the callback pointer:

	priv->irq_cb_list[irq_id] = NULL;
	priv->irq_cb_data[irq_id] = NULL;

and is not followed by a synchronize_irq(), while mtk_pci_irq_handler()
invokes priv->irq_cb_list[irq_id] directly from hardirq with no lock or
RCU.  Should the callback be unregistered and synchronized before the
workqueue, pools and drv_info are released?

[ ... ]

> @@ -1195,6 +1394,27 @@ int mtk_cldma_trb_process(void *dev, struct sk_buff *skb)
> +void mtk_cldma_fsm_state_listener(struct mtk_fsm_param *param, struct mtk_ctrl_trans *trans)
> +{
> +	struct cldma_dev *cd = trans->dev;
> +	int i;
> +
> +	switch (param->to) {
> +	case FSM_STATE_BOOTUP:
> +		if (param->fsm_flag & FSM_F_SAP_HS_START)
> +			mtk_cldma_dev_init(cd, CLDMA0);
> +		else if (param->fsm_flag & FSM_F_MD_HS_START)
> +			mtk_cldma_dev_init(cd, CLDMA1);
> +		break;
> +	case FSM_STATE_OFF:
> +		for (i = 0; i < NR_CLDMA; i++)
> +			mtk_cldma_dev_exit(cd, i);
> +		break;

[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.  Meanwhile
mtk_cldma_submit_tx() reads the published pointers locklessly:

	drv_info = cd->cldma_drv_info[que->hif_id];
	if (unlikely(!drv_info))
		return -EINVAL;

	txq = drv_info->txq[que->txqno];

and mtk_cldma_dev_exit() does kfree(drv_info) before clearing
cd->cldma_drv_info[hif_id], so the pointer is dangling across the free.
One way to have work still in flight: mtk_port_ch_disable()'s
wait_event_timeout() can expire, leaving the DISABLE TRB queued for the
service thread to pick up later.

> +	default:
> +		break;
> +	}
> +}

[ ... ]

> diff --git a/drivers/net/wwan/t9xx/pcie/mtk_pci.c b/drivers/net/wwan/t9xx/pcie/mtk_pci.c
> index dbc17da590bc0..bf4e110b485c8 100644
> --- a/drivers/net/wwan/t9xx/pcie/mtk_pci.c
> +++ b/drivers/net/wwan/t9xx/pcie/mtk_pci.c
> @@ -883,22 +883,34 @@ static int mtk_pci_dev_init(struct mtk_md_dev *mdev)

[ ... ]

>  static void mtk_pci_dev_exit(struct mtk_md_dev *mdev)
>  {
> +	mtk_fsm_evt_submit(mdev, FSM_EVT_DEV_RM, 0, NULL, 0,
> +			   EVT_MODE_BLOCKING | EVT_MODE_TOHEAD);
>  	mtk_trans_ctrl_exit(mdev);
> +	mtk_fsm_exit(mdev);
>  }

[Severity: High]
What happens here when the blocking removal event does not complete?

mtk_fsm_evt_submit() returns FSM_EVT_RET_FAIL if its kzalloc() fails, and
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.  mtk_trans_ctrl_exit() does not compensate:

drivers/net/wwan/t9xx/pcie/mtk_trans_ctrl.c:mtk_trans_ctrl_exit() {
	mtk_ctrl_exit(mdev);

	return 0;
}

and the devm-allocated trans/ctrl_blk/fsm are released once remove()
finishes, leaving those kthreads and the IRQ callback pointing at freed
memory.  The wait can also expire (BLOCKING_EVT_TIMEOUT is 60s) without
dequeuing or cancelling the event:

	ret = wait_event_timeout(fsm->evt_waitq,
				 (event->status != 0), BLOCKING_EVT_TIMEOUT);

so teardown then races with an FSM thread still using ctrl_blk/trans.

>  static int mtk_pci_dev_start(struct mtk_md_dev *mdev)
>  {
> +	mtk_fsm_evt_submit(mdev, FSM_EVT_DEV_ADD, 0, NULL, 0, 0);
> +	mtk_fsm_start(mdev);
>  	return 0;
>  }

[Severity: Medium]
Should these two return values be propagated?  mtk_pci_dev_start() is
declared int and mtk_pci_probe() already has an if (ret) handler for it,
but both calls are discarded and 0 is returned unconditionally.

mtk_fsm_evt_submit() returns FSM_EVT_RET_FAIL when its
kzalloc(sizeof(*event), ...) fails, so probe can succeed while the FSM
never leaves FSM_STATE_INVALID.  Every later handshake event is then
rejected by mtk_fsm_startup_act():

	if (fsm->state != FSM_STATE_ON && fsm->state != FSM_STATE_BOOTUP) {
		ret = -EPROTO;
		goto free_rt_data;
	}

leaving the device unusable with only a generic error log.

> diff --git a/drivers/net/wwan/t9xx/pcie/mtk_trans_ctrl.h b/drivers/net/wwan/t9xx/pcie/mtk_trans_ctrl.h
> index a3ff56ddf86fc..4b9c9db6ad715 100644
> --- a/drivers/net/wwan/t9xx/pcie/mtk_trans_ctrl.h
> +++ b/drivers/net/wwan/t9xx/pcie/mtk_trans_ctrl.h
> @@ -29,7 +29,6 @@
>  enum mtk_hif_id {
>  	CLDMA0,
>  	CLDMA1,
> -	CLDMA4,
>  	NR_CLDMA
>  };