[v4,03/13] rtw88: hci files

Commit Message

Tony Chuang Jan. 30, 2019, 4:02 a.m. UTC

From: Yan-Hsuan Chuang <[e-mail]>

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 <[e-mail]>
---
 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

Comments

Brian Norris Jan. 31, 2019, 10:36 p.m. UTC | #1

Hi,

A few scattered comments:

On Wed, Jan 30, 2019 at 12:02:10PM +0800, [e-mail] wrote:
> From: Yan-Hsuan Chuang <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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 int rtw_pci_init_rx_ring(struct rtw_dev *rtwdev,
> +				struct rtw_pci_rx_ring *rx_ring,
> +				u8 desc_size, u32 len)
> +{
> +	struct pci_dev *pdev = to_pci_dev(rtwdev->dev);
> +	struct sk_buff *skb = NULL;
> +	dma_addr_t dma;
> +	u8 *head;
> +	int ring_sz = desc_size * len;
> +	int buf_sz = RTK_PCI_RX_BUF_SIZE;
> +	int i, allocated;
> +	int ret = 0;
> +
> +	head = pci_zalloc_consistent(pdev, ring_sz, &dma);
> +	if (!head) {
> +		rtw_err(rtwdev, "failed to allocate rx ring\n");
> +		return -ENOMEM;
> +	}
> +	rx_ring->r.head = head;
> +
> +	for (i = 0; i < len; i++) {
> +		skb = dev_alloc_skb(buf_sz);
> +		if (!skb) {
> +			allocated = i;
> +			ret = -ENOMEM;
> +			goto err_out;
> +		}
> +
> +		memset(skb->data, 0, buf_sz);
> +		rx_ring->buf[i] = skb;
> +		ret = rtw_pci_reset_rx_desc(rtwdev, skb, rx_ring, i, desc_size);
> +		if (ret) {
> +			allocated = i;
> +			goto err_out;
> +		}
> +	}
> +
> +	rx_ring->r.dma = dma;
> +	rx_ring->r.len = len;
> +	rx_ring->r.desc_size = desc_size;
> +	rx_ring->r.wp = 0;
> +	rx_ring->r.rp = 0;
> +
> +	return 0;
> +
> +err_out:
> +	dev_kfree_skb_any(skb);

Maybe I'm misreading but...shouldn't this line not be here? You properly
iterate over the allocated SKBs below, and this looks like you're just
going to be double-freeing (or, negative ref-counting).

> +	for (i = 0; i < allocated; i++) {
> +		skb = rx_ring->buf[i];
> +		if (!skb)
> +			continue;
> +		dma = *((dma_addr_t *)skb->cb);
> +		pci_unmap_single(pdev, dma, buf_sz, PCI_DMA_FROMDEVICE);
> +		dev_kfree_skb_any(skb);
> +		rx_ring->buf[i] = NULL;
> +	}
> +	pci_free_consistent(pdev, ring_sz, head, dma);
> +
> +	rtw_err(rtwdev, "failed to init rx buffer\n");
> +
> +	return ret;
> +}
> +

...

> +static void rtw_pci_dma_check(struct rtw_dev *rtwdev,
> +			      struct rtw_pci_rx_ring *rx_ring,
> +			      u32 idx)
> +{
> +	struct rtw_chip_info *chip = rtwdev->chip;
> +	struct rtw_pci_rx_buffer_desc *buf_desc;
> +	u32 desc_sz = chip->rx_buf_desc_sz;
> +	u16 total_pkt_size;
> +	int i;
> +
> +	buf_desc = (struct rtw_pci_rx_buffer_desc *)(rx_ring->r.head +
> +						     idx * desc_sz);
> +	for (i = 0; i < 20; i++) {
> +		total_pkt_size = le16_to_cpu(buf_desc->total_pkt_size);
> +		if (total_pkt_size)
> +			return;
> +	}


Umm, what are you trying to do here? This is a non-sensical loop. I
*imagine* you're trying to do some kind of timeout loop here, but since
there's nothing telling the compiler that this is anything but normal
memory, this loop gets flattened by the compiler into a single check of
->total_pkt_size (I checked; my compiler gets rid of the loop).

So, at a minimum, you should just remove the loop. But I'm not sure if
this "check" function has any value at all...

> +
> +	if (i >= 20)
> +		rtw_warn(rtwdev, "pci bus timeout, drop packet\n");

...BTW, I'm seeing this get triggered quite a bit.

Do you have some kind of memory mapping/ordering issue or something? I
wouldn't think you should expect to just drop packets on the floor so
often like this.

> +}
> +
...

> +
> +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);
> +
> +		/* 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);
> +}
> +

...

> +static void rtw_pci_parse_configuration(struct rtw_dev *rtwdev,
> +					struct pci_dev *pdev)
> +{
> +	u16 link_control;
> +	u8 config;
> +

In general, this function still uses several magic numbers. It would be
nice to use macro constants, or at least include a few more comments.

> +	/* Disable Clk Request */
> +	pci_write_config_byte(pdev, 0x81, 0);

	pcie_capability_clear_word(pdev, PCI_EXP_LNKCTL,
				   PCI_EXP_LNKCTL_CLKREQ_EN);

Or even better, just:

	pci_disable_link_state(pdev, PCIE_LINK_STATE_CLKPM);

> +	/* leave D3 mode */
> +	pci_write_config_byte(pdev, 0x44, 0);

This looks like you're trying to do pci_set_power_state(dev, PCI_D0).
But that's already part of pci_enable_device(), no?

> +	pci_write_config_byte(pdev, 0x04, 0x06);

Use the PCI_COMMAND constant?

And, it looks like maybe you're really trying to do a read-modify-write?
But anyway, we have PCI_COMMAND_{IO,MEMORY,MASTER} constants for this.

> +	pci_write_config_byte(pdev, 0x04, 0x07);

Same here.

Also, what exactly is the purpose here? You're disabling IO and the
re-enabling it? Is that a hardware quirk, or something else?

> +
> +	pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &link_control);

What are you reading this for? You never use it. Remove?

> +
> +	pci_read_config_byte(pdev, 0x98, &config);
> +	config |= BIT(4);
> +	pci_write_config_byte(pdev, 0x98, config);

The above 3 can just be:

	pcie_capability_set_word(pdev, PCI_EXP_DEVCTL2,
				 PCI_EXP_DEVCTL2_COMP_TMOUT_DIS);

> +
> +	pci_write_config_byte(pdev, 0x70f, 0x17);

I couldn't figure out if this was any standard register. Add comments
and/or a macro definition?

> +}
> +
> +static int rtw_pci_claim(struct rtw_dev *rtwdev, struct pci_dev *pdev)
> +{
> +	int ret;
> +
> +	ret = pci_enable_device(pdev);
> +	if (ret) {
> +		rtw_err(rtwdev, "failed to enable pci device\n");
> +		return ret;
> +	}
> +
> +	pci_set_master(pdev);
> +	pci_set_drvdata(pdev, rtwdev->hw);
> +	SET_IEEE80211_DEV(rtwdev->hw, &pdev->dev);
> +
> +	return 0;
> +}
> +
> +static void rtw_pci_declaim(struct rtw_dev *rtwdev, struct pci_dev *pdev)
> +{
> +	pci_clear_master(pdev);
> +	pci_disable_device(pdev);
> +}
> +
> +static int rtw_pci_setup_resource(struct rtw_dev *rtwdev, struct pci_dev *pdev)
> +{
> +	struct rtw_pci *rtwpci;
> +	int ret;
> +
> +	rtwpci = (struct rtw_pci *)rtwdev->priv;
> +	rtwpci->pdev = pdev;
> +
> +	/* after this driver can access to hw registers */
> +	ret = rtw_pci_io_mapping(rtwdev, pdev);
> +	if (ret) {
> +		rtw_err(rtwdev, "failed to request pci io region\n");
> +		goto err_out;
> +	}
> +
> +	ret = rtw_pci_init(rtwdev);
> +	if (ret) {
> +		rtw_err(rtwdev, "failed to allocate pci resources\n");
> +		goto err_io_unmap;
> +	}
> +
> +	rtw_pci_parse_configuration(rtwdev, pdev);
> +	rtw_pci_phy_cfg(rtwdev);
> +
> +	return 0;
> +
> +err_io_unmap:
> +	rtw_pci_io_unmapping(rtwdev, pdev);
> +
> +err_out:
> +	return ret;
> +}
> +
> +static void rtw_pci_destroy(struct rtw_dev *rtwdev, struct pci_dev *pdev)
> +{
> +	rtw_pci_deinit(rtwdev);
> +	rtw_pci_io_unmapping(rtwdev, pdev);
> +}
> +
> +static int rtw_pci_probe(struct pci_dev *pdev,
> +			 const struct pci_device_id *id)
> +{
> +	struct ieee80211_hw *hw;
> +	struct rtw_dev *rtwdev;
> +	int drv_data_size;
> +	int ret;
> +
> +	drv_data_size = sizeof(struct rtw_dev) + sizeof(struct rtw_pci);
> +	hw = ieee80211_alloc_hw(drv_data_size, &rtw_ops);
> +	if (!hw) {
> +		dev_err(&pdev->dev, "failed to allocate hw\n");
> +		return -ENOMEM;
> +	}
> +
> +	rtwdev = hw->priv;
> +	rtwdev->hw = hw;
> +	rtwdev->dev = &pdev->dev;
> +	rtwdev->chip = (struct rtw_chip_info *)id->driver_data;
> +	rtwdev->hci.ops = &rtw_pci_ops;
> +	rtwdev->hci.type = RTW_HCI_TYPE_PCIE;
> +
> +	ret = rtw_core_init(rtwdev);
> +	if (ret)
> +		goto err_release_hw;
> +
> +	rtw_dbg(rtwdev,
> +		"rtw88 pci probe: vendor=0x%4.04X device=0x%4.04X rev=%d\n",
> +		pdev->vendor, pdev->device, pdev->revision);
> +
> +	ret = rtw_pci_claim(rtwdev, pdev);
> +	if (ret) {
> +		rtw_err(rtwdev, "failed to claim pci device\n");
> +		goto err_deinit_core;
> +	}
> +
> +	ret = rtw_pci_setup_resource(rtwdev, pdev);
> +	if (ret) {
> +		rtw_err(rtwdev, "failed to setup pci resources\n");
> +		goto err_pci_declaim;
> +	}
> +
> +	ret = rtw_chip_info_setup(rtwdev);
> +	if (ret) {
> +		rtw_err(rtwdev, "failed to setup chip information\n");
> +		goto err_destroy_pci;
> +	}
> +
> +	ret = rtw_register_hw(rtwdev, hw);
> +	if (ret) {
> +		rtw_err(rtwdev, "failed to register hw\n");
> +		goto err_destroy_pci;
> +	}
> +
> +	ret = request_irq(pdev->irq, &rtw_pci_interrupt_handler,
> +			  IRQF_SHARED, KBUILD_MODNAME, rtwdev);
> +	if (ret) {
> +		ieee80211_unregister_hw(hw);
> +		goto err_destroy_pci;
> +	}
> +
> +	return 0;
> +
> +err_destroy_pci:
> +	rtw_pci_destroy(rtwdev, pdev);
> +
> +err_pci_declaim:
> +	rtw_pci_declaim(rtwdev, pdev);
> +
> +err_deinit_core:
> +	rtw_core_deinit(rtwdev);
> +
> +err_release_hw:
> +	ieee80211_free_hw(hw);
> +
> +	return ret;
> +}
> +
> +static void rtw_pci_remove(struct pci_dev *pdev)
> +{
> +	struct ieee80211_hw *hw = pci_get_drvdata(pdev);
> +	struct rtw_dev *rtwdev;
> +	struct rtw_pci *rtwpci;
> +
> +	if (!hw)
> +		return;
> +
> +	rtwdev = hw->priv;
> +	rtwpci = (struct rtw_pci *)rtwdev->priv;
> +
> +	rtw_unregister_hw(rtwdev, hw);
> +	rtw_pci_disable_interrupt(rtwdev, rtwpci);
> +	rtw_pci_destroy(rtwdev, pdev);
> +	rtw_pci_declaim(rtwdev, pdev);
> +	free_irq(rtwpci->pdev->irq, rtwdev);
> +	rtw_core_deinit(rtwdev);
> +	ieee80211_free_hw(hw);
> +}
> +
> +static const struct pci_device_id rtw_pci_id_table[] = {
> +#ifdef CONFIG_RTW88_8822BE
> +	{ RTK_PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0xB822, rtw8822b_hw_spec) },
> +#endif
> +#ifdef CONFIG_RTW88_8822CE
> +	{ RTK_PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0xC822, rtw8822c_hw_spec) },
> +#endif
> +	{},
> +};
> +
> +static struct pci_driver rtw_pci_driver = {
> +	.name = "rtw_pci",
> +	.id_table = rtw_pci_id_table,
> +	.probe = rtw_pci_probe,
> +	.remove = rtw_pci_remove,
> +};
> +
> +struct rtw_hci_ops rtw_pci_ops = {

This struct is local to this unit. Please move it up above where it's
used and make it static.

> +	.tx = rtw_pci_tx,
> +	.setup = rtw_pci_setup,
> +	.start = rtw_pci_start,
> +	.stop = rtw_pci_stop,
> +
> +	.read8 = rtw_pci_read8,
> +	.read16 = rtw_pci_read16,
> +	.read32 = rtw_pci_read32,
> +	.write8 = rtw_pci_write8,
> +	.write16 = rtw_pci_write16,
> +	.write32 = rtw_pci_write32,
> +	.write_data_rsvd_page = rtw_pci_write_data_rsvd_page,
> +	.write_data_h2c = rtw_pci_write_data_h2c,
> +};
> +
> +MODULE_DEVICE_TABLE(pci, rtw_pci_id_table);

Normally, the MODULE_DEVICE_TABLE() is best kept closer to
rtw_pci_id_table (immediately following its definition). Move it up?

> +
> +module_pci_driver(rtw_pci_driver);

Same here -- once the other things move out of the way, keep this close
to rtw_pci_driver?

Brian

> +
> +MODULE_AUTHOR("Realtek Corporation");
> +MODULE_DESCRIPTION("Realtek 802.11ac wireless PCI driver");
> +MODULE_LICENSE("GPL");
...

Brian Norris Feb. 8, 2019, 10:28 p.m. UTC | #2

Hi,

One more comment for now, below:

On Wed, Jan 30, 2019 at 12:02:10PM +0800, [e-mail] wrote:
> From: Yan-Hsuan Chuang <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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);
> +}

Brian Norris Feb. 9, 2019, 2:14 a.m. UTC | #3

FYI, I have some more review comments because I'm trying to see why your
TX path doesn't work all that well. At least, it's not reporting things
correctly. (I know there's one ACK reporting bug you fixed in a
follow-up patch, but then, that patch is buggy too I think.)

On Wed, Jan 30, 2019 at 12:02:10PM +0800, [e-mail] wrote:
> From: Yan-Hsuan Chuang <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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/hci.h b/drivers/net/wireless/realtek/rtw88/hci.h
> new file mode 100644
> index 0000000..91b15ef
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw88/hci.h
> @@ -0,0 +1,211 @@

...

> +static int rtw_pci_xmit(struct rtw_dev *rtwdev,
> +			struct rtw_tx_pkt_info *pkt_info,
> +			struct sk_buff *skb, u8 queue)
> +{
> +	struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
> +	struct rtw_chip_info *chip = rtwdev->chip;
> +	struct rtw_pci_tx_ring *ring;
> +	struct rtw_pci_tx_data *tx_data;
> +	dma_addr_t dma;
> +	u32 tx_pkt_desc_sz = chip->tx_pkt_desc_sz;
> +	u32 tx_buf_desc_sz = chip->tx_buf_desc_sz;
> +	u32 size;
> +	u32 psb_len;
> +	u8 *pkt_desc;
> +	struct rtw_pci_tx_buffer_desc *buf_desc;
> +	u32 bd_idx;
> +
> +	ring = &rtwpci->tx_rings[queue];
> +
> +	size = skb->len;
> +
> +	if (queue == RTW_TX_QUEUE_BCN)
> +		rtw_pci_release_rsvd_page(rtwpci, ring);
> +	else if (!avail_desc(ring->r.wp, ring->r.rp, ring->r.len))
> +		return -ENOSPC;
> +
> +	pkt_desc = skb_push(skb, chip->tx_pkt_desc_sz);
> +	memset(pkt_desc, 0, tx_pkt_desc_sz);
> +	pkt_info->qsel = rtw_pci_get_tx_qsel(skb, queue);
> +	rtw_tx_fill_tx_desc(pkt_info, skb);
> +	dma = pci_map_single(rtwpci->pdev, skb->data, skb->len,
> +			     PCI_DMA_TODEVICE);
> +	if (pci_dma_mapping_error(rtwpci->pdev, dma))
> +		return -EBUSY;
> +
> +	/* after this we got dma mapped, there is no way back */
> +	buf_desc = get_tx_buffer_desc(ring, tx_buf_desc_sz);
> +	memset(buf_desc, 0, tx_buf_desc_sz);
> +	psb_len = (skb->len - 1) / 128 + 1;
> +	if (queue == RTW_TX_QUEUE_BCN)
> +		psb_len |= 1 << RTK_PCI_TXBD_OWN_OFFSET;
> +
> +	buf_desc[0].psb_len = cpu_to_le16(psb_len);
> +	buf_desc[0].buf_size = cpu_to_le16(tx_pkt_desc_sz);
> +	buf_desc[0].dma = cpu_to_le32(dma);
> +	buf_desc[1].buf_size = cpu_to_le16(size);
> +	buf_desc[1].dma = cpu_to_le32(dma + tx_pkt_desc_sz);
> +
> +	tx_data = rtw_pci_get_tx_data(skb);
> +	tx_data->dma = dma;
> +	skb_queue_tail(&ring->queue, skb);

IIUC, you have no locking for this queue. That seems like a bad idea. It
then gets pulled off this queue in your ISR, again without a lock. So
for example, if the only packet in your queue gets completed while you
are trying to queue another one, you might corrupt the list.

Brian

> +
> +	/* kick off tx queue */
> +	if (queue != RTW_TX_QUEUE_BCN) {
> +		if (++ring->r.wp >= ring->r.len)
> +			ring->r.wp = 0;
> +		bd_idx = rtw_pci_tx_queue_idx_addr[queue];
> +		rtw_write16(rtwdev, bd_idx, ring->r.wp & 0xfff);
> +	} else {
> +		u32 reg_bcn_work;
> +
> +		reg_bcn_work = rtw_read8(rtwdev, RTK_PCI_TXBD_BCN_WORK);
> +		reg_bcn_work |= BIT_PCI_BCNQ_FLAG;
> +		rtw_write8(rtwdev, RTK_PCI_TXBD_BCN_WORK, reg_bcn_work);
> +	}
> +
> +	return 0;
> +}
...

Tony Chuang Feb. 11, 2019, 5:48 a.m. UTC | #4

> From: Brian Norris [mailto:[e-mail]]
> 
> FYI, I have some more review comments because I'm trying to see why your
> TX path doesn't work all that well. At least, it's not reporting things
> correctly. (I know there's one ACK reporting bug you fixed in a
> follow-up patch, but then, that patch is buggy too I think.)
> 
> > +static int rtw_pci_xmit(struct rtw_dev *rtwdev,
> > +			struct rtw_tx_pkt_info *pkt_info,
> > +			struct sk_buff *skb, u8 queue)
> > +{
> > +	struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
> > +	struct rtw_chip_info *chip = rtwdev->chip;
> > +	struct rtw_pci_tx_ring *ring;
> > +	struct rtw_pci_tx_data *tx_data;
> > +	dma_addr_t dma;
> > +	u32 tx_pkt_desc_sz = chip->tx_pkt_desc_sz;
> > +	u32 tx_buf_desc_sz = chip->tx_buf_desc_sz;
> > +	u32 size;
> > +	u32 psb_len;
> > +	u8 *pkt_desc;
> > +	struct rtw_pci_tx_buffer_desc *buf_desc;
> > +	u32 bd_idx;
> > +
> > +	ring = &rtwpci->tx_rings[queue];
> > +
> > +	size = skb->len;
> > +
> > +	if (queue == RTW_TX_QUEUE_BCN)
> > +		rtw_pci_release_rsvd_page(rtwpci, ring);
> > +	else if (!avail_desc(ring->r.wp, ring->r.rp, ring->r.len))
> > +		return -ENOSPC;
> > +
> > +	pkt_desc = skb_push(skb, chip->tx_pkt_desc_sz);
> > +	memset(pkt_desc, 0, tx_pkt_desc_sz);
> > +	pkt_info->qsel = rtw_pci_get_tx_qsel(skb, queue);
> > +	rtw_tx_fill_tx_desc(pkt_info, skb);
> > +	dma = pci_map_single(rtwpci->pdev, skb->data, skb->len,
> > +			     PCI_DMA_TODEVICE);
> > +	if (pci_dma_mapping_error(rtwpci->pdev, dma))
> > +		return -EBUSY;
> > +
> > +	/* after this we got dma mapped, there is no way back */
> > +	buf_desc = get_tx_buffer_desc(ring, tx_buf_desc_sz);
> > +	memset(buf_desc, 0, tx_buf_desc_sz);
> > +	psb_len = (skb->len - 1) / 128 + 1;
> > +	if (queue == RTW_TX_QUEUE_BCN)
> > +		psb_len |= 1 << RTK_PCI_TXBD_OWN_OFFSET;
> > +
> > +	buf_desc[0].psb_len = cpu_to_le16(psb_len);
> > +	buf_desc[0].buf_size = cpu_to_le16(tx_pkt_desc_sz);
> > +	buf_desc[0].dma = cpu_to_le32(dma);
> > +	buf_desc[1].buf_size = cpu_to_le16(size);
> > +	buf_desc[1].dma = cpu_to_le32(dma + tx_pkt_desc_sz);
> > +
> > +	tx_data = rtw_pci_get_tx_data(skb);
> > +	tx_data->dma = dma;
> > +	skb_queue_tail(&ring->queue, skb);
> 
> IIUC, you have no locking for this queue. That seems like a bad idea. It
> then gets pulled off this queue in your ISR, again without a lock. So
> for example, if the only packet in your queue gets completed while you
> are trying to queue another one, you might corrupt the list.
> 

I think skb_queue_tail already has its own spinlock to protect the queue?
Cannot see why the list might be corrupted. Or I misunderstand you.

Yan-Hsuan

Tony Chuang Feb. 11, 2019, 6:15 a.m. UTC | #5

> -----Original Message-----
> From: Brian Norris [mailto:[e-mail]]
> 
> Hi,
> 
> One more comment for now, below:
> 
> On Wed, Jan 30, 2019 at 12:02:10PM +0800, [e-mail] wrote:
> > From: Yan-Hsuan Chuang <[e-mail]>
> >
> > 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 <[e-mail]>
> > ---
> >  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.

Unfortunately I think it will end up in an unbalanced unmap. But I have no idea
now about how to deal with the dma_map failure. Since the rx dma runs ring-based,
if the dma_map failed, seems the only way is to retry to dma_map again until it has
a mapped dma address. Otherwise the dma engine will dma contents of a packet to
an unknown dma address (previous skb in this case, also will corrupt the memory)
after it ran over a cycle and hit this desc.

Hence I think here we should put an err or WARN here to explicitly know there has
something went wrong. But I do not know how to deal with the dma failure :(
Need to re-construct them in my brain and consider the rx path.

Yan-Hsuan

Brian Norris Feb. 11, 2019, 5:56 p.m. UTC | #6

On Sun, Feb 10, 2019 at 9:48 PM Tony Chuang <[e-mail]> wrote:
> > From: Brian Norris [mailto:[e-mail]]
> > > +   tx_data = rtw_pci_get_tx_data(skb);
> > > +   tx_data->dma = dma;
> > > +   skb_queue_tail(&ring->queue, skb);
> >
> > IIUC, you have no locking for this queue. That seems like a bad idea. It
> > then gets pulled off this queue in your ISR, again without a lock. So
> > for example, if the only packet in your queue gets completed while you
> > are trying to queue another one, you might corrupt the list.
> >
>
> I think skb_queue_tail already has its own spinlock to protect the queue?
> Cannot see why the list might be corrupted. Or I misunderstand you.

Ah, no of course you're correct. I think I kept looking at the
definition of __skb_queue_tail() instead. It also doesn't help that,
in skbuff.h, the kerneldoc comments for __skb_queue_tail() are put
above skb_queue_tail(). So it's extra easy to confuse them...

And to be clear, so far I don't think I've seen actual corruption of
this queue yet. Just bad TX status reporting, including plenty of
driver WARN()s. So my comment was only theoretical (and incorrect).

Regards,
Brian

Tony Chuang Feb. 12, 2019, 6:18 a.m. UTC | #7

> From: Brian Norris [mailto:[e-mail]]
> 
> Hi,
> 
> A few scattered comments:
> 
> On Wed, Jan 30, 2019 at 12:02:10PM +0800, [e-mail] wrote:
> > From: Yan-Hsuan Chuang <[e-mail]>
> >
> > 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 <[e-mail]>
> > ---
> >  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 int rtw_pci_init_rx_ring(struct rtw_dev *rtwdev,
> > +				struct rtw_pci_rx_ring *rx_ring,
> > +				u8 desc_size, u32 len)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(rtwdev->dev);
> > +	struct sk_buff *skb = NULL;
> > +	dma_addr_t dma;
> > +	u8 *head;
> > +	int ring_sz = desc_size * len;
> > +	int buf_sz = RTK_PCI_RX_BUF_SIZE;
> > +	int i, allocated;
> > +	int ret = 0;
> > +
> > +	head = pci_zalloc_consistent(pdev, ring_sz, &dma);
> > +	if (!head) {
> > +		rtw_err(rtwdev, "failed to allocate rx ring\n");
> > +		return -ENOMEM;
> > +	}
> > +	rx_ring->r.head = head;
> > +
> > +	for (i = 0; i < len; i++) {
> > +		skb = dev_alloc_skb(buf_sz);
> > +		if (!skb) {
> > +			allocated = i;
> > +			ret = -ENOMEM;
> > +			goto err_out;
> > +		}
> > +
> > +		memset(skb->data, 0, buf_sz);
> > +		rx_ring->buf[i] = skb;
> > +		ret = rtw_pci_reset_rx_desc(rtwdev, skb, rx_ring, i, desc_size);
> > +		if (ret) {
> > +			allocated = i;
> > +			goto err_out;
> > +		}
> > +	}
> > +
> > +	rx_ring->r.dma = dma;
> > +	rx_ring->r.len = len;
> > +	rx_ring->r.desc_size = desc_size;
> > +	rx_ring->r.wp = 0;
> > +	rx_ring->r.rp = 0;
> > +
> > +	return 0;
> > +
> > +err_out:
> > +	dev_kfree_skb_any(skb);
> 
> Maybe I'm misreading but...shouldn't this line not be here? You properly
> iterate over the allocated SKBs below, and this looks like you're just
> going to be double-freeing (or, negative ref-counting).

Yeah, I think I should move this line above after reset_rx_desc failed.

> 
> > +	for (i = 0; i < allocated; i++) {
> > +		skb = rx_ring->buf[i];
> > +		if (!skb)
> > +			continue;
> > +		dma = *((dma_addr_t *)skb->cb);
> > +		pci_unmap_single(pdev, dma, buf_sz, PCI_DMA_FROMDEVICE);
> > +		dev_kfree_skb_any(skb);
> > +		rx_ring->buf[i] = NULL;
> > +	}
> > +	pci_free_consistent(pdev, ring_sz, head, dma);
> > +
> > +	rtw_err(rtwdev, "failed to init rx buffer\n");
> > +
> > +	return ret;
> > +}
> > +
> 
> ...
> 
> > +static void rtw_pci_dma_check(struct rtw_dev *rtwdev,
> > +			      struct rtw_pci_rx_ring *rx_ring,
> > +			      u32 idx)
> > +{
> > +	struct rtw_chip_info *chip = rtwdev->chip;
> > +	struct rtw_pci_rx_buffer_desc *buf_desc;
> > +	u32 desc_sz = chip->rx_buf_desc_sz;
> > +	u16 total_pkt_size;
> > +	int i;
> > +
> > +	buf_desc = (struct rtw_pci_rx_buffer_desc *)(rx_ring->r.head +
> > +						     idx * desc_sz);
> > +	for (i = 0; i < 20; i++) {
> > +		total_pkt_size = le16_to_cpu(buf_desc->total_pkt_size);
> > +		if (total_pkt_size)
> > +			return;
> > +	}
> 
> 
> Umm, what are you trying to do here? This is a non-sensical loop. I
> *imagine* you're trying to do some kind of timeout loop here, but since
> there's nothing telling the compiler that this is anything but normal
> memory, this loop gets flattened by the compiler into a single check of
> ->total_pkt_size (I checked; my compiler gets rid of the loop).
> 
> So, at a minimum, you should just remove the loop. But I'm not sure if
> this "check" function has any value at all...
> 
> > +
> > +	if (i >= 20)
> > +		rtw_warn(rtwdev, "pci bus timeout, drop packet\n");
> 
> ...BTW, I'm seeing this get triggered quite a bit.

It's quite strange though... triggered on my laptop as well.
I am looking for the reason that cause it. I found that 8822BE does not
trigger this. And it's like even if I added a volatile before it to hint the
compiler not to optimize, it still happens. Now trying to figure out why
the bus continues to timeout.

After this problem is resolved I'll either remove the loop or add proper
hint for the compiler to check the value. But it seems to take many days
to debug, so I think for this patch set I can just remain it here.

> 
> Do you have some kind of memory mapping/ordering issue or something? I
> wouldn't think you should expect to just drop packets on the floor so
> often like this.
> 

Looks like the cause is that the DMA might not have done when the interrupt
arrived. Need to do more test to figure it out.

> > +}
> > +
> ...
> 
> > +
> > +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);
> > +
> > +		/* 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);
> > +}
> > +

...

> 
> > +static void rtw_pci_parse_configuration(struct rtw_dev *rtwdev,
> > +					struct pci_dev *pdev)
> > +{
> > +	u16 link_control;
> > +	u8 config;
> > +
> 
> In general, this function still uses several magic numbers. It would be
> nice to use macro constants, or at least include a few more comments.
> 
> > +	/* Disable Clk Request */
> > +	pci_write_config_byte(pdev, 0x81, 0);
> 
> 	pcie_capability_clear_word(pdev, PCI_EXP_LNKCTL,
> 				   PCI_EXP_LNKCTL_CLKREQ_EN);
> 
> Or even better, just:
> 
> 	pci_disable_link_state(pdev, PCIE_LINK_STATE_CLKPM);
> 
> > +	/* leave D3 mode */
> > +	pci_write_config_byte(pdev, 0x44, 0);
> 
> This looks like you're trying to do pci_set_power_state(dev, PCI_D0).
> But that's already part of pci_enable_device(), no?
> 
> > +	pci_write_config_byte(pdev, 0x04, 0x06);
> 
> Use the PCI_COMMAND constant?
> 
> And, it looks like maybe you're really trying to do a read-modify-write?
> But anyway, we have PCI_COMMAND_{IO,MEMORY,MASTER} constants for
> this.
> 
> > +	pci_write_config_byte(pdev, 0x04, 0x07);
> 
> Same here.
> 
> Also, what exactly is the purpose here? You're disabling IO and the
> re-enabling it? Is that a hardware quirk, or something else?
> 
> > +
> > +	pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &link_control);
> 
> What are you reading this for? You never use it. Remove?
> 
> > +
> > +	pci_read_config_byte(pdev, 0x98, &config);
> > +	config |= BIT(4);
> > +	pci_write_config_byte(pdev, 0x98, config);
> 
> The above 3 can just be:
> 
> 	pcie_capability_set_word(pdev, PCI_EXP_DEVCTL2,
> 				 PCI_EXP_DEVCTL2_COMP_TMOUT_DIS);
> 
> > +
> > +	pci_write_config_byte(pdev, 0x70f, 0x17);
> 
> I couldn't figure out if this was any standard register. Add comments
> and/or a macro definition?
> 
> > +}
> > +


