RE: [PATCH v4 09/19] rtw89: add pci files

[Pkshih <pkshih@realtek.com>]

[-- Attachment #1: Type: text/plain, Size: 7423 bytes --]


> -----Original Message-----
> From: Pkshih
> Sent: Friday, June 25, 2021 6:07 PM
> To: 'Brian Norris'
> Cc: kvalo@codeaurora.org; linux-wireless@vger.kernel.org
> Subject: RE: [PATCH v4 09/19] rtw89: add pci files
> 
> 
> 
> > -----Original Message-----
> > From: Brian Norris [mailto:briannorris@chromium.org]
> > Sent: Saturday, June 19, 2021 3:13 AM
> > To: Pkshih
> > Cc: kvalo@codeaurora.org; linux-wireless@vger.kernel.org
> > Subject: Re: [PATCH v4 09/19] rtw89: add pci files
> >
> >  On Wed, Jun 16, 2021 at 1:31 AM Pkshih <pkshih@realtek.com> wrote:
> > > > -----Original Message-----
> > > > From: Brian Norris [mailto:briannorris@chromium.org]
> >
> > > > On Thu, Apr 29, 2021 at 04:01:39PM +0800, Ping-Ke Shih wrote:
> > > > > --- /dev/null
> > > > > +++ b/drivers/net/wireless/realtek/rtw89/pci.c
> > > > > +static irqreturn_t rtw89_pci_interrupt_threadfn(int irq, void *dev)
> > > > > +{
> > > > > +   struct rtw89_dev *rtwdev = dev;
> > > > > +   struct rtw89_pci *rtwpci = (struct rtw89_pci *)rtwdev->priv;
> > > > > +   u32 isrs[2];
> > > > > +   unsigned long flags;
> > > > > +   u32 unmask0_rx = 0;
> > > > > +
> > > > > +   isrs[0] = rtwpci->isrs[0];
> > > > > +   isrs[1] = rtwpci->isrs[1];
> >
> > By the way, I'm pretty sure you need to hold the irq_lock to safely read these.
> >
> 
> Will do it.
> 
> > ...
> >
> > > By your suggestions, I trace the flow and picture them below:
> >
> > Nice, thanks for that!
> >
> > > But, three exceptions
> > > 1. interrupt is still triggered, even we disable interrupt by step 1).
> > >    That means int_handler is executed again, but threadfn doesn't enable
> > >    interrupt yet.
> >
> > I think maybe that's what IRQF_ONESHOT is for? Do you need to use
> > that? But it might not be a complete solution.
> >
> 
> I tried IRQF_ONESHOT and it works well. But this flag is mutual exclusive with
> IRQF_SHARED that is in use.
> 
> I compare the interrupt count between these two flags, there is no significant
> difference when I running TCP/UDP TX/RX stress test. Surprisingly, interrupt
> count of using IRQF_SHARED is a little lower.
> 
> Since new flow (see below) can properly handle this case, I decide to use
> original flag IRQF_SHARED.
> 
> 
> > >    This is because interrupt event is on the way to host (I think the path is
> > >    long -- from WiFi MAC to PCI MAC to PCI bus to host).
> > >    There's race condition between disable interrupt and interrupt event.
> > >
> > >    I don't plan to fix the race condition, but make the driver handle it properly.
> > >
> > > 2. napi_poll doesn't start immediately at the step 7).
> > >    I don't trace the reason yet, but I think it's reasonable that
> > >    int_threadfn and napi_poll can be ansynchronous.
> > >    Because napi_poll can run in threaded mode as well.
> > >
> > > 3. Since int_threadfn and napi_poll are ansynchronous (similar to exception 2),
> > >    it looks like napi_poll is done before int_threadfn in some situations.
> > >
> > > I'll make the driver handle these cases in next submission (v6).
> >
> > ACK.
> >
> > > Another thing is I need to do local_bh_disable() before calling napi_schedule(),
> > > or kernel warns " NOHZ tick-stop error: Non-RCU local softirq work is pending, handler #08!!!"
> > > I think this is because __napi_schedule() does local_irq_save(), not very sure.
> > >
> > > I investigate other drivers that use napi_schedule() also do local_bh_disable()
> > > before calling napi_schedule(), or do spin_lock_bh(), or in bh context. I think
> > > these are equivalent.
> >
> > OK. I'll admit I'm not that familiar with the locking and context
> > expectations of NAPI APIs (and, they are basically undocumented), but
> > that sounds OK. I was mostly concerned that you were trying to use
> > BH-disable as a mutual exclusion mechanism, when that's not really
> > what it does.
> >
> > > > > +           spin_lock_irqsave(&rtwpci->irq_lock, flags);
> > > > > +           if (rtwpci->running) {
> > > > > +                   rtw89_pci_clear_intrs(rtwdev, rtwpci);
> > > >
> > > > Do you really want to clear interrupts here? I'm not that familiar with
> > > > the hardware here or anything, but that seems like a job for your ISR,
> > > > not the NAPI poll. It also seems like you might double-clear interrupts
> > > > without properly handling them, because you only called
> > > > rtw89_pci_recognize_intrs() in the ISR, not here.
> > >
> > > This chip is an edge-trigger interrupt, so originally I'd like to make "(IMR & ISR)"
> > > become 0, and then next RX packet can trigger the interrupt.
> >
> > But I believe that's racy. If you clear an interrupt now based on
> > rtwpci->halt_c2h_isr and rtwpci->isrs[], and later reread those fields
> > in rtw89_pci_recognize_intrs(), clobbering any saved values, then you
> > may lose an interrupt, I think.
> >
> > Overall, the state you're keeping around, and all the interactions
> > between your NAPI poll and your IRQ handler, are very complex and hard
> > to reason about. I believe your rtw88 driver has a much cleaner
> > interrupt dispatch logic -- it doesn't try to do smart things around
> > reading/writing the interrupt mask in 3 different places (IRQ handler,
> > threaded IRQ handler, and NAPI poll), but just read/stashes/clears the
> > mask in one place (threadfn) and avoids saving that state globally. I
> > think you might have better luck if you can imitate that. But again,
> > maybe I'm missing something.
> >
> 
> I read IRQ handler of rtw88 that looks much simpler, and I picture the
> new flow:
> 
> int_handler             int_threadfn              napi_poll
> -----------             ------------              ---------
>     |
>     |
>     | 1) dis. intr
>     o                      |
>                            | 2) read interrupt status locally
>                            | 3) do TX reclaim
>                            | 4) check if RX?
>                            | 5) re-enable intr
>                            |    (RX is optional)
>                            | 6) schedule_napi
>                            |    (if RX)
>                            : >>>-------------------+ 7) (tasklet start immediately)
>                            :                       |
>                            :                       | 8) set 'doing RX' flag
>                            :                       | 9) do RX things
>                            :                       | 10) clear 'doing RX' flag
>                            :                       | 11) re-enable intr of RX
>                            :                       |
>                            : <<<-------------------o
>                            :
>                            o
> 
> Step 2) read and clear interrupt status with spin_lock_irqsave, and use local
> variables to save the status. Then, the status will be correct, even more
> interrupts are triggered.
> 
> Step 8)/10) add a 'doing RX' flag we don't enable RX interrupt in this period, so
> step 5) will not make a mistake to enable RX interrupt improperly.
> 
> I attach the patch based on v5, and these changes will be included in v6.
> Further suggestions are welcome.
> 

