diff mbox series

[2/2] wifi: rtw88: Avoid using macid 0 in AP mode

Message ID e4be0a75-43b2-4ae5-9aab-5c4a88e78097@gmail.com (mailing list archive)
State Superseded
Delegated to: Ping-Ke Shih
Headers show
Series [1/2] wifi: rtw88: Fix USB devices not transmitting beacons | expand

Commit Message

Bitterblue Smith July 28, 2024, 10:51 p.m. UTC
In AP mode, the firmware stops transmitting beacons if it receives
H2C_CMD_RA_INFO for macid 0.

Leave macid 0 unused in AP mode. Macid 1 is already reserved for
broadcast/multicast. Assign macid 2 to the first connected client.

Tested with RTL8811CU and RTL8723DU.

Cc: stable@vger.kernel.org # 6.6.x
Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
---
Tagging this for inclusion only in 6.6+ because that's the oldest
stable kernel with USB support, and presumably the PCI devices don't
have this problem. My RTL8822CE doesn't.

I don't know if SDIO devices also have this problem.
---
 drivers/net/wireless/realtek/rtw88/main.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Ping-Ke Shih July 30, 2024, 6:33 a.m. UTC | #1
Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> 
> In AP mode, the firmware stops transmitting beacons if it receives
> H2C_CMD_RA_INFO for macid 0.
> 
> Leave macid 0 unused in AP mode. Macid 1 is already reserved for
> broadcast/multicast. Assign macid 2 to the first connected client.

Seemingly we missed to set mac_id in TX desc for a long time.

+#define RTW_TX_DESC_W1_MACID GENMASK(7, 0)
 #define RTW_TX_DESC_W1_QSEL GENMASK(12, 8)
 #define RTW_TX_DESC_W1_RATE_ID GENMASK(20, 16)

The mac_id should be from rtwvif->mac_id or si->mac_id according to
operating mode and role.

And I suppose mac_id assignment for AP is mac_id 0 for broadcast/multicast, and
other mac_id can be used by connected stations regularly.
Bitterblue Smith July 30, 2024, 12:55 p.m. UTC | #2
On 30/07/2024 09:33, Ping-Ke Shih wrote:
> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
>>
>> In AP mode, the firmware stops transmitting beacons if it receives
>> H2C_CMD_RA_INFO for macid 0.
>>
>> Leave macid 0 unused in AP mode. Macid 1 is already reserved for
>> broadcast/multicast. Assign macid 2 to the first connected client.
> 
> Seemingly we missed to set mac_id in TX desc for a long time.
> 
> +#define RTW_TX_DESC_W1_MACID GENMASK(7, 0)
>  #define RTW_TX_DESC_W1_QSEL GENMASK(12, 8)
>  #define RTW_TX_DESC_W1_RATE_ID GENMASK(20, 16)
> 
> The mac_id should be from rtwvif->mac_id or si->mac_id according to
> operating mode and role.
> 
> And I suppose mac_id assignment for AP is mac_id 0 for broadcast/multicast, and
> other mac_id can be used by connected stations regularly. 
> 

What about the concurrent AP + station scenario? Will the
station vif use the next available macid, whatever that is?
Just wondering, I don't use concurrent mode.

Also, do you mean that you will do all this? It's not clear to me.
Ping-Ke Shih July 31, 2024, 12:47 a.m. UTC | #3
Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> On 30/07/2024 09:33, Ping-Ke Shih wrote:
> > Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> >>
> >> In AP mode, the firmware stops transmitting beacons if it receives
> >> H2C_CMD_RA_INFO for macid 0.
> >>
> >> Leave macid 0 unused in AP mode. Macid 1 is already reserved for
> >> broadcast/multicast. Assign macid 2 to the first connected client.
> >
> > Seemingly we missed to set mac_id in TX desc for a long time.
> >
> > +#define RTW_TX_DESC_W1_MACID GENMASK(7, 0)
> >  #define RTW_TX_DESC_W1_QSEL GENMASK(12, 8)
> >  #define RTW_TX_DESC_W1_RATE_ID GENMASK(20, 16)
> >
> > The mac_id should be from rtwvif->mac_id or si->mac_id according to
> > operating mode and role.
> >
> > And I suppose mac_id assignment for AP is mac_id 0 for broadcast/multicast, and
> > other mac_id can be used by connected stations regularly.
> >
> 
> What about the concurrent AP + station scenario? Will the
> station vif use the next available macid, whatever that is?
> Just wondering, I don't use concurrent mode.

