2015-11-03 17:37:45

by Borja Salazar

Subject: [PATCH] ath9k: incorrect queue may be stopped/awaken

When channel context is enabled, we could be
stopping/awakening an incorrect queue.
---
drivers/net/wireless/ath/ath9k/xmit.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 3e3dac3..9b17a59 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -147,7 +147,12 @@ static void ath_txq_skb_done(struct ath_softc *sc, struct ath_txq *txq,
{
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
struct ath_frame_info *fi = get_frame_info(skb);
- int q = fi->txq;
+ int q;
+
+ if (ath9k_is_chanctx_enabled())
+ q = fi->txq;
+ else
+ q = info->hw_queue;

if (q < 0)
return;
@@ -158,10 +163,7 @@ static void ath_txq_skb_done(struct ath_softc *sc, struct ath_txq *txq,

if (txq->stopped &&
txq->pending_frames < sc->tx.txq_max_pending[q]) {
- if (ath9k_is_chanctx_enabled())
- ieee80211_wake_queue(sc->hw, info->hw_queue);
- else
- ieee80211_wake_queue(sc->hw, q);
+ ieee80211_wake_queue(sc->hw, q);
txq->stopped = false;
}
}
@@ -2299,17 +2301,17 @@ int ath_tx_start(struct ieee80211_hw *hw, struct sk_buff *skb,
* info are no longer valid (overwritten by the ath_frame_info data.
*/

- q = skb_get_queue_mapping(skb);
+ if (ath9k_is_chanctx_enabled())
+ q = skb_get_queue_mapping(skb);
+ else
+ q = info->hw_queue;

ath_txq_lock(sc, txq);
if (txq == sc->tx.txq_map[q]) {
fi->txq = q;
if (++txq->pending_frames > sc->tx.txq_max_pending[q] &&
!txq->stopped) {
- if (ath9k_is_chanctx_enabled())
- ieee80211_stop_queue(sc->hw, info->hw_queue);
- else
- ieee80211_stop_queue(sc->hw, q);
+ ieee80211_stop_queue(sc->hw, q);
txq->stopped = true;
}
}
--
2.3.6

2015-11-13 10:33:28

by Borja Salazar

Subject: [PATCH v2] ath9k: incorrect queue may be stopped/awaken

When channel context is enabled, we could be
stopping/awakening an incorrect queue.

Signed-off-by: Borja Salazar <[email protected]>
---
drivers/net/wireless/ath/ath9k/xmit.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 3e3dac3..9b17a59 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -147,7 +147,12 @@ static void ath_txq_skb_done(struct ath_softc *sc, struct ath_txq *txq,
{
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
struct ath_frame_info *fi = get_frame_info(skb);
- int q = fi->txq;
+ int q;
+
+ if (ath9k_is_chanctx_enabled())
+ q = fi->txq;
+ else
+ q = info->hw_queue;

if (q < 0)
return;
@@ -158,10 +163,7 @@ static void ath_txq_skb_done(struct ath_softc *sc, struct ath_txq *txq,

if (txq->stopped &&
txq->pending_frames < sc->tx.txq_max_pending[q]) {
- if (ath9k_is_chanctx_enabled())
- ieee80211_wake_queue(sc->hw, info->hw_queue);
- else
- ieee80211_wake_queue(sc->hw, q);
+ ieee80211_wake_queue(sc->hw, q);
txq->stopped = false;
}
}
@@ -2299,17 +2301,17 @@ int ath_tx_start(struct ieee80211_hw *hw, struct sk_buff *skb,
* info are no longer valid (overwritten by the ath_frame_info data.
*/

- q = skb_get_queue_mapping(skb);
+ if (ath9k_is_chanctx_enabled())
+ q = skb_get_queue_mapping(skb);
+ else
+ q = info->hw_queue;

ath_txq_lock(sc, txq);
if (txq == sc->tx.txq_map[q]) {
fi->txq = q;
if (++txq->pending_frames > sc->tx.txq_max_pending[q] &&
!txq->stopped) {
- if (ath9k_is_chanctx_enabled())
- ieee80211_stop_queue(sc->hw, info->hw_queue);
- else
- ieee80211_stop_queue(sc->hw, q);
+ ieee80211_stop_queue(sc->hw, q);
txq->stopped = true;
}
}
--
2.3.6

2015-11-13 06:31:08

by Janusz Dziedzic

Subject: Re: [PATCH] ath9k: incorrect queue may be stopped/awaken

On 3 November 2015 at 18:37, Borja Salazar <[email protected]> wrote:
> When channel context is enabled, we could be
> stopping/awakening an incorrect queue.
> ---
> drivers/net/wireless/ath/ath9k/xmit.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
> index 3e3dac3..9b17a59 100644
> --- a/drivers/net/wireless/ath/ath9k/xmit.c
> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> @@ -147,7 +147,12 @@ static void ath_txq_skb_done(struct ath_softc *sc, struct ath_txq *txq,
> {
> struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> struct ath_frame_info *fi = get_frame_info(skb);
> - int q = fi->txq;
> + int q;
> +
> + if (ath9k_is_chanctx_enabled())
> + q = fi->txq;
> + else
> + q = info->hw_queue;
>
> if (q < 0)
> return;
> @@ -158,10 +163,7 @@ static void ath_txq_skb_done(struct ath_softc *sc, struct ath_txq *txq,
>
> if (txq->stopped &&
> txq->pending_frames < sc->tx.txq_max_pending[q]) {
> - if (ath9k_is_chanctx_enabled())
> - ieee80211_wake_queue(sc->hw, info->hw_queue);
> - else
> - ieee80211_wake_queue(sc->hw, q);
> + ieee80211_wake_queue(sc->hw, q);
> txq->stopped = false;
> }
> }
> @@ -2299,17 +2301,17 @@ int ath_tx_start(struct ieee80211_hw *hw, struct sk_buff *skb,
> * info are no longer valid (overwritten by the ath_frame_info data.
> */
>
> - q = skb_get_queue_mapping(skb);
> + if (ath9k_is_chanctx_enabled())
> + q = skb_get_queue_mapping(skb);
> + else
> + q = info->hw_queue;
>
> ath_txq_lock(sc, txq);
> if (txq == sc->tx.txq_map[q]) {
> fi->txq = q;
> if (++txq->pending_frames > sc->tx.txq_max_pending[q] &&
> !txq->stopped) {
> - if (ath9k_is_chanctx_enabled())
> - ieee80211_stop_queue(sc->hw, info->hw_queue);
> - else
> - ieee80211_stop_queue(sc->hw, q);
> + ieee80211_stop_queue(sc->hw, q);
> txq->stopped = true;
> }
> }
> --
> 2.3.6
>
Hello, could you check it again?
I see such patch that enable hw_queues only for MCC and disable for
non-MCC mode.

ath9k: Enable HW queue control only for MCC

Enabling HW queue control for normal (non-mcc) mode
causes problems with queue management, resulting
in traffic stall. Since it is mainly required for
fairness in MCC mode, disable it for the general case.

Bug: https://dev.openwrt.org/ticket/18164

BR
Janusz

2015-11-12 19:58:00

by Kalle Valo

Subject: Re: [PATCH] ath9k: incorrect queue may be stopped/awaken

Borja Salazar <[email protected]> writes:

> When channel context is enabled, we could be
> stopping/awakening an incorrect queue.

Signed-off-by line is missing, I can't take this.

--
Kalle Valo

2015-11-13 10:49:14

by Borja Salazar

Subject: Re: [PATCH v2] ath9k: incorrect queue may be stopped/awaken

Hi,

Let me explain the patch more thoroughly. We are testing MCC and we've
found some issues with the queues handlers, the main problem is that
when we transmit a multicast/broadcast frame, where hw_queue is 8
(CAB), we check queue status, which is 2(q), and if it is full and we
have to stop it, we end stopping the wrong queue, 8, which is not
full. From this point onwards stations are unable to connect to the
AP.

Let me know if something is not clear.

Regards,


On Fri, Nov 13, 2015 at 11:33 AM, Borja Salazar <[email protected]> wrote:
> When channel context is enabled, we could be
> stopping/awakening an incorrect queue.
>
> Signed-off-by: Borja Salazar <[email protected]>
> ---
> drivers/net/wireless/ath/ath9k/xmit.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
> index 3e3dac3..9b17a59 100644
> --- a/drivers/net/wireless/ath/ath9k/xmit.c
> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> @@ -147,7 +147,12 @@ static void ath_txq_skb_done(struct ath_softc *sc, struct ath_txq *txq,
> {
> struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> struct ath_frame_info *fi = get_frame_info(skb);
> - int q = fi->txq;
> + int q;
> +
> + if (ath9k_is_chanctx_enabled())
> + q = fi->txq;
> + else
> + q = info->hw_queue;
>
> if (q < 0)
> return;
> @@ -158,10 +163,7 @@ static void ath_txq_skb_done(struct ath_softc *sc, struct ath_txq *txq,
>
> if (txq->stopped &&
> txq->pending_frames < sc->tx.txq_max_pending[q]) {
> - if (ath9k_is_chanctx_enabled())
> - ieee80211_wake_queue(sc->hw, info->hw_queue);
> - else
> - ieee80211_wake_queue(sc->hw, q);
> + ieee80211_wake_queue(sc->hw, q);
> txq->stopped = false;
> }
> }
> @@ -2299,17 +2301,17 @@ int ath_tx_start(struct ieee80211_hw *hw, struct sk_buff *skb,
> * info are no longer valid (overwritten by the ath_frame_info data.
> */
>
> - q = skb_get_queue_mapping(skb);
> + if (ath9k_is_chanctx_enabled())
> + q = skb_get_queue_mapping(skb);
> + else
> + q = info->hw_queue;
>
> ath_txq_lock(sc, txq);
> if (txq == sc->tx.txq_map[q]) {
> fi->txq = q;
> if (++txq->pending_frames > sc->tx.txq_max_pending[q] &&
> !txq->stopped) {
> - if (ath9k_is_chanctx_enabled())
> - ieee80211_stop_queue(sc->hw, info->hw_queue);
> - else
> - ieee80211_stop_queue(sc->hw, q);
> + ieee80211_stop_queue(sc->hw, q);
> txq->stopped = true;
> }
> }
> --
> 2.3.6
>

2015-11-13 11:37:28

by Kalle Valo

Subject: Re: [PATCH v2] ath9k: incorrect queue may be stopped/awaken

Borja Salazar <[email protected]> writes:

> Let me explain the patch more thoroughly. We are testing MCC and we've
> found some issues with the queues handlers, the main problem is that
> when we transmit a multicast/broadcast frame, where hw_queue is 8
> (CAB), we check queue status, which is 2(q), and if it is full and we
> have to stop it, we end stopping the wrong queue, 8, which is not
> full. From this point onwards stations are unable to connect to the
> AP.

This information should be in the commit log.

--
Kalle Valo