Re: rtlwifi/rtl8192cu AP mode broken with PS STA
From: Pkshih
Date: Tue Apr 06 2021 - 22:49:11 EST
On Tue, 2021-04-06 at 11:25 -0500, Larry Finger wrote:
> On 4/6/21 7:06 AM, Maciej S. Szmigiero wrote:
> > On 06.04.2021 12:00, Kalle Valo wrote:
> >> "Maciej S. Szmigiero" <mail@xxxxxxxxxxxxxxxxxxxxx> writes:
> >>
> >>> On 29.03.2021 00:54, Maciej S. Szmigiero wrote:
> >>>> Hi,
> >>>>
> >>>> It looks like rtlwifi/rtl8192cu AP mode is broken when a STA is using PS,
> >>>> since the driver does not update its beacon to account for TIM changes,
> >>>> so a station that is sleeping will never learn that it has packets
> >>>> buffered at the AP.
> >>>>
> >>>> Looking at the code, the rtl8192cu driver implements neither the set_tim()
> >>>> callback, nor does it explicitly update beacon data periodically, so it
> >>>> has no way to learn that it had changed.
> >>>>
> >>>> This results in the AP mode being virtually unusable with STAs that do
> >>>> PS and don't allow for it to be disabled (IoT devices, mobile phones,
> >>>> etc.).
> >>>>
> >>>> I think the easiest fix here would be to implement set_tim() for example
> >>>> the way rt2x00 driver does: queue a work or schedule a tasklet to update
> >>>> the beacon data on the device.
> >>>
> >>> Are there any plans to fix this?
> >>> The driver is listed as maintained by Ping-Ke.
> >>
> >> Yeah, power save is hard and I'm not surprised that there are drivers
> >> with broken power save mode support. If there's no fix available we
> >> should stop supporting AP mode in the driver.
> >>
> >
> > https://wireless.wiki.kernel.org/en/developers/documentation/mac80211/api
> > clearly documents that "For AP mode, it must (...) react to the set_tim()
> > callback or fetch each beacon from mac80211".
> >
> > The driver isn't doing either so no wonder the beacon it is sending
> > isn't getting updated.
> >
> > As I have said above, it seems to me that all that needs to be done here
> > is to queue a work in a set_tim() callback, then call
> > send_beacon_frame() from rtlwifi/core.c from this work.
> >
> > But I don't know the exact device semantics, maybe it needs some other
> > notification that the beacon has changed, too, or even tries to
> > manage the TIM bitmap by itself.
> >
> > It would be a shame to lose the AP mode for such minor thing, though.
> >
> > I would play with this myself, but unfortunately I don't have time
> > to work on this right now.
> >
> > That's where my question to Realtek comes: are there plans to actually
> > fix this?
>
> Yes, I am working on this. My only question is "if you are such an expert on the
> problem, why do you not fix it?"
>
> The example in rx200 is not particularly useful, and I have not found any other
> examples.
>
Hi Larry,
I have a draft patch that forks a work to do send_beacon_frame(), whose
behavior like Maciej mentioned.
I did test on RTL8821AE; it works well. But, it seems already work well even
I don't apply this patch, and I'm still digging why.
I don't have a rtl8192cu dongle on hand, but I'll try to find one.
---
Ping-Ke
From 44be80232aa49737c035ee4656d20a22f573d33e Mon Sep 17 00:00:00 2001
From: Ping-Ke Shih <pkshih@xxxxxxxxxxx>
Date: Tue, 6 Apr 2021 19:55:59 +0800
Subject: [PATCH] rtlwifi: implement set_tim by update beacon content
Once beacon content is changed, we update the content to wifi card by
send_beacon_frame().
Signed-off-by: Ping-Ke Shih <pkshih@xxxxxxxxxxx>
---
drivers/net/wireless/realtek/rtlwifi/core.c | 30 +++++++++++++++++++++
drivers/net/wireless/realtek/rtlwifi/core.h | 1 +
drivers/net/wireless/realtek/rtlwifi/pci.c | 3 +++
drivers/net/wireless/realtek/rtlwifi/usb.c | 3 +++
drivers/net/wireless/realtek/rtlwifi/wifi.h | 1 +
5 files changed, 38 insertions(+)
diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c b/drivers/net/wireless/realtek/rtlwifi/core.c
index 965bd9589045..ca47a70d9a86 100644
--- a/drivers/net/wireless/realtek/rtlwifi/core.c
+++ b/drivers/net/wireless/realtek/rtlwifi/core.c
@@ -1018,6 +1018,25 @@ static void send_beacon_frame(struct ieee80211_hw *hw,
}
}
+void rtl_update_beacon_work_callback(struct work_struct *work)
+{
+ struct rtl_works *rtlworks =
+ container_of(work, struct rtl_works, update_beacon_work);
+ struct ieee80211_hw *hw = rtlworks->hw;
+ struct rtl_priv *rtlpriv = rtl_priv(hw);
+ struct ieee80211_vif *vif = rtlpriv->mac80211.vif;
+
+ if (!vif) {
+ WARN_ONCE(true, "no vif to update beacon\n");
+ return;
+ }
+
+ mutex_lock(&rtlpriv->locks.conf_mutex);
+ send_beacon_frame(hw, vif);
+ mutex_unlock(&rtlpriv->locks.conf_mutex);
+}
+EXPORT_SYMBOL_GPL(rtl_update_beacon_work_callback);
+
static void rtl_op_bss_info_changed(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
struct ieee80211_bss_conf *bss_conf,
@@ -1747,6 +1766,16 @@ static void rtl_op_flush(struct ieee80211_hw *hw,
rtlpriv->intf_ops->flush(hw, queues, drop);
}
+static int rtl_op_set_tim(struct ieee80211_hw *hw, struct ieee80211_sta *sta,
+ bool set)
+{
+ struct rtl_priv *rtlpriv = rtl_priv(hw);
+
+ schedule_work(&rtlpriv->works.update_beacon_work);
+
+ return 0;
+}
+
/* Description:
* This routine deals with the Power Configuration CMD
* parsing for RTL8723/RTL8188E Series IC.
@@ -1903,6 +1932,7 @@ const struct ieee80211_ops rtl_ops = {
.sta_add = rtl_op_sta_add,
.sta_remove = rtl_op_sta_remove,
.flush = rtl_op_flush,
+ .set_tim = rtl_op_set_tim,
};
EXPORT_SYMBOL_GPL(rtl_ops);
diff --git a/drivers/net/wireless/realtek/rtlwifi/core.h b/drivers/net/wireless/realtek/rtlwifi/core.h
index 7447ff456710..345161b47442 100644
--- a/drivers/net/wireless/realtek/rtlwifi/core.h
+++ b/drivers/net/wireless/realtek/rtlwifi/core.h
@@ -60,5 +60,6 @@ void rtl_bb_delay(struct ieee80211_hw *hw, u32 addr, u32 data);
bool rtl_cmd_send_packet(struct ieee80211_hw *hw, struct sk_buff *skb);
bool rtl_btc_status_false(void);
void rtl_dm_diginit(struct ieee80211_hw *hw, u32 cur_igval);
+void rtl_update_beacon_work_callback(struct work_struct *work);
#endif
diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.c b/drivers/net/wireless/realtek/rtlwifi/pci.c
index 3776495fd9d0..c7365354737e 100644
--- a/drivers/net/wireless/realtek/rtlwifi/pci.c
+++ b/drivers/net/wireless/realtek/rtlwifi/pci.c
@@ -1200,6 +1200,8 @@ static void _rtl_pci_init_struct(struct ieee80211_hw *hw,
_rtl_pci_prepare_bcn_tasklet);
INIT_WORK(&rtlpriv->works.lps_change_work,
rtl_lps_change_work_callback);
+ INIT_WORK(&rtlpriv->works.update_beacon_work,
+ rtl_update_beacon_work_callback);
}
static int _rtl_pci_init_tx_ring(struct ieee80211_hw *hw,
@@ -1742,6 +1744,7 @@ static void rtl_pci_deinit(struct ieee80211_hw *hw)
synchronize_irq(rtlpci->pdev->irq);
tasklet_kill(&rtlpriv->works.irq_tasklet);
cancel_work_sync(&rtlpriv->works.lps_change_work);
+ cancel_work_sync(&rtlpriv->works.update_beacon_work);
flush_workqueue(rtlpriv->works.rtl_wq);
destroy_workqueue(rtlpriv->works.rtl_wq);
diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c
index 6c5e242b1bc5..b5e1f9403949 100644
--- a/drivers/net/wireless/realtek/rtlwifi/usb.c
+++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
@@ -805,6 +805,7 @@ static void rtl_usb_stop(struct ieee80211_hw *hw)
tasklet_kill(&rtlusb->rx_work_tasklet);
cancel_work_sync(&rtlpriv->works.lps_change_work);
+ cancel_work_sync(&rtlpriv->works.update_beacon_work);
flush_workqueue(rtlpriv->works.rtl_wq);
@@ -1031,6 +1032,8 @@ int rtl_usb_probe(struct usb_interface *intf,
rtl_fill_h2c_cmd_work_callback);
INIT_WORK(&rtlpriv->works.lps_change_work,
rtl_lps_change_work_callback);
+ INIT_WORK(&rtlpriv->works.update_beacon_work,
+ rtl_update_beacon_work_callback);
rtlpriv->usb_data_index = 0;
init_completion(&rtlpriv->firmware_loading_complete);
diff --git a/drivers/net/wireless/realtek/rtlwifi/wifi.h b/drivers/net/wireless/realtek/rtlwifi/wifi.h
index fdccfd29fd61..8152117c03ee 100644
--- a/drivers/net/wireless/realtek/rtlwifi/wifi.h
+++ b/drivers/net/wireless/realtek/rtlwifi/wifi.h
@@ -2487,6 +2487,7 @@ struct rtl_works {
struct work_struct lps_change_work;
struct work_struct fill_h2c_cmd;
+ struct work_struct update_beacon_work;
};
struct rtl_debug {
--
2.21.0