All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] brcmsmac: ap mode: update beacon when TIM changes
@ 2018-09-11 17:26 Ali MJ Al-Nasrawy
  2018-09-20 12:04 ` Kalle Valo
                   ` (4 more replies)
  0 siblings, 5 replies; 11+ messages in thread
From: Ali MJ Al-Nasrawy @ 2018-09-11 17:26 UTC (permalink / raw)
  To: Arend van Spriel
  Cc: linux-wireless, brcm80211-dev-list.pdl, brcm80211-dev-list,
	Ali MJ Al-Nasrawy

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

Signed-off-by: Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com>
---
 .../broadcom/brcm80211/brcmsmac/mac80211_if.c | 23 +++++++++++++++++++
 .../broadcom/brcm80211/brcmsmac/main.h        |  2 ++
 2 files changed, 25 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/mac80211_if.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/mac80211_if.c
index ddfdfe1..ee92bb5 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/mac80211_if.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/mac80211_if.c
@@ -502,6 +502,7 @@ brcms_ops_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
 	}
 
 	spin_lock_bh(&wl->lock);
+	wl->wlc->vif = vif;
 	wl->mute_tx = false;
 	brcms_c_mute(wl->wlc, false);
 	if (vif->type == NL80211_IFTYPE_STATION)
@@ -937,6 +938,27 @@ static void brcms_ops_set_tsf(struct ieee80211_hw *hw,
 	spin_unlock_bh(&wl->lock);
 }
 
+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*/
+	struct brcms_info *wl = hw->priv;
+	struct sk_buff *beacon;
+	u16 tim_offset = 0;
+	
+	beacon = ieee80211_beacon_get_tim(hw, wl->wlc->vif,
+					  &tim_offset, NULL);
+	if (beacon){
+		spin_lock_bh(&wl->lock);
+		brcms_c_set_new_beacon(wl->wlc, beacon, tim_offset,
+				       wl->wlc->vif->bss_conf.dtim_period);
+		spin_unlock_bh(&wl->lock);
+	}
+	
+	return 0;
+}
+
 static const struct ieee80211_ops brcms_ops = {
 	.tx = brcms_ops_tx,
 	.start = brcms_ops_start,
@@ -955,6 +977,7 @@ static const struct ieee80211_ops brcms_ops = {
 	.flush = brcms_ops_flush,
 	.get_tsf = brcms_ops_get_tsf,
 	.set_tsf = brcms_ops_set_tsf,
+	.set_tim = brcms_ops_beacon_set_tim,
 };
 
 void brcms_dpc(unsigned long data)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.h b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.h
index c4d135c..e3939fc 100644
--- 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;
+	
+	struct ieee80211_vif *vif;
 };
 
 /* antsel module specific state */
-- 
2.18.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] brcmsmac: ap mode: update beacon when TIM changes
  2018-09-11 17:26 [PATCH] brcmsmac: ap mode: update beacon when TIM changes Ali MJ Al-Nasrawy
@ 2018-09-20 12:04 ` Kalle Valo
  2018-09-22  9:07 ` Arend van Spriel
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 11+ messages in thread
From: Kalle Valo @ 2018-09-20 12:04 UTC (permalink / raw)
  To: Ali MJ Al-Nasrawy
  Cc: Arend van Spriel, linux-wireless, brcm80211-dev-list.pdl,
	brcm80211-dev-list, Ali MJ Al-Nasrawy

Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com> 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
> 
> Signed-off-by: Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com>

Arend, what do you think about this?

