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

["s.hauer@pengutronix.de" <s.hauer@pengutronix.de>]

On Wed, Jun 08, 2022 at 01:25:34PM +0000, Ping-Ke Shih wrote:
> 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.

Yes, will do and send a v3 with it.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |