On Mon, Nov 1, 2021 at 6:57 AM Ricardo Martinez
<ricardo.martinez@xxxxxxxxxxxxxxx> 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]
> diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
> ...
> +static int dpmaif_net_rx_push_thread(void *arg)
> +{
> ...
> + while (!kthread_should_stop()) {
> + if (skb_queue_empty(&q->skb_queue.skb_list)) {
> + if (wait_event_interruptible(q->rx_wq,
> + !skb_queue_empty(&q->skb_queue.skb_list) ||
> + kthread_should_stop()))
> + continue;
> + }
> +
> + if (kthread_should_stop())
> + break;
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]
> diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.h b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.h
> ...
> +/* lhif header feilds */
> +#define LHIF_HEADER_NW_TYPE GENMASK(31, 29)
> +#define LHIF_HEADER_NETIF GENMASK(28, 24)
> +#define LHIF_HEADER_F GENMASK(22, 20)
> +#define LHIF_HEADER_FLOW GENMASK(19, 16)
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;
};
> diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c
> ...
> +static int dpmaif_tx_send_skb_on_tx_thread(struct dpmaif_ctrl *dpmaif_ctrl,
> + struct dpmaif_tx_event *event)
> +{
> ...
> + struct ccci_header ccci_h;
> ...
> + skb = event->skb;
> ...
> + ccci_h = *(struct ccci_header *)skb->data;
> + skb_pull(skb, sizeof(struct ccci_header));
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