Sorry, I missed the changes of pci.h, so send reference patch again.

--
Ping-Ke


[-- Attachment #2: pci.patch --]
[-- Type: application/octet-stream, Size: 7660 bytes --]

diff --git a/pci.c b/pci.c
index 155f463..c4de554 100644
--- a/pci.c
+++ b/pci.c
@@ -593,44 +593,32 @@ static void rtw89_pci_isr_rxd_unavail(struct rtw89_dev *rtwdev,
 	}
 }
 
-static void rtw89_pci_clear_intrs(struct rtw89_dev *rtwdev,
-				  struct rtw89_pci *rtwpci)
-{
-	rtw89_write32(rtwdev, R_AX_HISR0, rtwpci->halt_c2h_isrs);
-	rtw89_write32(rtwdev, R_AX_PCIE_HISR00, rtwpci->isrs[0]);
-	rtw89_write32(rtwdev, R_AX_PCIE_HISR10, rtwpci->isrs[1]);
-}
-
 static void rtw89_pci_recognize_intrs(struct rtw89_dev *rtwdev,
-				      struct rtw89_pci *rtwpci)
+				      struct rtw89_pci *rtwpci,
+				      struct rtw89_pci_isrs *isrs)
 {
-	rtwpci->halt_c2h_isrs = rtw89_read32(rtwdev, R_AX_HISR0) & rtwpci->halt_c2h_intrs;
-	rtwpci->isrs[0] = rtw89_read32(rtwdev, R_AX_PCIE_HISR00) & rtwpci->intrs[0];
-	rtwpci->isrs[1] = rtw89_read32(rtwdev, R_AX_PCIE_HISR10) & rtwpci->intrs[1];
+	isrs->halt_c2h_isrs = rtw89_read32(rtwdev, R_AX_HISR0) & rtwpci->halt_c2h_intrs;
+	isrs->isrs[0] = rtw89_read32(rtwdev, R_AX_PCIE_HISR00) & rtwpci->intrs[0];
+	isrs->isrs[1] = rtw89_read32(rtwdev, R_AX_PCIE_HISR10) & rtwpci->intrs[1];
+
+	rtw89_write32(rtwdev, R_AX_HISR0, isrs->halt_c2h_isrs);
+	rtw89_write32(rtwdev, R_AX_PCIE_HISR00, isrs->isrs[0]);
+	rtw89_write32(rtwdev, R_AX_PCIE_HISR10, isrs->isrs[1]);
 }
 
 static void rtw89_pci_enable_intr(struct rtw89_dev *rtwdev,
-				  struct rtw89_pci *rtwpci)
+				  struct rtw89_pci *rtwpci, bool exclude_rx)
 {
+	if (exclude_rx || test_bit(RTW89_PCI_FLAG_DOING_RX, rtwpci->flags))
+		rtwpci->intrs[0] &= ~B_AX_RXDMA_INTS_MASK;
+	else
+		rtwpci->intrs[0] |= B_AX_RXDMA_INTS_MASK;
+
 	rtw89_write32(rtwdev, R_AX_HIMR0, rtwpci->halt_c2h_intrs);
 	rtw89_write32(rtwdev, R_AX_PCIE_HIMR00, rtwpci->intrs[0]);
 	rtw89_write32(rtwdev, R_AX_PCIE_HIMR10, rtwpci->intrs[1]);
 }
 
