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