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

[Pkshih <pkshih@realtek.com>]

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



> -----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.

Thank you
--
Ping-Ke



[-- Attachment #2: pci.patch --]
[-- Type: application/octet-stream, Size: 6714 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);
 	}