My basic idea is to assign mac_id to each vif when adding vif. For station mode,
sta->mac_id will reuse vif->mac_id. For AP mode, I will dynamically allocate an
sta->mac_id to a station, and vif->mac_id is to send broadcast/multicast packets
that are not belong to a sta. For example,

                  vif->mac_id      sta->mac_id
vif0 (STA mode)        0               0
vif1 (AP mode)         1               2...


> 
> Also, do you mean that you will do all this? It's not clear to me.

I can do it. Or are you interested in this?
Ping-Ke Shih July 31, 2024, 8:10 a.m. UTC | #4
Hi Bitterblue Smith,

> >
> > Also, do you mean that you will do all this? It's not clear to me.
> 
> I can do it. Or are you interested in this?
> 

FYI.
I have made a draft version, and will send it out afterward.
Bitterblue Smith July 31, 2024, 12:20 p.m. UTC | #5
On 31/07/2024 03:47, Ping-Ke Shih wrote:
> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
>> On 30/07/2024 09:33, Ping-Ke Shih wrote:
>>> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
>>>>
>>>> In AP mode, the firmware stops transmitting beacons if it receives
>>>> H2C_CMD_RA_INFO for macid 0.
>>>>
>>>> Leave macid 0 unused in AP mode. Macid 1 is already reserved for
>>>> broadcast/multicast. Assign macid 2 to the first connected client.
>>>
>>> Seemingly we missed to set mac_id in TX desc for a long time.
>>>
>>> +#define RTW_TX_DESC_W1_MACID GENMASK(7, 0)
>>>  #define RTW_TX_DESC_W1_QSEL GENMASK(12, 8)
>>>  #define RTW_TX_DESC_W1_RATE_ID GENMASK(20, 16)
>>>
>>> The mac_id should be from rtwvif->mac_id or si->mac_id according to
>>> operating mode and role.
>>>
>>> And I suppose mac_id assignment for AP is mac_id 0 for broadcast/multicast, and
>>> other mac_id can be used by connected stations regularly.
>>>
>>
>> What about the concurrent AP + station scenario? Will the
>> station vif use the next available macid, whatever that is?
>> Just wondering, I don't use concurrent mode.
> 
> My basic idea is to assign mac_id to each vif when adding vif. For station mode,
> sta->mac_id will reuse vif->mac_id. For AP mode, I will dynamically allocate an
> sta->mac_id to a station, and vif->mac_id is to send broadcast/multicast packets
> that are not belong to a sta. For example,
> 
>                   vif->mac_id      sta->mac_id
> vif0 (STA mode)        0               0
> vif1 (AP mode)         1               2...
> 
> 
>>
>> Also, do you mean that you will do all this? It's not clear to me.
> 
> I can do it. Or are you interested in this?
> 

