Re: [PATCH v6 3/7] net: wwan: t9xx: Add control DMA interface

[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 control DMA interface

This adds the CLDMA hardware interface used by the control plane, together
with the TRB service threads and PCIe transport glue that drive the GPD
rings.  The new code allocates TX/RX queues, maps SKB payloads for DMA and
completes requests back to the port layer.

> diff --git a/drivers/net/wwan/t9xx/pcie/mtk_cldma.c b/drivers/net/wwan/t9xx/pcie/mtk_cldma.c
> new file mode 100644
> index 0000000000000..097b20d8fbd38
> --- /dev/null
> +++ b/drivers/net/wwan/t9xx/pcie/mtk_cldma.c
> @@ -0,0 +1,1232 @@

[ ... ]

> +static void mtk_cldma_rx_done_work(struct work_struct *work)
> +{
> +	struct rx_req *req = NULL, *pre_req = NULL;
> +	struct rxq *rxq = container_of(work, struct rxq, rx_done_work);
> +	struct cldma_drv_info *drv_info;
> +	struct cldma_drv_ops *drv_ops;
> +	struct mtk_md_dev *mdev;
> +	struct sk_buff *rx_skb;
> +	int i, ret, idx;
> +
> +	drv_info = rxq->drv_info;
> +	mdev = drv_info->mdev;
> +	drv_ops = drv_info->drv_ops;
> +
> +again:
> +	for (i = 0; i < rxq->nr_gpds; i++) {
> +		req = rxq->req_pool + rxq->free_idx;
> +		if (!req->skb) {
> +			dev_err(mdev->dev,
> +				"Failed to get valid req cldma%d rxq%d req%d\n",
> +				drv_info->hw_id, rxq->rxqno, rxq->free_idx);
> +			goto out;
> +		}
> +
> +		if (req->gpd->rx_gpd.gpd_flags & CLDMA_GPD_FLAG_HWO)
> +			break;
> +
> +		dma_rmb(); /* read descriptor fields after HWO check */
> +
> +		mtk_cldma_rx_skb_adjust(mdev, rxq, req);
> +		rx_skb = req->skb;
> +		req->skb = NULL;
> +
> +		ret = mtk_cldma_reload_rx_skb(mdev, rxq, req);
> +		if (ret) {
> +			/* Alloc failed — recycle old buffer, drop packet.
> +			 * BD mode cannot recycle directly (BDP flag mismatch),
> +			 * so accept the stall and let reset recovery handle it.
> +			 */
> +			if (rxq->nr_bds) {
> +				dev_kfree_skb_any(rx_skb);
> +				goto out;
> +			}
> +
> +			skb_trim(rx_skb, 0);
> +			req->skb = rx_skb;
> +			req->data_dma_addr = dma_map_single(mdev->dev,
> +							    rx_skb->data,
> +							    req->mtu,
> +							    DMA_FROM_DEVICE);
> +			if (dma_mapping_error(mdev->dev, req->data_dma_addr)) {
> +				req->data_dma_addr = 0;
> +				dev_kfree_skb_any(rx_skb);
> +				req->skb = NULL;
> +				/* Advance free_idx below to prevent ring stall */
> +			} else {

[Severity: High]
Can the device DMA into freed memory here?  When both the refill in
mtk_cldma_reload_rx_skb() and this fallback re-map fail, req->skb and
req->data_dma_addr are cleared, but the descriptor still carries the old
programmed address:

mtk_cldma_rx_skb_adjust() {
	if (req->data_dma_addr) {
		dma_unmap_single(mdev->dev, req->data_dma_addr,
				 req->mtu, DMA_FROM_DEVICE);
		req->data_dma_addr = 0;
	}
	...
}

The else branch that reprograms rx_gpd.data_buff_ptr_h/l is skipped, so
gpd->rx_gpd still points at the buffer that was just unmapped and freed
with dev_kfree_skb_any(rx_skb).

Control then falls through to the arming code below, which arms the
previous slot unconditionally, so on the next processed request this
poisoned descriptor becomes pre_req and is handed back to the hardware.

Are the two failures independent?  If the first failure was itself a
dma_map_single() failure from IOVA or swiotlb exhaustion, the immediate
retry of the same mapping would fail too, which makes this path
reachable under memory pressure.

> +				req->gpd->rx_gpd.data_buff_ptr_h =
> +					cpu_to_le32((u64)req->data_dma_addr >> 32);
> +				req->gpd->rx_gpd.data_buff_ptr_l =
> +					cpu_to_le32(req->data_dma_addr);
> +			}
> +		} else {
> +			do {
> +				ret = rxq->rx_done(rx_skb, rxq->arg,
> +						   atomic_read(&rxq->need_exit) ? true : false);
> +				if (ret == -EAGAIN)
> +					usleep_range(1000, 2000);
> +			} while (ret == -EAGAIN);
> +		}
> +
> +		wmb(); /* ensure addr set done before HWO setup done  */
> +
> +		idx = rxq->free_idx == 0 ? rxq->nr_gpds - 1 : rxq->free_idx - 1;
> +		pre_req = rxq->req_pool + idx;
> +		pre_req->gpd->rx_gpd.gpd_flags |= CLDMA_GPD_FLAG_HWO;
> +		rxq->free_idx = (rxq->free_idx + 1) % rxq->nr_gpds;

Also, is the comment "Advance free_idx below to prevent ring stall"
inverted?  Advancing past a slot with no buffer leaves req->skb NULL, and
on the next wrap-around the check at the top of this loop:

	if (!req->skb) {
		dev_err(mdev->dev, ...);
		goto out;
	}

aborts the RX work permanently for that queue, which looks like exactly
the stall the comment says it is preventing.

> +	}

[ ... ]

> +static struct rxq *mtk_cldma_rxq_alloc(struct cldma_drv_info *drv_info, struct sk_buff *skb)
> +{
> +	struct trb_open_priv *trb_open_priv = (struct trb_open_priv *)skb->data;

[ ... ]

> +	INIT_WORK(&rxq->rx_done_work, mtk_cldma_rx_done_work);
> +
> +	drv_info->rxq[rxq->rxqno] = rxq;
> +	drv_ops->cldma_stop_queue(drv_info, DIR_RX, rxq->rxqno);
> +	drv_ops->cldma_setup_start_addr(drv_info, DIR_RX,
> +					rxq->rxqno, rxq->req_pool[0].gpd_dma_addr);
> +	drv_ops->cldma_start_queue(drv_info, DIR_RX, rxq->rxqno);

[Severity: Medium]
Should the result of cldma_stop_queue() be checked here and in the other
three call sites?  mtk_cldma_stop_queue() polls with read_poll_timeout()
but drops the -ETIMEDOUT and returns only the last sampled status:

mtk_cldma_drv.c:mtk_cldma_stop_queue() {
	mtk_pci_write32(drv_info->mdev, addr, val);

	read_poll_timeout(mtk_cldma_queue_status, active,
			  active == LINK_ERROR_VAL || !active,
			  WAIT_QUEUE_STOP, WAIT_QUEUE_STOP * 10, false,
			  drv_info, dir, qno);

	return active;
}

WAIT_QUEUE_STOP is 70, so the budget is roughly 700 us.  None of
mtk_cldma_txq_alloc(), mtk_cldma_rxq_alloc(), mtk_cldma_txq_free() or
mtk_cldma_rxq_free() looks at the value.

If a busy or hung modem does not stop the queue within that window, the
free paths go on to dma_unmap_single() the payload buffers and
dma_pool_free() the GPD and BD descriptors while the engine may still be
fetching them.  synchronize_irq() only quiesces the interrupt handler, not
the DMA engine.  Would returning the timeout and bailing out (or forcing a
CLDMA reset) be safer here?

> +	drv_ops->cldma_unmask_intr(drv_info, DIR_RX, rxq->rxqno, QUEUE_ERROR);
> +	drv_ops->cldma_unmask_intr(drv_info, DIR_RX, rxq->rxqno, QUEUE_XFER_DONE);
> +
> +	return rxq;

[ ... ]

> +static int mtk_cldma_start_xfer(struct cldma_drv_info *drv_info, u32 qno)
> +{
> +	struct cldma_drv_ops *drv_ops;
> +	struct txq *txq;
> +	u32 val;
> +
> +	txq = drv_info->txq[qno];
> +	drv_ops = drv_info->drv_ops;
> +
> +	val = drv_ops->cldma_get_tx_start_addr(drv_info, qno);
> +	if (unlikely(val == LINK_ERROR_VAL))
> +		return -EIO;
> +
> +	if (unlikely(!val)) {
> +		drv_ops->cldma_drv_init(drv_info);
> +		txq = drv_info->txq[qno];
> +		drv_ops->cldma_setup_start_addr(drv_info, DIR_TX, qno,
> +						txq->req_pool[txq->free_idx].gpd_dma_addr);
> +		drv_ops->cldma_start_queue(drv_info, DIR_TX, qno);
> +		txq->tx_started = true;
> +	} else if (unlikely(!txq->tx_started)) {

[Severity: Medium]
Are txq->free_idx and txq->tx_started safe to read here without any
annotation or lock?  This function runs from the TRB service thread via
mtk_cldma_tx(), while mtk_cldma_tx_done_work() advances the same field
from workqueue context with a plain store:

mtk_cldma_tx_done_work() {
	...
	txq->free_idx = (txq->free_idx + 1) % txq->nr_gpds;
}

The reader above uses txq->req_pool[txq->free_idx].gpd_dma_addr to
re-program the UL start address, so a stale index could point the hardware
at a GPD whose HWO bit has already been cleared.

Interesting that tx_done_work() itself uses READ_ONCE()/WRITE_ONCE() for
free_idx and wr_idx, which suggests the fields are known to be shared -
was the reader here just missed?

[Severity: Low]
Is zero a safe sentinel for "queue not programmed" given only the low half
of the address is read?

mtk_cldma_drv.c:mtk_cldma_get_tx_start_addr() {
	addr = drv_info->base_addr + drv_info->hw_regs->reg_cldma_ul_start_addrl_0 +
	       qno * HW_QUEUE_NUM;
	val = mtk_pci_read32(drv_info->mdev, addr);

	return val;
}

mtk_cldma_setup_start_addr() programs both ADDRL and ADDRH and
mtk_cldma_drv_init() puts CLDMA into 64-bit GPD mode, so a GPD DMA address
of, say, 0x1_0000_0000 would read back as zero here and trigger a full
cldma_drv_init() plus queue restart.  Should the high word be read too, as
the analogous t7xx code does?

> +		drv_ops->cldma_start_queue(drv_info, DIR_TX, qno);
> +		txq->tx_started = true;
> +	} else {
> +		drv_ops->cldma_resume_queue(drv_info, DIR_TX, qno);
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int mtk_cldma_open(struct cldma_dev *cd, struct sk_buff *skb)
> +{
> +	struct trb_open_priv *trb_open_priv = (struct trb_open_priv *)skb->data;
> +	struct trb *trb = (struct trb *)skb->cb;
> +	struct cldma_drv_info *drv_info;
> +	struct queue_info *que;
> +	struct txq *txq;
> +	struct rxq *rxq;
> +	int ret = 0;
> +
> +	que = radix_tree_lookup(&cd->trans->queue_tbl, trb->channel_id & 0xFFFF);
> +	drv_info = cd->cldma_drv_info[que->hif_id];
> +	if (!drv_info) {
> +		ret = -EIO;
> +		goto out;
> +	}

[Severity: Medium]
Can a failure here permanently poison the channel?  mtk_ch_status_check()
already incremented the user count before this function runs:

mtk_trans_ctrl.c:mtk_ch_status_check() {
	case TRB_CMD_ENABLE:
		...
		trans->usr_cnt[que->hif_id][que->txqno]++;
		if (trans->usr_cnt[que->hif_id][que->txqno] == 1)
			break;
	...
}

and mtk_ctrl_trb_handler() discards the dispatch result:

	if (kick) {
		mtk_cldma_trb_process(trans->dev, skb);
		trans_list->tx_burst_cnt[qno] = 0;
		kick = false;
	}

So on -EIO here, on -EINVAL for a zero MTU, or on -ENOMEM from
mtk_cldma_txq_alloc()/mtk_cldma_rxq_alloc(), usr_cnt stays at 1 with no
txq or rxq.  A retried ENABLE then takes the usr_cnt != 1 path and
mtk_cldma_check_ch_cfg() fails with -EINVAL because txq and rxq are NULL.
Since usr_cnt lives in the devm-allocated struct mtk_ctrl_trans and is not
re-zeroed by mtk_pcie_hif_init(), does the channel stay unusable until the
device is removed?

> +
> +	if (que->tx_mtu == 0 || que->rx_mtu == 0) {

[ ... ]

> +static int mtk_cldma_close(struct cldma_dev *cd, struct sk_buff *skb)
> +{
> +	struct trb *trb = (struct trb *)skb->cb;
> +	struct cldma_drv_info *drv_info;
> +	struct queue_info *que;
> +
> +	que = radix_tree_lookup(&cd->trans->queue_tbl, trb->channel_id & 0xFFFF);
> +	drv_info = cd->cldma_drv_info[que->hif_id];
> +	if (unlikely(!drv_info))
> +		return -EPIPE;

[Severity: Medium]
Does this early return leak the disable request?  Unlike the success path
below, this branch never sets trb->status nor calls
trb->trb_complete(skb), and mtk_ctrl_trb_handler() has already unlinked
the SKB and ignores the return value of mtk_cldma_trb_process().

The submitter is left waiting on the completion:

mtk_port.c:mtk_port_ch_disable() {
	ret = wait_event_timeout(port->trb_wq, trb->status <= 0,
				 MTK_DFLT_TRB_TIMEOUT);
	if (!ret)
		ret = -ETIMEDOUT;
	...
}

so the close path stalls for the full timeout, and since trb_complete() is
where the last reference is dropped, is the 3.5 KiB command SKB leaked
too?  This looks reachable after mtk_cldma_dev_exit() has NULLed
cldma_drv_info[] on modem off, with a userspace close arriving afterwards.

> +
> +	if (drv_info->txq[que->txqno])
> +		mtk_cldma_txq_free(drv_info, que->txqno);
> +	if (drv_info->rxq[que->rxqno])
> +		mtk_cldma_rxq_free(drv_info, que->rxqno);
> +
> +	trb->status = 0;
> +	trb->trb_complete(skb);
> +
> +	return 0;
> +}

[ ... ]

> +	} else {
> +		req->data_dma_addr = dma_map_single(mdev->dev, skb->data,
> +						    skb_headlen(skb), DMA_TO_DEVICE);
> +		ret = dma_mapping_error(mdev->dev, req->data_dma_addr);
> +		if (unlikely(ret)) {
> +			req->data_dma_addr = 0;
> +			goto err_exit;
> +		}

[ ... ]

> +err_exit:
> +	dev_err(mdev->dev, "Failed to map dma! error:%d\n", ret);
> +	return -EAGAIN;
> +}

[Severity: Medium]
Can a persistent mapping failure spin the TRB service thread at 100% CPU?
A mapping failure is reported with the same -EAGAIN used for an exhausted
descriptor ring, and mtk_cldma_submit_tx() returns before it consumes any
budget, so the ring-full backpressure does not apply:

mtk_trans_ctrl.c:mtk_ctrl_trb_handler() {
	case TRB_CMD_TX:
		err = mtk_cldma_submit_tx(trans->dev, skb);
		if (err) {
			...
			if (err == -EAGAIN)
				return;
	...
}

The SKB stays queued and req_budget stays positive, so:

mtk_ctrl_chs_is_busy_or_empty() {
	if (!skb_queue_empty(...) &&
	    mtk_cldma_get_tx_budget(srv->trans->dev, i, srv_que->qno))
		return false;
	...
}

reports work available and the thread's inner loop reruns immediately:

	do {
		mtk_ctrl_trb_process(srv);
		cond_resched();
	} while (!mtk_ctrl_chs_is_busy_or_empty(srv) && ...);

Would that also flood the log, given dev_err() is unconditional on every
retry?  Should the mapping failure use a distinct error and complete the
request, or add some backoff?

> +
> diff --git a/drivers/net/wwan/t9xx/pcie/mtk_trans_ctrl.c b/drivers/net/wwan/t9xx/pcie/mtk_trans_ctrl.c
> new file mode 100644
> index 0000000000000..55297cfcdbc61
> --- /dev/null
> +++ b/drivers/net/wwan/t9xx/pcie/mtk_trans_ctrl.c
> @@ -0,0 +1,541 @@

[ ... ]

> +static struct mtk_ctrl_info_desc mtk_ctrl_info_tbl[] = {
> +	{2304, &ctrl_info_name(m9xx)},
> +	{0, NULL},
> +};

[Severity: Medium]
Does this table cover both PCI IDs the driver claims?  t9xx_pci_table[]
lists both:

	MTK_PCI_DEV_CFG(0x0900, mtk_dev_cfg_0900),
	CEI_PCI_DEV_CFG(0x01CA, mtk_dev_cfg_0900),

but this table holds only 2304 (0x0900), so a 0x01CA device finds no
match, trans->queue_info stays NULL and mtk_trans_ctrl_init() aborts with
"Failed to get ctrl info!".

The mismatch also persists to the end of the series with the IDs inverted:
mtk_ctrl_info_tbl[] and cldma_drv_info_tbl[] are then keyed on 0x01CA only,
while t9xx_pci_table[] still claims both, so the 0x0900 device becomes the
one that can never bind.  Since both entries share mtk_dev_cfg_0900, is
the omission intentional?

> +
> +#define QUEUE_CHL_MASK	0xFFFF
> +
> +static bool mtk_queue_list_is_full(struct mtk_ctrl_trans *trans, struct queue_info *que)
> +{
> +	return trans->trans_list[que->hif_id].skb_list[que->txqno].qlen >= SKB_LIST_MAX_LEN;
> +}

[Severity: Medium]
Is qlen safe to read here without the queue lock, and is the depth limit
actually enforced?  In mtk_pcie_hif_submit_skb() the fullness test and the
insertion are separate critical sections:

	if (mtk_queue_list_is_full(trans, que) && !force_send)
		return -EAGAIN;
	...
	if (trb->cmd == TRB_CMD_DISABLE)
		skb_queue_head(&trans->trans_list[que->hif_id].skb_list[que->txqno], skb);
	else
		skb_queue_tail(&trans->trans_list[que->hif_id].skb_list[que->txqno], skb);

so concurrent submitters on the same hardware queue can all pass the test
and push past SKB_LIST_MAX_LEN.

Separately, mtk_ctrl_trb_handler() walks the list with skb_peek(),
skb_queue_is_last() and skb_peek_next() outside the lock, and reads a
freshly published SKB's control block with no acquire barrier:

			skb_next = skb_peek_next(skb, skb_list);
			trb_next = (struct trb *)skb_next->cb;
			if (trb_next->cmd != TRB_CMD_TX)
				kick = true;

Can that observe stale ->cb contents on a weakly ordered CPU and
mis-decide whether to kick the hardware?  Multiple ports share one
hardware queue, so concurrent producers plus this single consumer look
like normal operation.

[ ... ]

> +static int mtk_pcie_hif_exit(struct mtk_md_dev *mdev)
> +{
> +	struct mtk_ctrl_blk *ctrl_blk = mdev->ctrl_blk;
> +	struct mtk_ctrl_trans *trans;
> +
> +	trans = ctrl_blk->ctrl_hw_priv;
> +
> +	atomic_set(&trans->available, 0);
> +	mtk_ctrl_trb_srv_exit(trans);
> +	mtk_ctrl_remove_radix_tree(trans);
> +	mtk_cldma_exit(trans);
> +
> +	return 0;
> +}
> +
> +static int mtk_pcie_hif_submit_skb(struct mtk_md_dev *mdev, struct sk_buff *skb, bool force_send)
> +{

[ ... ]

> +	que = radix_tree_lookup(&trans->queue_tbl, trb->channel_id & QUEUE_CHL_MASK);
> +	if (!que) {
> +		dev_warn(mdev->dev, "lookup que fail, ch_id: %x, que: 0x%p\n",
> +			 trb->channel_id, que);
> +		return -EINVAL;
> +	}
> +
> +	if (!atomic_read(&trans->available))
> +		return -EIO;

[Severity: High]
Does this atomic actually provide mutual exclusion against teardown?  It
looks like a state flag rather than a lock, and the objects it is guarding
are freed immediately after it is cleared:

mtk_pcie_hif_exit() {
	atomic_set(&trans->available, 0);
	mtk_ctrl_trb_srv_exit(trans);      /* kthread_stop, kfree(srv), trb_srv[i] = NULL */
	mtk_ctrl_remove_radix_tree(trans); /* radix_tree_delete + kfree(queue) */
	mtk_cldma_exit(trans);
}

The radix-tree lookup above is also lockless with no RCU read-side
section, while mtk_ctrl_remove_radix_tree() deletes and kfree()s each
entry with no grace period:

	radix_tree_delete(&trans->queue_tbl, iter.index);
	kfree(queue);

So can this sequence occur?

CPU0 (write/close on a WWAN port)
mtk_pcie_hif_submit_skb()
   que = radix_tree_lookup(...);       /* que valid */
   atomic_read(&trans->available);     /* 1 */
   <preempted>

CPU1 (FSM thread on FSM_STATE_OFF)
mtk_pcie_hif_exit()
   atomic_set(&trans->available, 0);
   kfree(srv); trans->trb_srv[i] = NULL;
   kfree(queue);

CPU0 resumes and uses the freed que, then:

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

which dereferences a NULL or freed trb_srv.  The SKB is also stranded
because the service thread has already run mtk_ctrl_chs_flush() and
exited.  Would a mutex or RCU around the lookup and submit be more
appropriate than the flag?

> +
> +	if (mtk_queue_list_is_full(trans, que) && !force_send)
> +		return -EAGAIN;

[ ... ]

> +static int mtk_pcie_hif_cmd_func(struct mtk_md_dev *mdev, int cmd, void *data)
> +{
> +	struct mtk_ctrl_blk *ctrl_blk = mdev->ctrl_blk;
> +	struct mtk_ctrl_trans *trans;
> +	struct queue_info *que;
> +
> +	switch (cmd) {
> +	case HIF_CTRL_CMD_CHECK_TX_FULL:
> +		trans = ctrl_blk->ctrl_hw_priv;
> +		que = radix_tree_lookup(&trans->queue_tbl,
> +					((union ctrl_hif_cmd_data *)data)->rx_ch & QUEUE_CHL_MASK);
> +		if (!que) {
> +			dev_warn(mdev->dev, "Failed to find que to check tx full\n");
> +			return -EINVAL;
> +		}
> +		return mtk_queue_list_is_full(trans, que);

This path does the same lockless lookup and dereference without even
testing trans->available.  Is that intentional?

> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

[ ... ]

The remaining items below were also raised during review but appear to be
resolved by later patches in this series, or could not be shown to be
reachable.  They are listed for completeness only.

[Severity: Low]
At this commit, mtk_ctrl_cfg_m9xx.c defines an empty table:

	static const struct queue_info mtk_queue_info_m9xx[] = {
	};

with .queue_info_num = ARRAY_SIZE(mtk_queue_info_m9xx), so queue_info_num
is 0 and mtk_trans_ctrl_init() rejects it via "trans->queue_info_num <= 0",
failing probe for every device.  The later patch "net: wwan: t9xx: Add
control port" populates four entries, so this is only a transient
bisect-window concern - was that ordering deliberate?

[Severity: Low]
None of the new control-plane and CLDMA code is reachable at this commit:
mtk_ctrl_init() only stores ops, mtk_ctrl_exit() only NULLs
mdev->ctrl_blk, mtk_pci_dev_start() is a bare return 0, and nothing
assigns cd->cldma_drv_info[], so there are no DMA pools, no base_addr, no
IRQ handler and the tx_done_work/rx_done_work items are never queued.  The
TX and RX flows described in the commit message start working only after
the later "Add control port" and "Add FSM thread" patches.  Could the
commit message note that the paths are wired up later in the series?

[Severity: Low]
Related to the above: at this commit mtk_trans_ctrl_exit() calls only
mtk_ctrl_exit(), which never invokes ops->exit, so mtk_pcie_hif_exit() has
no caller and the radix-tree entries, cldma_dev, trb_srv/srv_que objects
and TRB kthreads would have no release path.  This is vacuous here since
ops->init is not called either, and the FSM teardown added later closes it.

[Severity: Low]
In mtk_ctrl_trb_handler(), the default case unlinks the SKB without
completing or freeing it, and kick stays false:

		default:
			skb_unlink(skb, skb_list);
		}

No in-tree producer can reach it, since mtk_pcie_hif_submit_skb()
completes TRB_CMD_STOP and TRB_CMD_RECOVER without queueing them and the
only other values assigned to trb->cmd are ENABLE, TX and DISABLE.  Would
completing the request there be worth it as hardening?

[Severity: Low]
In the non-BD TX path, mtk_cldma_txbuf_set() maps skb_headlen(skb) while
mtk_cldma_submit_tx() advertises data_buff_len = skb->len and records
req->data_len = skb->len for the unmap.  These agree today because non-BD
mode is chosen only when que->tx_mtu <= que->tx_frag_size, and the port
layer builds a frag_list only when port->tx_mtu > port->tx_frag_size, so
every non-BD SKB is linear.  Would an explicit skb_is_nonlinear() check or
a comment make the invariant clearer?

[Severity: Low]
skb->data is overlaid with struct trb_open_priv in mtk_ch_status_check()
and mtk_cldma_rxq_alloc() (including loading the rx_done function pointer
that is later called) with no length check on the linear region.  The only
producer, mtk_port_ch_enable(), allocates Q_MTU_3_5K and skb_put()s
sizeof(struct trb_open_priv), so there is enough room in practice.