Hi Sascha,
thanks for this patch!
On Mon, May 30, 2022 at 3:55 PM Sascha Hauer <[email protected]> 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 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
- 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
- [2]: rtw88: Move rtw_chip_cfg_csi_rate() out of rtw_vif_watch_dog_iter()
- [3]: rtw88: Move rtw_update_sta_info() out of rtw_ra_mask_info_update_iter()
These three patches are freshly rebased to apply on top of your series.
If you (or Ping-Ke) think those are needed for USB support then please
feel free to include them in your series, squash them into one of your
patches or just ask me to send them as an individual series)
I am running out of time for today. I'll be able to continue on this
later this week/during the weekend.
Best regards,
Martin
[0] https://lore.kernel.org/all/[email protected]/
[1] https://github.com/xdarklight/linux/commit/420bb40511151fbc9f5447aced4fde3a7bb0566b.patch
[2] https://github.com/xdarklight/linux/commit/cc08cc8fb697157b99304ad3ec89b1cca0900697.patch
[3] https://github.com/xdarklight/linux/commit/5db636e3035a425e104fba7983301b0085366268.patch
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 <[email protected]> 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