No, no, it's all yours. :)
Maciej S. Szmigiero Aug. 8, 2024, 9:38 a.m. UTC | #6
On 31.07.2024 14:20, Bitterblue Smith wrote:
> On 31/07/2024 03:47, Ping-Ke Shih wrote:
>> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
>>> On 30/07/2024 09:33, Ping-Ke Shih wrote:
>>>> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
>>>>>
>>>>> In AP mode, the firmware stops transmitting beacons if it receives
>>>>> H2C_CMD_RA_INFO for macid 0.
>>>>>
>>>>> Leave macid 0 unused in AP mode. Macid 1 is already reserved for
>>>>> broadcast/multicast. Assign macid 2 to the first connected client.
>>>>
>>>> Seemingly we missed to set mac_id in TX desc for a long time.
>>>>
>>>> +#define RTW_TX_DESC_W1_MACID GENMASK(7, 0)
>>>>   #define RTW_TX_DESC_W1_QSEL GENMASK(12, 8)
>>>>   #define RTW_TX_DESC_W1_RATE_ID GENMASK(20, 16)
>>>>
>>>> The mac_id should be from rtwvif->mac_id or si->mac_id according to
>>>> operating mode and role.
>>>>
>>>> And I suppose mac_id assignment for AP is mac_id 0 for broadcast/multicast, and
>>>> other mac_id can be used by connected stations regularly.
>>>>
>>>
>>> What about the concurrent AP + station scenario? Will the
>>> station vif use the next available macid, whatever that is?
>>> Just wondering, I don't use concurrent mode.
>>
>> My basic idea is to assign mac_id to each vif when adding vif. For station mode,
>> sta->mac_id will reuse vif->mac_id. For AP mode, I will dynamically allocate an
>> sta->mac_id to a station, and vif->mac_id is to send broadcast/multicast packets
>> that are not belong to a sta. For example,
>>
>>                    vif->mac_id      sta->mac_id
>> vif0 (STA mode)        0               0
>> vif1 (AP mode)         1               2...
>>
>>
>>>
>>> Also, do you mean that you will do all this? It's not clear to me.
>>
>> I can do it. Or are you interested in this?
>>
> 
> No, no, it's all yours. :)
> 

It would be nice to have either this submitted fix or alternative one merged soon
since the AP mode is completely broken otherwise (at least on RTL8821CU).

Thanks,
Maciej
Ping-Ke Shih Aug. 19, 2024, 2:59 a.m. UTC | #7
Maciej S. Szmigiero <mail@maciej.szmigiero.name> wrote:
> On 31.07.2024 14:20, Bitterblue Smith wrote:
> > On 31/07/2024 03:47, Ping-Ke Shih wrote:
> >> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> >>> On 30/07/2024 09:33, Ping-Ke Shih wrote:
> >>>> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> >>>>>
> >>>>> In AP mode, the firmware stops transmitting beacons if it receives
> >>>>> H2C_CMD_RA_INFO for macid 0.
> >>>>>
> >>>>> Leave macid 0 unused in AP mode. Macid 1 is already reserved for
> >>>>> broadcast/multicast. Assign macid 2 to the first connected client.
> >>>>
> >>>> Seemingly we missed to set mac_id in TX desc for a long time.
> >>>>
> >>>> +#define RTW_TX_DESC_W1_MACID GENMASK(7, 0)
> >>>>   #define RTW_TX_DESC_W1_QSEL GENMASK(12, 8)
> >>>>   #define RTW_TX_DESC_W1_RATE_ID GENMASK(20, 16)
> >>>>
> >>>> The mac_id should be from rtwvif->mac_id or si->mac_id according to
> >>>> operating mode and role.
> >>>>
> >>>> And I suppose mac_id assignment for AP is mac_id 0 for broadcast/multicast, and
> >>>> other mac_id can be used by connected stations regularly.
> >>>>
> >>>
> >>> What about the concurrent AP + station scenario? Will the
> >>> station vif use the next available macid, whatever that is?
> >>> Just wondering, I don't use concurrent mode.
> >>
> >> My basic idea is to assign mac_id to each vif when adding vif. For station mode,
> >> sta->mac_id will reuse vif->mac_id. For AP mode, I will dynamically allocate an
> >> sta->mac_id to a station, and vif->mac_id is to send broadcast/multicast packets
> >> that are not belong to a sta. For example,
> >>
> >>                    vif->mac_id      sta->mac_id
> >> vif0 (STA mode)        0               0
> >> vif1 (AP mode)         1               2...
> >>
> >>
> >>>
> >>> Also, do you mean that you will do all this? It's not clear to me.
> >>
> >> I can do it. Or are you interested in this?
> >>
> >
> > No, no, it's all yours. :)
> >
> 
> It would be nice to have either this submitted fix or alternative one merged soon
> since the AP mode is completely broken otherwise (at least on RTL8821CU).

I have sent a patch [2] to replace this one, but still needs
patch 1/2 "wifi: rtw88: Fix USB devices not transmitting beacons". [1]