-- 
https://patchwork.kernel.org/patch/10596025/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] brcmsmac: ap mode: update beacon when TIM changes
  2018-09-11 17:26 [PATCH] brcmsmac: ap mode: update beacon when TIM changes Ali MJ Al-Nasrawy
  2018-09-20 12:04 ` Kalle Valo
@ 2018-09-22  9:07 ` Arend van Spriel
  2018-09-22 13:17   ` Ali MJ Al-Nasrawy
  2018-09-22 13:41 ` [PATCH v2] brcmsmac: AP " Ali MJ Al-Nasrawy
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 11+ messages in thread
From: Arend van Spriel @ 2018-09-22  9:07 UTC (permalink / raw)
  To: Ali MJ Al-Nasrawy
  Cc: linux-wireless, brcm80211-dev-list.pdl, brcm80211-dev-list

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@broadcom.com>
> Signed-off-by: Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com>
> ---
>  .../broadcom/brcm80211/brcmsmac/mac80211_if.c | 23 +++++++++++++++++++
>  .../broadcom/brcm80211/brcmsmac/main.h        |  2 ++
>  2 files changed, 25 insertions(+)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/mac80211_if.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/mac80211_if.c
> index ddfdfe1..ee92bb5 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/mac80211_if.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/mac80211_if.c
> @@ -502,6 +502,7 @@ brcms_ops_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
>  	}
>
>  	spin_lock_bh(&wl->lock);
> +	wl->wlc->vif = vif;
>  	wl->mute_tx = false;
>  	brcms_c_mute(wl->wlc, false);
>  	if (vif->type == NL80211_IFTYPE_STATION)
> @@ -937,6 +938,27 @@ static void brcms_ops_set_tsf(struct ieee80211_hw *hw,
>  	spin_unlock_bh(&wl->lock);
>  }
>
> +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.

> +	struct brcms_info *wl = hw->priv;
> +	struct sk_buff *beacon;
> +	u16 tim_offset = 0;
> +	
> +	beacon = ieee80211_beacon_get_tim(hw, wl->wlc->vif,
> +					  &tim_offset, NULL);
> +	if (beacon){
> +		spin_lock_bh(&wl->lock);
> +		brcms_c_set_new_beacon(wl->wlc, beacon, tim_offset,
> +				       wl->wlc->vif->bss_conf.dtim_period);
> +		spin_unlock_bh(&wl->lock);
> +	}
> +	
> +	return 0;
> +}
> +
>  static const struct ieee80211_ops brcms_ops = {
>  	.tx = brcms_ops_tx,
>  	.start = brcms_ops_start,
> @@ -955,6 +977,7 @@ static const struct ieee80211_ops brcms_ops = {
>  	.flush = brcms_ops_flush,
>  	.get_tsf = brcms_ops_get_tsf,
>  	.set_tsf = brcms_ops_set_tsf,
> +	.set_tim = brcms_ops_beacon_set_tim,
>  };
>
>  void brcms_dpc(unsigned long data)
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.h b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.h
> index c4d135c..e3939fc 100644
> --- 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.

> +	struct ieee80211_vif *vif;
>  };
>
>  /* antsel module specific state */
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] brcmsmac: ap mode: update beacon when TIM changes
  2018-09-22  9:07 ` Arend van Spriel
@ 2018-09-22 13:17   ` Ali MJ Al-Nasrawy
  0 siblings, 0 replies; 11+ messages in thread
From: Ali MJ Al-Nasrawy @ 2018-09-22 13:17 UTC (permalink / raw)
  To: Arend van Spriel
  Cc: linux-wireless, brcm80211-dev-list.pdl, brcm80211-dev-list

On Sat, 22 Sep 2018 11:07:36 +0200
Arend van Spriel <arend.vanspriel@broadcom.com> 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@broadcom.com>

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] brcmsmac: AP mode: update beacon when TIM changes
  2018-09-11 17:26 [PATCH] brcmsmac: ap mode: update beacon when TIM changes Ali MJ Al-Nasrawy
  2018-09-20 12:04 ` Kalle Valo
  2018-09-22  9:07 ` Arend van Spriel
@ 2018-09-22 13:41 ` Ali MJ Al-Nasrawy
  2018-09-23  9:54 ` [PATCH v3] " Ali MJ Al-Nasrawy
  2018-10-03 16:21 ` [PATCH v4] " Ali MJ Al-Nasrawy
  4 siblings, 0 replies; 11+ messages in thread
From: Ali MJ Al-Nasrawy @ 2018-09-22 13:41 UTC (permalink / raw)
  To: Arend van Spriel
  Cc: linux-wireless, brcm80211-dev-list.pdl, brcm80211-dev-list,
	Kalle Valo, Ali MJ Al-Nasrawy

Beacons+TIM are created/updated for 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.

Signed-off-by: Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com>
---
 .../broadcom/brcm80211/brcmsmac/mac80211_if.c | 21 +++++++++++++++++++
 .../broadcom/brcm80211/brcmsmac/main.h        |  1 +
 2 files changed, 22 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/mac80211_if.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/mac80211_if.c
index ddfdfe1..85e7b77 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/mac80211_if.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/mac80211_if.c
@@ -502,6 +502,7 @@ brcms_ops_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
 	}
 
 	spin_lock_bh(&wl->lock);
+	wl->wlc->vif = vif;
 	wl->mute_tx = false;
 	brcms_c_mute(wl->wlc, false);
 	if (vif->type == NL80211_IFTYPE_STATION)
@@ -937,6 +938,25 @@ static void brcms_ops_set_tsf(struct ieee80211_hw *hw,
 	spin_unlock_bh(&wl->lock);
 }
 
+static int brcms_ops_beacon_set_tim(struct ieee80211_hw *hw,
+				 struct ieee80211_sta *sta, bool set)
+{
+	struct brcms_info *wl = hw->priv;
+	struct sk_buff *beacon;
+	u16 tim_offset = 0;
+	
+	beacon = ieee80211_beacon_get_tim(hw, wl->wlc->vif,
+					  &tim_offset, NULL);
+	if (beacon){
+		spin_lock_bh(&wl->lock);
+		brcms_c_set_new_beacon(wl->wlc, beacon, tim_offset,
+				       wl->wlc->vif->bss_conf.dtim_period);
+		spin_unlock_bh(&wl->lock);
+	}
+	
+	return 0;
+}
+
 static const struct ieee80211_ops brcms_ops = {
 	.tx = brcms_ops_tx,
 	.start = brcms_ops_start,
@@ -955,6 +975,7 @@ static const struct ieee80211_ops brcms_ops = {
 	.flush = brcms_ops_flush,
 	.get_tsf = brcms_ops_get_tsf,
 	.set_tsf = brcms_ops_set_tsf,
+	.set_tim = brcms_ops_beacon_set_tim,
 };
 
 void brcms_dpc(unsigned long data)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.h b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.h
index c4d135c..6ece25c 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.h
@@ -568,6 +568,7 @@ struct brcms_c_info {
 	u16 beacon_tim_offset;
 	u16 beacon_dtim_period;
 	struct sk_buff *probe_resp;
+	struct ieee80211_vif *vif;
 };
 
 /* antsel module specific state */
-- 
2.18.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3] brcmsmac: AP mode: update beacon when TIM changes
  2018-09-11 17:26 [PATCH] brcmsmac: ap mode: update beacon when TIM changes Ali MJ Al-Nasrawy
                   ` (2 preceding siblings ...)
  2018-09-22 13:41 ` [PATCH v2] brcmsmac: AP " Ali MJ Al-Nasrawy
@ 2018-09-23  9:54 ` Ali MJ Al-Nasrawy
  2018-09-24  8:15   ` Arend van Spriel
  2018-09-26  9:04   ` Ali MJ Al-Nasrawy
  2018-10-03 16:21 ` [PATCH v4] " Ali MJ Al-Nasrawy
  4 siblings, 2 replies; 11+ messages in thread
From: Ali MJ Al-Nasrawy @ 2018-09-23  9:54 UTC (permalink / raw)
  To: Arend van Spriel
  Cc: linux-wireless, brcm80211-dev-list.pdl, brcm80211-dev-list,
	Kalle Valo, Ali MJ Al-Nasrawy

Beacons are not updated to reflect TIM changes. This is not compliant with
power-saving client stations as the beacons do not have valid TIM and can cause
the network to stall at random occasions with highly variable latencies.
Fix it by updating beacon templates on mac80211 set_tim callback.

Addresses an issue described in:
https://marc.info/?i=20180911163534.21312d08%20()%20manjaro

Signed-off-by: Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com>
---

v2 -> v3:
	- removed tabs from blank lines
	- extended the commit message to answer "Why"

 .../broadcom/brcm80211/brcmsmac/mac80211_if.c | 21 +++++++++++++++++++
 .../broadcom/brcm80211/brcmsmac/main.h        |  1 +
 2 files changed, 22 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/mac80211_if.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/mac80211_if.c
index ddfdfe1..c6b258f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/mac80211_if.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/mac80211_if.c
@@ -502,6 +502,7 @@ brcms_ops_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
 	}
 
 	spin_lock_bh(&wl->lock);
+	wl->wlc->vif = vif;
 	wl->mute_tx = false;
 	brcms_c_mute(wl->wlc, false);
 	if (vif->type == NL80211_IFTYPE_STATION)
@@ -937,6 +938,25 @@ static void brcms_ops_set_tsf(struct ieee80211_hw *hw,
 	spin_unlock_bh(&wl->lock);
 }
 
+static int brcms_ops_beacon_set_tim(struct ieee80211_hw *hw,
+				 struct ieee80211_sta *sta, bool set)
+{
+	struct brcms_info *wl = hw->priv;
+	struct sk_buff *beacon;
+	u16 tim_offset = 0;
+
+	beacon = ieee80211_beacon_get_tim(hw, wl->wlc->vif,
+					  &tim_offset, NULL);
+	if (beacon){
+		spin_lock_bh(&wl->lock);
+		brcms_c_set_new_beacon(wl->wlc, beacon, tim_offset,
+				       wl->wlc->vif->bss_conf.dtim_period);
+		spin_unlock_bh(&wl->lock);
+	}
+
+	return 0;
+}
+
 static const struct ieee80211_ops brcms_ops = {
 	.tx = brcms_ops_tx,
 	.start = brcms_ops_start,
@@ -955,6 +975,7 @@ static const struct ieee80211_ops brcms_ops = {
 	.flush = brcms_ops_flush,
 	.get_tsf = brcms_ops_get_tsf,
 	.set_tsf = brcms_ops_set_tsf,
+	.set_tim = brcms_ops_beacon_set_tim,
 };
 
 void brcms_dpc(unsigned long data)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.h b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.h
index c4d135c..6ece25c 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.h
@@ -568,6 +568,7 @@ struct brcms_c_info {
 	u16 beacon_tim_offset;
 	u16 beacon_dtim_period;
 	struct sk_buff *probe_resp;
+	struct ieee80211_vif *vif;
 };
 
 /* antsel module specific state */
-- 
2.18.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] brcmsmac: AP mode: update beacon when TIM changes
  2018-09-23  9:54 ` [PATCH v3] " Ali MJ Al-Nasrawy
