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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > > > ---
> > > > .../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();