After some testing, this function I think can be removed.
(rtw_pci_parse_configuration)


> > +
> > +static const struct pci_device_id rtw_pci_id_table[] = {
> > +#ifdef CONFIG_RTW88_8822BE
> > +	{ RTK_PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0xB822,
> rtw8822b_hw_spec) },
> > +#endif
> > +#ifdef CONFIG_RTW88_8822CE
> > +	{ RTK_PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0xC822,
> rtw8822c_hw_spec) },
> > +#endif
> > +	{},
> > +};
> > +
> > +static struct pci_driver rtw_pci_driver = {
> > +	.name = "rtw_pci",
> > +	.id_table = rtw_pci_id_table,
> > +	.probe = rtw_pci_probe,
> > +	.remove = rtw_pci_remove,
> > +};
> > +
> > +struct rtw_hci_ops rtw_pci_ops = {
> 
> This struct is local to this unit. Please move it up above where it's
> used and make it static.
> 
> > +	.tx = rtw_pci_tx,
> > +	.setup = rtw_pci_setup,
> > +	.start = rtw_pci_start,
> > +	.stop = rtw_pci_stop,
> > +
> > +	.read8 = rtw_pci_read8,
> > +	.read16 = rtw_pci_read16,
> > +	.read32 = rtw_pci_read32,
> > +	.write8 = rtw_pci_write8,
> > +	.write16 = rtw_pci_write16,
> > +	.write32 = rtw_pci_write32,
> > +	.write_data_rsvd_page = rtw_pci_write_data_rsvd_page,
> > +	.write_data_h2c = rtw_pci_write_data_h2c,
> > +};
> > +
> > +MODULE_DEVICE_TABLE(pci, rtw_pci_id_table);
> 
> Normally, the MODULE_DEVICE_TABLE() is best kept closer to
> rtw_pci_id_table (immediately following its definition). Move it up?
> 
> > +
> > +module_pci_driver(rtw_pci_driver);
> 
> Same here -- once the other things move out of the way, keep this close
> to rtw_pci_driver?

OK, that's reasonable.

> 
> > +
> > +MODULE_AUTHOR("Realtek Corporation");
> > +MODULE_DESCRIPTION("Realtek 802.11ac wireless PCI driver");
> > +MODULE_LICENSE("GPL");
> ...
> 


Yan-Hsuan

Brian Norris Feb. 12, 2019, 10:04 p.m. UTC | #8

On Mon, Feb 11, 2019 at 10:19 PM Tony Chuang <[e-mail]> wrote:
> > From: Brian Norris [mailto:[e-mail]]
> > On Wed, Jan 30, 2019 at 12:02:10PM +0800, [e-mail] wrote:
> > > 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 int rtw_pci_init_rx_ring(struct rtw_dev *rtwdev,
> > > +                           struct rtw_pci_rx_ring *rx_ring,
> > > +                           u8 desc_size, u32 len)
> > > +{
> > > +   struct pci_dev *pdev = to_pci_dev(rtwdev->dev);
> > > +   struct sk_buff *skb = NULL;
> > > +   dma_addr_t dma;
> > > +   u8 *head;
> > > +   int ring_sz = desc_size * len;
> > > +   int buf_sz = RTK_PCI_RX_BUF_SIZE;
> > > +   int i, allocated;
> > > +   int ret = 0;
> > > +
> > > +   head = pci_zalloc_consistent(pdev, ring_sz, &dma);
> > > +   if (!head) {
> > > +           rtw_err(rtwdev, "failed to allocate rx ring\n");
> > > +           return -ENOMEM;
> > > +   }
> > > +   rx_ring->r.head = head;
> > > +
> > > +   for (i = 0; i < len; i++) {
> > > +           skb = dev_alloc_skb(buf_sz);
> > > +           if (!skb) {
> > > +                   allocated = i;
> > > +                   ret = -ENOMEM;
> > > +                   goto err_out;
> > > +           }
> > > +
> > > +           memset(skb->data, 0, buf_sz);
> > > +           rx_ring->buf[i] = skb;
> > > +           ret = rtw_pci_reset_rx_desc(rtwdev, skb, rx_ring, i, desc_size);
> > > +           if (ret) {
> > > +                   allocated = i;
> > > +                   goto err_out;
> > > +           }
> > > +   }
> > > +
> > > +   rx_ring->r.dma = dma;
> > > +   rx_ring->r.len = len;
> > > +   rx_ring->r.desc_size = desc_size;
> > > +   rx_ring->r.wp = 0;
> > > +   rx_ring->r.rp = 0;
> > > +
> > > +   return 0;
> > > +
> > > +err_out:
> > > +   dev_kfree_skb_any(skb);
> >
> > Maybe I'm misreading but...shouldn't this line not be here? You properly
> > iterate over the allocated SKBs below, and this looks like you're just
> > going to be double-freeing (or, negative ref-counting).
>
> Yeah, I think I should move this line above after reset_rx_desc failed.

Ah yes, so it's not technically incorrect, it's just a bit strange to
read. Yes, moving it up to the failure location would make it clearer
IMO. Thanks.

> > > +   for (i = 0; i < allocated; i++) {
> > > +           skb = rx_ring->buf[i];
> > > +           if (!skb)
> > > +                   continue;
> > > +           dma = *((dma_addr_t *)skb->cb);
> > > +           pci_unmap_single(pdev, dma, buf_sz, PCI_DMA_FROMDEVICE);
> > > +           dev_kfree_skb_any(skb);
> > > +           rx_ring->buf[i] = NULL;
> > > +   }
> > > +   pci_free_consistent(pdev, ring_sz, head, dma);
> > > +
> > > +   rtw_err(rtwdev, "failed to init rx buffer\n");
> > > +
> > > +   return ret;
> > > +}
> > > +

...

> After some testing, this function I think can be removed.
> (rtw_pci_parse_configuration)

Why was it here in the first place? Were there any hardware quirks
that this was covering? Or, are you just saying that the default
values are already correct (plus, the PCI framework already brings you
out of D3)? I do also see that removing this function doesn't actually
have a net effect on the the PCI configuration, judging by lspci
-vvvxxxx.

Brian

Tony Chuang Feb. 13, 2019, 8:08 a.m. UTC | #9

> -----Original Message-----
> From: Brian Norris [mailto:[e-mail]]
> 
> > > > +   for (i = 0; i < allocated; i++) {
> > > > +           skb = rx_ring->buf[i];
> > > > +           if (!skb)
> > > > +                   continue;
> > > > +           dma = *((dma_addr_t *)skb->cb);
> > > > +           pci_unmap_single(pdev, dma, buf_sz,
> PCI_DMA_FROMDEVICE);
> > > > +           dev_kfree_skb_any(skb);
> > > > +           rx_ring->buf[i] = NULL;
> > > > +   }
> > > > +   pci_free_consistent(pdev, ring_sz, head, dma);
> > > > +
> > > > +   rtw_err(rtwdev, "failed to init rx buffer\n");
> > > > +
> > > > +   return ret;
> > > > +}
> > > > +
> 
> ...
> 
> > After some testing, this function I think can be removed.
> > (rtw_pci_parse_configuration)
> 
> Why was it here in the first place? Were there any hardware quirks
> that this was covering? Or, are you just saying that the default
> values are already correct (plus, the PCI framework already brings you
> out of D3)? I do also see that removing this function doesn't actually
> have a net effect on the the PCI configuration, judging by lspci
> -vvvxxxx.

This was written for developing 8822BE. And it was not tested at that
moment so I was not sure if it can be removed. Now many test item has
been tested, it seems that the configuration parsing code is no more
needed.
And I see that pci_set_master() will enter D0. So it can be removed.

Thanks.

Yan-Hsuan

Tony Chuang Feb. 13, 2019, 11:08 a.m. UTC | #10

Hi

> On Behalf Of Tony Chuang
> Subject: RE: [PATCH v4 03/13] rtw88: hci files
> 
> > From: Brian Norris [mailto:[e-mail]]
> >
> > Hi,
> >
> > A few scattered comments:
> >
> > > +static void rtw_pci_dma_check(struct rtw_dev *rtwdev,
> > > +			      struct rtw_pci_rx_ring *rx_ring,
> > > +			      u32 idx)
> > > +{
> > > +	struct rtw_chip_info *chip = rtwdev->chip;
> > > +	struct rtw_pci_rx_buffer_desc *buf_desc;
> > > +	u32 desc_sz = chip->rx_buf_desc_sz;
> > > +	u16 total_pkt_size;
> > > +	int i;
> > > +
> > > +	buf_desc = (struct rtw_pci_rx_buffer_desc *)(rx_ring->r.head +
> > > +						     idx * desc_sz);
> > > +	for (i = 0; i < 20; i++) {
> > > +		total_pkt_size = le16_to_cpu(buf_desc->total_pkt_size);
> > > +		if (total_pkt_size)
> > > +			return;
> > > +	}
> >
> >
> > Umm, what are you trying to do here? This is a non-sensical loop. I
> > *imagine* you're trying to do some kind of timeout loop here, but since
> > there's nothing telling the compiler that this is anything but normal
> > memory, this loop gets flattened by the compiler into a single check of
> > ->total_pkt_size (I checked; my compiler gets rid of the loop).
> >
> > So, at a minimum, you should just remove the loop. But I'm not sure if
> > this "check" function has any value at all...
> >
> > > +
> > > +	if (i >= 20)
> > > +		rtw_warn(rtwdev, "pci bus timeout, drop packet\n");
> >
> > ...BTW, I'm seeing this get triggered quite a bit.
> 
> It's quite strange though... triggered on my laptop as well.
> I am looking for the reason that cause it. I found that 8822BE does not
> trigger this. And it's like even if I added a volatile before it to hint the
> compiler not to optimize, it still happens. Now trying to figure out why
> the bus continues to timeout.
> 
> After this problem is resolved I'll either remove the loop or add proper
> hint for the compiler to check the value. But it seems to take many days
> to debug, so I think for this patch set I can just remain it here.
> 
> >
> > Do you have some kind of memory mapping/ordering issue or something? I
> > wouldn't think you should expect to just drop packets on the floor so
> > often like this.
> >
> 
> Looks like the cause is that the DMA might not have done when the interrupt
> arrived. Need to do more test to figure it out.
> 


I tested and checked again. I found that I didn't enable DMA sync for
the TRX path. After some work it can be resolved if I turn DMA sync on.
So I will include this in the PATCH v5.


> 
> 
> Yan-Hsuan
> 

Yan-Hsuan

Brian Norris Feb. 13, 2019, 7:21 p.m. UTC | #11

On Wed, Feb 13, 2019 at 3:08 AM Tony Chuang <[e-mail]> wrote:
> > On Behalf Of Tony Chuang
> > > From: Brian Norris [mailto:[e-mail]]
> > > Do you have some kind of memory mapping/ordering issue or something? I
> > > wouldn't think you should expect to just drop packets on the floor so
> > > often like this.
> > >
> >
> > Looks like the cause is that the DMA might not have done when the interrupt
> > arrived. Need to do more test to figure it out.
>
>
> I tested and checked again. I found that I didn't enable DMA sync for
> the TRX path. After some work it can be resolved if I turn DMA sync on.
> So I will include this in the PATCH v5.

Great! I'd also highly recommend dropping that loop. It just pretends
to do something that it doesn't really do. Leaving a single iteration
as a sort of debugging check is OK, if it's truly a bug any time we
fail it.

Regards,
Brian

Grant Grundler Feb. 14, 2019, 11:05 p.m. UTC | #12

Brian - thanks for the relatively thorough review. Well done!

Comments on two of the points you raised. Will trim out the rest.

On Thu, Jan 31, 2019 at 2:36 PM Brian Norris <[e-mail]> wrote:
...
> On Wed, Jan 30, 2019 at 12:02:10PM +0800, [e-mail] wrote:
...
> > +err_out:
> > +     dev_kfree_skb_any(skb);
>
> Maybe I'm misreading but...shouldn't this line not be here? You properly
> iterate over the allocated SKBs below, and this looks like you're just
> going to be double-freeing (or, negative ref-counting).

Kernel will panic immediately? The branch to reach err_out only
happens when skb is NULL.

...
> > +static void rtw_pci_dma_check(struct rtw_dev *rtwdev,
> > +                           struct rtw_pci_rx_ring *rx_ring,
> > +                           u32 idx)
> > +{
> > +     struct rtw_chip_info *chip = rtwdev->chip;
> > +     struct rtw_pci_rx_buffer_desc *buf_desc;
> > +     u32 desc_sz = chip->rx_buf_desc_sz;
> > +     u16 total_pkt_size;
> > +     int i;
> > +
> > +     buf_desc = (struct rtw_pci_rx_buffer_desc *)(rx_ring->r.head +
> > +                                                  idx * desc_sz);
> > +     for (i = 0; i < 20; i++) {
> > +             total_pkt_size = le16_to_cpu(buf_desc->total_pkt_size);
> > +             if (total_pkt_size)
> > +                     return;
> > +     }
>
>
> Umm, what are you trying to do here? This is a non-sensical loop. I
> *imagine* you're trying to do some kind of timeout loop here,

My guess is this is trying to mitigate a race between the CPU reading
memory and RTL88* device in updating this word of memory.  This is the
first function called in the packet processing loop in
rtw_pci_rx_isr(). This code suggests that the CPU is seeing the
interrupt before the RTL has finished updating memory via DMA and is
likely a "Bug" in the RTL device firmware.

I'm suggesting this is a bug for two reasons:
1) NIC vendors historically have tried to reduce packet handling
latency by sending the interrupt a few microseconds BEFORE the DMA is
actually completed. This has always failed in some weird way whenever
the PCI host controllers were a bit slower or perhaps even coalescing
DMA writes (or something like that).

2) PCI-Express interrupts are "In band" (MSI or not). This means once
the interrupt arrives, the corresponding device driver should be able
to assume all DMA from the device is complete.  This race is only
possible for "out of band" IRQ lines which are found on other busses
and historic PCI and PCI-X parallel busses.

