RE: rtlwifi/rtl8192cu AP mode broken with PS STA

[Pkshih <pkshih@realtek.com>]

> -----Original Message-----
> From: Maciej S. Szmigiero [mailto:mail@maciej.szmigiero.name]
> Sent: Thursday, April 08, 2021 4:53 AM
> To: Larry Finger; Pkshih
> Cc: linux-wireless@vger.kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> johannes@sipsolutions.net; kvalo@codeaurora.org
> Subject: Re: rtlwifi/rtl8192cu AP mode broken with PS STA
> 
> On 07.04.2021 06:21, Larry Finger wrote:
> > On 4/6/21 9:48 PM, Pkshih wrote:
> >> 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.
> 
> That's great, thanks!
> 
> >> 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.
> 
> It looks like PCI rtlwifi hardware uses a tasklet (specifically,
> _rtl_pci_prepare_bcn_tasklet() in pci.c) to periodically transfer the
> current beacon to the NIC.

Got it.

> 
> This tasklet is scheduled on a RTL_IMR_BCNINT interrupt, which sounds
> like a beacon interval interrupt.
> 

Yes, PCI series update every beacon, so TIM and DTIM count maintained by
mac80211 work properly.

> >> I don't have a rtl8192cu dongle on hand, but I'll try to find one.
> >
> > Maceij,
> >
> > Does this patch fix the problem?
> 
> The beacon seems to be updating now and STAs no longer get stuck in PS
> mode.
> Although sometimes (every 2-3 minutes with continuous 1s interval pings)
> there is around 5s delay in updating the transmitted beacon - don't know
> why, maybe the NIC hardware still has the old version in queue?

Since USB device doesn't update every beacon, dtim_count isn't updated neither.
It leads STA doesn't awake properly. Please try to fix dtim_period=1 in
hostapd.conf, which tells STA awakes every beacon interval.

> 
> I doubt, however that this set_tim() callback should be added for every
> rtlwifi device type.
> 
> As I have said above, PCI devices seem to already have a mechanism in
> place to update their beacon each beacon interval.
> Your test that RTL8821AE works without this patch confirms that (at
> least for the rtl8821ae driver).
> 
> It seems this callback is only necessarily for USB rtlwifi devices.
> Which currently means rtl8192cu only.
> 

I agree with you.

--
Ping-Ke