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

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

Hi Simon,

> > +static void mtk_cldma_tx_done_work(...)
> > +	if (!req->data_vm_addr || (req->gpd->tx_gpd.gpd_flags & CLDMA_GPD_FLAG_HWO))
>
> [Severity: High]
> Is a dma_rmb() needed after checking the hardware ownership flag?

The GPD descriptors are allocated from a coherent DMA pool
(dma_pool_zalloc), so CPU cache coherency is guaranteed by the DMA
mapping. A dma_rmb() is not needed because coherent memory ensures
the CPU always observes the latest values written by the device.

Additionally, there is an rmb() before this check that ensures the
HWO flag is read before any subsequent field accesses.

> > +static void mtk_cldma_rx_done_work(...)
> > +	if (req->gpd->rx_gpd.gpd_flags & CLDMA_GPD_FLAG_HWO)
> > +		break;
> > +	mtk_cldma_rx_skb_adjust(mdev, rxq, req);
>
> [Severity: High]
> Similarly, would we need a dma_rmb() here before attempting to read the
> receive length out of the descriptor in mtk_cldma_rx_skb_adjust?

Same reasoning as above. GPD descriptors use coherent DMA memory
(dma_pool_zalloc), so no additional memory barrier is needed.

> > +	ret = mtk_cldma_reload_rx_skb(mdev, rxq, req);
> > +	if (ret)
> > +		goto out;
>
> [Severity: High]
> If mtk_cldma_reload_rx_skb fails due to memory pressure, it jumps to out
> without advancing rxq->free_idx. Will this cause the worker to permanently
> stall the queue?

If mtk_cldma_reload_rx_skb() fails, the code jumps to "out" which
unmasks the interrupt. On the next hardware interrupt, rx_done_work
runs again and retries from the same free_idx. This creates a retry
loop that depends on memory becoming available. Under sustained
memory pressure the RX queue stalls but recovers once memory is
freed. This is acceptable for a control plane interface with small,
infrequent messages.

> > +static void mtk_cldma_txq_free(...)
> > +	kfree(req->bd_dsc_pool);
> > +	kfree(txq->req_pool);
> > +	kfree(txq);
>
> [Severity: Critical]
> Does this incorrectly free memory managed by devres?

We will convert all four from devm_kcalloc to kcalloc in v5. These
are runtime-managed resources (allocated on queue open, freed on
queue close/teardown), so they should use kcalloc/kfree rather
than devres.

> > +static int mtk_cldma_reload_rx_skb(...)
> > +	dev_kfree_skb_any(bd_dsc->skb);
>
> [Severity: High]
> Can this lead to a double free?

We will add bd_dsc->skb = NULL after dev_kfree_skb_any()
in the error path to prevent double free during driver teardown.

> > +static int mtk_cldma_txbuf_set(...)
> > +	req->data_dma_addr = dma_map_single(mdev->dev, skb->data,
> > +					skb->len, DMA_TO_DEVICE);
>
> [Severity: High]
> Does this code safely handle non-linear SKBs?

When nr_bds == 0, the SKB is always linear — it is allocated
internally via __dev_alloc_skb() with data copied via skb_put().
This is a control plane interface where SKBs are constructed
internally (AT commands, MBIM control messages), not received
from the network stack. Paged fragments are never used.

When nr_bds > 0, fragmentation uses skb_shinfo(skb)->frag_list
(linked SKBs), not paged fragments (skb_shinfo(skb)->frags).

> > +static int mtk_cldma_submit_tx(...)
> > +	req->data_vm_addr = skb->data;
> > +	wmb();
> > +	req->gpd->tx_gpd.gpd_flags |= CLDMA_GPD_FLAG_HWO;
>
> [Severity: High]
> Could there be a race condition here with mtk_cldma_tx_done_work?

No race exists. tx_done_work checks:
  if (!req->data_vm_addr || (gpd_flags & CLDMA_GPD_FLAG_HWO))
      break;

It only proceeds when data_vm_addr is non-NULL AND HWO is cleared.
In submit_tx, data_vm_addr is set before HWO via wmb(). After
data_vm_addr is set but before HWO, tx_done_work sees HWO still
set (hardware hasn't completed DMA) and breaks. The rmb() in
tx_done_work ensures HWO is read first. tx_done_work only
processes this req after hardware clears HWO upon DMA completion.

> > +static void mtk_pci_remove(struct pci_dev *pdev)
> > +	pci_clear_master(pdev);
> > +	mtk_pci_dev_exit(mdev);
>
> [Severity: High]
> Is it intended to call pci_clear_master before mtk_pci_dev_exit?

Before this point, mtk_pci_pldr() has already power-cycled the
modem firmware via ACPI PXP._OFF/_ON. After PLDR, the modem is
in a fresh boot state with no active DMA. The CLDMA queues are
idle — there are no pending hardware DMA transactions.

pci_clear_master() disables bus mastering as a safety measure
before teardown. Since the hardware is already reset and idle,
this is the intended ordering.

> > +	err = mtk_cldma_submit_tx(trans->dev, skb);
> > +	if (err) {
> > +		if (err == -EAGAIN)
> > +			return;
>
> [Severity: High]
> If mtk_cldma_submit_tx returns -EAGAIN ... could this cause the
> mtk_ctrl_trb_thread loop to spin continuously without sleeping?

The -EAGAIN path returns from mtk_ctrl_trb_handler() to
mtk_ctrl_trb_thread(), which calls wait_event_interruptible().
After -EAGAIN, the skb remains on the list (non-empty), but the
CLDMA ring is full. The ring drains as hardware completes DMA,
triggering tx_done_work which calls wake_up on the trb_waitq.
The kthread sleeps until hardware completion wakes it — no
busy-spin occurs.

> > +	wait_event_interruptible(srv->trb_waitq,
> > +				!mtk_ctrl_chs_is_busy_or_empty(srv) ||
>
> [Severity: High]
> Can this cause an infinite loop if a signal is delivered to the thread?

Kernel threads in this driver are not targeted by user signals.
The kthread_stop and kthread_should_park mechanisms are the
primary lifecycle management, and both are checked in the wait
condition. This pattern is consistent with other WWAN drivers
(e.g., t7xx) and widely used across drivers/net/ where
wait_event_interruptible is the standard choice for kthread
wait loops.

> > +	radix_tree_for_each_slot(slot, &trans->queue_tbl, &iter, 0) {
> > +		queue = radix_tree_deref_slot(slot);
> > +		radix_tree_delete(&trans->queue_tbl, iter.index);
>
> [Severity: High]
> Is it safe to iterate over the radix tree and call radix_tree_delete
> without holding rcu_read_lock?

This function is only called during teardown after all users have
been stopped (FSM shutdown, kthreads stopped, queues drained).
There is no concurrent access to the radix tree at this point.

The tree was initialized with INIT_RADIX_TREE (not RCU-tagged),
and we are the sole accessor during teardown. The cursor-based
iteration is safe for single-threaded deletion.

Thanks.

Jack Wu