"DMA is complete" doesn't necessarily means data is visible to CPU
though on Intel X86, it usually is.  The DMA API requires memory
allocated via pci_[z]alloc_consistent() be visible and cache coherent
though. So I don't expect CPU to perform any action to make the last
bits of DMA'd data visible in this case.  "Streaming Data" requires
pci_unmap* calls or respect dma_sync_to_cpu() (or whatever it's
called) to guarantee the data is visible - that's not what is being
accessed in this code though AFAICT.


> but since
> there's nothing telling the compiler that this is anything but normal
> memory, this loop gets flattened by the compiler into a single check of
> ->total_pkt_size (I checked; my compiler gets rid of the loop).

This is another bug in the code actually: the code accessing tx
descriptor rings should be declared volatile since they are updated by
the device which is not visible to the compiler.  Compiler won't
optimize (or reorder) volatile code (by "volatile code" I mean code
that is accessing data marked "volatile").

> So, at a minimum, you should just remove the loop. But I'm not sure if
> this "check" function has any value at all...

If Realtek can't fix what appears to be a bug in the firmware, I
suspect a polling loop will be required - but add a "udelay(1)" in the
loop.

>
> > +
> > +     if (i >= 20)
> > +             rtw_warn(rtwdev, "pci bus timeout, drop packet\n");
>
> ...BTW, I'm seeing this get triggered quite a bit.
>
> Do you have some kind of memory mapping/ordering issue or something? I
> wouldn't think you should expect to just drop packets on the floor so
> often like this.

Based on the code above, I'm willing to bet you are correct the driver
has memory ordering issues. Since the descriptor rings aren't declared
volatile, the compiler is free to reorder accesses to those fields as
it sees fit. This is a real problem given the device may update the
fields in the different order than what the compiler emits as memory
accesses.

cheers,
grant

Tony Chuang Feb. 20, 2019, 11:19 a.m. UTC | #13

Hi

> -----Original Message-----
> From: Grant Grundler [mailto:[e-mail]]
> On Thu, Jan 31, 2019 at 2:36 PM Brian Norris <[e-mail]>
> wrote:
> ...
> > On Wed, Jan 30, 2019 at 12:02:10PM +0800, [e-mail] wrote:
> ...
> > > +static void rtw_pci_dma_check(struct rtw_dev *rtwdev,
> > > +                           struct rtw_pci_rx_ring *rx_ring,
> > > +                           u32 idx)
> > > +{
> > > +     struct rtw_chip_info *chip = rtwdev->chip;
> > > +     struct rtw_pci_rx_buffer_desc *buf_desc;
> > > +     u32 desc_sz = chip->rx_buf_desc_sz;
> > > +     u16 total_pkt_size;
> > > +     int i;
> > > +
> > > +     buf_desc = (struct rtw_pci_rx_buffer_desc *)(rx_ring->r.head +
> > > +                                                  idx * desc_sz);
> > > +     for (i = 0; i < 20; i++) {
> > > +             total_pkt_size = le16_to_cpu(buf_desc->total_pkt_size);
> > > +             if (total_pkt_size)
> > > +                     return;
> > > +     }
> >
> >
> > Umm, what are you trying to do here? This is a non-sensical loop. I
> > *imagine* you're trying to do some kind of timeout loop here,
> 
> My guess is this is trying to mitigate a race between the CPU reading
> memory and RTL88* device in updating this word of memory.  This is the
> first function called in the packet processing loop in
> rtw_pci_rx_isr(). This code suggests that the CPU is seeing the
> interrupt before the RTL has finished updating memory via DMA and is
> likely a "Bug" in the RTL device firmware.
> 
> I'm suggesting this is a bug for two reasons:
> 1) NIC vendors historically have tried to reduce packet handling
> latency by sending the interrupt a few microseconds BEFORE the DMA is
> actually completed. This has always failed in some weird way whenever
> the PCI host controllers were a bit slower or perhaps even coalescing
> DMA writes (or something like that).
> 
> 2) PCI-Express interrupts are "In band" (MSI or not). This means once
> the interrupt arrives, the corresponding device driver should be able
> to assume all DMA from the device is complete.  This race is only
> possible for "out of band" IRQ lines which are found on other busses
> and historic PCI and PCI-X parallel busses.
> 
> "DMA is complete" doesn't necessarily means data is visible to CPU
> though on Intel X86, it usually is.  The DMA API requires memory
> allocated via pci_[z]alloc_consistent() be visible and cache coherent
> though. So I don't expect CPU to perform any action to make the last
> bits of DMA'd data visible in this case.  "Streaming Data" requires
> pci_unmap* calls or respect dma_sync_to_cpu() (or whatever it's
> called) to guarantee the data is visible - that's not what is being
> accessed in this code though AFAICT.
> 
> 
> > but since
> > there's nothing telling the compiler that this is anything but normal
> > memory, this loop gets flattened by the compiler into a single check of
> > ->total_pkt_size (I checked; my compiler gets rid of the loop).
> 
> This is another bug in the code actually: the code accessing tx
> descriptor rings should be declared volatile since they are updated by
> the device which is not visible to the compiler.  Compiler won't
> optimize (or reorder) volatile code (by "volatile code" I mean code
> that is accessing data marked "volatile").
> 
> > So, at a minimum, you should just remove the loop. But I'm not sure if
> > this "check" function has any value at all...
> 
> If Realtek can't fix what appears to be a bug in the firmware, I
> suspect a polling loop will be required - but add a "udelay(1)" in the
> loop.
> 
> >
> > > +
> > > +     if (i >= 20)
> > > +             rtw_warn(rtwdev, "pci bus timeout, drop packet\n");
> >
> > ...BTW, I'm seeing this get triggered quite a bit.
> >
> > Do you have some kind of memory mapping/ordering issue or something? I
> > wouldn't think you should expect to just drop packets on the floor so
> > often like this.
> 
> Based on the code above, I'm willing to bet you are correct the driver
> has memory ordering issues. Since the descriptor rings aren't declared
> volatile, the compiler is free to reorder accesses to those fields as
> it sees fit. This is a real problem given the device may update the
> fields in the different order than what the compiler emits as memory
> accesses.
> 

This is fixed in PATCH v5. We reset and use rx tag to sync dma.
So the polling can be removed and the bus timeout has not happened again.

Thanks
Yan-Hsuan

Brian Norris March 22, 2019, 2:36 p.m. UTC | #14

[Following up a little late on this one]

Hi Grant and Tony,

On Wed, Feb 20, 2019 at 11:19:10AM +0000, Tony Chuang wrote:
> > -----Original Message-----
> > From: Grant Grundler [mailto:[e-mail]]

> > This is another bug in the code actually: the code accessing tx
> > descriptor rings should be declared volatile since they are updated by
> > the device which is not visible to the compiler.  Compiler won't
> > optimize (or reorder) volatile code (by "volatile code" I mean code
> > that is accessing data marked "volatile").

If I'm understanding the code correctly, the TX ISR assumes that
everything between the last entry we processed (ring->r.rp) and a
certain point (cur_rp) is complete. The former index was saved during
the previous interrupt and the latter is determined via a register read.
So assuming properly synchronized interrupts (such as with PCIe MSI) and
no device-/firmware-related ordering bugs, then I think we're OK -- the
only "volatile" piece is the register read, which already embeds
volatile in the IO accessors.

> This is fixed in PATCH v5. We reset and use rx tag to sync dma.
> So the polling can be removed and the bus timeout has not happened again.

Yes, I tested the later versions of this series, and I didn't see this
problem so far. I'll follow up there with my Review/Test information
eventually.

Thanks,
Brian

Patch

