NACK/Cmnt: [PATCH 0/2][SRU][B/E/F/OEM-OSP1-B] Fix Wi-Fi association after being off-ed and on

[Stefan Bader]

On 14.05.20 10:47, You-Sheng Yang wrote:
> BugLink: https://bugs.launchpad.net/bugs/1878296
> 
> [Impact]
> With the rtl8723bu wireless adapter, if the wifi is turned off and then
> turned on again, the system can't connect to a wireless network until
> the system is rebooted.
> 
> [Fix]
> Commit e542e66b7c2e("rtl8xxxu: add bluetooth co-existence support for
> single antenna"), which landed v5.5 and as its subject suggests, support
> bluetooth co-existence for RTL8723BU and fixes WiFi disconnection
> problem when Bluetooth is in action.

Somehow your Impact and Fix section (and btw, the whole justification should be
in the bug report (Bug Description)) do not make sense. Your impact and the bug
report talk about issues after turning Wi-Fi off and on again.
The Fix is about bluetooth co-existence. Not obvious why this would make a
difference to the reported bug.
> 
> Commit a9bb0b515778("rtl8xxxu: Improve TX performance of RTL8723BU on
> rtl8xxxu driver") was added as a prerequisite to aforementioned fix.
> 

Both of those patches are rather extensive, adding a lot of new code and both
have one follow-up which drops unused variables to avoid build warnings.
If we put changes like this into SRU, I would rather include those fixups, too.
Even if those are just warnings.

> [Test Case]
> 1. Turn on Wi-Fi and get associated with some AP.
> 2. Pair some bluetooth device.
> 3. Turn off Wi-Fi and turn again.
> 4. Expect Wi-Fi reconnects to the previous associated AP

OK, so the test-case would be the missing link. You should update the bug
report's title so it something like "failing to re-associate while bluetooth is
active" or so (if you have a better way saying it, use that).
Then an updated SRU justification (Impact should also mention the relationship
between bluetooth and the issue) should go into the "Bug Description".

Once that is done, please re-submit adding:

commit 4fcef86091327d92008ca0328d45075343e7edea
Author: YueHaibing <yuehaibing at huawei.com>
Date:   Wed Oct 23 15:53:42 2019 +0800

    rtl8xxxu: remove set but not used variable 'rate_mask'
  Fixes: a9bb0b515778

commit eac08515d7bd665d306cefa2ae9f3de56e875d6d
Author: zhengbin <zhengbin13 at huawei.com>
Date:   Tue Nov 19 10:25:14 2019 +0800

    rtl8xxxu: Remove set but not used variable 'vif','dev','len'
  Fixes: e542e66b7c2e

Also, I would strongly suggest to take a current copy of bionic maser-next,
apply the set and do a test compile using cranky. Jesse should be able to help
if you have questions about the set-up. Reasoning is that Bionic is the oldest
kernel compared to where the patches come from. If there are issues caused by
warnings treated as errors it would be there. Also there it is most likely to
find functions not existing.

-Stefan
> 
> [Regression Potential]
> Low. Both commits only affect RTL8723BU.

PS: If RTL8723BU gets broken and all angry users were to show up at your door,
you might consider this differently. ;)
> 
> Chris Chiu (2):
>   rtl8xxxu: Improve TX performance of RTL8723BU on rtl8xxxu driver
>   rtl8xxxu: add bluetooth co-existence support for single antenna
> 
>  .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h  |  92 +++-
>  .../realtek/rtl8xxxu/rtl8xxxu_8723b.c         |   2 -
>  .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 499 +++++++++++++++++-
>  3 files changed, 579 insertions(+), 14 deletions(-)
>