Message-ID: <5B2DA6FDDF928F4E855344EE0A5C39D1D5C9E04E@RTITMBSVM04.realtek.com.tw>
Date:   Fri, 15 Nov 2019 00:31:28 +0000
From:   Pkshih <pkshih@...ltek.com>
To:     Lucas Stach <dev@...xeye.de>, wlanfae <wlanfae@...ltek.com>
CC:     "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: long delays in rtl8723 drivers in irq disabled sections


> -----Original Message-----
> From: Lucas Stach [mailto:dev@...xeye.de]
> Sent: Friday, November 15, 2019 5:25 AM
> To: Pkshih; wlanfae
> Cc: linux-wireless@...r.kernel.org; netdev@...r.kernel.org
> Subject: Re: long delays in rtl8723 drivers in irq disabled sections
> 
> Am Donnerstag, den 14.11.2019, 01:41 +0000 schrieb Pkshih:
> > > -----Original Message-----
> > > From: Lucas Stach [mailto:dev@...xeye.de]
> > > Sent: Thursday, November 14, 2019 6:11 AM
> > > To: Pkshih; wlanfae
> > > Cc: linux-wireless@...r.kernel.org; netdev@...r.kernel.org
> > > Subject: Re: long delays in rtl8723 drivers in irq disabled sections
> > >
> > > Hi PK,
> > >
> > > Am Mittwoch, den 13.11.2019, 03:43 +0000 schrieb Pkshih:
> > > > > -----Original Message-----
> > > > > From: linux-wireless-owner@...r.kernel.org [mailto:linux-wireless-owner@...r.kernel.org] On
> > > Behalf
> > > > > Of Lucas Stach
> > > > > Sent: Wednesday, November 13, 2019 5:02 AM
> > > > > To: wlanfae; Pkshih
> > > > > Cc: linux-wireless@...r.kernel.org; netdev@...r.kernel.org
> > > > > Subject: long delays in rtl8723 drivers in irq disabled sections
> > > > >
> > > > > Hi all,
> > > > >
> > > > > while investigating some latency issues on my laptop I stumbled across
> > > > > quite large delays in the rtl8723 PHY code, which are done in IRQ
> > > > > disabled atomic sections, which is blocking IRQ servicing for all
> > > > > devices in the system.
> > > > >
> > > > > Specifically there are 3 consecutive 1ms delays in
> > > > > rtl8723_phy_rf_serial_read(), which is used in an IRQ disabled call
> > > > > path. Sadly those delays don't have any comment in the code explaining
> > > > > why they are needed. I hope that anyone can tell if those delays are
> > > > > strictly neccessary and if so if they really need to be this long.
> > > > >
> > > >
> > > > These delays are because read RF register is an indirect access that hardware
> > > > needs time to accomplish read action, but there's no ready bit, so delay
> > > > is required to guarantee the read value is correct.
> > >
> > > Thanks for the confirmation, I suspected something like this.
> > >
> > > > It is possible to use smaller delay, but it's exactly required.
> > >
> > > 1ms seems like an eternity on modern hardware, even for an indirect
> > > read.
> > >
> >
> > For 8723be, three 1ms delays can be replaced by one 120us delay, likes
> >
> > @@ -89,12 +89,10 @@ u32 rtl8723_phy_rf_serial_read(struct ieee80211_hw *hw,
> >             (newoffset << 23) | BLSSIREADEDGE;
> >         rtl_set_bbreg(hw, RFPGA0_XA_HSSIPARAMETER2, MASKDWORD,
> >                       tmplong & (~BLSSIREADEDGE));
> > -       mdelay(1);
> >         rtl_set_bbreg(hw, pphyreg->rfhssi_para2, MASKDWORD, tmplong2);
> > -       mdelay(1);
> >         rtl_set_bbreg(hw, RFPGA0_XA_HSSIPARAMETER2, MASKDWORD,
> >                       tmplong | BLSSIREADEDGE);
> > -       mdelay(1);
> > +       udelay(120);
> >         if (rfpath == RF90_PATH_A)
> >                 rfpi_enable = (u8) rtl_get_bbreg(hw, RFPGA0_XA_HSSIPARAMETER1,
> >                                                  BIT(8));
> >
> > I think it'd be better.
> 
> Yes, that looks much better. Even better would be a small comment on
> how you arrived at 120us. Some internal documentation, or is this
> mostly empirical?
> 

120us is maximum stall time with little tolerance suggested by our designer.
Not only 8723be/8723de, I'll take a while to check other chips.

> > > > An alternative way is to prevent calling this function in IRQ disabled flow.
> > > > Could you share the calling trace?
> > >
> > > Sure, trimmed callstack below. As you can see the IRQ disabled section
> > > is started via a spin_lock_irqsave(). The trace is from a 8723de
> > > module, which is still out of tree, but the same code is present in
> > > mainline and used by the other 8723 variants.
> >
> > By now, 8723DE will be upstream through rtw88 instead of rtlwifi.
> 
> I haven't seen any patches for this particular chip yet. Is there any
> roadmap on when we can expect this support to be added to the upstream
> rtw88 driver?
> 

8723DE is under review internally, and it will start to be upstream at 5.5 or 5.6.

> > > I don't know if this function needs to guard against something running
> > > in the IRQ handler, so depending on the answer to that the solution
> > > might be as simple as not disabling IRQs when taking the spinlock.
> > >
> > > kworker/-276     4d...    0us : _raw_spin_lock_irqsave
> > > kworker/-276     4d...    0us : rtl8723_phy_rf_serial_read <-rtl8723de_phy_set_rf_reg
> > > kworker/-276     4d...    1us : rtl8723_phy_query_bb_reg <-rtl8723_phy_rf_serial_read
> > > kworker/-276     4d...    3us : rtl8723_phy_set_bb_reg <-rtl8723_phy_rf_serial_read
> > > kworker/-276     4d...    4us : __const_udelay <-rtl8723_phy_rf_serial_read
> > > kworker/-276     4d...    4us!: delay_mwaitx <-rtl8723_phy_rf_serial_read
> > > kworker/-276     4d... 1004us : rtl8723_phy_set_bb_reg <-rtl8723_phy_rf_serial_read
> > > [...]
> > >
> >
> > I check TX/RX interrupt handlers, and I don't find one calls RF read function
> > by now. I suspect that old code controls RF to do PS in interrupt context, so
> > _irqsave version is used to ensure read RF isn't interrupted or deadlock.
> > So, I change spin_lock to non-irqsave version, and do some tests on 8723BE
> > that works well.
> >
> > What do you think about two fixes mentioned above? If they're ok, I can send
> > two patches to resolve this long delays.
> 
> Yes, both changes do make sense to me. If we can avoid an unnecessary
> IRQ disable we should do so. Even then shrinking the waits to bare
> minimum as required by the hardware seems to be a good thing,
> especially since the wait is still done under a spinlock, so can not
> use a sleeping wait.
> 

Some callers are still in tasklet context. I'll check if it's possible to move
them to work queue, but I think this isn't a short term task.

PK

Powered by blists - more mailing lists