Re: rtlwifi/rtl8192cu AP mode broken with PS STA

[Pkshih <pkshih@realtek.com>]

[-- Attachment #1: Type: text/plain, Size: 3068 bytes --]

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@maciej.szmigiero.name> 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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-rtlwifi-implement-set_tim-by-update-beacon-content.patch --]
[-- Type: text/x-patch; name="0001-rtlwifi-implement-set_tim-by-update-beacon-content.patch", Size: 5209 bytes --]

From 44be80232aa49737c035ee4656d20a22f573d33e Mon Sep 17 00:00:00 2001
From: Ping-Ke Shih <pkshih@realtek.com>
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@realtek.com>
---
 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