rtl8821ae dbi read question

rtl8821ae dbi read question

[Nik Nyby]

In drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c, we have this 
function:

  static u8 _rtl8821ae_dbi_read(struct rtl_priv *rtlpriv, u16 addr)
  {
      u16 read_addr = addr & 0xfffc;
      u8 tmp = 0, count = 0, ret = 0;

      rtl_write_word(rtlpriv, REG_DBI_ADDR, read_addr);
      rtl_write_byte(rtlpriv, REG_DBI_FLAG, 0x2);
      tmp = rtl_read_byte(rtlpriv, REG_DBI_FLAG);
      count = 0;
      while (tmp && count < 20) {
          udelay(10);
          tmp = rtl_read_byte(rtlpriv, REG_DBI_FLAG);
          count++;
      }
      if (0 == tmp) {
          read_addr = REG_DBI_RDATA + addr % 4;
          ret = rtl_read_word(rtlpriv, read_addr);
      }
      return ret;
  }

Near the end of the function, in this line:

   ret = rtl_read_word(rtlpriv, read_addr);

rtl_read_word() returns a u16, but "ret" is declared as a u8. Is that a 
problem, or is this code correct?

What's prompting this question is that I'm getting frequent disconnects 
from my access point with my rtl8821ae device. I've experienced this 
behavior both before and after the recent change to this function in 
commit b8b8b16352cd90c6083033fd4487f04fae935c18.

Re: rtl8821ae dbi read question

[Larry Finger]

On 11/04/2017 06:27 PM, Nik Nyby wrote:
> In drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c, we have this function:
> 
>   static u8 _rtl8821ae_dbi_read(struct rtl_priv *rtlpriv, u16 addr)
>   {
>       u16 read_addr = addr & 0xfffc;
>       u8 tmp = 0, count = 0, ret = 0;
> 
>       rtl_write_word(rtlpriv, REG_DBI_ADDR, read_addr);
>       rtl_write_byte(rtlpriv, REG_DBI_FLAG, 0x2);
>       tmp = rtl_read_byte(rtlpriv, REG_DBI_FLAG);
>       count = 0;
>       while (tmp && count < 20) {
>           udelay(10);
>           tmp = rtl_read_byte(rtlpriv, REG_DBI_FLAG);
>           count++;
>       }
>       if (0 == tmp) {
>           read_addr = REG_DBI_RDATA + addr % 4;
>           ret = rtl_read_word(rtlpriv, read_addr);
>       }
>       return ret;
>   }
> 
> Near the end of the function, in this line:
> 
>    ret = rtl_read_word(rtlpriv, read_addr);
> 
> rtl_read_word() returns a u16, but "ret" is declared as a u8. Is that a problem, 
> or is this code correct?
> 
> What's prompting this question is that I'm getting frequent disconnects from my 
> access point with my rtl8821ae device. I've experienced this behavior both 
> before and after the recent change to this function in commit 
> b8b8b16352cd90c6083033fd4487f04fae935c18.

If you read the commit message for commit b8b8b16352cd, you will find that we do 
not understand why using a byte read causes failure, but reverting the change so 
that it is a word read made it function again. The "fix" was found by a user 
doing bisection, and verified on my system, where the RTL8821AE has stable 
connections. The transfer rates take wild swings, but the connection is not dropped.

If you NIC behaves differently, then you will need to help us debug the problem. 
If you had a kernel that worked, then you might try bisection. If that is not 
possible, you might try various combinations of module parameters aspm, 
int_clear, and msi. Those parameters default to 1, so setting one or more of 
them to zero is the test. Note, however, that changing any of these might cause 
the system to fail, thus I recommend that you test changes dynamically with 
modprobe, and not create a file in /etc/modprobe.d/ until you have verified that 
changing one or more of these parameters actually helps. That will keep from 
making your system unbootable.

Larry

Re: rtl8821ae dbi read question

[Nik Nyby]

On 11/05/2017 11:50 AM, Larry Finger wrote:
> 
> If you read the commit message for commit b8b8b16352cd, you will find 
> that we do not understand why using a byte read causes failure, but 
> reverting the change so that it is a word read made it function again. 
> The "fix" was found by a user doing bisection, and verified on my 
> system, where the RTL8821AE has stable connections. The transfer rates 
> take wild swings, but the connection is not dropped.
> 
> If you NIC behaves differently, then you will need to help us debug the 
> problem. If you had a kernel that worked, then you might try bisection. 

I just tested on kernels 4.12 and 4.13 - my connection gets dropped on 
those kernels as well. Right now I'm using 4.14-rc7.

