[ath9k-devel] [PATCH] ath9k: Configure beacons for AP vif if this has not happened yet

[ath9k-devel] [PATCH] ath9k: Configure beacons for AP vif if this has not happened yet

[Benjamin Berg]

Right now there is a bug where beaconing might not be enabled correctly
if the user has configuring multiple VIFs.
The issue surfaces if the userspace first creates the AP devices and
only then configures the first VIF (ath9k_bss_info_changed is called).
In this case the current ath9k_allow_beacon_config implementation will
not allow configuration as multiple AP VIFs are already present and
beaconing is never configured in the driver.

This issue was probably introduced back in 2012 by commit ef4ad6336
"ath9k: Cleanup beacon logic". The fix in this patch simply checks
whether beaconing has been configured yet (or configuration is scheduled)
and allows the configuration in that case. This works around the issue
here, but I have no idea whether it is a sane solution.

Signed-off-by: Benjamin Berg <benjamin@sipsolutions.net>
---
 drivers/net/wireless/ath/ath9k/beacon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath9k/beacon.c b/drivers/net/wireless/ath/ath9k/beacon.c
index cb366ad..0eb83b8 100644
--- a/drivers/net/wireless/ath/ath9k/beacon.c
+++ b/drivers/net/wireless/ath/ath9k/beacon.c
@@ -522,7 +522,8 @@ static bool ath9k_allow_beacon_config(struct ath_softc *sc,
 
 	if (sc->sc_ah->opmode == NL80211_IFTYPE_AP) {
 		if ((vif->type != NL80211_IFTYPE_AP) ||
-		    (sc->nbcnvifs > 1)) {
+		    test_bit(ATH_OP_BEACONS, &common->op_flags) ||
+		    (sc->ps_flags & PS_BEACON_SYNC)) {
 			ath_dbg(common, CONFIG,
 				"An AP interface is already present !\n");
 			return false;
-- 
2.1.4

[ath9k-devel] [PATCH] ath9k: Configure beacons for AP vif if this has not happened yet

[Kalle Valo]

Benjamin Berg <benjamin@sipsolutions.net> writes:

> Right now there is a bug where beaconing might not be enabled correctly
> if the user has configuring multiple VIFs.
> The issue surfaces if the userspace first creates the AP devices and
> only then configures the first VIF (ath9k_bss_info_changed is called).
> In this case the current ath9k_allow_beacon_config implementation will
> not allow configuration as multiple AP VIFs are already present and
> beaconing is never configured in the driver.
>
> This issue was probably introduced back in 2012 by commit ef4ad6336
> "ath9k: Cleanup beacon logic". The fix in this patch simply checks
> whether beaconing has been configured yet (or configuration is scheduled)
> and allows the configuration in that case. This works around the issue
> here, but I have no idea whether it is a sane solution.
>
> Signed-off-by: Benjamin Berg <benjamin@sipsolutions.net>

Sorry, I guess I missed this the last time but ath9k patches need to be
CCed to linux-wireless for wider review.

-- 
Kalle Valo

[ath9k-devel] [PATCH] ath9k: Configure beacons for AP vif if this has not happened yet

[Felix Fietkau]

On 2015-03-13 14:33, Benjamin Berg wrote:
> Right now there is a bug where beaconing might not be enabled correctly
> if the user has configuring multiple VIFs.
> The issue surfaces if the userspace first creates the AP devices and
> only then configures the first VIF (ath9k_bss_info_changed is called).
> In this case the current ath9k_allow_beacon_config implementation will
> not allow configuration as multiple AP VIFs are already present and
> beaconing is never configured in the driver.
> 
> This issue was probably introduced back in 2012 by commit ef4ad6336
> "ath9k: Cleanup beacon logic". The fix in this patch simply checks
> whether beaconing has been configured yet (or configuration is scheduled)
> and allows the configuration in that case. This works around the issue
> here, but I have no idea whether it is a sane solution.
> 
> Signed-off-by: Benjamin Berg <benjamin@sipsolutions.net>
I sent the following patch yesterday - I think it addresses the exact 
same issue. Please test it to see if it works for you as well.

Subject: [PATCH 4.0] ath9k: fix tracking of enabled AP beacons

sc->nbcnvifs tracks assigned beacon slots, not enabled beacons.
Therefore, it cannot be used to decide if cur_conf->enable_beacon (bool)
should be updated, or if beacons have been enabled already.
With the current code (depending on the order of calls), beacons often
do not get enabled in an AP+STA setup.
To fix tracking of enabled beacons, convert cur_conf->enable_beacon to a
bitmask of enabled beacon slots.

Cc: stable at vger.kernel.org
Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 drivers/net/wireless/ath/ath9k/beacon.c | 20 ++++++++++++--------
 drivers/net/wireless/ath/ath9k/common.h |  2 +-
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/beacon.c b/drivers/net/wireless/ath/ath9k/beacon.c
index cb366ad..f50a6bc 100644
--- a/drivers/net/wireless/ath/ath9k/beacon.c
+++ b/drivers/net/wireless/ath/ath9k/beacon.c
@@ -219,12 +219,15 @@ void ath9k_beacon_remove_slot(struct ath_softc *sc, struct ieee80211_vif *vif)
 	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
 	struct ath_vif *avp = (void *)vif->drv_priv;
 	struct ath_buf *bf = avp->av_bcbuf;
+	struct ath_beacon_config *cur_conf = &sc->cur_chan->beacon;
 
 	ath_dbg(common, CONFIG, "Removing interface at beacon slot: %d\n",
 		avp->av_bslot);
 
 	tasklet_disable(&sc->bcon_tasklet);
 
+	cur_conf->enable_beacon &= ~BIT(avp->av_bslot);
+
 	if (bf && bf->bf_mpdu) {
 		struct sk_buff *skb = bf->bf_mpdu;
 		dma_unmap_single(sc->dev, bf->bf_buf_addr,
@@ -521,8 +524,7 @@ static bool ath9k_allow_beacon_config(struct ath_softc *sc,
 	}
 
 	if (sc->sc_ah->opmode == NL80211_IFTYPE_AP) {
-		if ((vif->type != NL80211_IFTYPE_AP) ||
-		    (sc->nbcnvifs > 1)) {
+		if (vif->type != NL80211_IFTYPE_AP) {
 			ath_dbg(common, CONFIG,
 				"An AP interface is already present !\n");
 			return false;
@@ -616,12 +618,14 @@ void ath9k_beacon_config(struct ath_softc *sc, struct ieee80211_vif *vif,
 	 * enabling/disabling SWBA.
 	 */
 	if (changed & BSS_CHANGED_BEACON_ENABLED) {
-		if (!bss_conf->enable_beacon &&
-		    (sc->nbcnvifs <= 1)) {
-			cur_conf->enable_beacon = false;
-		} else if (bss_conf->enable_beacon) {
-			cur_conf->enable_beacon = true;
-			ath9k_cache_beacon_config(sc, ctx, bss_conf);
+		bool enabled = cur_conf->enable_beacon;
+
+		if (!bss_conf->enable_beacon) {
+			cur_conf->enable_beacon &= ~BIT(avp->av_bslot);
+		} else {
+			cur_conf->enable_beacon |= BIT(avp->av_bslot);
+			if (!enabled)
+				ath9k_cache_beacon_config(sc, ctx, bss_conf);
 		}
 	}
 
diff --git a/drivers/net/wireless/ath/ath9k/common.h b/drivers/net/wireless/ath/ath9k/common.h
index 2b79a56..d237373 100644
--- a/drivers/net/wireless/ath/ath9k/common.h
+++ b/drivers/net/wireless/ath/ath9k/common.h
@@ -54,7 +54,7 @@ struct ath_beacon_config {
 	u16 dtim_period;
 	u16 bmiss_timeout;
 	u8 dtim_count;
-	bool enable_beacon;
+	u8 enable_beacon;
 	bool ibss_creator;
 	u32 nexttbtt;
 	u32 intval;
-- 2.2.2 --

[ath9k-devel] [PATCH] ath9k: Configure beacons for AP vif if this has not happened yet

[Benjamin Berg]

On Fr, 2015-03-13 at 15:27 +0100, Felix Fietkau wrote:
> I sent the following patch yesterday - I think it addresses the exact 
> same issue. Please test it to see if it works for you as well.

The patch looks a lot better. I just backported it and it is working
just fine for me.

Benjamin

> Subject: [PATCH 4.0] ath9k: fix tracking of enabled AP beacons
> 
> sc->nbcnvifs tracks assigned beacon slots, not enabled beacons.
> Therefore, it cannot be used to decide if cur_conf->enable_beacon (bool)
> should be updated, or if beacons have been enabled already.
> With the current code (depending on the order of calls), beacons often
> do not get enabled in an AP+STA setup.
> To fix tracking of enabled beacons, convert cur_conf->enable_beacon to a
> bitmask of enabled beacon slots.
> 
> Cc: stable at vger.kernel.org
> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
> ---
>  drivers/net/wireless/ath/ath9k/beacon.c | 20 ++++++++++++--------
>  drivers/net/wireless/ath/ath9k/common.h |  2 +-
>  2 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/beacon.c b/drivers/net/wireless/ath/ath9k/beacon.c
> index cb366ad..f50a6bc 100644
> --- a/drivers/net/wireless/ath/ath9k/beacon.c
> +++ b/drivers/net/wireless/ath/ath9k/beacon.c
> @@ -219,12 +219,15 @@ void ath9k_beacon_remove_slot(struct ath_softc *sc, struct ieee80211_vif *vif)
>  	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
>  	struct ath_vif *avp = (void *)vif->drv_priv;
>  	struct ath_buf *bf = avp->av_bcbuf;
> +	struct ath_beacon_config *cur_conf = &sc->cur_chan->beacon;
>  
>  	ath_dbg(common, CONFIG, "Removing interface at beacon slot: %d\n",
>  		avp->av_bslot);
>  
>  	tasklet_disable(&sc->bcon_tasklet);
>  
> +	cur_conf->enable_beacon &= ~BIT(avp->av_bslot);
> +
>  	if (bf && bf->bf_mpdu) {
>  		struct sk_buff *skb = bf->bf_mpdu;
>  		dma_unmap_single(sc->dev, bf->bf_buf_addr,
> @@ -521,8 +524,7 @@ static bool ath9k_allow_beacon_config(struct ath_softc *sc,
>  	}
>  
>  	if (sc->sc_ah->opmode == NL80211_IFTYPE_AP) {
> -		if ((vif->type != NL80211_IFTYPE_AP) ||
> -		    (sc->nbcnvifs > 1)) {
> +		if (vif->type != NL80211_IFTYPE_AP) {
>  			ath_dbg(common, CONFIG,
>  				"An AP interface is already present !\n");
>  			return false;
> @@ -616,12 +618,14 @@ void ath9k_beacon_config(struct ath_softc *sc, struct ieee80211_vif *vif,
>  	 * enabling/disabling SWBA.
>  	 */
>  	if (changed & BSS_CHANGED_BEACON_ENABLED) {
> -		if (!bss_conf->enable_beacon &&
> -		    (sc->nbcnvifs <= 1)) {
> -			cur_conf->enable_beacon = false;
> -		} else if (bss_conf->enable_beacon) {
> -			cur_conf->enable_beacon = true;
> -			ath9k_cache_beacon_config(sc, ctx, bss_conf);
> +		bool enabled = cur_conf->enable_beacon;
> +
> +		if (!bss_conf->enable_beacon) {
> +			cur_conf->enable_beacon &= ~BIT(avp->av_bslot);
> +		} else {
> +			cur_conf->enable_beacon |= BIT(avp->av_bslot);
> +			if (!enabled)
> +				ath9k_cache_beacon_config(sc, ctx, bss_conf);
>  		}
>  	}
>  
> diff --git a/drivers/net/wireless/ath/ath9k/common.h b/drivers/net/wireless/ath/ath9k/common.h
> index 2b79a56..d237373 100644
> --- a/drivers/net/wireless/ath/ath9k/common.h
> +++ b/drivers/net/wireless/ath/ath9k/common.h
> @@ -54,7 +54,7 @@ struct ath_beacon_config {
>  	u16 dtim_period;
>  	u16 bmiss_timeout;
>  	u8 dtim_count;
> -	bool enable_beacon;
> +	u8 enable_beacon;
>  	bool ibss_creator;
>  	u32 nexttbtt;
>  	u32 intval;
> -- 2.2.2 --
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: This is a digitally signed message part
Url : http://lists.ath9k.org/pipermail/ath9k-devel/attachments/20150313/3efb06f4/attachment-0001.pgp 

[ath9k-devel] [PATCH] ath9k: Configure beacons for AP vif if this has not happened yet

[Ben Greear]

Any idea how far back this needs to be backported for stable?

Just to 4.0?

Thanks,
Ben

On 03/13/2015 07:27 AM, Felix Fietkau wrote:
> On 2015-03-13 14:33, Benjamin Berg wrote:
>> Right now there is a bug where beaconing might not be enabled correctly
>> if the user has configuring multiple VIFs.
>> The issue surfaces if the userspace first creates the AP devices and
>> only then configures the first VIF (ath9k_bss_info_changed is called).
>> In this case the current ath9k_allow_beacon_config implementation will
>> not allow configuration as multiple AP VIFs are already present and
>> beaconing is never configured in the driver.
>>
>> This issue was probably introduced back in 2012 by commit ef4ad6336
>> "ath9k: Cleanup beacon logic". The fix in this patch simply checks
>> whether beaconing has been configured yet (or configuration is scheduled)
>> and allows the configuration in that case. This works around the issue
>> here, but I have no idea whether it is a sane solution.
>>
>> Signed-off-by: Benjamin Berg <benjamin@sipsolutions.net>
> I sent the following patch yesterday - I think it addresses the exact 
> same issue. Please test it to see if it works for you as well.
> 
> Subject: [PATCH 4.0] ath9k: fix tracking of enabled AP beacons
> 
> sc->nbcnvifs tracks assigned beacon slots, not enabled beacons.
> Therefore, it cannot be used to decide if cur_conf->enable_beacon (bool)
> should be updated, or if beacons have been enabled already.
> With the current code (depending on the order of calls), beacons often
> do not get enabled in an AP+STA setup.
> To fix tracking of enabled beacons, convert cur_conf->enable_beacon to a
> bitmask of enabled beacon slots.
> 
> Cc: stable at vger.kernel.org
> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
> ---
>  drivers/net/wireless/ath/ath9k/beacon.c | 20 ++++++++++++--------
>  drivers/net/wireless/ath/ath9k/common.h |  2 +-
>  2 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/beacon.c b/drivers/net/wireless/ath/ath9k/beacon.c
> index cb366ad..f50a6bc 100644
> --- a/drivers/net/wireless/ath/ath9k/beacon.c
> +++ b/drivers/net/wireless/ath/ath9k/beacon.c
> @@ -219,12 +219,15 @@ void ath9k_beacon_remove_slot(struct ath_softc *sc, struct ieee80211_vif *vif)
>  	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
>  	struct ath_vif *avp = (void *)vif->drv_priv;
>  	struct ath_buf *bf = avp->av_bcbuf;
> +	struct ath_beacon_config *cur_conf = &sc->cur_chan->beacon;
>  
>  	ath_dbg(common, CONFIG, "Removing interface at beacon slot: %d\n",
>  		avp->av_bslot);
>  
>  	tasklet_disable(&sc->bcon_tasklet);
>  
> +	cur_conf->enable_beacon &= ~BIT(avp->av_bslot);
> +
>  	if (bf && bf->bf_mpdu) {
>  		struct sk_buff *skb = bf->bf_mpdu;
>  		dma_unmap_single(sc->dev, bf->bf_buf_addr,
> @@ -521,8 +524,7 @@ static bool ath9k_allow_beacon_config(struct ath_softc *sc,
>  	}
>  
>  	if (sc->sc_ah->opmode == NL80211_IFTYPE_AP) {
> -		if ((vif->type != NL80211_IFTYPE_AP) ||
> -		    (sc->nbcnvifs > 1)) {
> +		if (vif->type != NL80211_IFTYPE_AP) {
>  			ath_dbg(common, CONFIG,
>  				"An AP interface is already present !\n");
>  			return false;
> @@ -616,12 +618,14 @@ void ath9k_beacon_config(struct ath_softc *sc, struct ieee80211_vif *vif,
>  	 * enabling/disabling SWBA.
>  	 */
>  	if (changed & BSS_CHANGED_BEACON_ENABLED) {
> -		if (!bss_conf->enable_beacon &&
> -		    (sc->nbcnvifs <= 1)) {
> -			cur_conf->enable_beacon = false;
> -		} else if (bss_conf->enable_beacon) {
> -			cur_conf->enable_beacon = true;
> -			ath9k_cache_beacon_config(sc, ctx, bss_conf);
> +		bool enabled = cur_conf->enable_beacon;
> +
> +		if (!bss_conf->enable_beacon) {
> +			cur_conf->enable_beacon &= ~BIT(avp->av_bslot);
> +		} else {
> +			cur_conf->enable_beacon |= BIT(avp->av_bslot);
> +			if (!enabled)
> +				ath9k_cache_beacon_config(sc, ctx, bss_conf);
>  		}
>  	}
>  
> diff --git a/drivers/net/wireless/ath/ath9k/common.h b/drivers/net/wireless/ath/ath9k/common.h
> index 2b79a56..d237373 100644
> --- a/drivers/net/wireless/ath/ath9k/common.h
> +++ b/drivers/net/wireless/ath/ath9k/common.h
> @@ -54,7 +54,7 @@ struct ath_beacon_config {
>  	u16 dtim_period;
>  	u16 bmiss_timeout;
>  	u8 dtim_count;
> -	bool enable_beacon;
> +	u8 enable_beacon;
>  	bool ibss_creator;
>  	u32 nexttbtt;
>  	u32 intval;
> -- 2.2.2 --
> _______________________________________________
> ath9k-devel mailing list
> ath9k-devel at lists.ath9k.org
> https://lists.ath9k.org/mailman/listinfo/ath9k-devel
> 


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com