2010-01-21 04:52:09

by Bob Copeland

[permalink] [raw]
Subject: [PATCH 0/2] ath5k AP mode fixes

Here are a couple of fixes to ath5k AP mode that have been
kicking around for a bit.

The sleep clock is disabled by legacy HAL for AP mode and at
least one person had beaconing issues as a result of our not
also doing so.

Although we decided 'fix setup for CAB queue' was not ideal,
there's no alternative at the moment and the current setup is
causing problems for people, so IMO we should just take this
patch as-is for now.

Bob Copeland (2):
ath5k: dont use external sleep clock in AP mode
ath5k: fix setup for CAB queue

drivers/net/wireless/ath/ath5k/ath5k.h | 2 +-
drivers/net/wireless/ath/ath5k/base.c | 22 +++++++++++++++++++---
drivers/net/wireless/ath/ath5k/qcu.c | 5 +++--
drivers/net/wireless/ath/ath5k/reset.c | 5 +++--
4 files changed, 26 insertions(+), 8 deletions(-)




2010-01-21 04:52:09

by Bob Copeland

[permalink] [raw]
Subject: [PATCH 2/2] ath5k: fix setup for CAB queue

The beacon sent gating doesn't seem to work with any combination
of flags. Thus, buffered frames tend to stay buffered forever,
using up tx descriptors.

Instead, use the DBA gating and hold transmission of the buffered
frames until 80% of the beacon interval has elapsed using the ready
time. This fixes the following error in AP mode:

ath5k phy0: no further txbuf available, dropping packet

Add a comment to acknowledge that this isn't the best solution.

Signed-off-by: Bob Copeland <[email protected]>
---
drivers/net/wireless/ath/ath5k/ath5k.h | 2 +-
drivers/net/wireless/ath/ath5k/base.c | 22 +++++++++++++++++++---
drivers/net/wireless/ath/ath5k/qcu.c | 5 +++--
3 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/ath5k.h b/drivers/net/wireless/ath/ath5k/ath5k.h
index 66bcb50..ad4d446 100644
--- a/drivers/net/wireless/ath/ath5k/ath5k.h
+++ b/drivers/net/wireless/ath/ath5k/ath5k.h
@@ -535,7 +535,7 @@ struct ath5k_txq_info {
u32 tqi_cbr_period; /* Constant bit rate period */
u32 tqi_cbr_overflow_limit;
u32 tqi_burst_time;
- u32 tqi_ready_time; /* Not used */
+ u32 tqi_ready_time; /* Time queue waits after an event */
};

/*
diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index b501537..535a6af 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -1516,7 +1516,8 @@ ath5k_beaconq_config(struct ath5k_softc *sc)

ret = ath5k_hw_get_tx_queueprops(ah, sc->bhalq, &qi);
if (ret)
- return ret;
+ goto err;
+
if (sc->opmode == NL80211_IFTYPE_AP ||
sc->opmode == NL80211_IFTYPE_MESH_POINT) {
/*
@@ -1543,10 +1544,25 @@ ath5k_beaconq_config(struct ath5k_softc *sc)
if (ret) {
ATH5K_ERR(sc, "%s: unable to update parameters for beacon "
"hardware queue!\n", __func__);
- return ret;
+ goto err;
}
+ ret = ath5k_hw_reset_tx_queue(ah, sc->bhalq); /* push to h/w */
+ if (ret)
+ goto err;

- return ath5k_hw_reset_tx_queue(ah, sc->bhalq); /* push to h/w */;
+ /* reconfigure cabq with ready time to 80% of beacon_interval */
+ ret = ath5k_hw_get_tx_queueprops(ah, AR5K_TX_QUEUE_ID_CAB, &qi);
+ if (ret)
+ goto err;
+
+ qi.tqi_ready_time = (sc->bintval * 80) / 100;
+ ret = ath5k_hw_set_tx_queueprops(ah, AR5K_TX_QUEUE_ID_CAB, &qi);
+ if (ret)
+ goto err;
+
+ ret = ath5k_hw_reset_tx_queue(ah, AR5K_TX_QUEUE_ID_CAB);
+err:
+ return ret;
}

static void
diff --git a/drivers/net/wireless/ath/ath5k/qcu.c b/drivers/net/wireless/ath/ath5k/qcu.c
index abe36c0..9122a85 100644
--- a/drivers/net/wireless/ath/ath5k/qcu.c
+++ b/drivers/net/wireless/ath/ath5k/qcu.c
@@ -408,12 +408,13 @@ int ath5k_hw_reset_tx_queue(struct ath5k_hw *ah, unsigned int queue)
break;

case AR5K_TX_QUEUE_CAB:
+ /* XXX: use BCN_SENT_GT, if we can figure out how */
AR5K_REG_ENABLE_BITS(ah, AR5K_QUEUE_MISC(queue),
- AR5K_QCU_MISC_FRSHED_BCN_SENT_GT |
+ AR5K_QCU_MISC_FRSHED_DBA_GT |
AR5K_QCU_MISC_CBREXP_DIS |
AR5K_QCU_MISC_CBREXP_BCN_DIS);