I have tested [1] + [2] can work on PCI devices.
Can anyone help to test if [1] + [2] also works on USB devices?

[1] https://lore.kernel.org/linux-wireless/9174a776-4771-4351-85fa-476e240d8ace@gmail.com/
[2] https://lore.kernel.org/linux-wireless/20240819025248.17939-1-pkshih@realtek.com/T/#u
Maciej S. Szmigiero Aug. 19, 2024, 9:20 p.m. UTC | #8
On 19.08.2024 04:59, Ping-Ke Shih wrote:
> Maciej S. Szmigiero <mail@maciej.szmigiero.name> wrote:
>> On 31.07.2024 14:20, Bitterblue Smith wrote:
>>> On 31/07/2024 03:47, Ping-Ke Shih wrote:
>>>> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
>>>>> On 30/07/2024 09:33, Ping-Ke Shih wrote:
>>>>>> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
>>>>>>>
>>>>>>> In AP mode, the firmware stops transmitting beacons if it receives
>>>>>>> H2C_CMD_RA_INFO for macid 0.
>>>>>>>
>>>>>>> Leave macid 0 unused in AP mode. Macid 1 is already reserved for
>>>>>>> broadcast/multicast. Assign macid 2 to the first connected client.
>>>>>>
>>>>>> Seemingly we missed to set mac_id in TX desc for a long time.
>>>>>>
>>>>>> +#define RTW_TX_DESC_W1_MACID GENMASK(7, 0)
>>>>>>    #define RTW_TX_DESC_W1_QSEL GENMASK(12, 8)
>>>>>>    #define RTW_TX_DESC_W1_RATE_ID GENMASK(20, 16)
>>>>>>
>>>>>> The mac_id should be from rtwvif->mac_id or si->mac_id according to
>>>>>> operating mode and role.
>>>>>>
>>>>>> And I suppose mac_id assignment for AP is mac_id 0 for broadcast/multicast, and
>>>>>> other mac_id can be used by connected stations regularly.
>>>>>>
>>>>>
>>>>> What about the concurrent AP + station scenario? Will the
>>>>> station vif use the next available macid, whatever that is?
>>>>> Just wondering, I don't use concurrent mode.
>>>>
>>>> My basic idea is to assign mac_id to each vif when adding vif. For station mode,
>>>> sta->mac_id will reuse vif->mac_id. For AP mode, I will dynamically allocate an
>>>> sta->mac_id to a station, and vif->mac_id is to send broadcast/multicast packets
>>>> that are not belong to a sta. For example,
>>>>
>>>>                     vif->mac_id      sta->mac_id
>>>> vif0 (STA mode)        0               0
>>>> vif1 (AP mode)         1               2...
>>>>
>>>>
>>>>>
>>>>> Also, do you mean that you will do all this? It's not clear to me.
>>>>
>>>> I can do it. Or are you interested in this?
>>>>
>>>
>>> No, no, it's all yours. :)
>>>
>>
>> It would be nice to have either this submitted fix or alternative one merged soon
>> since the AP mode is completely broken otherwise (at least on RTL8821CU).
> 
> I have sent a patch [2] to replace this one, but still needs
> patch 1/2 "wifi: rtw88: Fix USB devices not transmitting beacons". [1]
> 
> I have tested [1] + [2] can work on PCI devices.
> Can anyone help to test if [1] + [2] also works on USB devices?

I tested these patches on USB RTL8821CU and can confirm that the AP mode seems to
work as good with your patch [2] as it did with the Bitterblue's one.

The only issue with either your or Bitterblue's patches is that I occasionally get the
following messages in the kernel log in the AP mode:
> kernel: [T1234] rtw_8821cu 4-1.2:1.2: error beacon valid
> kernel: [T1234] rtw_8821cu 4-1.2:1.2: failed to download drv rsvd page
> kernel: [T1234] rtw_8821cu 4-1.2:1.2: failed to download beacon

Around the time these messages are logged ping RTT of a connected (battery powered)
STA climbs into multiple seconds range so I guess there might be something wrong
with beacon DTIM update here.