-static void rtw89_pci_enable_intr_unmask0(struct rtw89_dev *rtwdev,
-					  struct rtw89_pci *rtwpci, u32 unmask0)
-{
-	rtwpci->intrs[0] &= ~unmask0;
-	rtw89_pci_enable_intr(rtwdev, rtwpci);
-}
-
-static void rtw89_pci_enable_intr_mask0(struct rtw89_dev *rtwdev,
-					struct rtw89_pci *rtwpci, u32 mask0)
-{
-	rtwpci->intrs[0] |= mask0;
-	rtw89_pci_enable_intr(rtwdev, rtwpci);
-}
-
 static void rtw89_pci_disable_intr(struct rtw89_dev *rtwdev,
 				   struct rtw89_pci *rtwpci)
 {
@@ -639,54 +627,45 @@ static void rtw89_pci_disable_intr(struct rtw89_dev *rtwdev,
 	rtw89_write32(rtwdev, R_AX_PCIE_HIMR10, 0);
 }
 
-static void rtw89_pci_try_napi_schedule(struct rtw89_dev *rtwdev, u32 *unmask0_rx)
-{
-	if (*unmask0_rx && !napi_reschedule(&rtwdev->napi)) {
-		/* if can't invoke napi, RX_IMR must be off already */
-		*unmask0_rx = 0;
-	}
-}
-
 static irqreturn_t rtw89_pci_interrupt_threadfn(int irq, void *dev)
 {
 	struct rtw89_dev *rtwdev = dev;
 	struct rtw89_pci *rtwpci = (struct rtw89_pci *)rtwdev->priv;
-	u32 isrs[2];
+	struct rtw89_pci_isrs isrs;
 	unsigned long flags;
-	u32 unmask0_rx = 0;
+	bool rx = false;
 
-	isrs[0] = rtwpci->isrs[0];
-	isrs[1] = rtwpci->isrs[1];
+	spin_lock_irqsave(&rtwpci->irq_lock, flags);
+	rtw89_pci_recognize_intrs(rtwdev, rtwpci, &isrs);
+	spin_unlock_irqrestore(&rtwpci->irq_lock, flags);
 
 	/* TX ISR */
-	if (isrs[0] & B_AX_TXDMA_CH12_INT)
+	if (isrs.isrs[0] & B_AX_TXDMA_CH12_INT)
 		rtw89_pci_isr_txch_dma(rtwdev, rtwpci, RTW89_TXCH_CH12);
 
 	/* RX ISR */
-	if (isrs[0] & (B_AX_RXDMA_INT | B_AX_RXP1DMA_INT))
-		unmask0_rx = B_AX_RXDMA_INTS_MASK;
-	if (isrs[0] & B_AX_RPQDMA_INT)
+	if (isrs.isrs[0] & (B_AX_RXDMA_INT | B_AX_RXP1DMA_INT))
+		rx = true;
+	if (isrs.isrs[0] & B_AX_RPQDMA_INT)
 		rtw89_pci_isr_rpq_dma(rtwdev, rtwpci);
-	if (isrs[0] & B_AX_RDU_INT) {
+	if (isrs.isrs[0] & B_AX_RDU_INT) {
 		rtw89_pci_isr_rxd_unavail(rtwdev, rtwpci);
-		unmask0_rx = B_AX_RXDMA_INTS_MASK;
+		rx = true;
 	}
 
-	if (rtwpci->halt_c2h_isrs & B_AX_HALT_C2H_INT_EN)
+	if (isrs.halt_c2h_isrs & B_AX_HALT_C2H_INT_EN)
 		rtw89_ser_notify(rtwdev, rtw89_mac_get_err_status(rtwdev));
 
-	/* invoke napi and disable rx_imr within bh_disable, because we must
-	 * ensure napi_poll re-enable rx_imr after this.
-	 */
-	local_bh_disable();
 	spin_lock_irqsave(&rtwpci->irq_lock, flags);
-	rtw89_pci_try_napi_schedule(rtwdev, &unmask0_rx);
-	if (rtwpci->running) {
-		rtw89_pci_clear_intrs(rtwdev, rtwpci);
-		rtw89_pci_enable_intr_unmask0(rtwdev, rtwpci, unmask0_rx);
-	}
+	if (likely(rtwpci->running))
+		rtw89_pci_enable_intr(rtwdev, rtwpci, rx);
 	spin_unlock_irqrestore(&rtwpci->irq_lock, flags);
-	local_bh_enable();
+
+	if (likely(rtwpci->running) && rx) {
+		local_bh_disable();
+		napi_schedule(&rtwdev->napi);
+		local_bh_enable();
+	}
 
 	return IRQ_HANDLED;
 }
@@ -696,22 +675,23 @@ static irqreturn_t rtw89_pci_interrupt_handler(int irq, void *dev)
 	struct rtw89_dev *rtwdev = dev;
 	struct rtw89_pci *rtwpci = (struct rtw89_pci *)rtwdev->priv;
 	unsigned long flags;
+	irqreturn_t irqret = IRQ_WAKE_THREAD;
+
+	spin_lock_irqsave(&rtwpci->irq_lock, flags);
 
 	/* If interrupt event is on the road, it is still trigger interrupt
 	 * even we have done pci_stop() to turn off IMR.
 	 */
-	if (!rtwpci->running)
-		return IRQ_HANDLED;
+	if (unlikely(!rtwpci->running)) {
+		irqret = IRQ_HANDLED;
+		goto exit;
+	}
 
-	/* Disable interrupt here to avoid more interrupts being issued before
-	 * the threadfn ends.
-	 */
-	spin_lock_irqsave(&rtwpci->irq_lock, flags);
 	rtw89_pci_disable_intr(rtwdev, rtwpci);
-	rtw89_pci_recognize_intrs(rtwdev, rtwpci);
+exit:
 	spin_unlock_irqrestore(&rtwpci->irq_lock, flags);
 
-	return IRQ_WAKE_THREAD;
+	return irqret;
 }
 
 #define case_TXCHADDRS(txch) \
@@ -1197,7 +1177,8 @@ static int rtw89_pci_ops_start(struct rtw89_dev *rtwdev)
 
 	spin_lock_irqsave(&rtwpci->irq_lock, flags);
 	rtwpci->running = true;
-	rtw89_pci_enable_intr_mask0(rtwdev, rtwpci, B_AX_RXDMA_INTS_MASK);
+	clear_bit(RTW89_PCI_FLAG_DOING_RX, rtwpci->flags);
+	rtw89_pci_enable_intr(rtwdev, rtwpci, false);
 	spin_unlock_irqrestore(&rtwpci->irq_lock, flags);
 
 	return 0;
@@ -2808,6 +2789,8 @@ static int rtw89_pci_napi_poll(struct napi_struct *napi, int budget)
 	u32 cnt;
 	int ret;
 
+	set_bit(RTW89_PCI_FLAG_DOING_RX, rtwpci->flags);
+
 	ret = rtw89_pci_poll_rxq_dma(rtwdev, rtwpci, budget);
 	if (ret < budget) {
 		napi_complete_done(napi, ret);
@@ -2817,10 +2800,9 @@ static int rtw89_pci_napi_poll(struct napi_struct *napi, int budget)
 			return ret;
 
 		spin_lock_irqsave(&rtwpci->irq_lock, flags);
-		if (rtwpci->running) {
-			rtw89_pci_clear_intrs(rtwdev, rtwpci);
-			rtw89_pci_enable_intr_mask0(rtwdev, rtwpci, B_AX_RXDMA_INTS_MASK);
-		}
+		clear_bit(RTW89_PCI_FLAG_DOING_RX, rtwpci->flags);
+		if (likely(rtwpci->running))
+			rtw89_pci_enable_intr(rtwdev, rtwpci, false);
 		spin_unlock_irqrestore(&rtwpci->irq_lock, flags);
 	}
 
diff --git a/pci.h b/pci.h
index 353ecc6..cdc1f0c 100644
--- a/pci.h
+++ b/pci.h
@@ -356,6 +356,12 @@ enum rtw89_pcie_clkdly_hw {
 	PCIE_CLKDLY_HW_200US = 0x5
 };
 
+enum rtw89_pci_flags {
+	RTW89_PCI_FLAG_DOING_RX,
+
+	NUM_OF_RTW89_PCI_FLAGS
+};
+
 struct rtw89_pci_bd_ram {
 	u8 start_idx;
 	u8 max_num;
@@ -491,6 +497,11 @@ struct rtw89_pci_rx_ring {
 	struct rtw89_rx_desc_info diliver_desc;
 };
 
+struct rtw89_pci_isrs {
+	u32 halt_c2h_isrs;
+	u32 isrs[2];
+};
+
 struct rtw89_pci {
 	struct pci_dev *pdev;
 
@@ -499,14 +510,13 @@ struct rtw89_pci {
 	/* protect TRX resources (exclude RXQ) */
 	spinlock_t trx_lock;
 	bool running;
+	DECLARE_BITMAP(flags, NUM_OF_RTW89_PCI_FLAGS);
 	struct rtw89_pci_tx_ring tx_rings[RTW89_TXCH_NUM];
 	struct rtw89_pci_rx_ring rx_rings[RTW89_RXCH_NUM];
 	struct sk_buff_head h2c_queue;
 
-	u32 halt_c2h_isrs;
 	u32 halt_c2h_intrs;
 	u32 intrs[2];
-	u32 isrs[2];
 	u16 link_ctrl;
 	void __iomem *mmap;
 };