- ath5k_hw_reg_write(ah, ((AR5K_TUNE_BEACON_INTERVAL -
+ ath5k_hw_reg_write(ah, ((tq->tqi_ready_time -
(AR5K_TUNE_SW_BEACON_RESP -
AR5K_TUNE_DMA_BEACON_RESP) -
AR5K_TUNE_ADDITIONAL_SWBA_BACKOFF) * 1024) |
--
1.6.3.3



2010-01-21 04:52:09

by Bob Copeland

[permalink] [raw]
Subject: [PATCH 1/2] ath5k: dont use external sleep clock in AP mode

When using the external sleep clock in AP mode, the
TSF increments too quickly, causing beacon interval
to be much lower than it is supposed to be, resulting
in lots of beacon-not-ready interrupts.

This fixes http://bugzilla.kernel.org/show_bug.cgi?id=14802.

Signed-off-by: Bob Copeland <[email protected]>
---
drivers/net/wireless/ath/ath5k/reset.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/reset.c b/drivers/net/wireless/ath/ath5k/reset.c
index 6690923..a35a7db 100644
--- a/drivers/net/wireless/ath/ath5k/reset.c
+++ b/drivers/net/wireless/ath/ath5k/reset.c
@@ -1374,8 +1374,9 @@ int ath5k_hw_reset(struct ath5k_hw *ah, enum nl80211_iftype op_mode,
* Set clocks to 32KHz operation and use an
* external 32KHz crystal when sleeping if one
* exists */
- if (ah->ah_version == AR5K_AR5212)
- ath5k_hw_set_sleep_clock(ah, true);
+ if (ah->ah_version == AR5K_AR5212 &&
+ ah->ah_op_mode != NL80211_IFTYPE_AP)
+ ath5k_hw_set_sleep_clock(ah, true);

/*
* Disable beacons and reset the register
--
1.6.3.3



2010-01-21 11:29:58

by Nick Kossifidis

[permalink] [raw]
Subject: Re: [PATCH 1/2] ath5k: dont use external sleep clock in AP mode

2010/1/21 Bob Copeland <[email protected]>:
> When using the external sleep clock in AP mode, the
> TSF increments too quickly, causing beacon interval
> to be much lower than it is supposed to be, resulting
> in lots of beacon-not-ready interrupts.
>
> This fixes http://bugzilla.kernel.org/show_bug.cgi?id=14802.
>
> Signed-off-by: Bob Copeland <[email protected]>
> ---
>  drivers/net/wireless/ath/ath5k/reset.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath5k/reset.c b/drivers/net/wireless/ath/ath5k/reset.c
> index 6690923..a35a7db 100644
> --- a/drivers/net/wireless/ath/ath5k/reset.c
> +++ b/drivers/net/wireless/ath/ath5k/reset.c
> @@ -1374,8 +1374,9 @@ int ath5k_hw_reset(struct ath5k_hw *ah, enum nl80211_iftype op_mode,
>         * Set clocks to 32KHz operation and use an
>         * external 32KHz crystal when sleeping if one
>         * exists */
> -       if (ah->ah_version == AR5K_AR5212)
> -                       ath5k_hw_set_sleep_clock(ah, true);
> +       if (ah->ah_version == AR5K_AR5212 &&
> +           ah->ah_op_mode != NL80211_IFTYPE_AP)
> +               ath5k_hw_set_sleep_clock(ah, true);
>
>        /*
>         * Disable beacons and reset the register

Acked-by: Nick Kossifidis <[email protected]>



--
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

2010-01-21 11:31:09

by Nick Kossifidis

[permalink] [raw]
Subject: Re: [PATCH 2/2] ath5k: fix setup for CAB queue

2010/1/21 Bob Copeland <[email protected]>:
> The beacon sent gating doesn't seem to work with any combination
> of flags.  Thus, buffered frames tend to stay buffered forever,
> using up tx descriptors.
>
> Instead, use the DBA gating and hold transmission of the buffered
> frames until 80% of the beacon interval has elapsed using the ready
> time.  This fixes the following error in AP mode:
>
>   ath5k phy0: no further txbuf available, dropping packet
>
> Add a comment to acknowledge that this isn't the best solution.
>
> Signed-off-by: Bob Copeland <[email protected]>
> ---
>  drivers/net/wireless/ath/ath5k/ath5k.h |    2 +-
>  drivers/net/wireless/ath/ath5k/base.c  |   22 +++++++++++++++++++---
>  drivers/net/wireless/ath/ath5k/qcu.c   |    5 +++--
>  3 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath5k/ath5k.h b/drivers/net/wireless/ath/ath5k/ath5k.h
> index 66bcb50..ad4d446 100644
> --- a/drivers/net/wireless/ath/ath5k/ath5k.h
> +++ b/drivers/net/wireless/ath/ath5k/ath5k.h
> @@ -535,7 +535,7 @@ struct ath5k_txq_info {
>        u32     tqi_cbr_period; /* Constant bit rate period */
>        u32     tqi_cbr_overflow_limit;
>        u32     tqi_burst_time;
> -       u32     tqi_ready_time; /* Not used */
> +       u32     tqi_ready_time; /* Time queue waits after an event */
>  };
>
>  /*
> diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
> index b501537..535a6af 100644
> --- a/drivers/net/wireless/ath/ath5k/base.c
> +++ b/drivers/net/wireless/ath/ath5k/base.c
> @@ -1516,7 +1516,8 @@ ath5k_beaconq_config(struct ath5k_softc *sc)
>
>        ret = ath5k_hw_get_tx_queueprops(ah, sc->bhalq, &qi);
>        if (ret)
> -               return ret;
> +               goto err;
> +
>        if (sc->opmode == NL80211_IFTYPE_AP ||
>                sc->opmode == NL80211_IFTYPE_MESH_POINT) {
>                /*
> @@ -1543,10 +1544,25 @@ ath5k_beaconq_config(struct ath5k_softc *sc)
>        if (ret) {
>                ATH5K_ERR(sc, "%s: unable to update parameters for beacon "
>                        "hardware queue!\n", __func__);
> -               return ret;
> +               goto err;
>        }
> +       ret = ath5k_hw_reset_tx_queue(ah, sc->bhalq); /* push to h/w */
> +       if (ret)
> +               goto err;
>
> -       return ath5k_hw_reset_tx_queue(ah, sc->bhalq); /* push to h/w */;
> +       /* reconfigure cabq with ready time to 80% of beacon_interval */
> +       ret = ath5k_hw_get_tx_queueprops(ah, AR5K_TX_QUEUE_ID_CAB, &qi);
> +       if (ret)
> +               goto err;
> +
> +       qi.tqi_ready_time = (sc->bintval * 80) / 100;
> +       ret = ath5k_hw_set_tx_queueprops(ah, AR5K_TX_QUEUE_ID_CAB, &qi);
> +       if (ret)
> +               goto err;
> +
> +       ret = ath5k_hw_reset_tx_queue(ah, AR5K_TX_QUEUE_ID_CAB);
> +err:
> +       return ret;
>  }
>
>  static void
> diff --git a/drivers/net/wireless/ath/ath5k/qcu.c b/drivers/net/wireless/ath/ath5k/qcu.c
> index abe36c0..9122a85 100644
> --- a/drivers/net/wireless/ath/ath5k/qcu.c
> +++ b/drivers/net/wireless/ath/ath5k/qcu.c
> @@ -408,12 +408,13 @@ int ath5k_hw_reset_tx_queue(struct ath5k_hw *ah, unsigned int queue)
>                        break;
>
>                case AR5K_TX_QUEUE_CAB:
> +                       /* XXX: use BCN_SENT_GT, if we can figure out how */
>                        AR5K_REG_ENABLE_BITS(ah, AR5K_QUEUE_MISC(queue),
> -                               AR5K_QCU_MISC_FRSHED_BCN_SENT_GT |
> +                               AR5K_QCU_MISC_FRSHED_DBA_GT |
>                                AR5K_QCU_MISC_CBREXP_DIS |
>                                AR5K_QCU_MISC_CBREXP_BCN_DIS);
>
> -                       ath5k_hw_reg_write(ah, ((AR5K_TUNE_BEACON_INTERVAL -
> +                       ath5k_hw_reg_write(ah, ((tq->tqi_ready_time -
>                                (AR5K_TUNE_SW_BEACON_RESP -
>                                AR5K_TUNE_DMA_BEACON_RESP) -
>                                AR5K_TUNE_ADDITIONAL_SWBA_BACKOFF) * 1024) |


It would be great if we had some feedback from Atheros on BCN_SENT_GT...

Acked-by: Nick Kossifidis <[email protected]>


--
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

2010-01-21 22:15:16

by Michael Büsch

[permalink] [raw]
Subject: Re: [PATCH 0/2] ath5k AP mode fixes

On Thursday 21 January 2010 05:51:02 Bob Copeland wrote:
> Here are a couple of fixes to ath5k AP mode that have been
> kicking around for a bit.
>
> The sleep clock is disabled by legacy HAL for AP mode and at
> least one person had beaconing issues as a result of our not
> also doing so.
>
> Although we decided 'fix setup for CAB queue' was not ideal,
> there's no alternative at the moment and the current setup is
> causing problems for people, so IMO we should just take this
> patch as-is for now.
>
> Bob Copeland (2):
> ath5k: dont use external sleep clock in AP mode
> ath5k: fix setup for CAB queue


Thanks a lot for these, Bob. I'll test them on my AP.

Important: Please also add CC:stable to the patches to make
sure they reach the endusers for 2.6.32.x.
2.6.32.x currently is unusable in AP mode, so these patches
are pretty important fixes (if they actually fix the issues.
But I assume that for now ;) )

--
Greetings, Michael.