[PATCH 0/3] rtw88: fix some TX/RX bugs

[PATCH 0/3] rtw88: fix some TX/RX bugs

[Ping-Ke Shih]

This patchset fix bugs happened during TX/RX performance test.
First is 88221c doesn't set proper RX_FLAG_DECRYPTED. Second is to fix
count of available TX queue, and then it can wake ieee80211 queue
correctly; otherwise, it can't tx anymore. Third patch is to refine
NAPI deinit flow to safely stop PCI; without this fix, system spin while
it is going to down. 

Guo-Feng Fan (1):
  rtw88: 8821c: Don't set RX_FLAG_DECRYPTED if packet has no encryption

Po-Hao Huang (1):
  rtw88: refine napi deinit flow

Yu-Yen Ting (1):
  rtw88: Fix potential unrecoverable tx queue stop

 drivers/net/wireless/realtek/rtw88/pci.c      | 29 ++++++++++++++-----
 drivers/net/wireless/realtek/rtw88/pci.h      |  1 +
 drivers/net/wireless/realtek/rtw88/rtw8821c.c |  3 +-
 3 files changed, 25 insertions(+), 8 deletions(-)

-- 
2.21.0

[PATCH 1/3] rtw88: 8821c: Don't set RX_FLAG_DECRYPTED if packet has no encryption

[Ping-Ke Shih]

From: Guo-Feng Fan <vincent_fann@realtek.com>

The value of GET_RX_DESC_SWDEC() indicates that if this RX
packet requires software decryption or not. And software
decryption is required when the packet was encrypted and the
hardware failed to decrypt it.

So, GET_RX_DESC_SWDEC() is negative does not mean that this
packet is decrypted, it might just have no encryption at all.
To actually see if the packet is decrypted, driver needs to
further check if the hardware has successfully decrypted it,
with a specific type of encryption algorithm.

Signed-off-by: Guo-Feng Fan <vincent_fann@realtek.com>
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
 drivers/net/wireless/realtek/rtw88/rtw8821c.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/realtek/rtw88/rtw8821c.c b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
index 33c6cf1206c8..785b8181513f 100644
--- a/drivers/net/wireless/realtek/rtw88/rtw8821c.c
+++ b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
@@ -581,7 +581,8 @@ static void rtw8821c_query_rx_desc(struct rtw_dev *rtwdev, u8 *rx_desc,
 	pkt_stat->phy_status = GET_RX_DESC_PHYST(rx_desc);
 	pkt_stat->icv_err = GET_RX_DESC_ICV_ERR(rx_desc);
 	pkt_stat->crc_err = GET_RX_DESC_CRC32(rx_desc);
-	pkt_stat->decrypted = !GET_RX_DESC_SWDEC(rx_desc);
+	pkt_stat->decrypted = !GET_RX_DESC_SWDEC(rx_desc) &&
+			      GET_RX_DESC_ENC_TYPE(rx_desc) != RX_DESC_ENC_NONE;
 	pkt_stat->is_c2h = GET_RX_DESC_C2H(rx_desc);
 	pkt_stat->pkt_len = GET_RX_DESC_PKT_LEN(rx_desc);
 	pkt_stat->drv_info_sz = GET_RX_DESC_DRV_INFO_SIZE(rx_desc);
-- 
2.21.0

Re: [PATCH 1/3] rtw88: 8821c: Don't set RX_FLAG_DECRYPTED if packet has no encryption

[Kalle Valo]

Ping-Ke Shih <pkshih@realtek.com> wrote:

> From: Guo-Feng Fan <vincent_fann@realtek.com>
> 
> The value of GET_RX_DESC_SWDEC() indicates that if this RX
> packet requires software decryption or not. And software
> decryption is required when the packet was encrypted and the
> hardware failed to decrypt it.
> 
> So, GET_RX_DESC_SWDEC() is negative does not mean that this
> packet is decrypted, it might just have no encryption at all.
> To actually see if the packet is decrypted, driver needs to
> further check if the hardware has successfully decrypted it,
> with a specific type of encryption algorithm.
> 
> Signed-off-by: Guo-Feng Fan <vincent_fann@realtek.com>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>

3 patches applied to wireless-drivers-next.git, thanks.

559f6cb31837 rtw88: 8821c: Don't set RX_FLAG_DECRYPTED if packet has no encryption
a548909d7ad7 rtw88: Fix potential unrecoverable tx queue stop
7bd3760c71f7 rtw88: refine napi deinit flow

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20210415084703.27255-2-pkshih@realtek.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH 2/3] rtw88: Fix potential unrecoverable tx queue stop

[Ping-Ke Shih]

From: Yu-Yen Ting <steventing@realtek.com>

If there are lots of packets to be transmitted, the driver would check
whether the available descriptors are sufficient according the read/write
point of tx queue. Once the available descriptor is not enough,
ieee80211_stop_queue is called.

TX ISR, meanwhile, is releasing the tx resources after the packets are
transmitted. This routine may call ieee80211_wake_queue by checking the
available descriptor.

The potential queue stop problem would occur when the tx queue is
stopped due to the heavy traffic. Then thare is no chance to wake the
queue up because the read point is not updated immediately, as a result,
no more packets coulde be transmitted in this queue.

This patch makes sure the ieee80211_wake_queue could be called properly
and avoids the race condition when ring->r.rp, ring->queue_stopped are
updated.

Signed-off-by: Yu-Yen Ting <steventing@realtek.com>
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
 drivers/net/wireless/realtek/rtw88/pci.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
index b8115b31839e..adea3c7551ab 100644
--- a/drivers/net/wireless/realtek/rtw88/pci.c
+++ b/drivers/net/wireless/realtek/rtw88/pci.c
@@ -950,10 +950,12 @@ static int rtw_pci_tx_write(struct rtw_dev *rtwdev,
 		return ret;
 
 	ring = &rtwpci->tx_rings[queue];
+	spin_lock_bh(&rtwpci->irq_lock);
 	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;
 	}
+	spin_unlock_bh(&rtwpci->irq_lock);
 
 	return 0;
 }
@@ -968,7 +970,7 @@ static void rtw_pci_tx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
 	struct sk_buff *skb;
 	u32 count;
 	u32 bd_idx_addr;
-	u32 bd_idx, cur_rp;
+	u32 bd_idx, cur_rp, rp_idx;
 	u16 q_map;
 
 	ring = &rtwpci->tx_rings[hw_queue];
@@ -977,6 +979,7 @@ static void rtw_pci_tx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
 	bd_idx = rtw_read32(rtwdev, bd_idx_addr);
 	cur_rp = bd_idx >> 16;
 	cur_rp &= TRX_BD_IDX_MASK;
+	rp_idx = ring->r.rp;
 	if (cur_rp >= ring->r.rp)
 		count = cur_rp - ring->r.rp;
 	else
@@ -1000,12 +1003,15 @@ static void rtw_pci_tx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
 		}
 
 		if (ring->queue_stopped &&
-		    avail_desc(ring->r.wp, ring->r.rp, ring->r.len) > 4) {
+		    avail_desc(ring->r.wp, rp_idx, ring->r.len) > 4) {
 			q_map = skb_get_queue_mapping(skb);
 			ieee80211_wake_queue(hw, q_map);
 			ring->queue_stopped = false;
 		}
 
+		if (++rp_idx >= ring->r.len)
+			rp_idx = 0;
+
 		skb_pull(skb, rtwdev->chip->tx_pkt_desc_sz);
 
 		info = IEEE80211_SKB_CB(skb);
-- 
2.21.0

[PATCH 3/3] rtw88: refine napi deinit flow

[Ping-Ke Shih]

From: Po-Hao Huang <phhuang@realtek.com>

We used to stop napi before disabling irqs. And it turns out
to cause some problem when we try to stop device while interrupt arrives.

