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

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

On Mon, 17 Aug 2026 at 22:36, Jakub Kicinski <kuba@kernel.org> wrote:
> [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:
...
> 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:

You are right on both counts. Will fix in v7. On double failure
(both reload and remap fail), will goto out instead of falling
through, to avoid arming a descriptor with a stale address.

> [Severity: Medium]
> Should the result of cldma_stop_queue() be checked here and in the other
> three call sites?
...
> 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.

Will fix in v7. Will make mtk_cldma_stop_queue() properly propagate
-ETIMEDOUT, and check the return value in all 4 call sites. In the
alloc paths, will abort queue setup on timeout. In the free paths,
will log dev_warn and proceed with cleanup (resources cannot be
leaked, and the device is likely heading for reset at that point).

> [Severity: Medium]
> Are txq->free_idx and txq->tx_started safe to read here without any
> annotation or lock?

Will fix in v7. Will use READ_ONCE(txq->free_idx) in
mtk_cldma_start_xfer() for consistency with tx_done_work.

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

Will fix in v7. Will read both ADDRL and ADDRH in
mtk_cldma_get_tx_start_addr() and return a u64, then check the
full 64-bit value against zero in mtk_cldma_start_xfer().

> [Severity: Medium]
> Can a failure here permanently poison the channel?  mtk_ch_status_check()
> already incremented the user count before this function runs:
...
> 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.

Will fix in v7. Will decrement usr_cnt on mtk_cldma_open() error
paths to allow retry.

> [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().

Will fix in v7. Will call trb->trb_complete(skb) with
trb->status = -EPIPE on the !drv_info error path of
mtk_cldma_close().

> [Severity: Medium]
> Can a persistent mapping failure spin the TRB service thread at 100% CPU?
...
> 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?

Will fix in v7. Will change the dma_map failure in
mtk_cldma_txbuf_set() from -EAGAIN to -ENOMEM, and complete the
TRB with error in the handler instead of retrying indefinitely.

> [Severity: Medium]
> Does this table cover both PCI IDs the driver claims?  t9xx_pci_table[]
> lists both:
...
> 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!".

Will fix in v7. Both IDs are valid — 0x01CA is Compal's production
ID, 0x0900 is MTK's reference board ID. Will add the missing entry
to mtk_ctrl_info_tbl[] and cldma_drv_info_tbl[] so all three
tables are consistent.

> [Severity: Medium]
> Is qlen safe to read here without the queue lock, and is the depth limit
> actually enforced?
...
> Can that observe stale ->cb contents on a weakly ordered CPU and
> mis-decide whether to kick the hardware?

The SKB_LIST_MAX_LEN check is a soft backpressure limit, not a
hard safety boundary. Overshooting by a few entries under concurrent
submission is acceptable — the queue memory is bounded by the number
of active ports.

The trb_handler list traversal runs in a single kthread (one
consumer per srv_que). Producers call skb_queue_tail() which takes
the list lock internally and provides the necessary release barrier.
The consumer's skb_peek/skb_peek_next reads are safe because the
list structure is updated atomically under the lock by
skb_queue_tail, and trb->cmd is initialized before the
skb_queue_tail call (the spinlock provides the acquire/release
ordering).

> [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:
...
> 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:
...
> So can this sequence occur?
...
> CPU0 resumes and uses the freed que, then:
...
> 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?
...
> This path does the same lockless lookup and dereference without even
> testing trans->available.  Is that intentional?

You are right that the flag alone does not prevent this TOCTOU race.
In practice, the FSM disables all ports before calling hif_exit(),
which flushes pending TRBs and blocks new submissions. But a late
userspace close could race with teardown.

Will fix in v7. Will add a mutex to serialize the submit path
against the exit path, and add the available check to
mtk_pcie_hif_cmd_func() as well.

> [Severity: Low]
> At this commit, mtk_ctrl_cfg_m9xx.c defines an empty table:
...
> 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.

Yes, this is deliberate. The queue_info entries describe port
configurations (channel IDs, MTUs, queue mappings) which belong to the
port layer introduced in the next patch "Add control port". At this
commit the CLDMA infrastructure is in place but no ports are defined
yet, so the table is intentionally empty.

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

Will update the commit message to note that paths are wired up by
the later "Add control port" and "Add FSM thread" patches.

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

Right, this is vacuous — ops->init is never called at this commit
either, so there is nothing to tear down. The FSM teardown path
added in P5 closes the init/exit lifecycle.

> [Severity: Low]
> In mtk_ctrl_trb_handler(), the default case unlinks the SKB without
> completing or freeing it, and kick stays false:
...
> 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?

Will fix in v7. Will complete the TRB with -EINVAL and free the
SKB in the default case as hardening, even though no in-tree
producer can reach it today.

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

Will fix in v7. Will add a comment documenting the invariant: non-BD
mode is only used when tx_mtu <= tx_frag_size, which guarantees the
SKB is always linear, so skb_headlen() == skb->len.

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

No fix needed. Q_MTU_3_5K (3584 bytes) is far larger than
sizeof(struct trb_open_priv), and mtk_port_ch_enable() is the only
producer of ENABLE TRBs, so the allocation is always sufficient.

Thanks.

--
Jack