Date:   Fri, 8 Feb 2019 14:28:56 -0800
From:   Brian Norris <briannorris@chromium.org>
To:     yhchuang@realtek.com
Cc:     kvalo@codeaurora.org, johannes@sipsolutions.net,
        Larry.Finger@lwfinger.net, pkshih@realtek.com, tehuang@realtek.com,
        sgruszka@redhat.com, linux-wireless@vger.kernel.org
Subject: Re: [PATCH v4 03/13] rtw88: hci files
Message-ID: <20190208222855.GA31454@google.com>
References: <1548820940-15237-1-git-send-email-yhchuang@realtek.com>
 <1548820940-15237-4-git-send-email-yhchuang@realtek.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1548820940-15237-4-git-send-email-yhchuang@realtek.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Hi,

One more comment for now, below:

On Wed, Jan 30, 2019 at 12:02:10PM +0800, yhchuang@realtek.com wrote:
> From: Yan-Hsuan Chuang <yhchuang@realtek.com>
> 
> hci files for Realtek 802.11ac wireless network chips
> 
> For now there is only PCI bus supported by rtwlan, in the future it
> will also have USB/SDIO
> 
> Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
> ---
>  drivers/net/wireless/realtek/rtw88/hci.h |  211 ++++++
>  drivers/net/wireless/realtek/rtw88/pci.c | 1210 ++++++++++++++++++++++++++++++
>  drivers/net/wireless/realtek/rtw88/pci.h |  229 ++++++
>  3 files changed, 1650 insertions(+)
>  create mode 100644 drivers/net/wireless/realtek/rtw88/hci.h
>  create mode 100644 drivers/net/wireless/realtek/rtw88/pci.c
>  create mode 100644 drivers/net/wireless/realtek/rtw88/pci.h

...

> diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
> new file mode 100644
> index 0000000..ef3c9bb
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> @@ -0,0 +1,1210 @@

> +static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
> +			   u8 hw_queue)
> +{
> +	struct rtw_chip_info *chip = rtwdev->chip;
> +	struct rtw_pci_rx_ring *ring;
> +	struct rtw_rx_pkt_stat pkt_stat;
> +	struct ieee80211_rx_status rx_status;
> +	struct sk_buff *skb, *new;
> +	u32 cur_wp, cur_rp, tmp;
> +	u32 count;
> +	u32 pkt_offset;
> +	u32 pkt_desc_sz = chip->rx_pkt_desc_sz;
> +	u32 buf_desc_sz = chip->rx_buf_desc_sz;
> +	u8 *rx_desc;
> +	dma_addr_t dma;
> +
> +	ring = &rtwpci->rx_rings[RTW_RX_QUEUE_MPDU];
> +
> +	tmp = rtw_read32(rtwdev, RTK_PCI_RXBD_IDX_MPDUQ);
> +	cur_wp = tmp >> 16;
> +	cur_wp &= 0xfff;
> +	if (cur_wp >= ring->r.wp)
> +		count = cur_wp - ring->r.wp;
> +	else
> +		count = ring->r.len - (ring->r.wp - cur_wp);
> +
> +	cur_rp = ring->r.rp;
> +	while (count--) {
> +		rtw_pci_dma_check(rtwdev, ring, cur_rp);
> +		skb = ring->buf[cur_rp];
> +		dma = *((dma_addr_t *)skb->cb);
> +		pci_unmap_single(rtwpci->pdev, dma, RTK_PCI_RX_BUF_SIZE,
> +				 PCI_DMA_FROMDEVICE);
> +		rx_desc = skb->data;
> +		chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat, &rx_status);
> +
> +		/* offset from rx_desc to payload */
> +		pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
> +			     pkt_stat.shift;
> +
> +		if (pkt_stat.is_c2h) {
> +			/* keep rx_desc, halmac needs it */
> +			skb_put(skb, pkt_stat.pkt_len + pkt_offset);
> +
> +			/* pass offset for further operation */
> +			*((u32 *)skb->cb) = pkt_offset;
> +			skb_queue_tail(&rtwdev->c2h_queue, skb);
> +			ieee80211_queue_work(rtwdev->hw, &rtwdev->c2h_work);
> +		} else {
> +			/* remove rx_desc, maybe use skb_pull? */
> +			skb_put(skb, pkt_stat.pkt_len);
> +			skb_reserve(skb, pkt_offset);
> +
> +			/* alloc a smaller skb to mac80211 */
> +			new = dev_alloc_skb(pkt_stat.pkt_len);
> +			if (!new) {
> +				new = skb;
> +			} else {
> +				skb_put_data(new, skb->data, skb->len);
> +				dev_kfree_skb_any(skb);
> +			}
> +			/* TODO: merge into rx.c */
> +			rtw_rx_stats(rtwdev, pkt_stat.vif, skb);
> +			memcpy(new->cb, &rx_status, sizeof(rx_status));
> +			ieee80211_rx_irqsafe(rtwdev->hw, new);
> +		}
> +
> +		/* skb delivered to mac80211, alloc a new one in rx ring */
> +		new = dev_alloc_skb(RTK_PCI_RX_BUF_SIZE);
> +		if (WARN(!new, "rx routine starvation\n"))
> +			return;
> +
> +		ring->buf[cur_rp] = new;
> +		rtw_pci_reset_rx_desc(rtwdev, new, ring, cur_rp, buf_desc_sz);

You aren't handling failures from this function. It's not quite clear to
me whether that will just leak the SKB, or if it will end in an
unbalanced unmap later.

Brian

> +
> +		/* host read next element in ring */
> +		if (++cur_rp >= ring->r.len)
> +			cur_rp = 0;
> +	}
> +
> +	ring->r.rp = cur_rp;
> +	ring->r.wp = cur_wp;
> +	rtw_write16(rtwdev, RTK_PCI_RXBD_IDX_MPDUQ, ring->r.rp);
> +}