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

[Wu. JackBB (GSM)]

Hi Simon,

After further internal review, we are correcting several
responses from our previous reply. The following items will
be fixed in v5.

On [date] Jack Wu wrote:

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

We Will remove the rmb() and add dma_rmb() after the HWO check in
both TX and RX done paths:

  /* TX done */
  if (!req->data_vm_addr ||
      (req->gpd->tx_gpd.gpd_flags & CLDMA_GPD_FLAG_HWO))
      break;
  dma_rmb();

  /* RX done */
  if (req->gpd->rx_gpd.gpd_flags & CLDMA_GPD_FLAG_HWO)
      break;
  dma_rmb();
  mtk_cldma_rx_skb_adjust(mdev, rxq, req);

>> [Severity: High]
>> If mtk_cldma_reload_rx_skb fails due to memory pressure, it jumps
>> to out without advancing rxq->free_idx.

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

Will change the failure path to recycle the old buffer and return
the descriptor to hardware, dropping the current packet to
guarantee forward progress.

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

Will change skb->len to skb_headlen(skb).

>> [Severity: High]
>> Could there be a race condition here with mtk_cldma_tx_done_work?

> No race exists. tx_done_work sees HWO still set.

Will add a free_idx == wr_idx boundary check at the top of the
done_work loop (using READ_ONCE/WRITE_ONCE) to prevent done_work
from crossing into submit territory on descriptor reuse.

Thanks