linux-kernel - Re: [PATCH v2 05/10] rtw88: iterate over vif/sta list non-atomically

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e0aa1ba4336ab130712e1fcb425e6fd0adca4145.camel@realtek.com>
Date:   Wed, 8 Jun 2022 13:25:34 +0000
From:   Ping-Ke Shih <pkshih@...ltek.com>
To:     "martin.blumenstingl@...glemail.com" 
        <martin.blumenstingl@...glemail.com>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>
CC:     "johannes@...solutions.net" <johannes@...solutions.net>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "neojou@...il.com" <neojou@...il.com>,
        "kvalo@...nel.org" <kvalo@...nel.org>,
        "tony0620emma@...il.com" <tony0620emma@...il.com>,
        "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux@...i-kroll.de" <linux@...i-kroll.de>
Subject: Re: [PATCH v2 05/10] rtw88: iterate over vif/sta list non-atomically

Hi Martin and Sascha,

Thank you both.

On Wed, 2022-06-08 at 00:06 +0200, Martin Blumenstingl wrote:
> Hi Sascha,
> 
> thanks for this patch!
> 
> On Mon, May 30, 2022 at 3:55 PM Sascha Hauer <s.hauer@...gutronix.de> wrote:
> [...]
> >  drivers/net/wireless/realtek/rtw88/phy.c  |   6 +-
> >  drivers/net/wireless/realtek/rtw88/ps.c   |   2 +-
> >  drivers/net/wireless/realtek/rtw88/util.c | 103 ++++++++++++++++++++++
> >  drivers/net/wireless/realtek/rtw88/util.h |  12 ++-
> >  4 files changed, 116 insertions(+), 7 deletions(-)
> I compared the changes from this patch with my earlier work. I would
> like to highlight a few functions to understand if they were left out
> on purpose or by accident.
> 
> __fw_recovery_work() in main.c (unfortunately I am not sure how to
> trigger/test this "firmware recovery" logic):

This can be triggered by 'echo 1 > /sys/kernel/debug/ieee80211/rtw88/fw_crash',
but only the latest firmware of 8822c can support this.

> - this is already called with &rtwdev->mutex held
> - it uses rtw_iterate_keys_rcu() (which internally uses rtw_write32
> from rtw_sec_clear_cam). feel free to either add [0] to your series or
> even squash it into an existing patch

ieee80211_iter_keys() check lockdep_assert_wiphy(hw->wiphy), but we don't
hold the lock in this work; it also do mutex_lock(&local->key_mtx) that 
I'm afraid it could cause deadlock.
 
So, I think we can use _rcu version to collect key list like sta and vif.

> - it uses rtw_iterate_stas_atomic() (which internally uses
> rtw_fw_send_h2c_command from rtw_fw_media_status_report)
> - it uses rtw_iterate_vifs_atomic() (which internally can read/write
> registers from rtw_chip_config_bfee)
> - in my previous series I simply replaced the
> rtw_iterate_stas_atomic() and rtw_iterate_vifs_atomic() calls with the
> non-atomic variants (for the rtw_iterate_keys_rcu() call I did some
> extra cleanup, see [0])
> 
> rtw_wow_fw_media_status() in wow.c (unfortunately I am also not sure
> how to test WoWLAN):
> - I am not sure if &rtwdev->mutex is held when this function is called
> - it uses rtw_iterate_stas_atomic() (which internally uses
> rtw_fw_send_h2c_command from rtw_fw_media_status_report)
> - in my previous series I simply replaced rtw_iterate_stas_atomic()
> with it's non-atomic variant
> 
> Additionally I rebased my SDIO work on top of your USB series.
> This makes SDIO support a lot easier - so thank you for your work!
> I found that three of my previous patches (in addition to [0] which I
> already mentioned earlier) are still needed to get rid of some
> warnings when using the SDIO interface (the same warnings may or may
> not be there with the USB interface - it just depends on whether your
> AP makes rtw88 hit that specific code-path):
> - [1]: rtw88: Configure the registers from rtw_bf_assoc() outside the RCU lock

I think we need this.

> - [2]: rtw88: Move rtw_chip_cfg_csi_rate() out of rtw_vif_watch_dog_iter()

I think we don't need this, but just use rtw_iterate_vifs() to
iterate rtw_vif_watch_dog_iter.

> - [3]: rtw88: Move rtw_update_sta_info() out of rtw_ra_mask_info_update_iter()

Need partial -- hold rtwdev->mutex before entering rtw_ra_mask_info_update().

Then, use rtw_iterate_stas() to iterate rtw_ra_mask_info_update_iter. 
No need others.


Sascha, could you squash Martin's patches into your patchset?
And, then I can do more tests on PCI cards.

I do long run on 8822CE with the patchset v2. It works fine.
After adding more, I will verify it again.

By the way, I still don't have resource to check PS of 8822CU.
Sorry for that.

Ping-Ke