| Message ID | 20231221164353.603258-14-martin.kaistra@linutronix.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | wifi: rtl8xxxu: Add concurrent mode for 8188f | expand |
Am 22.12.23 um 09:59 schrieb Ping-Ke Shih: > On Fri, 2023-12-22 at 09:25 +0100, Martin Kaistra wrote: >> >> Am 22.12.23 um 09:05 schrieb Martin Kaistra: >>> Am 22.12.23 um 02:45 schrieb Ping-Ke Shih: >>>> On Fri, Dec 22, 2023 at 12:45 AM Martin Kaistra >>>> <martin.kaistra@linutronix.de> wrote: >>>>> Check first whether priv->vifs[0] exists and is of type STATION, then go >>>>> to priv->vifs[1]. Make sure to call refresh_rate_mask for both >>>>> interfaces. >>>>> >>>>> Signed-off-by: Martin Kaistra <martin.kaistra@linutronix.de> >>>>> --- >>>>> .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 19 +++++++++++-------- >>>>> 1 file changed, 11 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >>>>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >>>>> index c5b71892369c9..fd0108668bcda 100644 >>>>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >>>>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >>>>> @@ -7200,11 +7200,15 @@ static void rtl8xxxu_watchdog_callback(struct >>>>> work_struct *work) >>>>> { >>>>> struct ieee80211_vif *vif; >>>>> struct rtl8xxxu_priv *priv; >>>>> + int i; >>>>> >>>>> priv = container_of(work, struct rtl8xxxu_priv, ra_watchdog.work); >>>>> - vif = priv->vif; >>>>> + for (i = 0; i < ARRAY_SIZE(priv->vifs); i++) { >>>>> + vif = priv->vifs[i]; >>>>> + >>>>> + if (!vif || vif->type != NL80211_IFTYPE_STATION) >>>>> + continue; >>>> >>>> Currently, this loop becomes to get RSSI and update rate mask, but only for >>>> station mode. That means we don't update them for peer stations in AP mode. >>>> Maybe, we need ieee80211_iterate_stations_atomic() for all stations, but >>>> ieee80211_ave_rssi() is only for 'vif', so we need to add a driver level >>>> RSSI for all stations (macid). Also, we need to extend priv->rssi_level for >>>> all macid as well. >>>> >>>> I suppose _default_ value can work to stations in AP mode, so you can decide >>>> if you will defer this support temporarily. >>>> >>>> (Sorry, I don't dig rtl8xxxu very deeply, so I'm not able to tell you all >>>> things in one go.) >>> >>> It probably makes sense to fix this, however if it's ok for you, I would like to >>> do it separatly from this series. > > Ok to me. :-) > >>> >>> >>>>> - if (vif && vif->type == NL80211_IFTYPE_STATION) { >>>>> int signal; >>>>> struct ieee80211_sta *sta; >>>>> >>>>> @@ -7215,22 +7219,21 @@ static void rtl8xxxu_watchdog_callback(struct >>>>> work_struct *work) >>>>> >>>>> dev_dbg(dev, "%s: no sta found\n", __func__); >>>>> rcu_read_unlock(); >>>>> - goto out; >>>>> + continue; >>>>> } >>>>> rcu_read_unlock(); >>>>> >>>>> signal = ieee80211_ave_rssi(vif); >>>>> >>>>> - priv->fops->report_rssi(priv, 0, >>>>> + priv->fops->report_rssi(priv, rtl8xxxu_get_macid(priv, sta), >>>>> rtl8xxxu_signal_to_snr(signal)); >>>>> >>>>> - if (priv->fops->set_crystal_cap) >>>>> - rtl8xxxu_track_cfo(priv); >>>>> - >>>>> rtl8xxxu_refresh_rate_mask(priv, signal, sta, false); >>>> >>>> It seems like 'sta' isn't protected by RCU read lock. >>>> (an existing bug, not introduced by this patch) >>> >>> I will add a patch which moves the rcu_read_unlock() to fix this. >> >> Actually, looking at the code again, rtl8xxxu_refresh_rate_mask() seems to >> already contain calls to rcu_read_lock(). Just the call to >> rtl8xxxu_get_macid(priv, sta) in there might need additional protection. >> > > > We must use RCU lock to protect 'sta' from dereference to whole access, but > it looks like > > rtl8xxxu_watchdog_callback() > > rcu_read_lock(); > sta = ... > rcu_read_unlock() // after this point, use 'sta' is unsafe.. > > rtl8xxxu_refresh_rate_mask(sta) > rcu_read_lock(); > rate_bitmap = sta->... > rcu_read_unlock(); > > If it is not an easy fix, does it make sense to you to do this also separately from this series?
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c index c5b71892369c9..fd0108668bcda 100644 --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c @@ -7200,11 +7200,15 @@ static void rtl8xxxu_watchdog_callback(struct work_struct *work) { struct ieee80211_vif *vif; struct rtl8xxxu_priv *priv; + int i; priv = container_of(work, struct rtl8xxxu_priv, ra_watchdog.work); - vif = priv->vif; + for (i = 0; i < ARRAY_SIZE(priv->vifs); i++) { + vif = priv->vifs[i]; + + if (!vif || vif->type != NL80211_IFTYPE_STATION) + continue; - if (vif && vif->type == NL80211_IFTYPE_STATION) { int signal; struct ieee80211_sta *sta; @@ -7215,22 +7219,21 @@ static void rtl8xxxu_watchdog_callback(struct work_struct *work) dev_dbg(dev, "%s: no sta found\n", __func__); rcu_read_unlock(); - goto out; + continue; } rcu_read_unlock(); signal = ieee80211_ave_rssi(vif); - priv->fops->report_rssi(priv, 0, + priv->fops->report_rssi(priv, rtl8xxxu_get_macid(priv, sta), rtl8xxxu_signal_to_snr(signal)); - if (priv->fops->set_crystal_cap) - rtl8xxxu_track_cfo(priv); - rtl8xxxu_refresh_rate_mask(priv, signal, sta, false); } -out: + if (priv->fops->set_crystal_cap) + rtl8xxxu_track_cfo(priv); + schedule_delayed_work(&priv->ra_watchdog, 2 * HZ); }
Check first whether priv->vifs[0] exists and is of type STATION, then go to priv->vifs[1]. Make sure to call refresh_rate_mask for both interfaces. Signed-off-by: Martin Kaistra <martin.kaistra@linutronix.de> --- .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)