> If that is not possible, you might try various combinations of module 
> parameters aspm, int_clear, and msi
Thanks for these suggestions. Disabling ASPM solves my connection 
dropping issue! Previously, I was getting disconnected around 10 seconds 
after starting "git clone" on a large git repository. Now, I haven't 
seen the connection drop once.

I can help debug the issue. I'm using a Lenovo Ideapad 320-15ABR laptop. 
Here's the lspci -vv info for my NIC:

01:00.0 Network controller: Realtek Semiconductor Co., Ltd. RTL8821AE 
802.11ac PCIe Wireless Network Adapter
         Subsystem: Lenovo RTL8821AE 802.11ac PCIe Wireless Network Adapter
         Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- 
ParErr- Stepping- SERR- FastB2B- DisINTx+
         Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- 
<TAbort- <MAbort- >SERR- <PERR- INTx-
         Latency: 0, Cache Line Size: 32 bytes
         Interrupt: pin A routed to IRQ 33
         Region 0: I/O ports at 2000 [size=256]
         Region 2: Memory at f0b00000 (64-bit, non-prefetchable) [size=16K]
         Capabilities: <access denied>
         Kernel driver in use: rtl8821ae

Re: rtl8821ae dbi read question

[Larry Finger]

On 11/05/2017 02:53 PM, Nik Nyby wrote:
> On 11/05/2017 11:50 AM, Larry Finger wrote:
>>
>> If you read the commit message for commit b8b8b16352cd, you will find that we 
>> do not understand why using a byte read causes failure, but reverting the 
>> change so that it is a word read made it function again. The "fix" was found 
>> by a user doing bisection, and verified on my system, where the RTL8821AE has 
>> stable connections. The transfer rates take wild swings, but the connection is 
>> not dropped.
>>
>> If you NIC behaves differently, then you will need to help us debug the 
>> problem. If you had a kernel that worked, then you might try bisection. 
> 
> I just tested on kernels 4.12 and 4.13 - my connection gets dropped on those 
> kernels as well. Right now I'm using 4.14-rc7.
> 
>> If that is not possible, you might try various combinations of module 
>> parameters aspm, int_clear, and msi
> Thanks for these suggestions. Disabling ASPM solves my connection dropping 
> issue! Previously, I was getting disconnected around 10 seconds after starting 
> "git clone" on a large git repository. Now, I haven't seen the connection drop 
> once.
> 
> I can help debug the issue. I'm using a Lenovo Ideapad 320-15ABR laptop. Here's 
> the lspci -vv info for my NIC:
> 
> 01:00.0 Network controller: Realtek Semiconductor Co., Ltd. RTL8821AE 802.11ac 
> PCIe Wireless Network Adapter
>          Subsystem: Lenovo RTL8821AE 802.11ac PCIe Wireless Network Adapter
>          Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- 
> Stepping- SERR- FastB2B- DisINTx+
>          Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- 
> <MAbort- >SERR- <PERR- INTx-
>          Latency: 0, Cache Line Size: 32 bytes
>          Interrupt: pin A routed to IRQ 33
>          Region 0: I/O ports at 2000 [size=256]
>          Region 2: Memory at f0b00000 (64-bit, non-prefetchable) [size=16K]
>          Capabilities: <access denied>
>          Kernel driver in use: rtl8821ae

If "aspm=0" fixes your issue, then there is nothing more to debug. That option 
is there to handle cases where the PCI connection does not behave the way that 
most do. This condition seems to be most common on Lenovo machines. Frequently, 
ASPM problems are accompanied by PCI errors, but that seems not to be the case 
for your machine. Create a suitable .conf file in /etc/modprobe.d/ and you 
should be OK.

Larry

Re: rtl8821ae dbi read question

[Nik Nyby]

I also want to note that adding rtl8821ae.aspm=0 to my grub kernel boot 
command doesn't fix my problem. (I'm building this driver into the 
kernel, not as a module). My connection dropping problem is fixed only 
if I comment out the aspm init code in the driver, per this patch:
   https://patchwork.kernel.org/patch/9951511/

Re: rtl8821ae dbi read question

[James Cameron]

On Sun, Nov 05, 2017 at 04:15:36PM -0500, Nik Nyby wrote:
> I also want to note that adding rtl8821ae.aspm=0 to my grub kernel
> boot command doesn't fix my problem. (I'm building this driver into
> the kernel, not as a module). My connection dropping problem is
> fixed only if I comment out the aspm init code in the driver, per
> this patch:
>   https://patchwork.kernel.org/patch/9951511/

Interesting; that patch was for testing, and did more than just aspm=0.

Check for a BIOS update from your vendor.  Device registers can be
configured and persist despite warm reboot, which implies there is no
register reset in driver start or device reset, which further implies
that the BIOS can easily affect the outcome.