To safely stop pci, we do three steps:
1. disable interrupt
2. synchronize_irq
3. stop_napi
Since step 2 and 3 may not finish as expected when interrupt is enabled,
use rtwpci->running to decide whether interrupt should be re-enabled at
the time.

Fixes: 9e2fd29864c5 ("rtw88: add napi support")
Signed-off-by: Po-Hao Huang <phhuang@realtek.com>
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
 drivers/net/wireless/realtek/rtw88/pci.c | 19 ++++++++++++++-----
 drivers/net/wireless/realtek/rtw88/pci.h |  1 +
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
index adea3c7551ab..f59a4c462e3b 100644
--- a/drivers/net/wireless/realtek/rtw88/pci.c
+++ b/drivers/net/wireless/realtek/rtw88/pci.c
@@ -581,23 +581,30 @@ static int rtw_pci_start(struct rtw_dev *rtwdev)
 {
 	struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
 
+	rtw_pci_napi_start(rtwdev);
+
 	spin_lock_bh(&rtwpci->irq_lock);
+	rtwpci->running = true;
 	rtw_pci_enable_interrupt(rtwdev, rtwpci, false);
 	spin_unlock_bh(&rtwpci->irq_lock);
 
-	rtw_pci_napi_start(rtwdev);
-
 	return 0;
 }
 
 static void rtw_pci_stop(struct rtw_dev *rtwdev)
 {
 	struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
+	struct pci_dev *pdev = rtwpci->pdev;
 
+	spin_lock_bh(&rtwpci->irq_lock);
+	rtwpci->running = false;
+	rtw_pci_disable_interrupt(rtwdev, rtwpci);
+	spin_unlock_bh(&rtwpci->irq_lock);
+
+	synchronize_irq(pdev->irq);
 	rtw_pci_napi_stop(rtwdev);
 
 	spin_lock_bh(&rtwpci->irq_lock);
-	rtw_pci_disable_interrupt(rtwdev, rtwpci);
 	rtw_pci_dma_release(rtwdev, rtwpci);
 	spin_unlock_bh(&rtwpci->irq_lock);
 }
@@ -1212,7 +1219,8 @@ static irqreturn_t rtw_pci_interrupt_threadfn(int irq, void *dev)
 		rtw_fw_c2h_cmd_isr(rtwdev);
 
 	/* all of the jobs for this interrupt have been done */
-	rtw_pci_enable_interrupt(rtwdev, rtwpci, rx);
+	if (rtwpci->running)
+		rtw_pci_enable_interrupt(rtwdev, rtwpci, rx);
 	spin_unlock_bh(&rtwpci->irq_lock);
 
 	return IRQ_HANDLED;
@@ -1633,7 +1641,8 @@ static int rtw_pci_napi_poll(struct napi_struct *napi, int budget)
 	if (work_done < budget) {
 		napi_complete_done(napi, work_done);
 		spin_lock_bh(&rtwpci->irq_lock);
-		rtw_pci_enable_interrupt(rtwdev, rtwpci, false);
+		if (rtwpci->running)
+			rtw_pci_enable_interrupt(rtwdev, rtwpci, false);
 		spin_unlock_bh(&rtwpci->irq_lock);
 		/* When ISR happens during polling and before napi_complete
 		 * while no further data is received. Data on the dma_ring will
diff --git a/drivers/net/wireless/realtek/rtw88/pci.h b/drivers/net/wireless/realtek/rtw88/pci.h
index e76fc549a788..0ffae887527a 100644
--- a/drivers/net/wireless/realtek/rtw88/pci.h
+++ b/drivers/net/wireless/realtek/rtw88/pci.h
@@ -211,6 +211,7 @@ struct rtw_pci {
 	spinlock_t irq_lock;
 	u32 irq_mask[4];
 	bool irq_enabled;
+	bool running;
 
 	/* napi structure */
 	struct net_device netdev;
-- 
2.21.0