@ 2018-09-24  8:15   ` Arend van Spriel
  2018-09-24 10:24     ` Ali MJ Al-Nasrawy
  2018-09-26  9:04   ` Ali MJ Al-Nasrawy
  1 sibling, 1 reply; 11+ messages in thread
From: Arend van Spriel @ 2018-09-24  8:15 UTC (permalink / raw)
  To: Ali MJ Al-Nasrawy
  Cc: linux-wireless, brcm80211-dev-list.pdl, brcm80211-dev-list, Kalle Valo

On 9/23/2018 11:54 AM, Ali MJ Al-Nasrawy wrote:
> Beacons are not updated to reflect TIM changes. This is not compliant with
> power-saving client stations as the beacons do not have valid TIM and can cause
> the network to stall at random occasions with highly variable latencies.
> Fix it by updating beacon templates on mac80211 set_tim callback.
>
> Addresses an issue described in:
> https://marc.info/?i=20180911163534.21312d08%20()%20manjaro
>
> Signed-off-by: Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com>
> ---

Missing 'v1 -> v2' in this changelog.

Regards,
Arend

> v2 -> v3:
> 	- removed tabs from blank lines
> 	- extended the commit message to answer "Why"
>
>  .../broadcom/brcm80211/brcmsmac/mac80211_if.c | 21 +++++++++++++++++++
>  .../broadcom/brcm80211/brcmsmac/main.h        |  1 +
>  2 files changed, 22 insertions(+)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] brcmsmac: AP mode: update beacon when TIM changes
  2018-09-24  8:15   ` Arend van Spriel
@ 2018-09-24 10:24     ` Ali MJ Al-Nasrawy
  0 siblings, 0 replies; 11+ messages in thread
From: Ali MJ Al-Nasrawy @ 2018-09-24 10:24 UTC (permalink / raw)
  To: Arend van Spriel
  Cc: linux-wireless, brcm80211-dev-list.pdl, brcm80211-dev-list

On Mon, 24 Sep 2018 10:15:49 +0200
Arend van Spriel <arend.vanspriel@broadcom.com> wrote:

> On 9/23/2018 11:54 AM, Ali MJ Al-Nasrawy wrote:
> > Beacons are not updated to reflect TIM changes. This is not
> > compliant with power-saving client stations as the beacons do not
> > have valid TIM and can cause the network to stall at random
> > occasions with highly variable latencies. Fix it by updating beacon
> > templates on mac80211 set_tim callback.
> >
> > Addresses an issue described in:
> > https://marc.info/?i=20180911163534.21312d08%20()%20manjaro
> >
> > Signed-off-by: Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com>
> > ---  
> 
> Missing 'v1 -> v2' in this changelog.

Sorry, missed that...

v1 -> v2:
	-removed FIXME comment
	-removed blank line before 'vif' member declaration

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] brcmsmac: AP mode: update beacon when TIM changes
  2018-09-23  9:54 ` [PATCH v3] " Ali MJ Al-Nasrawy
  2018-09-24  8:15   ` Arend van Spriel
@ 2018-09-26  9:04   ` Ali MJ Al-Nasrawy
  1 sibling, 0 replies; 11+ messages in thread
From: Ali MJ Al-Nasrawy @ 2018-09-26  9:04 UTC (permalink / raw)
  To: Arend van Spriel
  Cc: linux-wireless, brcm80211-dev-list.pdl, brcm80211-dev-list

Hi,

I suggest here some improvements to this patch but I don't have strong
opinion about them so any feedback is appreciated:

-split it into two: one for storing the current vif into brcms_c_info
and also setting it to NULL on remove_interface to avoid dangling
pointers; and an other one for actually updating the beacon; I believe
them to be separate changes.

On Sun, 23 Sep 2018 12:54:25 +0300
Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com> wrote: 
> +static int brcms_ops_beacon_set_tim(struct ieee80211_hw *hw,
> +				 struct ieee80211_sta *sta, bool set)
> +{
> +	struct brcms_info *wl = hw->priv;
> +	struct sk_buff *beacon;
> +	u16 tim_offset = 0;
> +
> +	beacon = ieee80211_beacon_get_tim(hw, wl->wlc->vif,
> +					  &tim_offset, NULL);
> +	if (beacon){
> +		spin_lock_bh(&wl->lock);
> +		brcms_c_set_new_beacon(wl->wlc, beacon, tim_offset,
> +
> wl->wlc->vif->bss_conf.dtim_period);
> +		spin_unlock_bh(&wl->lock);
> +	}
> +
> +	return 0;
> +}
> +

-on brcms_ops_beacon_set_tim, include ieee80211_beacon_get_tim call in
the lock context and also check for vif to be null before passing it as
argument.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v4] brcmsmac: AP mode: update beacon when TIM changes
  2018-09-11 17:26 [PATCH] brcmsmac: ap mode: update beacon when TIM changes Ali MJ Al-Nasrawy
                   ` (3 preceding siblings ...)
  2018-09-23  9:54 ` [PATCH v3] " Ali MJ Al-Nasrawy
@ 2018-10-03 16:21 ` Ali MJ Al-Nasrawy
  2018-10-13 17:01   ` Kalle Valo
  4 siblings, 1 reply; 11+ messages in thread
From: Ali MJ Al-Nasrawy @ 2018-10-03 16:21 UTC (permalink / raw)
  To: Arend van Spriel
  Cc: linux-wireless, brcm80211-dev-list.pdl, brcm80211-dev-list,
	Ali MJ Al-Nasrawy

Beacons are not updated to reflect TIM changes. This is not compliant with
power-saving client stations as the beacons do not have valid TIM and can
cause the network to stall at random occasions and to have highly variable
latencies.
Fix it by updating beacon templates on mac80211 set_tim callback.

Addresses an issue described in:
https://marc.info/?i=20180911163534.21312d08%20()%20manjaro

Signed-off-by: Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com>
---

v3 -> v4:
	-set vif to NULL on remove_interface callback to prevent dangling
pointers.
	-access vif from within &wl->lock context only.
	-perform null pointer check before passing vif to
ieee80211_beacon_get_tim.

v2 -> v3:
	- remove tabs from blank lines
	- extend the commit message to answer "Why"

v1 -> v2:
	-remove FIXME comment
	-remove blank line before 'vif' member declaration

 .../broadcom/brcm80211/brcmsmac/mac80211_if.c | 26 +++++++++++++++++++
 .../broadcom/brcm80211/brcmsmac/main.h        |  1 +
 2 files changed, 27 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/mac80211_if.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/mac80211_if.c
index ddfdfe1..257968f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/mac80211_if.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/mac80211_if.c
@@ -502,6 +502,7 @@ brcms_ops_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
 	}
 
 	spin_lock_bh(&wl->lock);
+	wl->wlc->vif = vif;
 	wl->mute_tx = false;
 	brcms_c_mute(wl->wlc, false);
 	if (vif->type == NL80211_IFTYPE_STATION)
