Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: long delays in rtl8723 drivers in irq disabled sections
To:     Pkshih <pkshih@realtek.com>, Lucas Stach <dev@lynxeye.de>,
        wlanfae <wlanfae@realtek.com>
Cc:     "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>
References: <5de65447f1d115f436f764a7ec811c478afbe2e0.camel@lynxeye.de>
 <5B2DA6FDDF928F4E855344EE0A5C39D1D5C9CE47@RTITMBSVM04.realtek.com.tw>
 <e83f5b699c5652cbe2350ac3576215d24b748e03.camel@lynxeye.de>
 <5B2DA6FDDF928F4E855344EE0A5C39D1D5C9D5F6@RTITMBSVM04.realtek.com.tw>
From:   Larry Finger <Larry.Finger@lwfinger.net>
Message-ID: <a6d55cfc-9de9-ce6e-1dcf-814372772327@lwfinger.net>
Date:   Thu, 14 Nov 2019 15:25:59 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.2.1
MIME-Version: 1.0
In-Reply-To: <5B2DA6FDDF928F4E855344EE0A5C39D1D5C9D5F6@RTITMBSVM04.realtek.com.tw>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On 11/13/19 7:41 PM, Pkshih wrote:
> 
> 
>> -----Original Message-----
>> From: Lucas Stach [mailto:dev@lynxeye.de]
>> Sent: Thursday, November 14, 2019 6:11 AM
>> To: Pkshih; wlanfae
>> Cc: linux-wireless@vger.kernel.org; netdev@vger.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@vger.kernel.org [mailto:linux-wireless-owner@vger.kernel.org] On
>> Behalf
>>>> Of Lucas Stach
>>>> Sent: Wednesday, November 13, 2019 5:02 AM
>>>> To: wlanfae; Pkshih
>>>> Cc: linux-wireless@vger.kernel.org; netdev@vger.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.
> 
>>>
>>> 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