diff --git a/drivers/net/wireless/realtek/rtw88/hci.h b/drivers/net/wireless/realtek/rtw88/hci.h
new file mode 100644
index 0000000..91b15ef
--- /dev/null
+++ b/drivers/net/wireless/realtek/rtw88/hci.h
@@ -0,0 +1,211 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright(c) 2018  Realtek Corporation.
+ */
+
+#ifndef	__RTW_HCI_H__
+#define __RTW_HCI_H__
+
+/* ops for PCI, USB and SDIO */
+struct rtw_hci_ops {
+	int (*tx)(struct rtw_dev *rtwdev,
+		  struct rtw_tx_pkt_info *pkt_info,
+		  struct sk_buff *skb);
+	int (*setup)(struct rtw_dev *rtwdev);
+	int (*start)(struct rtw_dev *rtwdev);
+	void (*stop)(struct rtw_dev *rtwdev);
+
+	int (*write_data_rsvd_page)(struct rtw_dev *rtwdev, u8 *buf, u32 size);
+	int (*write_data_h2c)(struct rtw_dev *rtwdev, u8 *buf, u32 size);
+
+	u8 (*read8)(struct rtw_dev *rtwdev, u32 addr);
+	u16 (*read16)(struct rtw_dev *rtwdev, u32 addr);
+	u32 (*read32)(struct rtw_dev *rtwdev, u32 addr);
+	void (*write8)(struct rtw_dev *rtwdev, u32 addr, u8 val);
+	void (*write16)(struct rtw_dev *rtwdev, u32 addr, u16 val);
+	void (*write32)(struct rtw_dev *rtwdev, u32 addr, u32 val);
+};
+
+static inline int rtw_hci_tx(struct rtw_dev *rtwdev,
+			     struct rtw_tx_pkt_info *pkt_info,
+			     struct sk_buff *skb)
+{
+	return rtwdev->hci.ops->tx(rtwdev, pkt_info, skb);
+}
+
+static inline int rtw_hci_setup(struct rtw_dev *rtwdev)
+{
+	return rtwdev->hci.ops->setup(rtwdev);
+}
+
+static inline int rtw_hci_start(struct rtw_dev *rtwdev)
+{
+	return rtwdev->hci.ops->start(rtwdev);
+}
+
+static inline void rtw_hci_stop(struct rtw_dev *rtwdev)
+{
+	rtwdev->hci.ops->stop(rtwdev);
+}
+
+static inline int
+rtw_hci_write_data_rsvd_page(struct rtw_dev *rtwdev, u8 *buf, u32 size)
+{
+	return rtwdev->hci.ops->write_data_rsvd_page(rtwdev, buf, size);
+}
+
+static inline int
+rtw_hci_write_data_h2c(struct rtw_dev *rtwdev, u8 *buf, u32 size)
+{
+	return rtwdev->hci.ops->write_data_h2c(rtwdev, buf, size);
+}
+
+static inline u8 rtw_read8(struct rtw_dev *rtwdev, u32 addr)
+{
+	return rtwdev->hci.ops->read8(rtwdev, addr);
+}
+
+static inline u16 rtw_read16(struct rtw_dev *rtwdev, u32 addr)
+{
+	return rtwdev->hci.ops->read16(rtwdev, addr);
+}
+
+static inline u32 rtw_read32(struct rtw_dev *rtwdev, u32 addr)
+{
+	return rtwdev->hci.ops->read32(rtwdev, addr);
+}
+
+static inline void rtw_write8(struct rtw_dev *rtwdev, u32 addr, u8 val)
+{
+	rtwdev->hci.ops->write8(rtwdev, addr, val);
+}
+
+static inline void rtw_write16(struct rtw_dev *rtwdev, u32 addr, u16 val)
+{
+	rtwdev->hci.ops->write16(rtwdev, addr, val);
+}
+
+static inline void rtw_write32(struct rtw_dev *rtwdev, u32 addr, u32 val)
+{
+	rtwdev->hci.ops->write32(rtwdev, addr, val);
+}
+
+static inline void rtw_write8_set(struct rtw_dev *rtwdev, u32 addr, u8 bit)
+{
+	u8 val;
+
+	val = rtw_read8(rtwdev, addr);
+	rtw_write8(rtwdev, addr, val | bit);
+}
+
+static inline void rtw_writ16_set(struct rtw_dev *rtwdev, u32 addr, u16 bit)
+{
+	u16 val;
+
+	val = rtw_read16(rtwdev, addr);
+	rtw_write16(rtwdev, addr, val | bit);
+}
+
+static inline void rtw_write32_set(struct rtw_dev *rtwdev, u32 addr, u32 bit)
+{
+	u32 val;
+
+	val = rtw_read32(rtwdev, addr);
+	rtw_write32(rtwdev, addr, val | bit);
+}
+
+static inline void rtw_write8_clr(struct rtw_dev *rtwdev, u32 addr, u8 bit)
+{
+	u8 val;
+
+	val = rtw_read8(rtwdev, addr);
+	rtw_write8(rtwdev, addr, val & ~bit);
+}
+
+static inline void rtw_write16_clr(struct rtw_dev *rtwdev, u32 addr, u16 bit)
+{
+	u16 val;
+
+	val = rtw_read16(rtwdev, addr);
+	rtw_write16(rtwdev, addr, val & ~bit);
+}
+
+static inline void rtw_write32_clr(struct rtw_dev *rtwdev, u32 addr, u32 bit)
+{
+	u32 val;
+
+	val = rtw_read32(rtwdev, addr);
+	rtw_write32(rtwdev, addr, val & ~bit);
+}
+
+static inline u32
+rtw_read_rf(struct rtw_dev *rtwdev, enum rtw_rf_path rf_path,
+	    u32 addr, u32 mask)
+{
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&rtwdev->rf_lock, flags);
+	val = rtwdev->chip->ops->read_rf(rtwdev, rf_path, addr, mask);
+	spin_unlock_irqrestore(&rtwdev->rf_lock, flags);
+
+	return val;
+}
+
+static inline void
+rtw_write_rf(struct rtw_dev *rtwdev, enum rtw_rf_path rf_path,
+	     u32 addr, u32 mask, u32 data)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&rtwdev->rf_lock, flags);
+	rtwdev->chip->ops->write_rf(rtwdev, rf_path, addr, mask, data);
+	spin_unlock_irqrestore(&rtwdev->rf_lock, flags);
+}
+
+static inline u32
+rtw_read32_mask(struct rtw_dev *rtwdev, u32 addr, u32 mask)
+{
+	u32 shift = __ffs(mask);
+	u32 orig;
+	u32 ret;
+
+	orig = rtw_read32(rtwdev, addr);
+	ret = (orig & mask) >> shift;
+
+	return ret;
+}
+
+static inline void
+rtw_write32_mask(struct rtw_dev *rtwdev, u32 addr, u32 mask, u32 data)
+{
+	u32 shift = __ffs(mask);
+	u32 orig;
+	u32 set;
+
+	WARN(addr & 0x3, "should be 4-byte aligned, addr = 0x%08x\n", addr);
+
+	orig = rtw_read32(rtwdev, addr);
+	set = (orig & ~mask) | ((data << shift) & mask);
+	rtw_write32(rtwdev, addr, set);
+}
+
+static inline void
+rtw_write8_mask(struct rtw_dev *rtwdev, u32 addr, u32 mask, u8 data)
+{
+	u32 shift;
+	u8 orig, set;
+
+	mask &= 0xff;
+	shift = __ffs(mask);
+
+	orig = rtw_read8(rtwdev, addr);
+	set = (orig & ~mask) | ((data << shift) & mask);
+	rtw_write8(rtwdev, addr, set);
+}
+
+static inline enum rtw_hci_type rtw_hci_type(struct rtw_dev *rtwdev)
+{
+	return rtwdev->hci.type;
+}
+
+#endif
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 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2018  Realtek Corporation.
+ */
+
+#include <linux/module.h>
+#include <linux/pci.h>
+#include "main.h"
+#include "pci.h"
+#include "tx.h"
+#include "rx.h"
+#include "debug.h"
+
+static u32 rtw_pci_tx_queue_idx_addr[] = {
+	[RTW_TX_QUEUE_BK]	= RTK_PCI_TXBD_IDX_BKQ,
+	[RTW_TX_QUEUE_BE]	= RTK_PCI_TXBD_IDX_BEQ,
+	[RTW_TX_QUEUE_VI]	= RTK_PCI_TXBD_IDX_VIQ,
+	[RTW_TX_QUEUE_VO]	= RTK_PCI_TXBD_IDX_VOQ,
+	[RTW_TX_QUEUE_MGMT]	= RTK_PCI_TXBD_IDX_MGMTQ,
+	[RTW_TX_QUEUE_HI0]	= RTK_PCI_TXBD_IDX_HI0Q,
+	[RTW_TX_QUEUE_H2C]	= RTK_PCI_TXBD_IDX_H2CQ,
+};
+
+static u8 rtw_pci_get_tx_qsel(struct sk_buff *skb, u8 queue)
+{
+	switch (queue) {
+	case RTW_TX_QUEUE_BCN:
+		return TX_DESC_QSEL_BEACON;
+	case RTW_TX_QUEUE_H2C:
+		return TX_DESC_QSEL_H2C;
+	case RTW_TX_QUEUE_MGMT:
+		return TX_DESC_QSEL_MGMT;
+	case RTW_TX_QUEUE_HI0:
+		return TX_DESC_QSEL_HIGH;
+	default:
+		return skb->priority;
+	}
+};
+
+static u8 rtw_pci_read8(struct rtw_dev *rtwdev, u32 addr)
+{
+	struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
+
+	return readb(rtwpci->mmap + addr);
+}
+
+static u16 rtw_pci_read16(struct rtw_dev *rtwdev, u32 addr)
+{
+	struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
+
+	return readw(rtwpci->mmap + addr);
+}
+
+static u32 rtw_pci_read32(struct rtw_dev *rtwdev, u32 addr)
+{
+	struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
+
+	return readl(rtwpci->mmap + addr);
+}
+
+static void rtw_pci_write8(struct rtw_dev *rtwdev, u32 addr, u8 val)
+{
+	struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
+
+	writeb(val, rtwpci->mmap + addr);
+}
+
+static void rtw_pci_write16(struct rtw_dev *rtwdev, u32 addr, u16 val)
+{
+	struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
+
+	writew(val, rtwpci->mmap + addr);
+}
+
+static void rtw_pci_write32(struct rtw_dev *rtwdev, u32 addr, u32 val)
+{
+	struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
+
+	writel(val, rtwpci->mmap + addr);
+}
+
+static inline void *rtw_pci_get_tx_desc(struct rtw_pci_tx_ring *tx_ring, u8 idx)
+{
+	int offset = tx_ring->r.desc_size * idx;
+
+	return tx_ring->r.head + offset;
+}
+
+static void rtw_pci_free_tx_ring(struct rtw_dev *rtwdev,
+				 struct rtw_pci_tx_ring *tx_ring)
+{
+	struct pci_dev *pdev = to_pci_dev(rtwdev->dev);
+	struct rtw_pci_tx_data *tx_data;
+	struct sk_buff *skb, *tmp;
+	dma_addr_t dma;
+	u8 *head = tx_ring->r.head;
+	u32 len = tx_ring->r.len;
+	int ring_sz = len * tx_ring->r.desc_size;
+
+	/* free every skb remained in tx list */
+	skb_queue_walk_safe(&tx_ring->queue, skb, tmp) {
+		__skb_unlink(skb, &tx_ring->queue);
+		tx_data = rtw_pci_get_tx_data(skb);
+		dma = tx_data->dma;
+
+		pci_unmap_single(pdev, dma, skb->len, PCI_DMA_TODEVICE);
+		dev_kfree_skb_any(skb);
+	}
+
+	/* free the ring itself */
+	pci_free_consistent(pdev, ring_sz, head, tx_ring->r.dma);
+	tx_ring->r.head = NULL;
+}
+
+static void rtw_pci_free_rx_ring(struct rtw_dev *rtwdev,
+				 struct rtw_pci_rx_ring *rx_ring)
+{
+	struct pci_dev *pdev = to_pci_dev(rtwdev->dev);
+	struct sk_buff *skb;
+	dma_addr_t dma;
+	u8 *head = rx_ring->r.head;
+	int buf_sz = RTK_PCI_RX_BUF_SIZE;
+	int ring_sz = rx_ring->r.desc_size * rx_ring->r.len;
+	int i;
+
+	for (i = 0; i < rx_ring->r.len; i++) {
+		skb = rx_ring->buf[i];
+		if (!skb)
+			continue;
+
+		dma = *((dma_addr_t *)skb->cb);
+		pci_unmap_single(pdev, dma, buf_sz, PCI_DMA_FROMDEVICE);
+		dev_kfree_skb(skb);
+		rx_ring->buf[i] = NULL;
+	}
+
+	pci_free_consistent(pdev, ring_sz, head, rx_ring->r.dma);
+}
+
+static void rtw_pci_free_trx_ring(struct rtw_dev *rtwdev)
+{
+	struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
+	struct rtw_pci_tx_ring *tx_ring;
+	struct rtw_pci_rx_ring *rx_ring;
+	int i;
+
+	for (i = 0; i < RTK_MAX_TX_QUEUE_NUM; i++) {
+		tx_ring = &rtwpci->tx_rings[i];
+		rtw_pci_free_tx_ring(rtwdev, tx_ring);
+	}
+
+	for (i = 0; i < RTK_MAX_RX_QUEUE_NUM; i++) {
+		rx_ring = &rtwpci->rx_rings[i];
+		rtw_pci_free_rx_ring(rtwdev, rx_ring);
+	}
+}
+
+static int rtw_pci_init_tx_ring(struct rtw_dev *rtwdev,
+				struct rtw_pci_tx_ring *tx_ring,
+				u8 desc_size, u32 len)
+{
+	struct pci_dev *pdev = to_pci_dev(rtwdev->dev);
+	int ring_sz = desc_size * len;
+	dma_addr_t dma;
+	u8 *head;
+
+	head = pci_zalloc_consistent(pdev, ring_sz, &dma);
+	if (!head) {
+		rtw_err(rtwdev, "failed to allocate tx ring\n");
+		return -ENOMEM;
+	}
+
+	skb_queue_head_init(&tx_ring->queue);
+	tx_ring->r.head = head;
+	tx_ring->r.dma = dma;
+	tx_ring->r.len = len;
+	tx_ring->r.desc_size = desc_size;
+	tx_ring->r.wp = 0;
+	tx_ring->r.rp = 0;
+
+	return 0;
+}
+
+static int rtw_pci_reset_rx_desc(struct rtw_dev *rtwdev, struct sk_buff *skb,
+				 struct rtw_pci_rx_ring *rx_ring,
+				 u32 idx, u32 desc_sz)
+{
+	struct pci_dev *pdev = to_pci_dev(rtwdev->dev);
+	struct rtw_pci_rx_buffer_desc *buf_desc;
+	int buf_sz = RTK_PCI_RX_BUF_SIZE;
+	dma_addr_t dma;
+
+	if (!skb)
+		return -EINVAL;
+
+	dma = pci_map_single(pdev, skb->data, buf_sz, PCI_DMA_FROMDEVICE);
+	if (pci_dma_mapping_error(pdev, dma))
+		return -EBUSY;
+
+	*((dma_addr_t *)skb->cb) = dma;
+	buf_desc = (struct rtw_pci_rx_buffer_desc *)(rx_ring->r.head +
+						     idx * desc_sz);
+	memset(buf_desc, 0, sizeof(*buf_desc));
+	buf_desc->buf_size = cpu_to_le16(RTK_PCI_RX_BUF_SIZE);
+	buf_desc->dma = cpu_to_le32(dma);
+
+	return 0;
+}
+
+static int rtw_pci_init_rx_ring(struct rtw_dev *rtwdev,
+				struct rtw_pci_rx_ring *rx_ring,
+				u8 desc_size, u32 len)
+{
+	struct pci_dev *pdev = to_pci_dev(rtwdev->dev);
+	struct sk_buff *skb = NULL;
+	dma_addr_t dma;
+	u8 *head;
+	int ring_sz = desc_size * len;
+	int buf_sz = RTK_PCI_RX_BUF_SIZE;
+	int i, allocated;
+	int ret = 0;
+
+	head = pci_zalloc_consistent(pdev, ring_sz, &dma);
+	if (!head) {
+		rtw_err(rtwdev, "failed to allocate rx ring\n");
+		return -ENOMEM;
+	}
+	rx_ring->r.head = head;
+
+	for (i = 0; i < len; i++) {
+		skb = dev_alloc_skb(buf_sz);
+		if (!skb) {
+			allocated = i;
+			ret = -ENOMEM;
+			goto err_out;
+		}
+
+		memset(skb->data, 0, buf_sz);
+		rx_ring->buf[i] = skb;
+		ret = rtw_pci_reset_rx_desc(rtwdev, skb, rx_ring, i, desc_size);
+		if (ret) {
+			allocated = i;
+			goto err_out;
+		}
+	}
+
+	rx_ring->r.dma = dma;
+	rx_ring->r.len = len;
+	rx_ring->r.desc_size = desc_size;
+	rx_ring->r.wp = 0;
+	rx_ring->r.rp = 0;
+
+	return 0;
+
+err_out:
+	dev_kfree_skb_any(skb);
+	for (i = 0; i < allocated; i++) {
+		skb = rx_ring->buf[i];
+		if (!skb)
+			continue;
+		dma = *((dma_addr_t *)skb->cb);
+		pci_unmap_single(pdev, dma, buf_sz, PCI_DMA_FROMDEVICE);
+		dev_kfree_skb_any(skb);
+		rx_ring->buf[i] = NULL;
+	}
+	pci_free_consistent(pdev, ring_sz, head, dma);
+
+	rtw_err(rtwdev, "failed to init rx buffer\n");
+
+	return ret;
+}
+
+static int rtw_pci_init_trx_ring(struct rtw_dev *rtwdev)
+{
+	struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
+	struct rtw_pci_tx_ring *tx_ring;
+	struct rtw_pci_rx_ring *rx_ring;
+	struct rtw_chip_info *chip = rtwdev->chip;
+	int i = 0, j = 0, tx_alloced = 0, rx_alloced = 0;
+	int tx_desc_size, rx_desc_size;
+	u32 len;
+	int ret;
+
+	tx_desc_size = chip->tx_buf_desc_sz;
+
+	for (i = 0; i < RTK_MAX_TX_QUEUE_NUM; i++) {
+		tx_ring = &rtwpci->tx_rings[i];
+		len = max_num_of_tx_queue(i);
+		ret = rtw_pci_init_tx_ring(rtwdev, tx_ring, tx_desc_size, len);
+		if (ret)
+			goto out;
+	}
+
+	rx_desc_size = chip->rx_buf_desc_sz;
+
+	for (j = 0; j < RTK_MAX_RX_QUEUE_NUM; j++) {
+		rx_ring = &rtwpci->rx_rings[j];
+		ret = rtw_pci_init_rx_ring(rtwdev, rx_ring, rx_desc_size,
+					   RTK_MAX_RX_DESC_NUM);
+		if (ret)
+			goto out;
+	}
+
+	return 0;
+
+out:
+	tx_alloced = i;
+	for (i = 0; i < tx_alloced; i++) {
+		tx_ring = &rtwpci->tx_rings[i];
+		rtw_pci_free_tx_ring(rtwdev, tx_ring);
+	}
+
+	rx_alloced = j;
+	for (j = 0; j < rx_alloced; j++) {
+		rx_ring = &rtwpci->rx_rings[j];
+		rtw_pci_free_rx_ring(rtwdev, rx_ring);
+	}
+
+	return ret;
+}
+
+static void rtw_pci_deinit(struct rtw_dev *rtwdev)
+{
+	rtw_pci_free_trx_ring(rtwdev);
+}
+
+static int rtw_pci_init(struct rtw_dev *rtwdev)
+{
+	struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
+	int ret = 0;
+
+	rtwpci->irq_mask[0] = IMR_HIGHDOK |
+			      IMR_MGNTDOK |
+			      IMR_BKDOK |
+			      IMR_BEDOK |
+			      IMR_VIDOK |
+			      IMR_VODOK |
+			      IMR_ROK |
+			      IMR_BCNDMAINT_E |
+			      0;
+	rtwpci->irq_mask[1] = IMR_TXFOVW |
+			      0;
+	rtwpci->irq_mask[3] = IMR_H2CDOK |
+			      0;
+	spin_lock_init(&rtwpci->irq_lock);
+	ret = rtw_pci_init_trx_ring(rtwdev);
+
+	return ret;
+}
+
+static void rtw_pci_reset_buf_desc(struct rtw_dev *rtwdev)
+{
+	struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
+	u32 len;
+	u8 tmp;
+	dma_addr_t dma;
+
+	tmp = rtw_read8(rtwdev, RTK_PCI_CTRL + 3);
+	rtw_write8(rtwdev, RTK_PCI_CTRL + 3, tmp | 0xf7);
+
+	dma = rtwpci->tx_rings[RTW_TX_QUEUE_BCN].r.dma;
+	rtw_write32(rtwdev, RTK_PCI_TXBD_DESA_BCNQ, dma);
+
+	len = rtwpci->tx_rings[RTW_TX_QUEUE_H2C].r.len;
+	dma = rtwpci->tx_rings[RTW_TX_QUEUE_H2C].r.dma;
+	rtwpci->tx_rings[RTW_TX_QUEUE_H2C].r.rp = 0;
+	rtwpci->tx_rings[RTW_TX_QUEUE_H2C].r.wp = 0;
+	rtw_write16(rtwdev, RTK_PCI_TXBD_NUM_H2CQ, len);
+	rtw_write32(rtwdev, RTK_PCI_TXBD_DESA_H2CQ, dma);
+
+	len = rtwpci->tx_rings[RTW_TX_QUEUE_BK].r.len;
+	dma = rtwpci->tx_rings[RTW_TX_QUEUE_BK].r.dma;
+	rtwpci->tx_rings[RTW_TX_QUEUE_BK].r.rp = 0;
+	rtwpci->tx_rings[RTW_TX_QUEUE_BK].r.wp = 0;
+	rtw_write16(rtwdev, RTK_PCI_TXBD_NUM_BKQ, len);
+	rtw_write32(rtwdev, RTK_PCI_TXBD_DESA_BKQ, dma);
+
+	len = rtwpci->tx_rings[RTW_TX_QUEUE_BE].r.len;
+	dma = rtwpci->tx_rings[RTW_TX_QUEUE_BE].r.dma;
+	rtwpci->tx_rings[RTW_TX_QUEUE_BE].r.rp = 0;
+	rtwpci->tx_rings[RTW_TX_QUEUE_BE].r.wp = 0;
+	rtw_write16(rtwdev, RTK_PCI_TXBD_NUM_BEQ, len);
+	rtw_write32(rtwdev, RTK_PCI_TXBD_DESA_BEQ, dma);
+
+	len = rtwpci->tx_rings[RTW_TX_QUEUE_VO].r.len;
+	dma = rtwpci->tx_rings[RTW_TX_QUEUE_VO].r.dma;
+	rtwpci->tx_rings[RTW_TX_QUEUE_VO].r.rp = 0;
+	rtwpci->tx_rings[RTW_TX_QUEUE_VO].r.wp = 0;
+	rtw_write16(rtwdev, RTK_PCI_TXBD_NUM_VOQ, len);
+	rtw_write32(rtwdev, RTK_PCI_TXBD_DESA_VOQ, dma);
+
+	len = rtwpci->tx_rings[RTW_TX_QUEUE_VI].r.len;
+	dma = rtwpci->tx_rings[RTW_TX_QUEUE_VI].r.dma;
+	rtwpci->tx_rings[RTW_TX_QUEUE_VI].r.rp = 0;
+	rtwpci->tx_rings[RTW_TX_QUEUE_VI].r.wp = 0;
+	rtw_write16(rtwdev, RTK_PCI_TXBD_NUM_VIQ, len);
+	rtw_write32(rtwdev, RTK_PCI_TXBD_DESA_VIQ, dma);
+
+	len = rtwpci->tx_rings[RTW_TX_QUEUE_MGMT].r.len;
+	dma = rtwpci->tx_rings[RTW_TX_QUEUE_MGMT].r.dma;
+	rtwpci->tx_rings[RTW_TX_QUEUE_MGMT].r.rp = 0;
+	rtwpci->tx_rings[RTW_TX_QUEUE_MGMT].r.wp = 0;
+	rtw_write16(rtwdev, RTK_PCI_TXBD_NUM_MGMTQ, len);
+	rtw_write32(rtwdev, RTK_PCI_TXBD_DESA_MGMTQ, dma);
+
+	len = rtwpci->tx_rings[RTW_TX_QUEUE_HI0].r.len;
+	dma = rtwpci->tx_rings[RTW_TX_QUEUE_HI0].r.dma;
+	rtwpci->tx_rings[RTW_TX_QUEUE_HI0].r.rp = 0;
+	rtwpci->tx_rings[RTW_TX_QUEUE_HI0].r.wp = 0;
+	rtw_write16(rtwdev, RTK_PCI_TXBD_NUM_HI0Q, len);
+	rtw_write32(rtwdev, RTK_PCI_TXBD_DESA_HI0Q, dma);
+
+	len = rtwpci->rx_rings[RTW_RX_QUEUE_MPDU].r.len;
+	dma = rtwpci->rx_rings[RTW_RX_QUEUE_MPDU].r.dma;
+	rtwpci->rx_rings[RTW_RX_QUEUE_MPDU].r.rp = 0;
+	rtwpci->rx_rings[RTW_RX_QUEUE_MPDU].r.wp = 0;
+	rtw_write16(rtwdev, RTK_PCI_RXBD_NUM_MPDUQ, len & 0xfff);
+	rtw_write32(rtwdev, RTK_PCI_RXBD_DESA_MPDUQ, dma);
+
+	/* reset read/write point */
+	rtw_write32(rtwdev, RTK_PCI_TXBD_RWPTR_CLR, 0xffffffff);
+}
+
+static void rtw_pci_reset_trx_ring(struct rtw_dev *rtwdev)
+{
+	rtw_pci_reset_buf_desc(rtwdev);
+}
+
+static void rtw_pci_enable_interrupt(struct rtw_dev *rtwdev,
+				     struct rtw_pci *rtwpci)
+{
+	rtw_write32(rtwdev, RTK_PCI_HIMR0, rtwpci->irq_mask[0]);
+	rtw_write32(rtwdev, RTK_PCI_HIMR1, rtwpci->irq_mask[1]);
+	rtw_write32(rtwdev, RTK_PCI_HIMR3, rtwpci->irq_mask[3]);
+	rtwpci->irq_enabled = true;
+}
+
+static void rtw_pci_disable_interrupt(struct rtw_dev *rtwdev,
+				      struct rtw_pci *rtwpci)
+{
+	rtw_write32(rtwdev, RTK_PCI_HIMR0, 0);
+	rtw_write32(rtwdev, RTK_PCI_HIMR1, 0);
+	rtw_write32(rtwdev, RTK_PCI_HIMR3, 0);
+	rtwpci->irq_enabled = false;
+}
+
+static int rtw_pci_setup(struct rtw_dev *rtwdev)
+{
+	rtw_pci_reset_trx_ring(rtwdev);
+
+	return 0;
+}
+
+static int rtw_pci_start(struct rtw_dev *rtwdev)
+{
+	struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
+	unsigned long flags;
+
+	spin_lock_irqsave(&rtwpci->irq_lock, flags);
+	rtw_pci_enable_interrupt(rtwdev, rtwpci);
+	spin_unlock_irqrestore(&rtwpci->irq_lock, flags);
+
+	return 0;
+}
+
+static void rtw_pci_stop(struct rtw_dev *rtwdev)
+{
+	struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
+	unsigned long flags;
+
+	spin_lock_irqsave(&rtwpci->irq_lock, flags);
+	rtw_pci_disable_interrupt(rtwdev, rtwpci);
+	spin_unlock_irqrestore(&rtwpci->irq_lock, flags);
+}
+
+static u8 ac_to_hwq[] = {
+	[0] = RTW_TX_QUEUE_VO,
+	[1] = RTW_TX_QUEUE_VI,
+	[2] = RTW_TX_QUEUE_BE,
+	[3] = RTW_TX_QUEUE_BK,
+};
+
+static u8 rtw_hw_queue_mapping(struct sk_buff *skb)
+{
+	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
+	__le16 fc = hdr->frame_control;
+	u8 q_mapping = skb_get_queue_mapping(skb);
+	u8 queue;
+
+	if (unlikely(ieee80211_is_beacon(fc)))
+		queue = RTW_TX_QUEUE_BCN;
+	else if (unlikely(ieee80211_is_mgmt(fc) || ieee80211_is_ctl(fc)))
+		queue = RTW_TX_QUEUE_MGMT;
+	else
+		queue = ac_to_hwq[q_mapping];
+
+	return queue;
+}
+
+static void rtw_pci_release_rsvd_page(struct rtw_pci *rtwpci,
+				      struct rtw_pci_tx_ring *ring)
+{
+	struct sk_buff *prev = skb_dequeue(&ring->queue);
+	struct rtw_pci_tx_data *tx_data;
+	dma_addr_t dma;
+
+	if (!prev)
+		return;
+
+	tx_data = rtw_pci_get_tx_data(prev);
+	dma = tx_data->dma;
+	pci_unmap_single(rtwpci->pdev, dma, prev->len,
+			 PCI_DMA_TODEVICE);
+	dev_kfree_skb_any(prev);
+}
+
+static void rtw_pci_dma_check(struct rtw_dev *rtwdev,
+			      struct rtw_pci_rx_ring *rx_ring,
+			      u32 idx)
+{
+	struct rtw_chip_info *chip = rtwdev->chip;
+	struct rtw_pci_rx_buffer_desc *buf_desc;
+	u32 desc_sz = chip->rx_buf_desc_sz;
+	u16 total_pkt_size;
+	int i;
+
+	buf_desc = (struct rtw_pci_rx_buffer_desc *)(rx_ring->r.head +
+						     idx * desc_sz);
+	for (i = 0; i < 20; i++) {
+		total_pkt_size = le16_to_cpu(buf_desc->total_pkt_size);
+		if (total_pkt_size)
+			return;
+	}
+
+	if (i >= 20)
+		rtw_warn(rtwdev, "pci bus timeout, drop packet\n");
+}
+
+static int rtw_pci_xmit(struct rtw_dev *rtwdev,
+			struct rtw_tx_pkt_info *pkt_info,
+			struct sk_buff *skb, u8 queue)
+{
+	struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
+	struct rtw_chip_info *chip = rtwdev->chip;
+	struct rtw_pci_tx_ring *ring;
+	struct rtw_pci_tx_data *tx_data;
+	dma_addr_t dma;
+	u32 tx_pkt_desc_sz = chip->tx_pkt_desc_sz;
+	u32 tx_buf_desc_sz = chip->tx_buf_desc_sz;
+	u32 size;
+	u32 psb_len;
+	u8 *pkt_desc;
+	struct rtw_pci_tx_buffer_desc *buf_desc;
+	u32 bd_idx;
+
+	ring = &rtwpci->tx_rings[queue];
+
+	size = skb->len;
+
+	if (queue == RTW_TX_QUEUE_BCN)
+		rtw_pci_release_rsvd_page(rtwpci, ring);
+	else if (!avail_desc(ring->r.wp, ring->r.rp, ring->r.len))
+		return -ENOSPC;
+
+	pkt_desc = skb_push(skb, chip->tx_pkt_desc_sz);
+	memset(pkt_desc, 0, tx_pkt_desc_sz);
+	pkt_info->qsel = rtw_pci_get_tx_qsel(skb, queue);
+	rtw_tx_fill_tx_desc(pkt_info, skb);
+	dma = pci_map_single(rtwpci->pdev, skb->data, skb->len,
+			     PCI_DMA_TODEVICE);
+	if (pci_dma_mapping_error(rtwpci->pdev, dma))
+		return -EBUSY;
+
+	/* after this we got dma mapped, there is no way back */
+	buf_desc = get_tx_buffer_desc(ring, tx_buf_desc_sz);
+	memset(buf_desc, 0, tx_buf_desc_sz);
+	psb_len = (skb->len - 1) / 128 + 1;
+	if (queue == RTW_TX_QUEUE_BCN)
+		psb_len |= 1 << RTK_PCI_TXBD_OWN_OFFSET;
+
+	buf_desc[0].psb_len = cpu_to_le16(psb_len);
+	buf_desc[0].buf_size = cpu_to_le16(tx_pkt_desc_sz);
+	buf_desc[0].dma = cpu_to_le32(dma);
+	buf_desc[1].buf_size = cpu_to_le16(size);
+	buf_desc[1].dma = cpu_to_le32(dma + tx_pkt_desc_sz);
+
+	tx_data = rtw_pci_get_tx_data(skb);
+	tx_data->dma = dma;
+	skb_queue_tail(&ring->queue, skb);
+
+	/* kick off tx queue */
+	if (queue != RTW_TX_QUEUE_BCN) {
+		if (++ring->r.wp >= ring->r.len)
+			ring->r.wp = 0;
+		bd_idx = rtw_pci_tx_queue_idx_addr[queue];
+		rtw_write16(rtwdev, bd_idx, ring->r.wp & 0xfff);
+	} else {
+		u32 reg_bcn_work;
+
+		reg_bcn_work = rtw_read8(rtwdev, RTK_PCI_TXBD_BCN_WORK);
+		reg_bcn_work |= BIT_PCI_BCNQ_FLAG;
+		rtw_write8(rtwdev, RTK_PCI_TXBD_BCN_WORK, reg_bcn_work);
+	}
+
+	return 0;
+}
+
+static int rtw_pci_write_data_rsvd_page(struct rtw_dev *rtwdev, u8 *buf,
+					u32 size)
+{
+	struct sk_buff *skb;
+	struct rtw_tx_pkt_info pkt_info;
+	u32 tx_pkt_desc_sz;
+	u32 length;
+
+	tx_pkt_desc_sz = rtwdev->chip->tx_pkt_desc_sz;
+	length = size + tx_pkt_desc_sz;
+	skb = dev_alloc_skb(length);
+	if (!skb)
+		return -ENOMEM;
+
+	skb_reserve(skb, tx_pkt_desc_sz);
+	memcpy((u8 *)skb_put(skb, size), buf, size);
+	memset(&pkt_info, 0, sizeof(pkt_info));
+	pkt_info.tx_pkt_size = size;
+	pkt_info.offset = tx_pkt_desc_sz;
+
+	return rtw_pci_xmit(rtwdev, &pkt_info, skb, RTW_TX_QUEUE_BCN);
+}
+
+static int rtw_pci_write_data_h2c(struct rtw_dev *rtwdev, u8 *buf, u32 size)
+{
+	struct sk_buff *skb;
+	struct rtw_tx_pkt_info pkt_info;
+	u32 tx_pkt_desc_sz;
+	u32 length;
+
+	tx_pkt_desc_sz = rtwdev->chip->tx_pkt_desc_sz;
+	length = size + tx_pkt_desc_sz;
+	skb = dev_alloc_skb(length);
+	if (!skb)
+		return -ENOMEM;
+
+	skb_reserve(skb, tx_pkt_desc_sz);
+	memcpy((u8 *)skb_put(skb, size), buf, size);
+	memset(&pkt_info, 0, sizeof(pkt_info));
+	pkt_info.tx_pkt_size = size;
+
+	return rtw_pci_xmit(rtwdev, &pkt_info, skb, RTW_TX_QUEUE_H2C);
+}
+
+static int rtw_pci_tx(struct rtw_dev *rtwdev,
+		      struct rtw_tx_pkt_info *pkt_info,
+		      struct sk_buff *skb)
+{
+	struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
+	struct rtw_pci_tx_ring *ring;
+	u8 queue = rtw_hw_queue_mapping(skb);
+	int ret;
+
+	ret = rtw_pci_xmit(rtwdev, pkt_info, skb, queue);
+	if (ret)
+		return ret;
+
+	ring = &rtwpci->tx_rings[queue];
+	if (avail_desc(ring->r.wp, ring->r.rp, ring->r.len) < 2) {
+		ieee80211_stop_queue(rtwdev->hw, skb_get_queue_mapping(skb));
+		ring->queue_stopped = true;
+	}
+
+	return 0;
+}
+
+static void rtw_pci_tx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
+			   u8 hw_queue)
+{
+	struct ieee80211_hw *hw = rtwdev->hw;
+	struct ieee80211_tx_info *info;
+	struct rtw_pci_tx_ring *ring;
+	struct rtw_pci_tx_data *tx_data;
+	struct sk_buff *skb;
+	u32 count;
+	u32 bd_idx_addr;
+	u32 bd_idx, cur_rp;
+	u16 q_map;
+
+	ring = &rtwpci->tx_rings[hw_queue];
+
+	bd_idx_addr = rtw_pci_tx_queue_idx_addr[hw_queue];
+	bd_idx = rtw_read32(rtwdev, bd_idx_addr);
+	cur_rp = bd_idx >> 16;
+	cur_rp &= 0xfff;
+	if (cur_rp >= ring->r.rp)
+		count = cur_rp - ring->r.rp;
+	else
+		count = ring->r.len - (ring->r.rp - cur_rp);
+
+	while (count--) {
+		skb = skb_dequeue(&ring->queue);
+		tx_data = rtw_pci_get_tx_data(skb);
+		pci_unmap_single(rtwpci->pdev, tx_data->dma, skb->len,
+				 PCI_DMA_TODEVICE);
+
+		/* just free command packets from host to card */
+		if (hw_queue == RTW_TX_QUEUE_H2C) {
+			dev_kfree_skb_irq(skb);
+			continue;
+		}
+
+		if (ring->queue_stopped &&
+		    avail_desc(ring->r.wp, ring->r.rp, ring->r.len) > 4) {
+			q_map = skb_get_queue_mapping(skb);
+			ieee80211_wake_queue(hw, q_map);
+			ring->queue_stopped = false;
+		}
+
+		skb_pull(skb, rtwdev->chip->tx_pkt_desc_sz);
+
+		info = IEEE80211_SKB_CB(skb);
+		ieee80211_tx_info_clear_status(info);
+		info->flags |= IEEE80211_TX_STAT_ACK;
+		ieee80211_tx_status_irqsafe(hw, skb);
+	}
+
+	ring->r.rp = cur_rp;
+}
+
+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);
+
+		/* 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);
+}
+
+static void rtw_pci_irq_recognized(struct rtw_dev *rtwdev,
+				   struct rtw_pci *rtwpci, u32 *irq_status)
+{
+	irq_status[0] = rtw_read32(rtwdev, RTK_PCI_HISR0);
+	irq_status[1] = rtw_read32(rtwdev, RTK_PCI_HISR1);
+	irq_status[3] = rtw_read32(rtwdev, RTK_PCI_HISR3);
+	irq_status[0] &= rtwpci->irq_mask[0];
+	irq_status[1] &= rtwpci->irq_mask[1];
+	irq_status[3] &= rtwpci->irq_mask[3];
+	rtw_write32(rtwdev, RTK_PCI_HISR0, irq_status[0]);
+	rtw_write32(rtwdev, RTK_PCI_HISR1, irq_status[1]);
+	rtw_write32(rtwdev, RTK_PCI_HISR3, irq_status[3]);
+}
+
+static irqreturn_t rtw_pci_interrupt_handler(int irq, void *dev)
+{
+	struct rtw_dev *rtwdev = dev;
+	struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
+	u32 irq_status[4];
+
+	spin_lock(&rtwpci->irq_lock);
+	if (!rtwpci->irq_enabled)
+		goto out;
+
+	rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status);
+
+	if (irq_status[0] & IMR_MGNTDOK)
+		rtw_pci_tx_isr(rtwdev, rtwpci, RTW_TX_QUEUE_MGMT);
+	if (irq_status[0] & IMR_HIGHDOK)
+		rtw_pci_tx_isr(rtwdev, rtwpci, RTW_TX_QUEUE_HI0);
+	if (irq_status[0] & IMR_BEDOK)
+		rtw_pci_tx_isr(rtwdev, rtwpci, RTW_TX_QUEUE_BE);
+	if (irq_status[0] & IMR_BKDOK)
+		rtw_pci_tx_isr(rtwdev, rtwpci, RTW_TX_QUEUE_BK);
+	if (irq_status[0] & IMR_VODOK)
+		rtw_pci_tx_isr(rtwdev, rtwpci, RTW_TX_QUEUE_VO);
+	if (irq_status[0] & IMR_VIDOK)
+		rtw_pci_tx_isr(rtwdev, rtwpci, RTW_TX_QUEUE_VI);
+	if (irq_status[3] & IMR_H2CDOK)
+		rtw_pci_tx_isr(rtwdev, rtwpci, RTW_TX_QUEUE_H2C);
+	if (irq_status[0] & IMR_ROK)
+		rtw_pci_rx_isr(rtwdev, rtwpci, RTW_RX_QUEUE_MPDU);
+
+out:
+	spin_unlock(&rtwpci->irq_lock);
+
+	return IRQ_HANDLED;
+}
+
+static int rtw_pci_io_mapping(struct rtw_dev *rtwdev,
+			      struct pci_dev *pdev)
+{
+	struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
+	unsigned long len;
+	u8 bar_id = 2;
+	int ret;
+
+	ret = pci_request_regions(pdev, KBUILD_MODNAME);
+	if (ret) {
+		rtw_err(rtwdev, "failed to request pci regions\n");
+		return ret;
+	}
+
+	len = pci_resource_len(pdev, bar_id);
+	rtwpci->mmap = pci_iomap(pdev, bar_id, len);
+	if (!rtwpci->mmap) {
+		rtw_err(rtwdev, "failed to map pci memory\n");
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static void rtw_pci_io_unmapping(struct rtw_dev *rtwdev,
+				 struct pci_dev *pdev)
+{
+	struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
+
+	if (rtwpci->mmap) {
+		pci_iounmap(pdev, rtwpci->mmap);
+		pci_release_regions(pdev);
+	}
+}
+
+static void rtw_dbi_write8(struct rtw_dev *rtwdev, u16 addr, u8 data)
+{
+	u16 write_addr;
+	u16 remainder = addr & 0x3;
+	u8 flag;
+	u8 cnt = 20;
+
+	write_addr = ((addr & 0x0ffc) | (BIT(0) << (remainder + 12)));
+	rtw_write8(rtwdev, REG_DBI_WDATA_V1 + remainder, data);
+	rtw_write16(rtwdev, REG_DBI_FLAG_V1, write_addr);
+	rtw_write8(rtwdev, REG_DBI_FLAG_V1 + 2, 0x01);
+
+	flag = rtw_read8(rtwdev, REG_DBI_FLAG_V1 + 2);
+	while (flag && (cnt != 0)) {
+		udelay(10);
+		flag = rtw_read8(rtwdev, REG_DBI_FLAG_V1 + 2);
+		cnt--;
+	}
+
+	WARN(flag, "DBI write fail\n");
+}
+
+static void rtw_mdio_write(struct rtw_dev *rtwdev, u8 addr, u16 data, bool g1)
+{
+	u8 page;
+	u8 wflag;
+	u8 cnt;
+
+	rtw_write16(rtwdev, REG_MDIO_V1, data);
+
+	page = addr < 0x20 ? 0 : 1;
+	page += g1 ? 0 : 2;
+	rtw_write8(rtwdev, REG_PCIE_MIX_CFG, addr & 0x1f);
+	rtw_write8(rtwdev, REG_PCIE_MIX_CFG + 3, page);
+
+	rtw_write32_mask(rtwdev, REG_PCIE_MIX_CFG, BIT_MDIO_WFLAG_V1, 1);
+	wflag = rtw_read32_mask(rtwdev, REG_PCIE_MIX_CFG, BIT_MDIO_WFLAG_V1);
+
+	cnt = 20;
+	while (wflag && (cnt != 0)) {
+		udelay(10);
+		wflag = rtw_read32_mask(rtwdev, REG_PCIE_MIX_CFG,
+					BIT_MDIO_WFLAG_V1);
+		cnt--;
+	}
+
+	WARN(wflag, "MDIO write fail\n");
+}
+
+static void rtw_pci_phy_cfg(struct rtw_dev *rtwdev)
+{
+	struct rtw_chip_info *chip = rtwdev->chip;
+	struct rtw_intf_phy_para *para;
+	u16 cut;
+	u16 value;
+	u16 offset;
+	u16 ip_sel;
+	int i;
+
+	cut = BIT(0) << rtwdev->hal.cut_version;
+
+	for (i = 0; i < chip->intf_table->n_gen1_para; i++) {
+		para = &chip->intf_table->gen1_para[i];
+		if (!(para->cut_mask & cut))
+			continue;
+		if (para->offset == 0xffff)
+			break;
+		offset = para->offset;
+		value = para->value;
+		ip_sel = para->ip_sel;
+		if (para->ip_sel == RTW_IP_SEL_PHY)
+			rtw_mdio_write(rtwdev, offset, value, true);
+		else
+			rtw_dbi_write8(rtwdev, offset, value);
+	}
+
+	for (i = 0; i < chip->intf_table->n_gen2_para; i++) {
+		para = &chip->intf_table->gen2_para[i];
+		if (!(para->cut_mask & cut))
+			continue;
+		if (para->offset == 0xffff)
+			break;
+		offset = para->offset;
+		value = para->value;
+		ip_sel = para->ip_sel;
+		if (para->ip_sel == RTW_IP_SEL_PHY)
+			rtw_mdio_write(rtwdev, offset, value, false);
+		else
+			rtw_dbi_write8(rtwdev, offset, value);
+	}
+}
+
+static void rtw_pci_parse_configuration(struct rtw_dev *rtwdev,
+					struct pci_dev *pdev)
+{
+	u16 link_control;
+	u8 config;
+
+	/* Disable Clk Request */
+	pci_write_config_byte(pdev, 0x81, 0);
+	/* leave D3 mode */
+	pci_write_config_byte(pdev, 0x44, 0);
+	pci_write_config_byte(pdev, 0x04, 0x06);
+	pci_write_config_byte(pdev, 0x04, 0x07);
+
+	pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &link_control);
+
+	pci_read_config_byte(pdev, 0x98, &config);
+	config |= BIT(4);
+	pci_write_config_byte(pdev, 0x98, config);
+
+	pci_write_config_byte(pdev, 0x70f, 0x17);
+}
+
+static int rtw_pci_claim(struct rtw_dev *rtwdev, struct pci_dev *pdev)
+{
+	int ret;
+
+	ret = pci_enable_device(pdev);
+	if (ret) {
+		rtw_err(rtwdev, "failed to enable pci device\n");
+		return ret;
+	}
+
+	pci_set_master(pdev);
+	pci_set_drvdata(pdev, rtwdev->hw);
+	SET_IEEE80211_DEV(rtwdev->hw, &pdev->dev);
+
+	return 0;
+}
+
+static void rtw_pci_declaim(struct rtw_dev *rtwdev, struct pci_dev *pdev)
+{
+	pci_clear_master(pdev);
+	pci_disable_device(pdev);
+}
+
+static int rtw_pci_setup_resource(struct rtw_dev *rtwdev, struct pci_dev *pdev)
+{
+	struct rtw_pci *rtwpci;
+	int ret;
+
+	rtwpci = (struct rtw_pci *)rtwdev->priv;
+	rtwpci->pdev = pdev;
+
+	/* after this driver can access to hw registers */
+	ret = rtw_pci_io_mapping(rtwdev, pdev);
+	if (ret) {
+		rtw_err(rtwdev, "failed to request pci io region\n");
+		goto err_out;
+	}
+
+	ret = rtw_pci_init(rtwdev);
+	if (ret) {
+		rtw_err(rtwdev, "failed to allocate pci resources\n");
+		goto err_io_unmap;
+	}
+
+	rtw_pci_parse_configuration(rtwdev, pdev);
+	rtw_pci_phy_cfg(rtwdev);
+
+	return 0;
+
+err_io_unmap:
+	rtw_pci_io_unmapping(rtwdev, pdev);
+
+err_out:
+	return ret;
+}
+
+static void rtw_pci_destroy(struct rtw_dev *rtwdev, struct pci_dev *pdev)
+{
+	rtw_pci_deinit(rtwdev);
+	rtw_pci_io_unmapping(rtwdev, pdev);
+}
+
+static int rtw_pci_probe(struct pci_dev *pdev,
+			 const struct pci_device_id *id)
+{
+	struct ieee80211_hw *hw;
+	struct rtw_dev *rtwdev;
+	int drv_data_size;
+	int ret;
+
+	drv_data_size = sizeof(struct rtw_dev) + sizeof(struct rtw_pci);
+	hw = ieee80211_alloc_hw(drv_data_size, &rtw_ops);
+	if (!hw) {
+		dev_err(&pdev->dev, "failed to allocate hw\n");
+		return -ENOMEM;
+	}
+
+	rtwdev = hw->priv;
+	rtwdev->hw = hw;
+	rtwdev->dev = &pdev->dev;
+	rtwdev->chip = (struct rtw_chip_info *)id->driver_data;
+	rtwdev->hci.ops = &rtw_pci_ops;
+	rtwdev->hci.type = RTW_HCI_TYPE_PCIE;
+
+	ret = rtw_core_init(rtwdev);
+	if (ret)
+		goto err_release_hw;
+
+	rtw_dbg(rtwdev,
+		"rtw88 pci probe: vendor=0x%4.04X device=0x%4.04X rev=%d\n",
+		pdev->vendor, pdev->device, pdev->revision);
+
+	ret = rtw_pci_claim(rtwdev, pdev);
+	if (ret) {
+		rtw_err(rtwdev, "failed to claim pci device\n");
+		goto err_deinit_core;
+	}
+
+	ret = rtw_pci_setup_resource(rtwdev, pdev);
+	if (ret) {
+		rtw_err(rtwdev, "failed to setup pci resources\n");
+		goto err_pci_declaim;
+	}
+
+	ret = rtw_chip_info_setup(rtwdev);
+	if (ret) {
+		rtw_err(rtwdev, "failed to setup chip information\n");
+		goto err_destroy_pci;
+	}
+
+	ret = rtw_register_hw(rtwdev, hw);
+	if (ret) {
+		rtw_err(rtwdev, "failed to register hw\n");
+		goto err_destroy_pci;
+	}
+
+	ret = request_irq(pdev->irq, &rtw_pci_interrupt_handler,
+			  IRQF_SHARED, KBUILD_MODNAME, rtwdev);
+	if (ret) {
+		ieee80211_unregister_hw(hw);
+		goto err_destroy_pci;
+	}
+
+	return 0;
+
+err_destroy_pci:
+	rtw_pci_destroy(rtwdev, pdev);
+
+err_pci_declaim:
+	rtw_pci_declaim(rtwdev, pdev);
+
+err_deinit_core:
+	rtw_core_deinit(rtwdev);
+
+err_release_hw:
+	ieee80211_free_hw(hw);
+
+	return ret;
+}
+
+static void rtw_pci_remove(struct pci_dev *pdev)
+{
+	struct ieee80211_hw *hw = pci_get_drvdata(pdev);
+	struct rtw_dev *rtwdev;
+	struct rtw_pci *rtwpci;
+
+	if (!hw)
+		return;
+
+	rtwdev = hw->priv;
+	rtwpci = (struct rtw_pci *)rtwdev->priv;
+
+	rtw_unregister_hw(rtwdev, hw);
+	rtw_pci_disable_interrupt(rtwdev, rtwpci);
+	rtw_pci_destroy(rtwdev, pdev);
+	rtw_pci_declaim(rtwdev, pdev);
+	free_irq(rtwpci->pdev->irq, rtwdev);
+	rtw_core_deinit(rtwdev);
+	ieee80211_free_hw(hw);
+}
+
+static const struct pci_device_id rtw_pci_id_table[] = {
+#ifdef CONFIG_RTW88_8822BE
+	{ RTK_PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0xB822, rtw8822b_hw_spec) },
+#endif
+#ifdef CONFIG_RTW88_8822CE
+	{ RTK_PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0xC822, rtw8822c_hw_spec) },
+#endif
+	{},
+};
+
+static struct pci_driver rtw_pci_driver = {
+	.name = "rtw_pci",
+	.id_table = rtw_pci_id_table,
+	.probe = rtw_pci_probe,
+	.remove = rtw_pci_remove,
+};
+
+struct rtw_hci_ops rtw_pci_ops = {
+	.tx = rtw_pci_tx,
+	.setup = rtw_pci_setup,
+	.start = rtw_pci_start,
+	.stop = rtw_pci_stop,
+
+	.read8 = rtw_pci_read8,
+	.read16 = rtw_pci_read16,
+	.read32 = rtw_pci_read32,
+	.write8 = rtw_pci_write8,
+	.write16 = rtw_pci_write16,
+	.write32 = rtw_pci_write32,
+	.write_data_rsvd_page = rtw_pci_write_data_rsvd_page,
+	.write_data_h2c = rtw_pci_write_data_h2c,
+};
+
+MODULE_DEVICE_TABLE(pci, rtw_pci_id_table);
+
+module_pci_driver(rtw_pci_driver);
+
+MODULE_AUTHOR("Realtek Corporation");
+MODULE_DESCRIPTION("Realtek 802.11ac wireless PCI driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/net/wireless/realtek/rtw88/pci.h b/drivers/net/wireless/realtek/rtw88/pci.h
new file mode 100644
index 0000000..0b672f0
--- /dev/null
+++ b/drivers/net/wireless/realtek/rtw88/pci.h
@@ -0,0 +1,229 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright(c) 2018  Realtek Corporation.
+ */
+
+#ifndef __RTK_PCI_H_
+#define __RTK_PCI_H_
+
+#define RTK_PCI_DEVICE(vend, dev, hw_config)	\
+	PCI_DEVICE(vend, dev),			\
+	.driver_data = (kernel_ulong_t)&(hw_config),
+
+#define RTK_DEFAULT_TX_DESC_NUM 128
+#define RTK_BEQ_TX_DESC_NUM	256
+
+#define RTK_MAX_RX_DESC_NUM	512
+/* 8K + rx desc size */
+#define RTK_PCI_RX_BUF_SIZE	(8192 + 24)
+
+#define RTK_PCI_CTRL		0x300
+#define REG_DBI_WDATA_V1	0x03E8
+#define REG_DBI_FLAG_V1		0x03F0
+#define REG_MDIO_V1		0x03F4
+#define REG_PCIE_MIX_CFG	0x03F8
+#define BIT_MDIO_WFLAG_V1	BIT(5)
+
+#define BIT_PCI_BCNQ_FLAG	BIT(4)
+#define RTK_PCI_TXBD_DESA_BCNQ	0x308
+#define RTK_PCI_TXBD_DESA_H2CQ	0x1320
+#define RTK_PCI_TXBD_DESA_MGMTQ	0x310
+#define RTK_PCI_TXBD_DESA_BKQ	0x330
+#define RTK_PCI_TXBD_DESA_BEQ	0x328
+#define RTK_PCI_TXBD_DESA_VIQ	0x320
+#define RTK_PCI_TXBD_DESA_VOQ	0x318
+#define RTK_PCI_TXBD_DESA_HI0Q	0x340
+#define RTK_PCI_RXBD_DESA_MPDUQ	0x338
+
+/* BCNQ is specialized for rsvd page, does not need to specify a number */
+#define RTK_PCI_TXBD_NUM_H2CQ	0x1328
+#define RTK_PCI_TXBD_NUM_MGMTQ	0x380
+#define RTK_PCI_TXBD_NUM_BKQ	0x38A
+#define RTK_PCI_TXBD_NUM_BEQ	0x388
+#define RTK_PCI_TXBD_NUM_VIQ	0x386
+#define RTK_PCI_TXBD_NUM_VOQ	0x384
+#define RTK_PCI_TXBD_NUM_HI0Q	0x38C
+#define RTK_PCI_RXBD_NUM_MPDUQ	0x382
+#define RTK_PCI_TXBD_IDX_H2CQ	0x132C
+#define RTK_PCI_TXBD_IDX_MGMTQ	0x3B0
+#define RTK_PCI_TXBD_IDX_BKQ	0x3AC
+#define RTK_PCI_TXBD_IDX_BEQ	0x3A8
+#define RTK_PCI_TXBD_IDX_VIQ	0x3A4
+#define RTK_PCI_TXBD_IDX_VOQ	0x3A0
+#define RTK_PCI_TXBD_IDX_HI0Q	0x3B8
+#define RTK_PCI_RXBD_IDX_MPDUQ	0x3B4
+
+#define RTK_PCI_TXBD_RWPTR_CLR	0x39C
+
+#define RTK_PCI_HIMR0		0x0B0
+#define RTK_PCI_HISR0		0x0B4
+#define RTK_PCI_HIMR1		0x0B8
+#define RTK_PCI_HISR1		0x0BC
+#define RTK_PCI_HIMR2		0x10B0
+#define RTK_PCI_HISR2		0x10B4
+#define RTK_PCI_HIMR3		0x10B8
+#define RTK_PCI_HISR3		0x10BC
+/* IMR 0 */
+#define IMR_TIMER2		BIT(31)
+#define IMR_TIMER1		BIT(30)
+#define IMR_PSTIMEOUT		BIT(29)
+#define IMR_GTINT4		BIT(28)
+#define IMR_GTINT3		BIT(27)
+#define IMR_TBDER		BIT(26)
+#define IMR_TBDOK		BIT(25)
+#define IMR_TSF_BIT32_TOGGLE	BIT(24)
+#define IMR_BCNDMAINT0		BIT(20)
+#define IMR_BCNDOK0		BIT(16)
+#define IMR_HSISR_IND_ON_INT	BIT(15)
+#define IMR_BCNDMAINT_E		BIT(14)
+#define IMR_ATIMEND		BIT(12)
+#define IMR_HISR1_IND_INT	BIT(11)
+#define IMR_C2HCMD		BIT(10)
+#define IMR_CPWM2		BIT(9)
+#define IMR_CPWM		BIT(8)
+#define IMR_HIGHDOK		BIT(7)
+#define IMR_MGNTDOK		BIT(6)
+#define IMR_BKDOK		BIT(5)
+#define IMR_BEDOK		BIT(4)
+#define IMR_VIDOK		BIT(3)
+#define IMR_VODOK		BIT(2)
+#define IMR_RDU			BIT(1)
+#define IMR_ROK			BIT(0)
+/* IMR 1 */
+#define IMR_TXFIFO_TH_INT	BIT(30)
+#define IMR_BTON_STS_UPDATE	BIT(29)
+#define IMR_MCUERR		BIT(28)
+#define IMR_BCNDMAINT7		BIT(27)
+#define IMR_BCNDMAINT6		BIT(26)
+#define IMR_BCNDMAINT5		BIT(25)
+#define IMR_BCNDMAINT4		BIT(24)
+#define IMR_BCNDMAINT3		BIT(23)
+#define IMR_BCNDMAINT2		BIT(22)
+#define IMR_BCNDMAINT1		BIT(21)
+#define IMR_BCNDOK7		BIT(20)
+#define IMR_BCNDOK6		BIT(19)
+#define IMR_BCNDOK5		BIT(18)
+#define IMR_BCNDOK4		BIT(17)
+#define IMR_BCNDOK3		BIT(16)
+#define IMR_BCNDOK2		BIT(15)
+#define IMR_BCNDOK1		BIT(14)
+#define IMR_ATIMEND_E		BIT(13)
+#define IMR_ATIMEND		BIT(12)
+#define IMR_TXERR		BIT(11)
+#define IMR_RXERR		BIT(10)
+#define IMR_TXFOVW		BIT(9)
+#define IMR_RXFOVW		BIT(8)
+#define IMR_CPU_MGQ_TXDONE	BIT(5)
+#define IMR_PS_TIMER_C		BIT(4)
+#define IMR_PS_TIMER_B		BIT(3)
+#define IMR_PS_TIMER_A		BIT(2)
+#define IMR_CPUMGQ_TX_TIMER	BIT(1)
+/* IMR 3 */
+#define IMR_H2CDOK		BIT(16)
+
+/* one element is reserved to know if the ring is closed */
+static inline int avail_desc(u32 wp, u32 rp, u32 len)
+{
+	if (rp > wp)
+		return rp - wp - 1;
+	else
+		return len - wp + rp - 1;
+}
+
+#define RTK_PCI_TXBD_OWN_OFFSET 15
+#define RTK_PCI_TXBD_BCN_WORK	0x383
+
+struct rtw_pci_tx_buffer_desc {
+	__le16 buf_size;
+	__le16 psb_len;
+	__le32 dma;
+};
+
+struct rtw_pci_tx_data {
+	dma_addr_t dma;
+};
+
+struct rtw_pci_ring {
+	u8 *head;
+	dma_addr_t dma;
+
+	u8 desc_size;
+
+	u32 len;
+	u32 wp;
+	u32 rp;
+};
+
+struct rtw_pci_tx_ring {
+	struct rtw_pci_ring r;
+	struct sk_buff_head queue;
+	bool queue_stopped;
+};
+
+struct rtw_pci_rx_buffer_desc {
+	__le16 buf_size;
+	__le16 total_pkt_size;
+	__le32 dma;
+};
+
+struct rtw_pci_rx_ring {
+	struct rtw_pci_ring r;
+	struct sk_buff *buf[RTK_MAX_RX_DESC_NUM];
+};
+
+struct rtw_pci {
+	struct pci_dev *pdev;
+
+	/* used for pci interrupt */
+	spinlock_t irq_lock;
+	u32 irq_mask[4];
+	bool irq_enabled;
+
+	struct rtw_pci_tx_ring tx_rings[RTK_MAX_TX_QUEUE_NUM];
+	struct rtw_pci_rx_ring rx_rings[RTK_MAX_RX_QUEUE_NUM];
+
+	void __iomem *mmap;
+};
+
+extern struct rtw_hci_ops rtw_pci_ops;
+
+static u32 max_num_of_tx_queue(u8 queue)
+{
+	u32 max_num;
+
+	switch (queue) {
+	case RTW_TX_QUEUE_BE:
+		max_num = RTK_BEQ_TX_DESC_NUM;
+		break;
+	case RTW_TX_QUEUE_BCN:
+		max_num = 1;
+		break;
+	default:
+		max_num = RTK_DEFAULT_TX_DESC_NUM;
+		break;
+	}
+
+	return max_num;
+}
+
+static inline struct
+rtw_pci_tx_data *rtw_pci_get_tx_data(struct sk_buff *skb)
+{
+	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
+
+	BUILD_BUG_ON(sizeof(struct rtw_pci_tx_data) >
+		     sizeof(info->status.status_driver_data));
+
+	return (struct rtw_pci_tx_data *)info->status.status_driver_data;
+}
+
+static inline
+struct rtw_pci_tx_buffer_desc *get_tx_buffer_desc(struct rtw_pci_tx_ring *ring,
+						  u32 size)
+{
+	u8 *buf_desc;
+
+	buf_desc = ring->r.head + ring->r.wp * size;
+	return (struct rtw_pci_tx_buffer_desc *)buf_desc;
+}
+
+#endif