> [1] https://lore.kernel.org/linux-wireless/9174a776-4771-4351-85fa-476e240d8ace@gmail.com/
> [2] https://lore.kernel.org/linux-wireless/20240819025248.17939-1-pkshih@realtek.com/T/#u

Thanks,
Maciej
Ping-Ke Shih Aug. 20, 2024, 12:34 a.m. UTC | #9
Hi Bitterblue,

Maciej S. Szmigiero <mail@maciej.szmigiero.name> wrote:
> I tested these patches on USB RTL8821CU and can confirm that the AP mode seems to
> work as good with your patch [2] as it did with the Bitterblue's one.
> 
> The only issue with either your or Bitterblue's patches is that I occasionally get the
> following messages in the kernel log in the AP mode:
> > kernel: [T1234] rtw_8821cu 4-1.2:1.2: error beacon valid
> > kernel: [T1234] rtw_8821cu 4-1.2:1.2: failed to download drv rsvd page
> > kernel: [T1234] rtw_8821cu 4-1.2:1.2: failed to download beacon
> 
> Around the time these messages are logged ping RTT of a connected (battery powered)
> STA climbs into multiple seconds range so I guess there might be something wrong
> with beacon DTIM update here.

Can you also see this in your side? It seems like download beacon for change of DTIM
might be failed occasionally. 

Ping-Ke
Bitterblue Smith Aug. 20, 2024, 11:15 p.m. UTC | #10
On 20/08/2024 03:34, Ping-Ke Shih wrote:
> Hi Bitterblue,
> 
> Maciej S. Szmigiero <mail@maciej.szmigiero.name> wrote:
>> I tested these patches on USB RTL8821CU and can confirm that the AP mode seems to
>> work as good with your patch [2] as it did with the Bitterblue's one.
>>
>> The only issue with either your or Bitterblue's patches is that I occasionally get the
>> following messages in the kernel log in the AP mode:
>>> kernel: [T1234] rtw_8821cu 4-1.2:1.2: error beacon valid
>>> kernel: [T1234] rtw_8821cu 4-1.2:1.2: failed to download drv rsvd page
>>> kernel: [T1234] rtw_8821cu 4-1.2:1.2: failed to download beacon
>>
>> Around the time these messages are logged ping RTT of a connected (battery powered)
>> STA climbs into multiple seconds range so I guess there might be something wrong
>> with beacon DTIM update here.
> 
> Can you also see this in your side? It seems like download beacon for change of DTIM
> might be failed occasionally. 
> 
> Ping-Ke
> 

Yes, I have seen it with RTL8812AU. It only fails sometimes.
I don't know what is wrong.
diff mbox series

Patch

diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index b3a089b4f707..e0b1ccc3759c 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -316,11 +316,12 @@  static void rtw_ips_work(struct work_struct *work)
 	mutex_unlock(&rtwdev->mutex);
 }
 
-static u8 rtw_acquire_macid(struct rtw_dev *rtwdev)
+static u8 rtw_acquire_macid(struct rtw_dev *rtwdev, struct ieee80211_vif *vif)
 {
 	unsigned long mac_id;
 
-	mac_id = find_first_zero_bit(rtwdev->mac_id_map, RTW_MAX_MAC_ID_NUM);
+	mac_id = find_next_zero_bit(rtwdev->mac_id_map, RTW_MAX_MAC_ID_NUM,
+				    vif->type == NL80211_IFTYPE_AP ? 2 : 0);
 	if (mac_id < RTW_MAX_MAC_ID_NUM)
 		set_bit(mac_id, rtwdev->mac_id_map);
 
@@ -345,7 +346,7 @@  int rtw_sta_add(struct rtw_dev *rtwdev, struct ieee80211_sta *sta,
 	struct rtw_vif *rtwvif = (struct rtw_vif *)vif->drv_priv;
 	int i;
 
-	si->mac_id = rtw_acquire_macid(rtwdev);
+	si->mac_id = rtw_acquire_macid(rtwdev, vif);
 	if (si->mac_id >= RTW_MAX_MAC_ID_NUM)
 		return -ENOSPC;