Skip to contentSkip to conversation

ath9k: Configure beacons for AP vif if this has not happened yet

PATCH New Benjamin Berg 4 participants 5 messages Mar 13, 2015 Qualcomm Atheros ATH9K wireless driver

lore
Share & tools
lore

Benjamin Berg

author
#0 lore raw

PATCH 1/1 ath9k: Configure beacons for AP vif if this has not happened yet

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 <[email protected]>
---
 drivers/net/wireless/ath/ath9k/beacon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/wireless/ath/ath9k/beacon.c +2 -1
index cb366ad..0eb83b8 100644
@@ -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;
Signature · 1 line
2.1.4
#1 lore raw

PATCH 1/1 ath9k: Configure beacons for AP vif if this has not happened yet

Quoted 8 lines · 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.

Quoted 7 lines

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 <[email protected]>

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

Signature · 1 line
Kalle Valo
#2 lore raw

PATCH 1/1 ath9k: Configure beacons for AP vif if this has not happened yet

In reply to Benjamin Berg
Quoted 8 lines · 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.

Quoted 7 lines

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 <[email protected]>

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 <[email protected]>
---
 drivers/net/wireless/ath/ath9k/beacon.c | 20 ++++++++++++--------
 drivers/net/wireless/ath/ath9k/common.h |  2 +-
 2 files changed, 13 insertions(+), 9 deletions(-)
drivers/net/wireless/ath/ath9k/beacon.c +12 -8
index cb366ad..f50a6bc 100644
@@ -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);
 		}
 	}
drivers/net/wireless/ath/ath9k/common.h +1 -1
index 2b79a56..d237373 100644
@@ -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 --

author
#3 lore raw

PATCH 1/1 ath9k: Configure beacons for AP vif if this has not happened yet

Quoted 2 lines · Felix Fietkau

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

Quoted 74 lines

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 <[email protected]>
---
 drivers/net/wireless/ath/ath9k/beacon.c | 20 ++++++++++++--------
 drivers/net/wireless/ath/ath9k/common.h |  2 +-
 2 files changed, 13 insertions(+), 9 deletions(-)
drivers/net/wireless/ath/ath9k/beacon.c +12 -8
index cb366ad..f50a6bc 100644
@@ -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);
 		}
 	}
drivers/net/wireless/ath/ath9k/common.h +0 -0
index 2b79a56..d237373 100644

drivers/net/wireless/ath/ath9k/common.h struct ath_beacon_config {

Quoted 9 lines
drivers/net/wireless/ath/ath9k/common.h +1 -2
@@ -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

#4 lore raw

PATCH 1/1 ath9k: Configure beacons for AP vif if this has not happened yet

In reply to Felix Fietkau

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

Just to 4.0?

Thanks,
Ben

Quoted 106 lines · Felix Fietkau
Quoted 8 lines · 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.

Quoted 7 lines

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 <[email protected]>

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 <[email protected]>
---
 drivers/net/wireless/ath/ath9k/beacon.c | 20 ++++++++++++--------
 drivers/net/wireless/ath/ath9k/common.h |  2 +-
 2 files changed, 13 insertions(+), 9 deletions(-)
drivers/net/wireless/ath/ath9k/beacon.c +12 -8
index cb366ad..f50a6bc 100644
@@ -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);
 		}
 	}
drivers/net/wireless/ath/ath9k/common.h +1 -2
index 2b79a56..d237373 100644
@@ -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

Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com