From: Pkshih <pkshih@realtek.com>
To: Brian Norris <briannorris@chromium.org>
Cc: "kvalo@codeaurora.org" <kvalo@codeaurora.org>,
"linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Subject: RE: [PATCH v4 09/19] rtw89: add pci files
Date: Wed, 16 Jun 2021 08:31:25 +0000 [thread overview]
Message-ID: <45dd7da687a444d5acbc13eb67dcee97@realtek.com> (raw)
In-Reply-To: <YMFzAZUysQ5HxZlK@google.com>
> -----Original Message-----
> From: Brian Norris [mailto:briannorris@chromium.org]
> Sent: Thursday, June 10, 2021 10:04 AM
> To: Pkshih
> Cc: kvalo@codeaurora.org; linux-wireless@vger.kernel.org
> Subject: Re: [PATCH v4 09/19] rtw89: add pci files
>
> Hi,
>
> I'm slowly making my way through this series. I think a lot of this
> looks a lot better than the first rtw88 submission I looked at, so
> that's nice!
>
> Mostly small notes for this patch, but a few larger questions about IRQ
> handling and NAPI:
>
> 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 void rtw89_pci_recognize_intrs(struct rtw89_dev *rtwdev,
> > + struct rtw89_pci *rtwpci)
> > +{
> > + 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];
> > +}
> > +
> > +static void rtw89_pci_enable_intr(struct rtw89_dev *rtwdev,
> > + struct rtw89_pci *rtwpci)
> > +{
> > + 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;
>
> I might be misreading something, but I believe "mask" (as in, "mask
> an interrupt") is usually meant as "disable" -- see, for instance, the
> conventions in 'struct irq_chip' -- and this looks like you're using the
> term "unmask" to mean disable. This confuses my reading of code that
> calls this function.
>
> I'd suggest either swapping the names (unmask vs. mask) or else pick an
> independent name ("rx on" and "rx off"? something based on "on/off",
> "disable/enable"? "set/clear"?).
>
Understand.
I'll refine the interrupt and NAPI flow (see below), and I suppose these functions
will be removed, so I don't change the names right away.
> > + 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)
> > +{
> > + rtw89_write32(rtwdev, R_AX_HIMR0, 0);
> > + rtw89_write32(rtwdev, R_AX_PCIE_HIMR00, 0);
> > + 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];
> > + unsigned long flags;
> > + u32 unmask0_rx = 0;
> > +
> > + isrs[0] = rtwpci->isrs[0];
> > + isrs[1] = rtwpci->isrs[1];
> > +
> > + /* TX ISR */
> > + if (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)
> > + rtw89_pci_isr_rpq_dma(rtwdev, rtwpci);
> > + if (isrs[0] & B_AX_RDU_INT) {
> > + rtw89_pci_isr_rxd_unavail(rtwdev, rtwpci);
> > + unmask0_rx = B_AX_RXDMA_INTS_MASK;
> > + }
> > +
> > + if (rtwpci->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();
>
> I'm not sure I understand this; disabling BH isn't enough to ensure NAPI
> contexts aren't running in parallel with this -- that's what a lock is
> for. And, you're already holding irq_lock.
>
> The other part of this problem (I think) is that you have the ordering
> wrong here -- don't you want to set the interrupt state *before*
> scheduling NAPI? That way, if an RX event comes while we're doing this,
> we know that either the NAPI context is still running or scheduled (and
> will re-enable the RX interrupt when done), or else we're going to
> schedule a new poll (which will eventually re-enable the interrupt).
>
> In other words, I think you should
> 1. drop the BH disable/enable
> 2. set the interrupt state first, followed by napi_schedule() (if there
> was an RX IRQ pending)
> 3. stop trying to look at the napi_reschedule() return value (i.e., just
> use napi_schedule())
>
> Am I missing something? I'm just trying to reason through the code here;
> I haven't stress-tested my suggestsions or anything, nor experienced
> whatever problem you were trying to solve here.
By your suggestions, I trace the flow and picture them below:
int_handler int_threadfn napi_poll
----------- ------------ ---------
|
|
| 1) dis. intr
| 2) read status
o |
| 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) do RX things
: | 9) re-enable intr of RX
: |
: <<<-------------------o
:
o
In my preliminary test, it works as above flow normally.
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.
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).
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.
>
> > + 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);
> > + }
> > + spin_unlock_irqrestore(&rtwpci->irq_lock, flags);
> > + local_bh_enable();
> > +
> > + return IRQ_HANDLED;
> > +}
>
> ...
>
> > +static u32 rtw89_pci_ops_read32_cmac(struct rtw89_dev *rtwdev, u32 addr)
> > +{
> > + struct rtw89_pci *rtwpci = (struct rtw89_pci *)rtwdev->priv;
> > + u32 val = readl(rtwpci->mmap + addr);
> > + int count;
> > +
> > + for (count = 0; ; count++) {
> > + if (val != RTW89_R32_DEAD)
> > + return val;
> > + if (count >= MAC_REG_POOL_COUNT) {
> > + rtw89_warn(rtwdev, "addr 0x%x = 0xdeadbeef\n", addr);
>
> I understand this is a constant, but it might be better to either
> stringify the macro:
>
> rtw89_warn(rtwdev, "addr %#x = " #RTW89_R32_DEAD "\n", addr);
>
> or just use val:
>
> rtw89_warn(rtwdev, "addr %#x = %#x\n", addr, val);
>
Will do it.
> > + return RTW89_R32_DEAD;
> > + }
> > + rtw89_pci_ops_write32(rtwdev, R_AX_CK_EN, B_AX_CMAC_ALLCKEN);
> > + val = readl(rtwpci->mmap + addr);
> > + }
> > +
> > + return val;
> > +}
>
> ...
>
> > +static int
> > +rtw89_read16_mdio(struct rtw89_dev *rtwdev, u8 addr, u8 speed, u16 *val)
> > +{
> > + int ret;
> > +
> > + ret = rtw89_pci_check_mdio(rtwdev, addr, speed, B_AX_MDIO_RFLAG);
> > + if (ret) {
> > + rtw89_err(rtwdev, "[ERR]MDIO R16 0x%X fail!\n", addr);
>
> Dump |ret|?
Okay
>
> > + return ret;
> > + }
> > + *val = rtw89_read16(rtwdev, R_AX_MDIO_RDATA);
> > +
> > + return 0;
> > +}
>
> ...
>
> > +static int rtw89_dbi_read8(struct rtw89_dev *rtwdev, u16 addr, u8 *value)
> > +{
> > + u16 read_addr = addr & B_AX_DBI_ADDR_MSK;
> > + u8 flag;
> > + int ret;
> > +
> > + rtw89_write16(rtwdev, R_AX_DBI_FLAG, read_addr);
> > + rtw89_write8(rtwdev, R_AX_DBI_FLAG + 2, B_AX_DBI_RFLAG >> 16);
> > +
> > + ret = read_poll_timeout_atomic(rtw89_read8, flag, !flag, 10,
> > + 10 * RTW89_PCI_WR_RETRY_CNT, false,
> > + rtwdev, R_AX_DBI_FLAG + 2);
> > +
> > + if (!ret) {
> > + read_addr = R_AX_DBI_RDATA + (addr & 3);
> > + *value = rtw89_read8(rtwdev, read_addr);
> > + } else {
> > + WARN(1, "failed to read DBI register, addr=0x%04x\n", addr);
> > + ret = -EIO;
>
> You've got some weird whitespace here and a few other places. Search for
> the string "= " (2 spaces) -- there should only be 1.
Fixed
>
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int
> > +__get_target(struct rtw89_dev *rtwdev, u16 *target, enum rtw89_pcie_phy phy_rate)
> > +{
> > + u16 val, tar;
> > + int ret;
> > +
> > + /* Enable counter */
> > + ret = rtw89_read16_mdio(rtwdev, RAC_CTRL_PPR_V1, phy_rate, &val);
> > + if (ret)
> > + return ret;
> > + ret = rtw89_write16_mdio(rtwdev, RAC_CTRL_PPR_V1, val & ~BIT(12),
>
> You mostly do a good job on using macros with meaningful names instead
> of magic numbers, but you've still got quite a few uses of BIT() that
> probably should be macros. I'd suggest taking another pass through this
> driver for usages of raw BIT() and GENMASK(), and see where it's
> reasonable to add macro names (either in the .c file if it's a very
> local usage, or just add to the .h next to the register definitions).
I fix pci.c first, and I will pass through whole driver in next submission.
>
> > + phy_rate);
> ...
> > +}
> > +
> > +static int rtw89_pci_auto_refclk_cal(struct rtw89_dev *rtwdev, bool autook_en)
> > +{
> > + enum rtw89_pcie_phy phy_rate;
> > + u16 val16, mgn_set, div_set, tar;
> > + u8 val8, bdr_ori;
> > + bool l1_flag = false;
> > + int ret = 0;
> > +
> > + ret = rtw89_dbi_read8(rtwdev, RTW89_PCIE_PHY_RATE, &val8);
> > + if (ret) {
> > + rtw89_err(rtwdev, "[ERR]dbi_r8_pcie %X\n", RTW89_PCIE_PHY_RATE);
> > + return ret;
> > + }
> > +
> > + if (FIELD_GET(GENMASK(1, 0), val8) == 0x1) {
> > + phy_rate = PCIE_PHY_GEN1;
> > + } else if (FIELD_GET(GENMASK(1, 0), val8) == 0x2) {
> > + phy_rate = PCIE_PHY_GEN2;
> > + } else {
> > + rtw89_err(rtwdev, "[ERR]PCIe PHY rate not support\n");
>
> Dump the value of |val8| in this error message? Also, this probably
> makes more sense as "not supported".
Added.
>
> > + return -EOPNOTSUPP;
> > + }
> ...
> > + /* Obtain div and margin */
>
> Extra space.
Fixed
>
> ...
>
> > +static int rtw89_pci_napi_poll(struct napi_struct *napi, int budget)
> > +{
> > + struct rtw89_dev *rtwdev = container_of(napi, struct rtw89_dev, napi);
> > + struct rtw89_pci *rtwpci = (struct rtw89_pci *)rtwdev->priv;
> > + unsigned long flags;
> > + u32 cnt;
> > + int ret;
> > +
> > + ret = rtw89_pci_poll_rxq_dma(rtwdev, rtwpci, budget);
> > + if (ret < budget) {
> > + napi_complete_done(napi, ret);
> > +
> > + cnt = rtw89_pci_rxbd_recalc(rtwdev, &rtwpci->rx_rings[RTW89_RXCH_RXQ]);
> > + if (cnt && napi_reschedule(napi))
> > + return ret;
> > +
> > + 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, it seems that enable RX interrupt (step 9 of above picture) can already
raise interrupt.
>
> > + rtw89_pci_enable_intr_mask0(rtwdev, rtwpci, B_AX_RXDMA_INTS_MASK);
> > + }
> > + spin_unlock_irqrestore(&rtwpci->irq_lock, flags);
> > + }
> > +
> > + return ret;
> > +}
>
> ...
>
> > +static void rtw89_pci_remove(struct pci_dev *pdev)
> > +{
> > + struct ieee80211_hw *hw = pci_get_drvdata(pdev);
> > + struct rtw89_dev *rtwdev;
> > +
> > + if (!hw)
> > + return;
>
> Is this even possible (hw==NULL)? You always "set" this when probe() is
> successful, so remove() should only be called with a non-NULL drvdata.
Removed.
I have confirmed if probe() is unsuccessful, kernel won't call remove().
>
> > +
> > + rtwdev = hw->priv;
> > +
> > + rtw89_pci_free_irq(rtwdev, pdev);
> > + rtw89_core_napi_deinit(rtwdev);
> > + rtw89_core_unregister(rtwdev);
> > + rtw89_pci_clear_resource(rtwdev, pdev);
> > + rtw89_pci_declaim_device(rtwdev, pdev);
> > + rtw89_core_deinit(rtwdev);
> > + ieee80211_free_hw(hw);
> > +}
>
> Brian
> ------Please consider the environment before printing this e-mail.
next prev parent reply other threads:[~2021-06-16 8:31 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-29 8:01 [PATCH v4 00/19] rtw89: add Realtek 802.11ax driver Ping-Ke Shih
2021-04-29 8:01 ` [PATCH v4 01/19] rtw89: add CAM files Ping-Ke Shih
2021-04-29 8:01 ` [PATCH v4 02/19] rtw89: add BT coexistence files Ping-Ke Shih
2021-04-29 8:01 ` [PATCH v4 03/19] rtw89: add core and trx files Ping-Ke Shih
2021-04-29 8:01 ` [PATCH v4 04/19] rtw89: add debug files Ping-Ke Shih
2021-04-29 8:01 ` [PATCH v4 05/19] rtw89: add efuse files Ping-Ke Shih
2021-04-29 8:01 ` [PATCH v4 06/19] rtw89: add files to download and communicate with firmware Ping-Ke Shih
2021-04-30 1:10 ` Brian Norris
2021-05-01 0:39 ` Pkshih
2021-04-29 8:01 ` [PATCH v4 07/19] rtw89: add MAC files Ping-Ke Shih
2021-04-29 8:01 ` [PATCH v4 08/19] rtw89: implement mac80211 ops Ping-Ke Shih
2021-04-29 8:01 ` [PATCH v4 09/19] rtw89: add pci files Ping-Ke Shih
2021-06-10 2:03 ` Brian Norris
2021-06-16 8:31 ` Pkshih [this message]
2021-06-18 19:13 ` Brian Norris
2021-06-25 10:07 ` Pkshih
2021-07-01 0:47 ` Pkshih
2021-07-19 18:50 ` Brian Norris
2021-07-21 3:20 ` Pkshih
2021-04-29 8:01 ` [PATCH v4 10/19] rtw89: add phy files Ping-Ke Shih
2021-04-29 8:01 ` [PATCH v4 11/19] rtw89: define register names Ping-Ke Shih
2021-04-29 8:01 ` [PATCH v4 12/19] rtw89: add regulatory support Ping-Ke Shih
2021-04-29 8:01 ` [PATCH v4 13/19] rtw89: 8852a: add 8852a specific files Ping-Ke Shih
2021-04-29 21:10 ` Brian Norris
2021-04-29 23:43 ` Pkshih
2021-04-30 1:22 ` Brian Norris
2021-05-01 0:54 ` Pkshih
2021-04-29 8:01 ` [PATCH v4 14/19] rtw89: 8852a: add 8852a RFK files Ping-Ke Shih
2021-04-29 8:01 ` [PATCH v4 15/19] rtw89: 8852a: add 8852a RFK tables Ping-Ke Shih
2021-04-29 8:01 ` [PATCH v4 17/19] rtw89: add ser to recover error reported by firmware Ping-Ke Shih
2021-04-29 8:01 ` [PATCH v4 18/19] rtw89: add PS files Ping-Ke Shih
2021-04-29 8:01 ` [PATCH v4 19/19] rtw89: add Kconfig and Makefile Ping-Ke Shih
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=45dd7da687a444d5acbc13eb67dcee97@realtek.com \
--to=pkshih@realtek.com \
--cc=briannorris@chromium.org \
--cc=kvalo@codeaurora.org \
--cc=linux-wireless@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).