@@ -519,6 +520,11 @@ brcms_ops_add_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
 static void
 brcms_ops_remove_interface(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
 {
+	struct brcms_info *wl = hw->priv;
+
+	spin_lock_bh(&wl->lock);
+	wl->wlc->vif = NULL;
+	spin_unlock_bh(&wl->lock);
 }
 
 static int brcms_ops_config(struct ieee80211_hw *hw, u32 changed)
@@ -937,6 +943,25 @@ static void brcms_ops_set_tsf(struct ieee80211_hw *hw,
 	spin_unlock_bh(&wl->lock);
 }
 
+static int brcms_ops_beacon_set_tim(struct ieee80211_hw *hw,
+				 struct ieee80211_sta *sta, bool set)
+{
+	struct brcms_info *wl = hw->priv;
+	struct sk_buff *beacon = NULL;
+	u16 tim_offset = 0;
+
+	spin_lock_bh(&wl->lock);
+	if (wl->wlc->vif)
+		beacon = ieee80211_beacon_get_tim(hw, wl->wlc->vif,
+						  &tim_offset, NULL);
+	if (beacon)
+		brcms_c_set_new_beacon(wl->wlc, beacon, tim_offset,
+				       wl->wlc->vif->bss_conf.dtim_period);
+	spin_unlock_bh(&wl->lock);
+
+	return 0;
+}
+
 static const struct ieee80211_ops brcms_ops = {
 	.tx = brcms_ops_tx,
 	.start = brcms_ops_start,
@@ -955,6 +980,7 @@ static const struct ieee80211_ops brcms_ops = {
 	.flush = brcms_ops_flush,
 	.get_tsf = brcms_ops_get_tsf,
 	.set_tsf = brcms_ops_set_tsf,
+	.set_tim = brcms_ops_beacon_set_tim,
 };
 
 void brcms_dpc(unsigned long data)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.h b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.h
index c4d135c..9f76b88 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.h
@@ -563,6 +563,7 @@ struct brcms_c_info {
 
 	struct wiphy *wiphy;
 	struct scb pri_scb;
+	struct ieee80211_vif *vif;
 
 	struct sk_buff *beacon;
 	u16 beacon_tim_offset;
-- 
2.18.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v4] brcmsmac: AP mode: update beacon when TIM changes
  2018-10-03 16:21 ` [PATCH v4] " Ali MJ Al-Nasrawy
@ 2018-10-13 17:01   ` Kalle Valo
  0 siblings, 0 replies; 11+ messages in thread
From: Kalle Valo @ 2018-10-13 17:01 UTC (permalink / raw)
  To: Ali MJ Al-Nasrawy
  Cc: Arend van Spriel, linux-wireless, brcm80211-dev-list.pdl,
	brcm80211-dev-list, Ali MJ Al-Nasrawy

Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com> wrote:

> Beacons are not updated to reflect TIM changes. This is not compliant with
> power-saving client stations as the beacons do not have valid TIM and can
> cause the network to stall at random occasions and to have highly variable
> latencies.
> Fix it by updating beacon templates on mac80211 set_tim callback.
> 
> Addresses an issue described in:
> https://marc.info/?i=20180911163534.21312d08%20()%20manjaro
> 
> Signed-off-by: Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com>

Patch applied to wireless-drivers-next.git, thanks.

2258ee58baa5 brcmsmac: AP mode: update beacon when TIM changes

-- 
https://patchwork.kernel.org/patch/10625067/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-10-13 17:01 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-11 17:26 [PATCH] brcmsmac: ap mode: update beacon when TIM changes Ali MJ Al-Nasrawy
2018-09-20 12:04 ` Kalle Valo
2018-09-22  9:07 ` Arend van Spriel
2018-09-22 13:17   ` Ali MJ Al-Nasrawy
2018-09-22 13:41 ` [PATCH v2] brcmsmac: AP " Ali MJ Al-Nasrawy
2018-09-23  9:54 ` [PATCH v3] " Ali MJ Al-Nasrawy
2018-09-24  8:15   ` Arend van Spriel
2018-09-24 10:24     ` Ali MJ Al-Nasrawy
2018-09-26  9:04   ` Ali MJ Al-Nasrawy
2018-10-03 16:21 ` [PATCH v4] " Ali MJ Al-Nasrawy
2018-10-13 17:01   ` Kalle Valo

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.