ath9k, multiple stations, and AMPDUs

ath9k, multiple stations, and AMPDUs

[Ben Greear]

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:

[root@atom ~]# cat /debug/ath9k/phy0/xmit
                             BE         BK        VI        VO

MPDUs Queued:                2          0         0        85
MPDUs Completed:             2          0         0        85
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
[root@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 <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs

[Felix Fietkau]

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

Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs

[Ben Greear]

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 <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs

[Felix Fietkau]

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

Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs

[Johannes Berg]

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

Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs

[Johannes Berg]

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

Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs

[Ben Greear]

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 <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs

[Felix Fietkau]

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

Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs

[Ben Greear]

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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs

[Felix Fietkau]

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

Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs

[Johannes Berg]

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

Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs

[Felix Fietkau]

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

Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs

[Ben Greear]

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 <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs

[Felix Fietkau]

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

Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs

[Ben Greear]

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 <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs

[Felix Fietkau]

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

Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs

[Ben Greear]

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 <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs

[Johannes Berg]

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

Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs

[Ben Greear]

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 <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs

[Johannes Berg]

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