Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <947eb9a4-683e-7d10-d15c-28e2f18d192d@linux.intel.com>
Date:   Tue, 22 Feb 2022 10:40:24 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.1
Subject: Re: [PATCH net-next v4 08/13] net: wwan: t7xx: Add data path
 interface
Content-Language: en-US
To:     =?UTF-8?Q?Ilpo_J=c3=a4rvinen?= <ilpo.jarvinen@linux.intel.com>
Cc:     Netdev <netdev@vger.kernel.org>, linux-wireless@vger.kernel.org,
        kuba@kernel.org, davem@davemloft.net, johannes@sipsolutions.net,
        ryazanov.s.a@gmail.com, loic.poulain@linaro.org,
        m.chetan.kumar@intel.com, chandrashekar.devegowda@intel.com,
        linuxwwan@intel.com, chiranjeevi.rapolu@linux.intel.com,
        haijun.liu@mediatek.com, amir.hanania@intel.com,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        dinesh.sharma@intel.com, eliot.lee@intel.com,
        moises.veleta@intel.com, pierre-louis.bossart@intel.com,
        muralidharan.sethuraman@intel.com, Soumya.Prakash.Mishra@intel.com,
        sreehari.kancharla@intel.com
References: <20220114010627.21104-1-ricardo.martinez@linux.intel.com>
 <20220114010627.21104-9-ricardo.martinez@linux.intel.com>
 <ca592f64-c581-56a8-8d90-5341ebd8932d@linux.intel.com>
From:   "Martinez, Ricardo" <ricardo.martinez@linux.intel.com>
In-Reply-To: <ca592f64-c581-56a8-8d90-5341ebd8932d@linux.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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.

...