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

[Ping-Ke Shih <pkshih@realtek.com>]

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@pengutronix.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