[ath9k-devel] ath9k, multiple stations, and AMPDUs
I think I have narrowed down some problems I see when I create two STA interfaces on the same radio and associate with the same AP. I'm using wireless-testing plus some patches to the rx logic I posted earlier. It appears that the ath9k NIC does not transport AMPDUs properly, and after a few seconds, it's backlog is exhausted and the queues are stopped in ath_tx_start. debugfs seems to agree: [r...@atom ~]# cat /debug/ath9k/phy0/xmit BE BKVIVO MPDUs Queued:2 0 085 MPDUs Completed: 2 0 085 Aggregates: 0 0 0 0 AMPDUs Queued: 144 0 0 0 AMPDUs Completed: 20 0 0 0 AMPDUs Retried: 18 0 0 0 AMPDUs XRetried: 0 0 0 0 FIFO Underrun: 0 0 0 0 TXOP Exceeded: 0 0 0 0 TXTIMER Expiry: 0 0 0 0 DESC CFG Error: 0 0 0 0 DATA Underrun: 0 0 0 0 DELIM Underrun: 0 0 0 0 [r...@atom ~]# I can still receive packets on the interface, but nothing is transmitted. I tested against an a/b/g AP (instead of N), and it ran clean for many minutes. Any ideas how to debug this further? Thanks, Ben -- Ben Greear Candela Technologies Inc http://www.candelatech.com ___ ath9k-devel mailing list ath9k-devel@lists.ath9k.org https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs
On 2010-09-21 7:25 AM, Ben Greear wrote: > I think I have narrowed down some problems I see when I create > two STA interfaces on the same radio and associate with the > same AP. > > I'm using wireless-testing plus some patches to the rx logic > I posted earlier. > > It appears that the ath9k NIC does not transport AMPDUs properly, > and after a few seconds, it's backlog is exhausted and the queues > are stopped in ath_tx_start. ath9k currently looks up the sta by hw and not by vif on tx completion. That completely breaks aggregation in such a setup. Unfortunately I don't know an easy way to fix this yet. This will be fixed by my sw aggregation rewrite, but I don't know when I'll get that completed yet. - Felix ___ ath9k-devel mailing list ath9k-devel@lists.ath9k.org https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs
On 09/21/2010 03:10 AM, Felix Fietkau wrote: > On 2010-09-21 7:25 AM, Ben Greear wrote: >> I think I have narrowed down some problems I see when I create >> two STA interfaces on the same radio and associate with the >> same AP. >> >> I'm using wireless-testing plus some patches to the rx logic >> I posted earlier. >> >> It appears that the ath9k NIC does not transport AMPDUs properly, >> and after a few seconds, it's backlog is exhausted and the queues >> are stopped in ath_tx_start. > ath9k currently looks up the sta by hw and not by vif on tx completion. > That completely breaks aggregation in such a setup. Unfortunately I > don't know an easy way to fix this yet. > > This will be fixed by my sw aggregation rewrite, but I don't know when > I'll get that completed yet. If you have any more details on this, please let me know. I'm going to attempt to fix it...I certainly have a good test case :) Ben -- Ben Greear Candela Technologies Inc http://www.candelatech.com ___ ath9k-devel mailing list ath9k-devel@lists.ath9k.org https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs
On 2010-09-21 2:08 PM, Ben Greear wrote: > On 09/21/2010 03:10 AM, Felix Fietkau wrote: >> On 2010-09-21 7:25 AM, Ben Greear wrote: >>> I think I have narrowed down some problems I see when I create >>> two STA interfaces on the same radio and associate with the >>> same AP. >>> >>> I'm using wireless-testing plus some patches to the rx logic >>> I posted earlier. >>> >>> It appears that the ath9k NIC does not transport AMPDUs properly, >>> and after a few seconds, it's backlog is exhausted and the queues >>> are stopped in ath_tx_start. >> ath9k currently looks up the sta by hw and not by vif on tx completion. >> That completely breaks aggregation in such a setup. Unfortunately I >> don't know an easy way to fix this yet. >> >> This will be fixed by my sw aggregation rewrite, but I don't know when >> I'll get that completed yet. > > If you have any more details on this, please let me know. I'm going to > attempt to fix it...I certainly have a good test case :) ath_tx_complete_aggr completes an A-MPDU frame, which typically triggers the release of the next A-MPDU to the hw queue. To keep track of the Block ACK window, it needs to look up the TID, for which it needs a STA pointer. At that level, the driver typically doesn't have access to the vif. It might be possible to fix this by adding another sta lookup helper function in mac80211 that takes another address argument for the BSSID, so that it can get the sta entry for the correct vif. I don't know if Johannes wants something like that though. - Felix ___ ath9k-devel mailing list ath9k-devel@lists.ath9k.org https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs
On Tue, 2010-09-21 at 14:19 +0200, Felix Fietkau wrote: > To keep track of the Block ACK window, it needs to look up the TID, for > which it needs a STA pointer. At that level, the driver typically > doesn't have access to the vif. > > It might be possible to fix this by adding another sta lookup helper > function in mac80211 that takes another address argument for the BSSID, > so that it can get the sta entry for the correct vif. I don't know if > Johannes wants something like that though. Seems simple enough, and if it fixes things ... for iwlwifi I fixed the driver to keep track of the vif properly, though more work it does make a lot more things better than just this. johannes ___ ath9k-devel mailing list ath9k-devel@lists.ath9k.org https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs
On Tue, 2010-09-21 at 14:19 +0200, Felix Fietkau wrote: > It might be possible to fix this by adding another sta lookup helper > function in mac80211 that takes another address argument for the BSSID, > so that it can get the sta entry for the correct vif. I don't know if > Johannes wants something like that though. Actually, you want the RA, not the BSSID. johannes ___ ath9k-devel mailing list ath9k-devel@lists.ath9k.org https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs
On 09/21/2010 05:19 AM, Felix Fietkau wrote: > On 2010-09-21 2:08 PM, Ben Greear wrote: >> If you have any more details on this, please let me know. I'm going to >> attempt to fix it...I certainly have a good test case :) > ath_tx_complete_aggr completes an A-MPDU frame, which typically triggers > the release of the next A-MPDU to the hw queue. > To keep track of the Block ACK window, it needs to look up the TID, for > which it needs a STA pointer. At that level, the driver typically > doesn't have access to the vif. > > It might be possible to fix this by adding another sta lookup helper > function in mac80211 that takes another address argument for the BSSID, > so that it can get the sta entry for the correct vif. I don't know if > Johannes wants something like that though. Could we just poke a pointer to the STA into the ath_buf structure? Thanks, Ben -- Ben Greear Candela Technologies Inc http://www.candelatech.com ___ ath9k-devel mailing list ath9k-devel@lists.ath9k.org https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs
On 2010-09-21 7:25 PM, Ben Greear wrote: > On 09/21/2010 05:19 AM, Felix Fietkau wrote: >> On 2010-09-21 2:08 PM, Ben Greear wrote: > >>> If you have any more details on this, please let me know. I'm going to >>> attempt to fix it...I certainly have a good test case :) >> ath_tx_complete_aggr completes an A-MPDU frame, which typically triggers >> the release of the next A-MPDU to the hw queue. >> To keep track of the Block ACK window, it needs to look up the TID, for >> which it needs a STA pointer. At that level, the driver typically >> doesn't have access to the vif. >> >> It might be possible to fix this by adding another sta lookup helper >> function in mac80211 that takes another address argument for the BSSID, >> so that it can get the sta entry for the correct vif. I don't know if >> Johannes wants something like that though. > > Could we just poke a pointer to the STA into the ath_buf structure? No, that doesn't work because of RCU. - Felix ___ ath9k-devel mailing list ath9k-devel@lists.ath9k.org https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs
On 09/21/2010 11:00 AM, Felix Fietkau wrote: > On 2010-09-21 7:25 PM, Ben Greear wrote: >> On 09/21/2010 05:19 AM, Felix Fietkau wrote: >>> On 2010-09-21 2:08 PM, Ben Greear wrote: >> If you have any more details on this, please let me know. I'm going to attempt to fix it...I certainly have a good test case :) >>> ath_tx_complete_aggr completes an A-MPDU frame, which typically triggers >>> the release of the next A-MPDU to the hw queue. >>> To keep track of the Block ACK window, it needs to look up the TID, for >>> which it needs a STA pointer. At that level, the driver typically >>> doesn't have access to the vif. >>> >>> It might be possible to fix this by adding another sta lookup helper >>> function in mac80211 that takes another address argument for the BSSID, >>> so that it can get the sta entry for the correct vif. I don't know if >>> Johannes wants something like that though. >> >> Could we just poke a pointer to the STA into the ath_buf structure? > No, that doesn't work because of RCU. Would it also be bad to use skb->dev to find an STA? Thanks, Ben > > - Felix > -- > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Ben Greear Candela Technologies Inc http://www.candelatech.com ___ ath9k-devel mailing list ath9k-devel@lists.ath9k.org https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs
On 2010-09-21 8:04 PM, Ben Greear wrote: > On 09/21/2010 11:00 AM, Felix Fietkau wrote: >> On 2010-09-21 7:25 PM, Ben Greear wrote: >>> On 09/21/2010 05:19 AM, Felix Fietkau wrote: On 2010-09-21 2:08 PM, Ben Greear wrote: >>> > If you have any more details on this, please let me know. I'm going to > attempt to fix it...I certainly have a good test case :) ath_tx_complete_aggr completes an A-MPDU frame, which typically triggers the release of the next A-MPDU to the hw queue. To keep track of the Block ACK window, it needs to look up the TID, for which it needs a STA pointer. At that level, the driver typically doesn't have access to the vif. It might be possible to fix this by adding another sta lookup helper function in mac80211 that takes another address argument for the BSSID, so that it can get the sta entry for the correct vif. I don't know if Johannes wants something like that though. >>> >>> Could we just poke a pointer to the STA into the ath_buf structure? >> No, that doesn't work because of RCU. > > Would it also be bad to use skb->dev to find an STA? It would be bad to keep a STA pointer around anywhere in the skb or the ath_buf. You can only use it within a rcu_read_lock()/rcu_read_unlock() pair. - Felix ___ ath9k-devel mailing list ath9k-devel@lists.ath9k.org https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs
On Tue, 2010-09-21 at 20:00 +0200, Felix Fietkau wrote: > > Could we just poke a pointer to the STA into the ath_buf structure? > No, that doesn't work because of RCU. Well, it could work, if you walk all the structures upon sta_notify and remove now stale pointers (or just drop the frames or something). johannes ___ ath9k-devel mailing list ath9k-devel@lists.ath9k.org https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs
On 2010-09-21 9:28 PM, Johannes Berg wrote: > On Tue, 2010-09-21 at 20:00 +0200, Felix Fietkau wrote: > >> > Could we just poke a pointer to the STA into the ath_buf structure? > >> No, that doesn't work because of RCU. > > Well, it could work, if you walk all the structures upon sta_notify and > remove now stale pointers (or just drop the frames or something). I think it would be much better to just add the helper function that checks the RA on STA lookup. Keeps things simple, especially since nothing else in the tx path needs the vif. - Felix ___ ath9k-devel mailing list ath9k-devel@lists.ath9k.org https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs
On 09/21/2010 12:32 PM, Felix Fietkau wrote:
> On 2010-09-21 9:28 PM, Johannes Berg wrote:
>> On Tue, 2010-09-21 at 20:00 +0200, Felix Fietkau wrote:
>>
Could we just poke a pointer to the STA into the ath_buf structure?
>>
>>> No, that doesn't work because of RCU.
>>
>> Well, it could work, if you walk all the structures upon sta_notify and
>> remove now stale pointers (or just drop the frames or something).
> I think it would be much better to just add the helper function that
> checks the RA on STA lookup. Keeps things simple, especially since
> nothing else in the tx path needs the vif.
How about this. Seems to do the trick on my system:
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c
b/drivers/net/wireless/ath/ath9k/xmit.c
index 85a7323..09815a1 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 = tx_info->control.sta;
if (!sta) {
rcu_read_unlock();
Thanks,
Ben
--
Ben Greear
Candela Technologies Inc http://www.candelatech.com
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs
On 2010-09-21 10:19 PM, Ben Greear wrote: > On 09/21/2010 12:32 PM, Felix Fietkau wrote: >> On 2010-09-21 9:28 PM, Johannes Berg wrote: >>> On Tue, 2010-09-21 at 20:00 +0200, Felix Fietkau wrote: >>> > Could we just poke a pointer to the STA into the ath_buf structure? >>> No, that doesn't work because of RCU. >>> >>> Well, it could work, if you walk all the structures upon sta_notify and >>> remove now stale pointers (or just drop the frames or something). >> I think it would be much better to just add the helper function that >> checks the RA on STA lookup. Keeps things simple, especially since >> nothing else in the tx path needs the vif. > > How about this. Seems to do the trick on my system: > > > diff --git a/drivers/net/wireless/ath/ath9k/xmit.c > b/drivers/net/wireless/ath/ath9k/xmit.c > index 85a7323..09815a1 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 = tx_info->control.sta; As I mentioned in another email: at the time we get the tx status report, we have to consider the sta pointer stale. It may or may not still be valid. - Felix ___ ath9k-devel mailing list ath9k-devel@lists.ath9k.org https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs
On 09/21/2010 03:41 PM, Felix Fietkau wrote:
> On 2010-09-21 10:19 PM, Ben Greear wrote:
>> On 09/21/2010 12:32 PM, Felix Fietkau wrote:
>>> On 2010-09-21 9:28 PM, Johannes Berg wrote:
On Tue, 2010-09-21 at 20:00 +0200, Felix Fietkau wrote:
>> Could we just poke a pointer to the STA into the ath_buf structure?
> No, that doesn't work because of RCU.
Well, it could work, if you walk all the structures upon sta_notify and
remove now stale pointers (or just drop the frames or something).
>>> I think it would be much better to just add the helper function that
>>> checks the RA on STA lookup. Keeps things simple, especially since
>>> nothing else in the tx path needs the vif.
>>
>> How about this. Seems to do the trick on my system:
>>
>>
>> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c
>> b/drivers/net/wireless/ath/ath9k/xmit.c
>> index 85a7323..09815a1 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 = tx_info->control.sta;
> As I mentioned in another email: at the time we get the tx status
> report, we have to consider the sta pointer stale. It may or may not
> still be valid.
How about this one. I think it ensures that the sta will never be stale,
since it flushes the tx queue on vif removal. Minimal testing shows it
working, but of course I might be missing something.
diff --git a/drivers/net/wireless/ath/ath9k/main.c
b/drivers/net/wireless/ath/ath9k/main.c
index 8b327bc..8485729 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1459,6 +1459,12 @@ static void ath9k_remove_interface(struct ieee80211_hw
*hw,
mutex_lock(&sc->mutex);
+ /* Make sure we have no outstanding packets that might reference
+* the vif. This way, we can always reference tx_info->control.sta
+* in the tx_complete logic.
+*/
+ ath_drain_all_txq(sc, false);
+
/* Stop ANI */
sc->sc_flags &= ~SC_OP_ANI_RUN;
del_timer_sync(&common->ani.timer);
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c
b/drivers/net/wireless/ath/ath9k/xmit.c
index 85a7323..09815a1 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 = tx_info->control.sta;
if (!sta) {
rcu_read_unlock();
Thanks,
Ben
>
> - Felix
--
Ben Greear
Candela Technologies Inc http://www.candelatech.com
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs
On 2010-09-22 6:33 AM, Ben Greear wrote: > On 09/21/2010 03:41 PM, Felix Fietkau wrote: >> On 2010-09-21 10:19 PM, Ben Greear wrote: >>> On 09/21/2010 12:32 PM, Felix Fietkau wrote: On 2010-09-21 9:28 PM, Johannes Berg wrote: > On Tue, 2010-09-21 at 20:00 +0200, Felix Fietkau wrote: > >>> Could we just poke a pointer to the STA into the ath_buf structure? > >> No, that doesn't work because of RCU. > > Well, it could work, if you walk all the structures upon sta_notify and > remove now stale pointers (or just drop the frames or something). I think it would be much better to just add the helper function that checks the RA on STA lookup. Keeps things simple, especially since nothing else in the tx path needs the vif. >>> >>> How about this. Seems to do the trick on my system: >>> >>> >>> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c >>> b/drivers/net/wireless/ath/ath9k/xmit.c >>> index 85a7323..09815a1 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 = tx_info->control.sta; >> As I mentioned in another email: at the time we get the tx status >> report, we have to consider the sta pointer stale. It may or may not >> still be valid. > > How about this one. I think it ensures that the sta will never be stale, No, it doesn't. At least not in AP mode. > since it flushes the tx queue on vif removal. Minimal testing shows it > working, but of course I might be missing something. In AP mode, a vif has multiple sta. And draining the queue when a sta gets removed is not a good idea. - Felix ___ ath9k-devel mailing list ath9k-devel@lists.ath9k.org https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs
On 09/22/2010 01:31 AM, Felix Fietkau wrote:
> On 2010-09-22 6:33 AM, Ben Greear wrote:
>> On 09/21/2010 03:41 PM, Felix Fietkau wrote:
>>> As I mentioned in another email: at the time we get the tx status
>>> report, we have to consider the sta pointer stale. It may or may not
>>> still be valid.
>>
>> How about this one. I think it ensures that the sta will never be stale,
> No, it doesn't. At least not in AP mode.
>
>> since it flushes the tx queue on vif removal. Minimal testing shows it
>> working, but of course I might be missing something.
> In AP mode, a vif has multiple sta. And draining the queue when a sta
> gets removed is not a good idea.
Ok, here's another try. It does a lookup each time a tx is completed,
so it's not exactly efficient..
I think it might fail the lookup if NIC manages to change it's MAC
while pkts are in the queue, but at least it mostly works.
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..1b2840f 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -2419,7 +2419,8 @@ 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
+ * @addr2: local address (vif->sdata->dev->dev_addr). Can be NULL for 'any'.
*
* This function must be called under RCU lock and the
* resulting pointer is only valid under RCU lock as well.
@@ -2434,7 +2435,8 @@ struct ieee80211_sta *ieee80211_find_sta(struct
ieee80211_vif *vif,
* 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..1ba56a8 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -839,12 +839,18 @@ 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 &&
+ memcmp(sta->sdata->dev->dev_addr, localaddr, ETH_ALEN) != 0)
+ continue;
if (!sta->uploaded)
return NULL;
return &sta->sta;
--
Ben Greear
Candela Technologies Inc http://www.candelatech.com
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs
On Wed, 2010-09-22 at 21:58 -0700, Ben Greear wrote:
> + * @addr2: local address (vif->sdata->dev->dev_addr). Can be NULL for 'any'.
>*
>* This function must be called under RCU lock and the
>* resulting pointer is only valid under RCU lock as well.
> @@ -2434,7 +2435,8 @@ struct ieee80211_sta *ieee80211_find_sta(struct
> ieee80211_vif *vif,
>* 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);
That's not going to generate proper documentation. Also, it now probably
shouldn't be called "by_hw" any more.
> - /* Just return a random station ... first in list ... */
> + /* Just return a random station if localaddr is NULL
> +* ... first in list.
> +*/
Please use the comment style like this:
/*
* Just return a random station ...
*/
I know davem personally doesn't like it, but it's the regular style in
this code.
> for_each_sta_info(hw_to_local(hw), addr, sta, nxt) {
> + if (localaddr &&
> + memcmp(sta->sdata->dev->dev_addr, localaddr, ETH_ALEN) !=
> 0)
> + continue;
This should use compare_ether_addr() (if it's not aligned on a two-byte
boundary something is seriously wrong), and also sta->sdata->vif.addr
instead of dev->dev_addr.
johannes
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs
On 09/23/2010 01:33 AM, Johannes Berg wrote: > On Wed, 2010-09-22 at 21:58 -0700, Ben Greear wrote: > > >> + * @addr2: local address (vif->sdata->dev->dev_addr). Can be NULL for >> 'any'. >> * >> * This function must be called under RCU lock and the >> * resulting pointer is only valid under RCU lock as well. >> @@ -2434,7 +2435,8 @@ struct ieee80211_sta *ieee80211_find_sta(struct >> ieee80211_vif *vif, >> * 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); > > That's not going to generate proper documentation. Also, it now probably > shouldn't be called "by_hw" any more. I can't think of a better name, and it still does take the hw struct, but if you have a suggestion you like better, I will use it. I re-posted with the rest of the changes you suggested. Seems to be working fine. Thanks, Ben -- Ben Greear Candela Technologies Inc http://www.candelatech.com ___ ath9k-devel mailing list ath9k-devel@lists.ath9k.org https://lists.ath9k.org/mailman/listinfo/ath9k-devel
Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs
On Thu, 2010-09-23 at 06:56 -0700, Ben Greear wrote: > On 09/23/2010 01:33 AM, Johannes Berg wrote: > > On Wed, 2010-09-22 at 21:58 -0700, Ben Greear wrote: > > > > > >> + * @addr2: local address (vif->sdata->dev->dev_addr). Can be NULL for > >> 'any'. > >> * > >> * This function must be called under RCU lock and the > >> * resulting pointer is only valid under RCU lock as well. > >> @@ -2434,7 +2435,8 @@ struct ieee80211_sta *ieee80211_find_sta(struct > >> ieee80211_vif *vif, > >> * 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); > > > > That's not going to generate proper documentation. Also, it now probably > > shouldn't be called "by_hw" any more. > > I can't think of a better name, and it still does take the hw struct, > but if you have a suggestion you like better, I will use it. Yeah, but it doesn't use the hw struct for matching (well, not necessarily anyway) ... so something like _by_ifaddr() would be better? johannes ___ ath9k-devel mailing list ath9k-devel@lists.ath9k.org https://lists.ath9k.org/mailman/listinfo/ath9k-devel