Check also for different behaviour after cold reboot; that is a power
down for 30 seconds then power up.

Do you yet have a known working kernel to bisect against?

-- 
James Cameron
http://quozl.netrek.org/

Re: rtl8821ae dbi read question

[Larry Finger]

On 11/05/2017 03:15 PM, Nik Nyby wrote:
> I also want to note that adding rtl8821ae.aspm=0 to my grub kernel boot command 
> doesn't fix my problem. (I'm building this driver into the kernel, not as a 
> module). My connection dropping problem is fixed only if I comment out the aspm 
> init code in the driver, per this patch:
>    https://patchwork.kernel.org/patch/9951511/

Disabling all of _rtl8821ae_enable_aspm_back_door() may not be wise. We tried 
that patch as part of debugging.

That routine consists of two mdio r/w sequences, and 3 dbi r/w sequences. The 
third one of the latter is only used for RTL8812AE, thus it can be ignored.

What happens if you try disabling those r/w pairs one at a time? It is possible 
that one, or more of them, should be disabled when aspm is zero. It should not 
matter whether the driver is built in, or loaded as a module. Please note that I 
never build that code in as I always want the option of refreshing the driver 
without rebooting.

Larry

Re: rtl8821ae dbi read question

[Nik Nyby]

On 11/05/2017 09:09 PM, Larry Finger wrote:
> Disabling all of _rtl8821ae_enable_aspm_back_door() may not be wise. We 
> tried that patch as part of debugging.
> 
> That routine consists of two mdio r/w sequences, and 3 dbi r/w 
> sequences. The third one of the latter is only used for RTL8812AE, thus 
> it can be ignored.
> 
> What happens if you try disabling those r/w pairs one at a time? It is 
> possible that one, or more of them, should be disabled when aspm is 
> zero. 

I first tried disabling only the mdio sequences, then the dbi sequences. 
I was able to reproduce my problem in both cases. But I have found that 
my problem is resolved by only removing the enable_aspm() call, and 
leaving the call to _rtl8821ae_enable_aspm_back_door() intact.

If removing rtlpriv->intf_ops->enable_aspm(hw) is the same as setting 
aspm=0, then it's possible I'm not setting the option correctly. But as 
far as I can see right now, removing enable_aspm() is necessary for my 
stable connection.

> It should not matter whether the driver is built in, or loaded as 
> a module. Please note that I never build that code in as I always want 
> the option of refreshing the driver without rebooting.

Thanks for the info - I'll do the same.

Re: rtl8821ae dbi read question

[Larry Finger]

On 11/05/2017 10:09 PM, Nik Nyby wrote:
> On 11/05/2017 09:09 PM, Larry Finger wrote:
>> Disabling all of _rtl8821ae_enable_aspm_back_door() may not be wise. We tried 
>> that patch as part of debugging.
>>
>> That routine consists of two mdio r/w sequences, and 3 dbi r/w sequences. The 
>> third one of the latter is only used for RTL8812AE, thus it can be ignored.
>>
>> What happens if you try disabling those r/w pairs one at a time? It is 
>> possible that one, or more of them, should be disabled when aspm is zero. 
> 
> I first tried disabling only the mdio sequences, then the dbi sequences. I was 
> able to reproduce my problem in both cases. But I have found that my problem is 
> resolved by only removing the enable_aspm() call, and leaving the call to 
> _rtl8821ae_enable_aspm_back_door() intact.
> 
> If removing rtlpriv->intf_ops->enable_aspm(hw) is the same as setting aspm=0, 
> then it's possible I'm not setting the option correctly. But as far as I can see 
> right now, removing enable_aspm() is necessary for my stable connection.

Your results seem to indicate that aspm, which is support_aspm internally, is 
not being set to zero. The first executable statement in enable_aspm is

         if (!ppsc->support_aspm)
                 return;

That statement should disable the entire routine; however, you still need to 
disable the call to this routine. It is possible that the timing is messed up 
when built into the kernel. Building as a module will be instructive.

Larry

Re: rtl8821ae dbi read question

[Nik Nyby]

On 11/06/2017 08:52 AM, Larry Finger wrote:
> 
> Your results seem to indicate that aspm, which is support_aspm 
> internally, is not being set to zero. The first executable statement in 
> enable_aspm is
> 
>          if (!ppsc->support_aspm)
>                  return;
> 
> That statement should disable the entire routine; however, you still 
> need to disable the call to this routine. It is possible that the timing 
> is messed up when built into the kernel. Building as a module will be 
> instructive.
> 
> Larry
> 

FYI, when building this driver as a module, I'm not seeing my unstable 
connection problem anymore, with the aspm=0 config set in 
/etc/modprobe.d/. Thanks for your help with this.