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

[You-Sheng Yang]

On 2020-05-19 15:38, Stefan Bader wrote:
> 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

Thank you for catching those two fixes. Checked no more extra warnings
introduced. Resubmitted as
https://lists.ubuntu.com/archives/kernel-team/2020-May/110116.html.

>> [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. ;)

We must have confidence that maybe they'd show up with presents for we
backported a patch to improve TX performance. :)

>> 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(-)

You-Sheng Yang

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20200521/5b56c06a/attachment.sig>