* [PATCH] mac80211/ath9k: Support AMPDU with multiple VIFs.
@ 2010-09-23 13:54 greearb
2010-09-23 14:08 ` Johannes Berg
0 siblings, 1 reply; 5+ messages in thread
From: greearb @ 2010-09-23 13:54 UTC (permalink / raw)
To: linux-wireless; +Cc: Ben Greear
From: Ben Greear <greearb@candelatech.com>
The old ieee80211_find_sta_by_hw method didn't properly
find VIFS when there was more than one per AP. This caused
AMPDU logic in ath9k to get the wrong VIF when trying to
account for transmitted SKBs.
This patch changes ieee80211_find_sta_by_hw to take a
localaddr argument to distinguish between VIFs with the
same AP but different local addresses.
Signed-off-by: Ben Greear <greearb@candelatech.com>
---
:100644 100644 c5e7af4... 9165ac8... M drivers/net/wireless/ath/ath9k/recv.c
:100644 100644 85a7323... 6543828... M drivers/net/wireless/ath/ath9k/xmit.c
:100644 100644 12a49f0... 9b25334... M include/net/mac80211.h
:100644 100644 44e10a9... f543331... M net/mac80211/sta_info.c
drivers/net/wireless/ath/ath9k/recv.c | 6 +++++-
drivers/net/wireless/ath/ath9k/xmit.c | 2 +-
include/net/mac80211.h | 14 ++++----------
net/mac80211/sta_info.c | 11 +++++++++--
4 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index c5e7af4..9165ac8 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -977,7 +977,11 @@ static void ath9k_process_rssi(struct ath_common *common,
* at least one sdata of a wiphy on mac80211 but with ath9k virtual
* wiphy you'd have to iterate over every wiphy and each sdata.
*/
- sta = ieee80211_find_sta_by_hw(hw, hdr->addr2);
+ if (is_multicast_ether_addr(hdr->addr1))
+ sta = ieee80211_find_sta_by_hw(hw, hdr->addr2, NULL);
+ else
+ sta = ieee80211_find_sta_by_hw(hw, hdr->addr2, hdr->addr1);
+
if (sta) {
an = (struct ath_node *) sta->drv_priv;
if (rx_stats->rs_rssi != ATH9K_RSSI_BAD &&
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 85a7323..6543828 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -329,7 +329,7 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
rcu_read_lock();
/* XXX: use ieee80211_find_sta! */
- sta = ieee80211_find_sta_by_hw(hw, hdr->addr1);
+ sta = ieee80211_find_sta_by_hw(hw, hdr->addr1, hdr->addr2);
if (!sta) {
rcu_read_unlock();
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 12a49f0..9b25334 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -2419,22 +2419,16 @@ struct ieee80211_sta *ieee80211_find_sta(struct ieee80211_vif *vif,
* ieee80211_find_sta_by_hw - find a station on hardware
*
* @hw: pointer as obtained from ieee80211_alloc_hw()
- * @addr: station's address
+ * @addr: remote station's address
+ * @localaddr: local address (vif->sdata->vif.addr). Use NULL for 'any'.
*
* This function must be called under RCU lock and the
* resulting pointer is only valid under RCU lock as well.
*
- * NOTE: This function should not be used! When mac80211 is converted
- * internally to properly keep track of stations on multiple
- * virtual interfaces, it will not always know which station to
- * return here since a single address might be used by multiple
- * logical stations (e.g. consider a station connecting to another
- * BSSID on the same AP hardware without disconnecting first).
- *
- * DO NOT USE THIS FUNCTION.
*/
struct ieee80211_sta *ieee80211_find_sta_by_hw(struct ieee80211_hw *hw,
- const u8 *addr);
+ const u8 *addr,
+ const u8 *localaddr);
/**
* ieee80211_sta_block_awake - block station from waking up
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 44e10a9..f543331 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -839,12 +839,19 @@ void ieee80211_sta_expire(struct ieee80211_sub_if_data *sdata,
}
struct ieee80211_sta *ieee80211_find_sta_by_hw(struct ieee80211_hw *hw,
- const u8 *addr)
+ const u8 *addr,
+ const u8 *localaddr)
{
struct sta_info *sta, *nxt;
- /* Just return a random station ... first in list ... */
+ /*
+ * Just return a random station if localaddr is NULL
+ * ... first in list.
+ */
for_each_sta_info(hw_to_local(hw), addr, sta, nxt) {
+ if (localaddr &&
+ compare_ether_addr(sta->sdata->vif.addr, localaddr) != 0)
+ continue;
if (!sta->uploaded)
return NULL;
return &sta->sta;
--
1.7.2.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mac80211/ath9k: Support AMPDU with multiple VIFs.
2010-09-23 13:54 [PATCH] mac80211/ath9k: Support AMPDU with multiple VIFs greearb
@ 2010-09-23 14:08 ` Johannes Berg
2010-09-23 16:26 ` Ben Greear
0 siblings, 1 reply; 5+ messages in thread
From: Johannes Berg @ 2010-09-23 14:08 UTC (permalink / raw)
To: greearb; +Cc: linux-wireless, Ben Greear
On Thu, 2010-09-23 at 06:54 -0700, greearb@gmail.com wrote:
> *
> - * NOTE: This function should not be used! When mac80211 is converted
> - * internally to properly keep track of stations on multiple
> - * virtual interfaces, it will not always know which station to
> - * return here since a single address might be used by multiple
> - * logical stations (e.g. consider a station connecting to another
> - * BSSID on the same AP hardware without disconnecting first).
> - *
> - * DO NOT USE THIS FUNCTION.
Also, I'm not sure you really should remove this comment, since it still
applies when you pass NULL, and I'm still not convinced that ath9k
couldn't keep track of the virtual interface accurately?
johannes
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mac80211/ath9k: Support AMPDU with multiple VIFs.
2010-09-23 14:08 ` Johannes Berg
@ 2010-09-23 16:26 ` Ben Greear
0 siblings, 0 replies; 5+ messages in thread
From: Ben Greear @ 2010-09-23 16:26 UTC (permalink / raw)
To: Johannes Berg; +Cc: greearb, linux-wireless
On 09/23/2010 07:08 AM, Johannes Berg wrote:
> On Thu, 2010-09-23 at 06:54 -0700, greearb@gmail.com wrote:
>
>> *
>> - * NOTE: This function should not be used! When mac80211 is converted
>> - * internally to properly keep track of stations on multiple
>> - * virtual interfaces, it will not always know which station to
>> - * return here since a single address might be used by multiple
>> - * logical stations (e.g. consider a station connecting to another
>> - * BSSID on the same AP hardware without disconnecting first).
>> - *
>> - * DO NOT USE THIS FUNCTION.
>
> Also, I'm not sure you really should remove this comment, since it still
> applies when you pass NULL, and I'm still not convinced that ath9k
> couldn't keep track of the virtual interface accurately?
It seems ath9k would have to selectively remove buffers from it's tx queues
with vifs dissappeared. I had a previous patch that flushed all when interfaces
came and went, but it seems I would also have to do something similar
for AP mode when remote stations came and went. And for remote stations,
Felix didn't like flushing the queues entirely, so it would have to be for
just pkts belonging to that VIF.
That would mean quite a bit of re-write of the buffer flush logic as
far as I can tell.
As for the by_hw() method, it's only used in ath9k, and the only time
it gets passed a NULL is in the ath9k_process_rssi code when the
destination address is multicast (so we cannot reliably match on
a destination interface).
I'll update the warning comment to mention about passing in NULL
as localaddr.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] mac80211/ath9k: Support AMPDU with multiple VIFs.
@ 2010-09-23 16:44 greearb
2010-09-23 18:44 ` Johannes Berg
0 siblings, 1 reply; 5+ messages in thread
From: greearb @ 2010-09-23 16:44 UTC (permalink / raw)
To: linux-wireless; +Cc: Ben Greear
From: Ben Greear <greearb@candelatech.com>
The old ieee80211_find_sta_by_hw method didn't properly
find VIFS when there was more than one per AP. This caused
AMPDU logic in ath9k to get the wrong VIF when trying to
account for transmitted SKBs.
This patch changes ieee80211_find_sta_by_hw to take a
localaddr argument to distinguish between VIFs with the
same AP but different local addresses. The method name
is changed to ieee80211_find_sta_by_ifaddr.
Signed-off-by: Ben Greear <greearb@candelatech.com>
---
:100644 100644 c5e7af4... 9cd0597... M drivers/net/wireless/ath/ath9k/recv.c
:100644 100644 85a7323... f7da6b2... M drivers/net/wireless/ath/ath9k/xmit.c
:100644 100644 12a49f0... a0afea1... M include/net/mac80211.h
:100644 100644 44e10a9... ca2cba9... M net/mac80211/sta_info.c
drivers/net/wireless/ath/ath9k/recv.c | 6 +++++-
drivers/net/wireless/ath/ath9k/xmit.c | 3 +--
include/net/mac80211.h | 25 ++++++++++++++-----------
net/mac80211/sta_info.c | 15 +++++++++++----
4 files changed, 31 insertions(+), 18 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index c5e7af4..9cd0597 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -977,7 +977,11 @@ static void ath9k_process_rssi(struct ath_common *common,
* at least one sdata of a wiphy on mac80211 but with ath9k virtual
* wiphy you'd have to iterate over every wiphy and each sdata.
*/
- sta = ieee80211_find_sta_by_hw(hw, hdr->addr2);
+ if (is_multicast_ether_addr(hdr->addr1))
+ sta = ieee80211_find_sta_by_ifaddr(hw, hdr->addr2, NULL);
+ else
+ sta = ieee80211_find_sta_by_ifaddr(hw, hdr->addr2, hdr->addr1);
+
if (sta) {
an = (struct ath_node *) sta->drv_priv;
if (rx_stats->rs_rssi != ATH9K_RSSI_BAD &&
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 85a7323..f7da6b2 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -328,8 +328,7 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
rcu_read_lock();
- /* XXX: use ieee80211_find_sta! */
- sta = ieee80211_find_sta_by_hw(hw, hdr->addr1);
+ sta = ieee80211_find_sta_by_ifaddr(hw, hdr->addr1, hdr->addr2);
if (!sta) {
rcu_read_unlock();
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 12a49f0..a0afea1 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -2416,25 +2416,28 @@ struct ieee80211_sta *ieee80211_find_sta(struct ieee80211_vif *vif,
const u8 *addr);
/**
- * ieee80211_find_sta_by_hw - find a station on hardware
+ * ieee80211_find_sta_by_ifaddr - find a station on hardware
*
* @hw: pointer as obtained from ieee80211_alloc_hw()
- * @addr: station's address
+ * @addr: remote station's address
+ * @localaddr: local address (vif->sdata->vif.addr). Use NULL for 'any'.
*
* This function must be called under RCU lock and the
* resulting pointer is only valid under RCU lock as well.
*
- * NOTE: This function should not be used! When mac80211 is converted
- * internally to properly keep track of stations on multiple
- * virtual interfaces, it will not always know which station to
- * return here since a single address might be used by multiple
- * logical stations (e.g. consider a station connecting to another
- * BSSID on the same AP hardware without disconnecting first).
+ * NOTE: You may pass NULL for localaddr, but then you will just get
+ * the first STA that matches the remote address 'addr'.
+ * We can have multiple STA associated with multiple
+ * logical stations (e.g. consider a station connecting to another
+ * BSSID on the same AP hardware without disconnecting first).
+ * In this case, the result of this method with localaddr NULL
+ * is not reliable.
*
- * DO NOT USE THIS FUNCTION.
+ * DO NOT USE THIS FUNCTION with localaddr NULL if at all possible.
*/
-struct ieee80211_sta *ieee80211_find_sta_by_hw(struct ieee80211_hw *hw,
- const u8 *addr);
+struct ieee80211_sta *ieee80211_find_sta_by_ifaddr(struct ieee80211_hw *hw,
+ const u8 *addr,
+ const u8 *localaddr);
/**
* ieee80211_sta_block_awake - block station from waking up
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 44e10a9..ca2cba9 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -838,13 +838,20 @@ void ieee80211_sta_expire(struct ieee80211_sub_if_data *sdata,
mutex_unlock(&local->sta_mtx);
}
-struct ieee80211_sta *ieee80211_find_sta_by_hw(struct ieee80211_hw *hw,
- const u8 *addr)
+struct ieee80211_sta *ieee80211_find_sta_by_ifaddr(struct ieee80211_hw *hw,
+ const u8 *addr,
+ const u8 *localaddr)
{
struct sta_info *sta, *nxt;
- /* Just return a random station ... first in list ... */
+ /*
+ * Just return a random station if localaddr is NULL
+ * ... first in list.
+ */
for_each_sta_info(hw_to_local(hw), addr, sta, nxt) {
+ if (localaddr &&
+ compare_ether_addr(sta->sdata->vif.addr, localaddr) != 0)
+ continue;
if (!sta->uploaded)
return NULL;
return &sta->sta;
@@ -852,7 +859,7 @@ struct ieee80211_sta *ieee80211_find_sta_by_hw(struct ieee80211_hw *hw,
return NULL;
}
-EXPORT_SYMBOL_GPL(ieee80211_find_sta_by_hw);
+EXPORT_SYMBOL_GPL(ieee80211_find_sta_by_ifaddr);
struct ieee80211_sta *ieee80211_find_sta(struct ieee80211_vif *vif,
const u8 *addr)
--
1.7.2.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mac80211/ath9k: Support AMPDU with multiple VIFs.
2010-09-23 16:44 greearb
@ 2010-09-23 18:44 ` Johannes Berg
0 siblings, 0 replies; 5+ messages in thread
From: Johannes Berg @ 2010-09-23 18:44 UTC (permalink / raw)
To: greearb; +Cc: linux-wireless
On Thu, 2010-09-23 at 09:44 -0700, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
>
> The old ieee80211_find_sta_by_hw method didn't properly
> find VIFS when there was more than one per AP. This caused
> AMPDU logic in ath9k to get the wrong VIF when trying to
> account for transmitted SKBs.
>
> This patch changes ieee80211_find_sta_by_hw to take a
> localaddr argument to distinguish between VIFs with the
> same AP but different local addresses. The method name
> is changed to ieee80211_find_sta_by_ifaddr.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>
Acked-by: Johannes Berg <johannes@sipsolutions.net>
> ---
> :100644 100644 c5e7af4... 9cd0597... M drivers/net/wireless/ath/ath9k/recv.c
> :100644 100644 85a7323... f7da6b2... M drivers/net/wireless/ath/ath9k/xmit.c
> :100644 100644 12a49f0... a0afea1... M include/net/mac80211.h
> :100644 100644 44e10a9... ca2cba9... M net/mac80211/sta_info.c
> drivers/net/wireless/ath/ath9k/recv.c | 6 +++++-
> drivers/net/wireless/ath/ath9k/xmit.c | 3 +--
> include/net/mac80211.h | 25 ++++++++++++++-----------
> net/mac80211/sta_info.c | 15 +++++++++++----
> 4 files changed, 31 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
> index c5e7af4..9cd0597 100644
> --- a/drivers/net/wireless/ath/ath9k/recv.c
> +++ b/drivers/net/wireless/ath/ath9k/recv.c
> @@ -977,7 +977,11 @@ static void ath9k_process_rssi(struct ath_common *common,
> * at least one sdata of a wiphy on mac80211 but with ath9k virtual
> * wiphy you'd have to iterate over every wiphy and each sdata.
> */
> - sta = ieee80211_find_sta_by_hw(hw, hdr->addr2);
> + if (is_multicast_ether_addr(hdr->addr1))
> + sta = ieee80211_find_sta_by_ifaddr(hw, hdr->addr2, NULL);
> + else
> + sta = ieee80211_find_sta_by_ifaddr(hw, hdr->addr2, hdr->addr1);
> +
> if (sta) {
> an = (struct ath_node *) sta->drv_priv;
> if (rx_stats->rs_rssi != ATH9K_RSSI_BAD &&
> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
> index 85a7323..f7da6b2 100644
> --- a/drivers/net/wireless/ath/ath9k/xmit.c
> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> @@ -328,8 +328,7 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
>
> rcu_read_lock();
>
> - /* XXX: use ieee80211_find_sta! */
> - sta = ieee80211_find_sta_by_hw(hw, hdr->addr1);
> + sta = ieee80211_find_sta_by_ifaddr(hw, hdr->addr1, hdr->addr2);
> if (!sta) {
> rcu_read_unlock();
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 12a49f0..a0afea1 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -2416,25 +2416,28 @@ struct ieee80211_sta *ieee80211_find_sta(struct ieee80211_vif *vif,
> const u8 *addr);
>
> /**
> - * ieee80211_find_sta_by_hw - find a station on hardware
> + * ieee80211_find_sta_by_ifaddr - find a station on hardware
> *
> * @hw: pointer as obtained from ieee80211_alloc_hw()
> - * @addr: station's address
> + * @addr: remote station's address
> + * @localaddr: local address (vif->sdata->vif.addr). Use NULL for 'any'.
> *
> * This function must be called under RCU lock and the
> * resulting pointer is only valid under RCU lock as well.
> *
> - * NOTE: This function should not be used! When mac80211 is converted
> - * internally to properly keep track of stations on multiple
> - * virtual interfaces, it will not always know which station to
> - * return here since a single address might be used by multiple
> - * logical stations (e.g. consider a station connecting to another
> - * BSSID on the same AP hardware without disconnecting first).
> + * NOTE: You may pass NULL for localaddr, but then you will just get
> + * the first STA that matches the remote address 'addr'.
> + * We can have multiple STA associated with multiple
> + * logical stations (e.g. consider a station connecting to another
> + * BSSID on the same AP hardware without disconnecting first).
> + * In this case, the result of this method with localaddr NULL
> + * is not reliable.
> *
> - * DO NOT USE THIS FUNCTION.
> + * DO NOT USE THIS FUNCTION with localaddr NULL if at all possible.
> */
> -struct ieee80211_sta *ieee80211_find_sta_by_hw(struct ieee80211_hw *hw,
> - const u8 *addr);
> +struct ieee80211_sta *ieee80211_find_sta_by_ifaddr(struct ieee80211_hw *hw,
> + const u8 *addr,
> + const u8 *localaddr);
>
> /**
> * ieee80211_sta_block_awake - block station from waking up
> diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
> index 44e10a9..ca2cba9 100644
> --- a/net/mac80211/sta_info.c
> +++ b/net/mac80211/sta_info.c
> @@ -838,13 +838,20 @@ void ieee80211_sta_expire(struct ieee80211_sub_if_data *sdata,
> mutex_unlock(&local->sta_mtx);
> }
>
> -struct ieee80211_sta *ieee80211_find_sta_by_hw(struct ieee80211_hw *hw,
> - const u8 *addr)
> +struct ieee80211_sta *ieee80211_find_sta_by_ifaddr(struct ieee80211_hw *hw,
> + const u8 *addr,
> + const u8 *localaddr)
> {
> struct sta_info *sta, *nxt;
>
> - /* Just return a random station ... first in list ... */
> + /*
> + * Just return a random station if localaddr is NULL
> + * ... first in list.
> + */
> for_each_sta_info(hw_to_local(hw), addr, sta, nxt) {
> + if (localaddr &&
> + compare_ether_addr(sta->sdata->vif.addr, localaddr) != 0)
> + continue;
> if (!sta->uploaded)
> return NULL;
> return &sta->sta;
> @@ -852,7 +859,7 @@ struct ieee80211_sta *ieee80211_find_sta_by_hw(struct ieee80211_hw *hw,
>
> return NULL;
> }
> -EXPORT_SYMBOL_GPL(ieee80211_find_sta_by_hw);
> +EXPORT_SYMBOL_GPL(ieee80211_find_sta_by_ifaddr);
>
> struct ieee80211_sta *ieee80211_find_sta(struct ieee80211_vif *vif,
> const u8 *addr)
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-09-23 18:44 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-09-23 13:54 [PATCH] mac80211/ath9k: Support AMPDU with multiple VIFs greearb
2010-09-23 14:08 ` Johannes Berg
2010-09-23 16:26 ` Ben Greear
-- strict thread matches above, loose matches on Subject: below --
2010-09-23 16:44 greearb
2010-09-23 18:44 ` Johannes Berg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).