Search Linux Wireless

RE: long delays in rtl8723 drivers in irq disabled sections

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Lucas Stach [mailto:dev@xxxxxxxxxx]
> Sent: Thursday, November 14, 2019 6:11 AM
> To: Pkshih; wlanfae
> Cc: linux-wireless@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx [mailto:linux-wireless-owner@xxxxxxxxxxxxxxx] On
> Behalf
> > > Of Lucas Stach
> > > Sent: Wednesday, November 13, 2019 5:02 AM
> > > To: wlanfae; Pkshih
> > > Cc: linux-wireless@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx
> > > 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.

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

Thanks
PK






[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Wireless Personal Area Network]     [Linux Bluetooth]     [Wireless Regulations]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Hiking]     [MIPS Linux]     [ARM Linux]     [Linux RAID]

  Powered by Linux