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