On Mon, Nov 1, 2021 at 6:57 AM Ricardo Martinez
[off-list ref] wrote:

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

DPMAIF TX
Exposes the `dmpaif_tx_send_skb` function which can be used by the
network device to transmit packets.
The uplink data management uses a Descriptor Ring Buffer (DRB).
First DRB entry is a message type that will be followed by 1 or more
normal DRB entries. Message type DRB will hold the skb information
and each normal DRB entry holds a pointer to the skb payload.

DPMAIF RX
The downlink buffer management uses Buffer Address Table (BAT) and
Packet Information Table (PIT) rings.
The BAT ring holds the address of skb data buffer for the HW to use,
while the PIT contains metadata about a whole network packet including
a reference to the BAT entry holding the data buffer address.
The driver reads the PIT and BAT entries written by the modem, when
reaching a threshold, the driver will reload the PIT and BAT rings.

[skipped]

Looks like the above check is used to recheck thread state after the
wait_event_interruptible() call, so the check could be moved to the
skb_queue_empty() code block to avoid odd thread state checks.

...
+static void dpmaif_rx_skb(struct dpmaif_rx_queue *rxq, struct dpmaif_cur_rx_skb_info *rx_skb_info)
+{
+       struct sk_buff *new_skb;
+       u32 *lhif_header;
+
+       new_skb = rx_skb_info->cur_skb;
...
+       /* MD put the ccmni_index to the msg pkt,
+        * so we need push it alone. Maybe not needed.
+        */
+       lhif_header = skb_push(new_skb, sizeof(*lhif_header));
+       *lhif_header &= ~LHIF_HEADER_NETIF;
+       *lhif_header |= FIELD_PREP(LHIF_HEADER_NETIF, rx_skb_info->cur_chn_idx);

Why is the skb data field used to carry packet control data? Consider
using the skb control buffer (i.e skb->cb) to carry control data
between the driver layers to make metadata handling less expensive and
increase driver performance.

+       /* add data to rx thread skb list */
+       ccci_skb_enqueue(&rxq->skb_queue, new_skb);
+}
...
+void dpmaif_rxq_free(struct dpmaif_rx_queue *queue)
+{
...
+       while ((skb = skb_dequeue(&queue->skb_queue.skb_list)))
+               kfree_skb(skb);

skb_queue_purge()

...
+static int dpmaif_skb_queue_init_struct(struct dpmaif_ctrl *dpmaif_ctrl,
+                                       const unsigned int index)
+{
...
+       INIT_LIST_HEAD(&queue->skb_list.head);
+       spin_lock_init(&queue->skb_list.lock);
+       queue->skb_list.qlen = 0;

skb_queue_head_init()

[skipped]

Just place control data to the skb control buffer (i.e. skb->cb) and
define this control data as a structure:

struct rx_pkt_cb {
        u8 nw_type;
        u8 netif;
        u8 flow;
};

Place this metadata to the skb control buffer (i.e. skb->cb) to avoid
odd skb_push()/skb_pull() calls.

Also this looks like an abuse of ccci_header structure. In fact it
never passed to the modem along with a data packet, but searching
through the code show this as a structure usage place.

...
+int dpmaif_tx_send_skb(struct dpmaif_ctrl *dpmaif_ctrl, enum txq_type txqt, struct sk_buff *skb)
+{
...
+       if (txq->tx_submit_skb_cnt < txq->tx_list_max_len && tx_drb_available) {
+               struct dpmaif_tx_event *event;
...
+               event = kmalloc(sizeof(*event), GFP_ATOMIC);
...
+               INIT_LIST_HEAD(&event->entry);
+               event->qno = txqt;
+               event->skb = skb;
+               event->drb_cnt = send_drb_cnt;

Please, place the packet metadata (dpmaif_tx_event data) in the skb
control buffer (i.e. skb->cb) and use skb queue API as in Rx path.
This will allow you to avoid the per-packet metadata memory allocation
and make code simple.

+               spin_lock_irqsave(&txq->tx_event_lock, flags);
+               list_add_tail(&event->entry, &txq->tx_event_queue);
+               txq->tx_submit_skb_cnt++;
+               spin_unlock_irqrestore(&txq->tx_event_lock, flags);
+               wake_up(&dpmaif_ctrl->tx_wq);
+
+               return 0;
+       }
+
+       cb = dpmaif_ctrl->callbacks;
+       cb->state_notify(dpmaif_ctrl->mtk_dev, DMPAIF_TXQ_STATE_FULL, txqt);
+
+       return -EBUSY;

It is better to invert the above condition, handle TXQ full situation
as a corner case and packet queuing as a normal case. I.e. instead of:

        if (have_queue_space) {
                /* Enqueue packet */
                return 0;
        }
        /* Queue full notification emitting */
        return -EBUSY;

handle queuing like this:

        if (unlikely(!have_queue_space)) {
                /* Queue full notification emitting */
                return -EBUSY;
        }
        /* Enqueue packet */
        return 0;

This is a matter of taste, but makes code more readable.

--
Sergey