Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210115092405.8081-1-pkshih@realtek.com> <20210115092405.8081-4-pkshih@realtek.com>
In-Reply-To: <20210115092405.8081-4-pkshih@realtek.com>
From:   Brian Norris <briannorris@chromium.org>
Date:   Fri, 22 Jan 2021 14:57:49 -0800
Message-ID: <CA+ASDXPtwyZMByRDaAQv9b-DEBeRgPxpDz4+mCP-zi6P_-zr7w@mail.gmail.com>
Subject: Re: [PATCH v4 3/8] rtw88: add napi support
To:     Ping-Ke Shih <pkshih@realtek.com>
Cc:     Yan-Hsuan Chuang <tony0620emma@gmail.com>,
        Kalle Valo <kvalo@codeaurora.org>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        phhuang@realtek.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Jan 15, 2021 at 1:26 AM Ping-Ke Shih <pkshih@realtek.com> wrote:
> --- a/drivers/net/wireless/realtek/rtw88/pci.c
> +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> @@ -935,16 +935,49 @@ static void rtw_pci_tx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
>         ring->r.rp = cur_rp;
>  }
>
> -static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
> +static void rtw_pci_rx_isr(struct rtw_dev *rtwdev)
> +{
> +       struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
> +       struct napi_struct *napi = &rtwpci->napi;
> +
> +       napi_schedule(napi);

I don't claim to be all that familiar with NAPI, but my understanding
is that you are scheduling a NAPI poll, but immediately after you
return here, you're re-enabling your RX interrupt. That doesn't sound
like how NAPI is supposed to work -- you're supposed to wait to
re-enable your interrupt until you're done with your polling function.
Ref: https://wiki.linuxfoundation.org/networking/napi

> +}
...
> +static u32 rtw_pci_rx_napi(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
>                            u8 hw_queue)
...

Are you sure you don't want any locking in rtw_pci_rx_napi()?
Previously, you held irq_lock for the entirety of rtw_pci_rx_isr(),
but now all the RX work is being deferred to a NAPI context, without
any additional lock. IIUC, that means you can be both handling RX and
other ISR operations at the same time. Is that intentional?

> +static int rtw_pci_napi_poll(struct napi_struct *napi, int budget)
> +{
> +       struct rtw_pci *rtwpci = container_of(napi, struct rtw_pci, napi);
> +       struct rtw_dev *rtwdev = container_of((void *)rtwpci, struct rtw_dev,
> +                                             priv);
> +       int work_done = 0;
> +
> +       while (work_done < budget) {
> +               u32 work_done_once;
> +
> +               work_done_once = rtw_pci_rx_napi(rtwdev, rtwpci,
> +                                                RTW_RX_QUEUE_MPDU);
> +               if (work_done_once == 0)
> +                       break;
> +               work_done += work_done_once;
> +       }
> +       if (work_done < budget) {
> +               napi_complete_done(napi, work_done);
> +               /* When ISR happens during polling and before napi_complete
> +                * while no further data is received. Data on the dma_ring will
> +                * not be processed immediately. Check whether dma ring is
> +                * empty and perform napi_schedule accordingly.
> +                */
> +               if (rtw_pci_get_hw_rx_ring_nr(rtwdev, rtwpci, NULL, NULL))
> +                       napi_schedule(napi);
> +       }
> +
> +       return work_done;
> +}
> +
> +static void rtw_pci_napi_init(struct rtw_dev *rtwdev)
> +{
> +       struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
> +
> +       init_dummy_netdev(&rtwpci->netdev);
> +       netif_napi_add(&rtwpci->netdev, &rtwpci->napi, rtw_pci_napi_poll,
> +                      RTW_NAPI_WEIGHT_NUM);
> +       napi_enable(&rtwpci->napi);
> +}
...
> @@ -1547,6 +1624,8 @@ int rtw_pci_probe(struct pci_dev *pdev,
>                 goto err_destroy_pci;
>         }
>
> +       rtw_pci_napi_init(rtwdev);

You're initializing NAPI after you've already established your ISR,
and your ISR might start scheduling NAPI. Even if that's unlikely
(because you haven't initiated any RX traffic yet), it seems like an
ordering problem -- shouldn't you initialize the NAPI device, then set
up the ISR, and only then call napi_enable()?

Brian

> +
>         return 0;
>
>  err_destroy_pci: