Received-SPF: pass (google.com: domain of linux-wireless+bounces-1206-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <b8764bec-4dc6-4779-a3c3-d22e7d5acce9@linutronix.de>
Date: Fri, 22 Dec 2023 09:05:11 +0100
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [PATCH v2 13/21] wifi: rtl8xxxu: support multiple interfaces in
 watchdog_callback()
Content-Language: de-DE
To: Ping-Ke Shih <pkshih@realtek.com>,
 "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Cc: Jes Sorensen <Jes.Sorensen@gmail.com>, Kalle Valo <kvalo@kernel.org>,
 Bitterblue Smith <rtl8821cerfe2@gmail.com>,
 Sebastian Andrzej Siewior <bigeasy@linutronix.de>
References: <20231221164353.603258-1-martin.kaistra@linutronix.de>
 <20231221164353.603258-14-martin.kaistra@linutronix.de>
 <CAKFoaw3o-LkqADNvfE0TXmvr8_LK19mpVQtUZ66CWS=AN5AkJQ@mail.gmail.com>
 <f71b29e17ff14ee9b28dd514a94cf1be@realtek.com>
From: Martin Kaistra <martin.kaistra@linutronix.de>
In-Reply-To: <f71b29e17ff14ee9b28dd514a94cf1be@realtek.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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.


> 
>>
>> -       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.

> 
> 
>>          }
>>
>> -out:
>> +       if (priv->fops->set_crystal_cap)
>> +               rtl8xxxu_track_cfo(priv);
>> +
>>          schedule_delayed_work(&priv->ra_watchdog, 2 * HZ);
>>   }
>>
>> --
>> 2.39.2
>>
>>