All of lore.kernel.org
 help / color / mirror / Atom feed
From: Felix Fietkau <nbd@openwrt.org>
To: ath9k-devel@lists.ath9k.org
Subject: [ath9k-devel] ath9k, multiple stations, and AMPDUs
Date: Wed, 22 Sep 2010 10:31:56 +0200	[thread overview]
Message-ID: <4C99BEFC.2060209@openwrt.org> (raw)
In-Reply-To: <4C99870C.20006@candelatech.com>

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

WARNING: multiple messages have this Message-ID (diff)
From: Felix Fietkau <nbd@openwrt.org>
To: Ben Greear <greearb@candelatech.com>
Cc: Johannes Berg <johannes@sipsolutions.net>,
	"ath9k-devel@lists.ath9k.org" <ath9k-devel@venema.h4ckr.net>,
	"linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Subject: Re: [ath9k-devel] ath9k, multiple stations, and AMPDUs
Date: Wed, 22 Sep 2010 10:31:56 +0200	[thread overview]
Message-ID: <4C99BEFC.2060209@openwrt.org> (raw)
In-Reply-To: <4C99870C.20006@candelatech.com>

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

  reply	other threads:[~2010-09-22  8:31 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-09-21  5:25 [ath9k-devel] ath9k, multiple stations, and AMPDUs Ben Greear
2010-09-21  5:25 ` Ben Greear
2010-09-21 10:10 ` [ath9k-devel] " Felix Fietkau
2010-09-21 10:10   ` Felix Fietkau
2010-09-21 12:08   ` Ben Greear
2010-09-21 12:08     ` Ben Greear
2010-09-21 12:19     ` Felix Fietkau
2010-09-21 12:19       ` Felix Fietkau
2010-09-21 12:24       ` Johannes Berg
2010-09-21 12:24         ` Johannes Berg
2010-09-21 12:31       ` Johannes Berg
2010-09-21 12:31         ` Johannes Berg
2010-09-21 17:25       ` Ben Greear
2010-09-21 17:25         ` Ben Greear
2010-09-21 18:00         ` Felix Fietkau
2010-09-21 18:00           ` Felix Fietkau
2010-09-21 18:04           ` Ben Greear
2010-09-21 18:04             ` Ben Greear
2010-09-21 18:06             ` Felix Fietkau
2010-09-21 18:06               ` Felix Fietkau
2010-09-21 19:28           ` Johannes Berg
2010-09-21 19:28             ` Johannes Berg
2010-09-21 19:32             ` Felix Fietkau
2010-09-21 19:32               ` Felix Fietkau
2010-09-21 20:19               ` Ben Greear
2010-09-21 20:19                 ` Ben Greear
2010-09-21 22:41                 ` Felix Fietkau
2010-09-21 22:41                   ` Felix Fietkau
2010-09-22  4:33                   ` Ben Greear
2010-09-22  4:33                     ` Ben Greear
2010-09-22  8:31                     ` Felix Fietkau [this message]
2010-09-22  8:31                       ` Felix Fietkau
2010-09-23  4:58                       ` Ben Greear
2010-09-23  4:58                         ` Ben Greear
2010-09-23  8:33                         ` Johannes Berg
2010-09-23  8:33                           ` Johannes Berg
2010-09-23 13:56                           ` Ben Greear
2010-09-23 13:56                             ` Ben Greear
2010-09-23 14:05                             ` Johannes Berg
2010-09-23 14:05                               ` Johannes Berg

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4C99BEFC.2060209@openwrt.org \
    --to=nbd@openwrt.org \
    --cc=ath9k-devel@lists.ath9k.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.