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.
Regards,
Lucas
> -----Original Message-----
> From: [email protected] [mailto:[email protected]] On Behalf
> Of Lucas Stach
> Sent: Wednesday, November 13, 2019 5:02 AM
> To: wlanfae; Pkshih
> Cc: [email protected]; [email protected]
> 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. It is possible to
use smaller delay, but it's exactly required.
An alternative way is to prevent calling this function in IRQ disabled flow.
Could you share the calling trace?
Thanks
PK
Hi PK,
Am Mittwoch, den 13.11.2019, 03:43 +0000 schrieb Pkshih:
> > -----Original Message-----
> > From: [email protected] [mailto:[email protected]] On Behalf
> > Of Lucas Stach
> > Sent: Wednesday, November 13, 2019 5:02 AM
> > To: wlanfae; Pkshih
> > Cc: [email protected]; [email protected]
> > 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.
>
> 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.
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
[...]
Regards,
Lucas
> -----Original Message-----
> From: Lucas Stach [mailto:[email protected]]
> Sent: Thursday, November 14, 2019 6:11 AM
> To: Pkshih; wlanfae
> Cc: [email protected]; [email protected]
> 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: [email protected] [mailto:[email protected]] On
> Behalf
> > > Of Lucas Stach
> > > Sent: Wednesday, November 13, 2019 5:02 AM
> > > To: wlanfae; Pkshih
> > > Cc: [email protected]; [email protected]
> > > 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
On 11/13/19 7:41 PM, Pkshih wrote:
>
>
>> -----Original Message-----
>> From: Lucas Stach [mailto:[email protected]]
>> Sent: Thursday, November 14, 2019 6:11 AM
>> To: Pkshih; wlanfae
>> Cc: [email protected]; [email protected]
>> 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: [email protected] [mailto:[email protected]] On
>> Behalf
>>>> Of Lucas Stach
>>>> Sent: Wednesday, November 13, 2019 5:02 AM
>>>> To: wlanfae; Pkshih
>>>> Cc: [email protected]; [email protected]
>>>> 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.
Lucas,
If the above patch fixes the problem with the 8723de, I will modify the GitHub
driver. Although 8723de will be added to rtw88, I will keep the driver in
rtlwifi_new.
Larry
Am Donnerstag, den 14.11.2019, 01:41 +0000 schrieb Pkshih:
> > -----Original Message-----
> > From: Lucas Stach [mailto:[email protected]]
> > Sent: Thursday, November 14, 2019 6:11 AM
> > To: Pkshih; wlanfae
> > Cc: [email protected]; [email protected]
> > 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: [email protected] [mailto:[email protected]] On
> > Behalf
> > > > Of Lucas Stach
> > > > Sent: Wednesday, November 13, 2019 5:02 AM
> > > > To: wlanfae; Pkshih
> > > > Cc: [email protected]; [email protected]
> > > > 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?
> > > 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?
> > 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.
Regards,
Lucas
Hi Larry,
Am Donnerstag, den 14.11.2019, 15:25 -0600 schrieb Larry Finger:
[...]
> > > 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.
>
> Lucas,
>
> If the above patch fixes the problem with the 8723de, I will modify the GitHub
> driver. Although 8723de will be added to rtw88, I will keep the driver in
> rtlwifi_new.
I'm currently running the rtlwifi_new based modules, modified with the
reduced waits as suggested by PK, as well as removing the IRQ disable
from the spinlocks in both rtl8723de_phy_query_rf_reg() and
rtl8723de_phy_set_rf_reg().
I can confirm that with those changes rtl8723de no longer shows up my
latency traces.
Regards,
Lucas
> -----Original Message-----
> From: Lucas Stach [mailto:[email protected]]
> Sent: Friday, November 15, 2019 5:25 AM
> To: Pkshih; wlanfae
> Cc: [email protected]; [email protected]
> 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:[email protected]]
> > > Sent: Thursday, November 14, 2019 6:11 AM
> > > To: Pkshih; wlanfae
> > > Cc: [email protected]; [email protected]
> > > 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: [email protected] [mailto:[email protected]] On
> > > Behalf
> > > > > Of Lucas Stach
> > > > > Sent: Wednesday, November 13, 2019 5:02 AM
> > > > > To: wlanfae; Pkshih
> > > > > Cc: [email protected]; [email protected]
> > > > > 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