[lkml]   [2022]   [Jun]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [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 <> 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.


 \ /
  Last update: 2022-06-08 15:27    [W:0.078 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site