Re: [PATCH v2 08/14] net: wwan: t7xx: Add data path interface

[Sergey Ryazanov <ryazanov.s.a@gmail.com>]

On Mon, Nov 1, 2021 at 6:57 AM Ricardo Martinez
<ricardo.martinez@linux.intel.com> 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