On Sat, 22 Sep 2018 11:07:36 +0200 Arend van Spriel <arend.vanspriel@xxxxxxxxxxxx> wrote: > On 9/11/2018 7:26 PM, Ali MJ Al-Nasrawy wrote: > > Beacons+TIM are created/updated for fw beaconing only when > > BSS_CHANGED_BEACON. This is not compliant with power-saving > > stations. Fix it by updating beacon templates on mac80211 set_tim > > callback. Adresses the issue in: > > https://marc.info/?i=20180911163534.21312d08%20()%20manjaro > > A few remarks below but here is my .... > > Reviewed-by: Arend van Spriel <arend.vanspriel@xxxxxxxxxxxx> Thank you for your review and I shall submit a second version fixing the issues below... > > +static int brcms_ops_beacon_set_tim(struct ieee80211_hw *hw, > > + struct ieee80211_sta *sta, bool > > set) +{ > > + /*FIXME: this may be more efficiently handled by delegating > > + beacon upload to the beacon interrupt handler*/ > > No sure what you mean by this remark. The code below looks ok to me, > ie. same as with bss update. So please drop the remark. TIM is updated much more frequently than bss updates and scales with number of client stations and a simple test shows that it takes ~120us to update the beacon templates on Core i3! So I think it may be more efficient to split set_tim handler to a fast bottom half that just schedules an interrupt for the next beacon and delegates the beacon upload to the interrupt handler so that beacon templates are not updated several times per beacon interval. But I agree that this should neither be part of the code nor titled FIXME-I might have exaggerated a little bit ;) > > a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.h +++ > > b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.h @@ -568,6 > > +568,8 @@ struct brcms_c_info { u16 beacon_tim_offset; u16 > > beacon_dtim_period; struct sk_buff *probe_resp; > > + > > Just get rid of the empty line (+TAB) above. I am not keen on storing > the vif, but it seems there is no other way to get that. Okay.
