Re: [PATCH net-next v4 08/13] net: wwan: t7xx: Add data path interface

On 2/8/2022 12:19 AM, Ilpo Järvinen wrote:

On Thu, 13 Jan 2022, Ricardo Martinez wrote:

quoted

From: Haijun Liu <haijun.liu@mediatek.com>

Data Path Modem AP Interface (DPMAIF) HIF layer provides methods
for initialization, ISR, control and event handling of TX/RX flows.

...

quoted

+	bat_req->bat_mask[idx] = 1;

...

quoted

+		if (!bat_req->bat_mask[index])

...

quoted

+		bat->bat_mask[index] = 0;

Seem to be linux/bitmap.h

I wonder though if the loop in t7xx_dpmaif_avail_pkt_bat_cnt()
could be replaced with arithmetic calculation based on
bat_release_rd_idx and some other idx? It would make the bitmap
unnecessary.

A bitmap is needed since entries could be returned out of order.

...

quoted

+	hw_read_idx = t7xx_dpmaif_ul_get_rd_idx(&dpmaif_ctrl->hif_hw_info, q_num);
+
+	new_hw_rd_idx = hw_read_idx / DPMAIF_UL_DRB_ENTRY_WORD;

Is DPMAIF_UL_DRB_ENTRY_WORD size of an entry? In that case it
would probably make sense put it inside t7xx_dpmaif_ul_get_rd_idx?

Yes, moving this into t7xx_dpmaif_ul_get_rd_idx()

quoted

+	if (new_hw_rd_idx >= DPMAIF_DRB_ENTRY_SIZE) {

Is DPMAIF_DRB_ENTRY_SIZE telling the number of entries rather than
an "ENTRY_SIZE"? I think these both constant could likely be named
better.

...

quoted

+static int t7xx_txq_burst_send_skb(struct dpmaif_tx_queue *txq)
+{
+	int drb_remain_cnt, i;
+	unsigned long flags;
+	int drb_cnt = 0;
+	int ret = 0;
+
+	spin_lock_irqsave(&txq->tx_lock, flags);
+	drb_remain_cnt = t7xx_ring_buf_rd_wr_count(txq->drb_size_cnt, txq->drb_release_rd_idx,
+						   txq->drb_wr_idx, DPMAIF_WRITE);
+	spin_unlock_irqrestore(&txq->tx_lock, flags);
+
+	for (i = 0; i < DPMAIF_SKB_TX_BURST_CNT; i++) {
+		struct sk_buff *skb;
+
+		spin_lock_irqsave(&txq->tx_skb_lock, flags);
+		skb = list_first_entry_or_null(&txq->tx_skb_queue, struct sk_buff, list);
+		spin_unlock_irqrestore(&txq->tx_skb_lock, flags);
+
+		if (!skb)
+			break;
+
+		if (drb_remain_cnt < skb->cb[TX_CB_DRB_CNT]) {
+			spin_lock_irqsave(&txq->tx_lock, flags);
+			drb_remain_cnt = t7xx_ring_buf_rd_wr_count(txq->drb_size_cnt,
+								   txq->drb_release_rd_idx,
+								   txq->drb_wr_idx, DPMAIF_WRITE);
+			spin_unlock_irqrestore(&txq->tx_lock, flags);
+			continue;
+		}

...

quoted

+	if (drb_cnt > 0) {
+		txq->drb_lack = false;
+		ret = drb_cnt;
+	} else if (ret == -ENOMEM) {
+		txq->drb_lack = true;

Based on the variable name, I'd expect drb_lack be set true when
drb_remain_cnt < skb->cb[TX_CB_DRB_CNT] occurred but that doesn't
happen. Maybe that if branch within loop should set ret = -ENOMEM;
before continue?

This drb_lack logic is going to be dropped since it was intended for

multiple Tx queues but currently only one is used.

It would be nice if the drb check here and in
t7xx_check_tx_queue_drb_available could be consolidated into
a single place. That requires small refactoring (adding __
variant of that function which does just the check).

Please also check the other comments on skb->cb below.

...

quoted

+int t7xx_dpmaif_tx_send_skb(struct dpmaif_ctrl *dpmaif_ctrl, unsigned int txqt, struct sk_buff *skb)
+{
+	bool tx_drb_available = true;

...

quoted

+	send_drb_cnt = t7xx_get_drb_cnt_per_skb(skb);
+
+	txq = &dpmaif_ctrl->txq[txqt];
+	if (!(txq->tx_skb_stat++ % DPMAIF_SKB_TX_BURST_CNT))
+		tx_drb_available = t7xx_check_tx_queue_drb_available(txq, send_drb_cnt);
+
+	if (!tx_drb_available || txq->tx_submit_skb_cnt >= txq->tx_list_max_len) {

Because of the modulo if, drbs might not be available despite
variable claims them to be. Is it intentional?

It is intentional, I'll refactor this to do the  DRB and tx_list_max_len checks independently for clarity.

...