diff mbox series

[v2,13/21] wifi: rtl8xxxu: support multiple interfaces in watchdog_callback()

Message ID 20231221164353.603258-14-martin.kaistra@linutronix.de (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series wifi: rtl8xxxu: Add concurrent mode for 8188f | expand

Commit Message

Martin Kaistra Dec. 21, 2023, 4:43 p.m. UTC
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(-)

Comments

Ping-Ke Shih Dec. 22, 2023, 1:45 a.m. UTC | #1
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.)

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


>         }
>
> -out:
> +       if (priv->fops->set_crystal_cap)
> +               rtl8xxxu_track_cfo(priv);
> +
>         schedule_delayed_work(&priv->ra_watchdog, 2 * HZ);
>  }
>
> --
> 2.39.2
>
>
Martin Kaistra Dec. 22, 2023, 8:05 a.m. UTC | #2
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
>>
>>
Martin Kaistra Dec. 22, 2023, 8:25 a.m. UTC | #3
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.
> 
> 
>>
>>>
>>> -       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.

> 
>>
>>
>>>          }
>>>
>>> -out:
>>> +       if (priv->fops->set_crystal_cap)
>>> +               rtl8xxxu_track_cfo(priv);
>>> +
>>>          schedule_delayed_work(&priv->ra_watchdog, 2 * HZ);
>>>   }
>>>
>>> -- 
>>> 2.39.2
>>>
>>>
> 
>
Ping-Ke Shih Dec. 22, 2023, 8:59 a.m. UTC | #4
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();
Martin Kaistra Dec. 22, 2023, 9:10 a.m. UTC | #5
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?
Ping-Ke Shih Dec. 22, 2023, 10:12 a.m. UTC | #6
On Fri, 2023-12-22 at 10:10 +0100, Martin Kaistra wrote:
> 
> 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.
> > > > > > 
> > > > > > -       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?

Sure. As I said, this isn't introduced by this patchset. We can fix it later.
diff mbox series

Patch

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);
 }