| 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 |
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.
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.
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?
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.
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. :)
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
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
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
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
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 --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;
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(-)