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

["Martinez, Ricardo" <ricardo.martinez@linux.intel.com>]

On 2/8/2022 12:19 AM, Ilpo Järvinen wrote:
> On Thu, 13 Jan 2022, Ricardo Martinez wrote:
>
>> 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.
...
>> +	bat_req->bat_mask[idx] = 1;
> ...
>> +		if (!bat_req->bat_mask[index])
> ...
>> +		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.

...

>> +	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()
>> +	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.
...
>> +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;
>> +		}
> ...
>> +	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.
...
>
>> +int t7xx_dpmaif_tx_send_skb(struct dpmaif_ctrl *dpmaif_ctrl, unsigned int txqt, struct sk_buff *skb)
>> +{
>> +	bool tx_drb_available = true;
> ...
>> +	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.

...