| Message ID | 20220530135457.1104091-6-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | RTW88: Add support for USB variants | expand |
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 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/20220108005533.947787-6-martin.blumenstingl@googlemail.com/ [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 <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
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
On Mon, 2022-05-30 at 15:54 +0200, Sascha Hauer wrote: > The driver uses ieee80211_iterate_active_interfaces_atomic() > and ieee80211_iterate_stations_atomic() in several places and does > register accesses in the iterators. This doesn't cope with upcoming > USB support as registers can only be accessed non-atomically. > > Split these into a two stage process: First use the atomic iterator > functions to collect all active interfaces or stations on a list, then > iterate over the list non-atomically and call the iterator on each > entry. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > Suggested-by: Pkshih <pkshih@realtek.com> > --- > > Notes: > Changes since v1: > - Change subject > - Add some lockdep_assert_held(&rtwdev->mutex); > - make locally used functions static > - Add comment how &rtwdev->mutex protects us from stations/interfaces > being deleted between collecting them and iterating over them. > > 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(-) > > [...] > + > +struct rtw_vifs_entry { > + struct list_head list; > + struct ieee80211_vif *vif; > + u8 mac[ETH_ALEN]; > +}; > + > +struct rtw_iter_vifs_data { > + struct rtw_dev *rtwdev; > + struct list_head list; > +}; > + > +void rtw_collect_vif_iter(void *data, u8 *mac, struct ieee80211_vif *vif) You do this change in patch "rtw88: Add common USB chip support". Please move to here. -void rtw_collect_vif_iter(void *data, u8 *mac, struct ieee80211_vif *vif) +static void rtw_collect_vif_iter(void *data, u8 *mac, struct ieee80211_vif *vif) > +{ > + struct rtw_iter_vifs_data *iter_stas = data; > + struct rtw_vifs_entry *vifs_entry; > + > + vifs_entry = kmalloc(sizeof(*vifs_entry), GFP_ATOMIC); > + if (!vifs_entry) > + return; > + > + vifs_entry->vif = vif; > + ether_addr_copy(vifs_entry->mac, mac); > + list_add_tail(&vifs_entry->list, &iter_stas->list); > +} > + > +void rtw_iterate_vifs(struct rtw_dev *rtwdev, > + void (*iterator)(void *data, u8 *mac, > + struct ieee80211_vif *vif), > + void *data) > +{ > + struct rtw_iter_vifs_data iter_data; > + struct rtw_vifs_ent [...] > diff --git a/drivers/net/wireless/realtek/rtw88/util.h b/drivers/net/wireless/realtek/rtw88/util.h > index 0c23b5069be0b..dc89655254002 100644 > --- a/drivers/net/wireless/realtek/rtw88/util.h > +++ b/drivers/net/wireless/realtek/rtw88/util.h > @@ -7,9 +7,6 @@ > > struct rtw_dev; > > -#define rtw_iterate_vifs(rtwdev, iterator, data) \ > - ieee80211_iterate_active_interfaces(rtwdev->hw, \ > - IEEE80211_IFACE_ITER_NORMAL, iterator, data) > #define rtw_iterate_vifs_atomic(rtwdev, iterator, data) \ > ieee80211_iterate_active_interfaces_atomic(rtwdev->hw, \ > IEEE80211_IFACE_ITER_NORMAL, iterator, data) After read Martin's patches, I think we can review all places where use _aotmic version of vif and sta, even we don't really meet problem for now. Then, use non-atomic version if possible. Ping-Ke
diff --git a/drivers/net/wireless/realtek/rtw88/phy.c b/drivers/net/wireless/realtek/rtw88/phy.c index e505d17f107e4..5521a7c2c1afe 100644 --- a/drivers/net/wireless/realtek/rtw88/phy.c +++ b/drivers/net/wireless/realtek/rtw88/phy.c @@ -300,7 +300,7 @@ static void rtw_phy_stat_rssi(struct rtw_dev *rtwdev) data.rtwdev = rtwdev; data.min_rssi = U8_MAX; - rtw_iterate_stas_atomic(rtwdev, rtw_phy_stat_rssi_iter, &data); + rtw_iterate_stas(rtwdev, rtw_phy_stat_rssi_iter, &data); dm_info->pre_min_rssi = dm_info->min_rssi; dm_info->min_rssi = data.min_rssi; @@ -544,7 +544,7 @@ static void rtw_phy_ra_info_update(struct rtw_dev *rtwdev) if (rtwdev->watch_dog_cnt & 0x3) return; - rtw_iterate_stas_atomic(rtwdev, rtw_phy_ra_info_update_iter, rtwdev); + rtw_iterate_stas(rtwdev, rtw_phy_ra_info_update_iter, rtwdev); } static u32 rtw_phy_get_rrsr_mask(struct rtw_dev *rtwdev, u8 rate_idx) @@ -597,7 +597,7 @@ static void rtw_phy_rrsr_update(struct rtw_dev *rtwdev) struct rtw_dm_info *dm_info = &rtwdev->dm_info; dm_info->rrsr_mask_min = RRSR_RATE_ORDER_MAX; - rtw_iterate_stas_atomic(rtwdev, rtw_phy_rrsr_mask_min_iter, rtwdev); + rtw_iterate_stas(rtwdev, rtw_phy_rrsr_mask_min_iter, rtwdev); rtw_write32(rtwdev, REG_RRSR, dm_info->rrsr_val_init & dm_info->rrsr_mask_min); } diff --git a/drivers/net/wireless/realtek/rtw88/ps.c b/drivers/net/wireless/realtek/rtw88/ps.c index bfa64c038f5f0..a7213ff2c2244 100644 --- a/drivers/net/wireless/realtek/rtw88/ps.c +++ b/drivers/net/wireless/realtek/rtw88/ps.c @@ -58,7 +58,7 @@ int rtw_leave_ips(struct rtw_dev *rtwdev) return ret; } - rtw_iterate_vifs_atomic(rtwdev, rtw_restore_port_cfg_iter, rtwdev); + rtw_iterate_vifs(rtwdev, rtw_restore_port_cfg_iter, rtwdev); rtw_coex_ips_notify(rtwdev, COEX_IPS_LEAVE); diff --git a/drivers/net/wireless/realtek/rtw88/util.c b/drivers/net/wireless/realtek/rtw88/util.c index 2c515af214e76..ed61c962fa27f 100644 --- a/drivers/net/wireless/realtek/rtw88/util.c +++ b/drivers/net/wireless/realtek/rtw88/util.c @@ -105,3 +105,106 @@ void rtw_desc_to_mcsrate(u16 rate, u8 *mcs, u8 *nss) *mcs = rate - DESC_RATEMCS0; } } + +struct rtw_stas_entry { + struct list_head list; + struct ieee80211_sta *sta; +}; + +struct rtw_iter_stas_data { + struct rtw_dev *rtwdev; + struct list_head list; +}; + +static void rtw_collect_sta_iter(void *data, struct ieee80211_sta *sta) +{ + struct rtw_iter_stas_data *iter_stas = data; + struct rtw_stas_entry *stas_entry; + + stas_entry = kmalloc(sizeof(*stas_entry), GFP_ATOMIC); + if (!stas_entry) + return; + + stas_entry->sta = sta; + list_add_tail(&stas_entry->list, &iter_stas->list); +} + +void rtw_iterate_stas(struct rtw_dev *rtwdev, + void (*iterator)(void *data, + struct ieee80211_sta *sta), + void *data) +{ + struct rtw_iter_stas_data iter_data; + struct rtw_stas_entry *sta_entry, *tmp; + + /* &rtwdev->mutex makes sure no stations can be removed between + * collecting the stations and iterating over them. + */ + lockdep_assert_held(&rtwdev->mutex); + + iter_data.rtwdev = rtwdev; + INIT_LIST_HEAD(&iter_data.list); + + ieee80211_iterate_stations_atomic(rtwdev->hw, rtw_collect_sta_iter, + &iter_data); + + list_for_each_entry_safe(sta_entry, tmp, &iter_data.list, + list) { + list_del_init(&sta_entry->list); + iterator(data, sta_entry->sta); + kfree(sta_entry); + } +} + +struct rtw_vifs_entry { + struct list_head list; + struct ieee80211_vif *vif; + u8 mac[ETH_ALEN]; +}; + +struct rtw_iter_vifs_data { + struct rtw_dev *rtwdev; + struct list_head list; +}; + +void rtw_collect_vif_iter(void *data, u8 *mac, struct ieee80211_vif *vif) +{ + struct rtw_iter_vifs_data *iter_stas = data; + struct rtw_vifs_entry *vifs_entry; + + vifs_entry = kmalloc(sizeof(*vifs_entry), GFP_ATOMIC); + if (!vifs_entry) + return; + + vifs_entry->vif = vif; + ether_addr_copy(vifs_entry->mac, mac); + list_add_tail(&vifs_entry->list, &iter_stas->list); +} + +void rtw_iterate_vifs(struct rtw_dev *rtwdev, + void (*iterator)(void *data, u8 *mac, + struct ieee80211_vif *vif), + void *data) +{ + struct rtw_iter_vifs_data iter_data; + struct rtw_vifs_entry *vif_entry, *tmp; + + /* &rtwdev->mutex makes sure no interfaces can be removed between + * collecting the interfaces and iterating over them. + */ + lockdep_assert_held(&rtwdev->mutex); + + iter_data.rtwdev = rtwdev; + INIT_LIST_HEAD(&iter_data.list); + + ieee80211_iterate_active_interfaces_atomic(rtwdev->hw, + IEEE80211_IFACE_ITER_NORMAL, + rtw_collect_vif_iter, &iter_data); + + list_for_each_entry_safe(vif_entry, tmp, &iter_data.list, + list) { + list_del_init(&vif_entry->list); + iterator(data, vif_entry->mac, vif_entry->vif); + kfree(vif_entry); + } +} diff --git a/drivers/net/wireless/realtek/rtw88/util.h b/drivers/net/wireless/realtek/rtw88/util.h index 0c23b5069be0b..dc89655254002 100644 --- a/drivers/net/wireless/realtek/rtw88/util.h +++ b/drivers/net/wireless/realtek/rtw88/util.h @@ -7,9 +7,6 @@ struct rtw_dev; -#define rtw_iterate_vifs(rtwdev, iterator, data) \ - ieee80211_iterate_active_interfaces(rtwdev->hw, \ - IEEE80211_IFACE_ITER_NORMAL, iterator, data) #define rtw_iterate_vifs_atomic(rtwdev, iterator, data) \ ieee80211_iterate_active_interfaces_atomic(rtwdev->hw, \ IEEE80211_IFACE_ITER_NORMAL, iterator, data) @@ -20,6 +17,15 @@ struct rtw_dev; #define rtw_iterate_keys_rcu(rtwdev, vif, iterator, data) \ ieee80211_iter_keys_rcu((rtwdev)->hw, vif, iterator, data) +void rtw_iterate_vifs(struct rtw_dev *rtwdev, + void (*iterator)(void *data, u8 *mac, + struct ieee80211_vif *vif), + void *data); +void rtw_iterate_stas(struct rtw_dev *rtwdev, + void (*iterator)(void *data, + struct ieee80211_sta *sta), + void *data); + static inline u8 *get_hdr_bssid(struct ieee80211_hdr *hdr) { __le16 fc = hdr->frame_control;
The driver uses ieee80211_iterate_active_interfaces_atomic() and ieee80211_iterate_stations_atomic() in several places and does register accesses in the iterators. This doesn't cope with upcoming USB support as registers can only be accessed non-atomically. Split these into a two stage process: First use the atomic iterator functions to collect all active interfaces or stations on a list, then iterate over the list non-atomically and call the iterator on each entry. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> Suggested-by: Pkshih <pkshih@realtek.com> --- Notes: Changes since v1: - Change subject - Add some lockdep_assert_held(&rtwdev->mutex); - make locally used functions static - Add comment how &rtwdev->mutex protects us from stations/interfaces being deleted between collecting them and iterating over them. 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(-)