The approach of this change is flawed, as it triggers tx status processing
from more callsites, yet the chips only have one global tx status queue.
Subsequent patches will properly fix the issue that this one tried to address.
Signed-off-by: Felix Fietkau <[email protected]>
---
drivers/net/wireless/ath/ath9k/ar9003_mac.c | 8 ++------
drivers/net/wireless/ath/ath9k/beacon.c | 6 ------
2 files changed, 2 insertions(+), 12 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/ar9003_mac.c b/drivers/net/wireless/ath/ath9k/ar9003_mac.c
index 09b8c9d..73d44bc 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_mac.c
+++ b/drivers/net/wireless/ath/ath9k/ar9003_mac.c
@@ -346,7 +346,6 @@ static bool ar9003_hw_get_isr(struct ath_hw *ah, enum ath9k_int *masked)
static int ar9003_hw_proc_txdesc(struct ath_hw *ah, void *ds,
struct ath_tx_status *ts)
{
- struct ar9003_txc *txc = (struct ar9003_txc *) ds;
struct ar9003_txs *ads;
u32 status;
@@ -356,11 +355,7 @@ static int ar9003_hw_proc_txdesc(struct ath_hw *ah, void *ds,
if ((status & AR_TxDone) == 0)
return -EINPROGRESS;
- ts->qid = MS(ads->ds_info, AR_TxQcuNum);
- if (!txc || (MS(txc->info, AR_TxQcuNum) == ts->qid))
- ah->ts_tail = (ah->ts_tail + 1) % ah->ts_size;
- else
- return -ENOENT;
+ ah->ts_tail = (ah->ts_tail + 1) % ah->ts_size;
if ((MS(ads->ds_info, AR_DescId) != ATHEROS_VENDOR_ID) ||
(MS(ads->ds_info, AR_TxRxDesc) != 1)) {
@@ -374,6 +369,7 @@ static int ar9003_hw_proc_txdesc(struct ath_hw *ah, void *ds,
ts->ts_seqnum = MS(status, AR_SeqNum);
ts->tid = MS(status, AR_TxTid);
+ ts->qid = MS(ads->ds_info, AR_TxQcuNum);
ts->desc_id = MS(ads->status1, AR_TxDescId);
ts->ts_tstamp = ads->status4;
ts->ts_status = 0;
diff --git a/drivers/net/wireless/ath/ath9k/beacon.c b/drivers/net/wireless/ath/ath9k/beacon.c
index b8967e4..6d1e465 100644
--- a/drivers/net/wireless/ath/ath9k/beacon.c
+++ b/drivers/net/wireless/ath/ath9k/beacon.c
@@ -355,7 +355,6 @@ void ath_beacon_tasklet(unsigned long data)
struct ath_common *common = ath9k_hw_common(ah);
struct ath_buf *bf = NULL;
struct ieee80211_vif *vif;
- struct ath_tx_status ts;
bool edma = !!(ah->caps.hw_caps & ATH9K_HW_CAP_EDMA);
int slot;
u32 bfaddr, bc = 0;
@@ -462,11 +461,6 @@ void ath_beacon_tasklet(unsigned long data)
ath9k_hw_txstart(ah, sc->beacon.beaconq);
sc->beacon.ast_be_xmit += bc; /* XXX per-vif? */
- if (edma) {
- spin_lock_bh(&sc->sc_pcu_lock);
- ath9k_hw_txprocdesc(ah, bf->bf_desc, (void *)&ts);
- spin_unlock_bh(&sc->sc_pcu_lock);
- }
}
}
--
1.7.3.2
On 2012-02-29 7:08 PM, John W. Linville wrote:
> On Mon, Feb 27, 2012 at 07:58:39PM +0100, Felix Fietkau wrote:
>> The approach of this change is flawed, as it triggers tx status processing
>> from more callsites, yet the chips only have one global tx status queue.
>> Subsequent patches will properly fix the issue that this one tried to address.
>>
>> Signed-off-by: Felix Fietkau <[email protected]>
>
> How flawed is the current approach? That commit looks like it has been
> around for months already. Does it really _need_ to be fixed in 3.3?
It only slightly reduces the severity of the issue it tried to address,
while adding some ugly race conditions. It is not possible to properly
fix the issue without a revert of that commit, so I really would like to
see this patch series in 3.3.
- Felix
Signed-off-by: Felix Fietkau <[email protected]>
---
drivers/net/wireless/ath/ath9k/beacon.c | 2 +-
drivers/net/wireless/ath/ath9k/xmit.c | 7 +++++--
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/beacon.c b/drivers/net/wireless/ath/ath9k/beacon.c
index 6d1e465..43882f9 100644
--- a/drivers/net/wireless/ath/ath9k/beacon.c
+++ b/drivers/net/wireless/ath/ath9k/beacon.c
@@ -91,7 +91,7 @@ static void ath_beacon_setup(struct ath_softc *sc, struct ath_vif *avp,
info.txpower = MAX_RATE_POWER;
info.keyix = ATH9K_TXKEYIX_INVALID;
info.keytype = ATH9K_KEY_TYPE_CLEAR;
- info.flags = ATH9K_TXDESC_NOACK;
+ info.flags = ATH9K_TXDESC_NOACK | ATH9K_TXDESC_INTREQ;
info.buf_addr[0] = bf->bf_buf_addr;
info.buf_len[0] = roundup(skb->len, 4);
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 3182408..cf5a267 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -2297,9 +2297,12 @@ void ath_tx_edma_tasklet(struct ath_softc *sc)
break;
}
- /* Skip beacon completions */
- if (ts.qid == sc->beacon.beaconq)
+ /* Process beacon completions separately */
+ if (ts.qid == sc->beacon.beaconq) {
+ sc->beacon.tx_processed = true;
+ sc->beacon.tx_last = !(ts.ts_status & ATH9K_TXERR_MASK);
continue;
+ }
txq = &sc->tx.txq[ts.qid];
--
1.7.3.2
On Mon, Feb 27, 2012 at 07:58:39PM +0100, Felix Fietkau wrote:
> The approach of this change is flawed, as it triggers tx status processing
> from more callsites, yet the chips only have one global tx status queue.
> Subsequent patches will properly fix the issue that this one tried to address.
>
> Signed-off-by: Felix Fietkau <[email protected]>
How flawed is the current approach? That commit looks like it has been
around for months already. Does it really _need_ to be fixed in 3.3?
John
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
Not doing so could cause the tx status queue to overflow during longer
periods of time without non-beacon tx. These events are also required
for proper drv_tx_last_beacon handling.
Signed-off-by: Felix Fietkau <[email protected]>
---
drivers/net/wireless/ath/ath9k/mac.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/mac.c b/drivers/net/wireless/ath/ath9k/mac.c
index e196aba..5f4ae6c 100644
--- a/drivers/net/wireless/ath/ath9k/mac.c
+++ b/drivers/net/wireless/ath/ath9k/mac.c
@@ -745,7 +745,11 @@ int ath9k_hw_beaconq_setup(struct ath_hw *ah)
qi.tqi_aifs = 1;
qi.tqi_cwmin = 0;
qi.tqi_cwmax = 0;
- /* NB: don't enable any interrupts */
+
+ if (ah->caps.hw_caps & ATH9K_HW_CAP_EDMA)
+ qi.tqi_qflags = TXQ_FLAG_TXOKINT_ENABLE |
+ TXQ_FLAG_TXERRINT_ENABLE;
+
return ath9k_hw_setuptxqueue(ah, ATH9K_TX_QUEUE_BEACON, &qi);
}
EXPORT_SYMBOL(ath9k_hw_beaconq_setup);
--
1.7.3.2
Since AR9003 uses a global tx status queue, processing tx status outside of
the regular tx tasklet is dangerous and messes up hardware/software
synchronization of tx status events.
Signed-off-by: Felix Fietkau <[email protected]>
---
drivers/net/wireless/ath/ath9k/main.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 4a00806..8cbbc17 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -2332,6 +2332,7 @@ static int ath9k_tx_last_beacon(struct ieee80211_hw *hw)
struct ath_vif *avp;
struct ath_buf *bf;
struct ath_tx_status ts;
+ bool edma = !!(ah->caps.hw_caps & ATH9K_HW_CAP_EDMA);
int status;
vif = sc->beacon.bslot[0];
@@ -2342,7 +2343,7 @@ static int ath9k_tx_last_beacon(struct ieee80211_hw *hw)
if (!avp->is_bslot_active)
return 0;
- if (!sc->beacon.tx_processed) {
+ if (!sc->beacon.tx_processed && !edma) {
tasklet_disable(&sc->bcon_tasklet);
bf = avp->av_bcbuf;
--
1.7.3.2
>On Wed, Feb 29, 2012 at 07:24:44PM +0100, Felix Fietkau wrote:
>> On 2012-02-29 7:08 PM, John W. Linville wrote:
> >> On Mon, Feb 27, 2012 at 07:58:39PM +0100, Felix Fietkau wrote:
> >>> The approach of this change is flawed, as it triggers tx status processing
> >>> from more callsites, yet the chips only have one global tx status queue.
> >>> Subsequent patches will properly fix the issue that this one tried to address.
>> >>
>> >> Signed-off-by: Felix Fietkau <[email protected]>
> >>
> >> How flawed is the current approach? That commit looks like it has been
> > >around for months already. Does it really _need_ to be fixed in 3.3?
>> It only slightly reduces the severity of the issue it tried to address,
>> while adding some ugly race conditions. It is not possible to properly
>> fix the issue without a revert of that commit, so I really would like to
>> see this patch series in 3.3.
>
>I am happy to take this series for 3.4, but it seems quite late
>for 3.3. We have been living with it for a while already.
As said, it is not a regression. The latest patch fix it in proper manner. So it fine to have it from 3.4 onwards.
-Rajkumar
On Wed, Feb 29, 2012 at 07:24:44PM +0100, Felix Fietkau wrote:
> On 2012-02-29 7:08 PM, John W. Linville wrote:
> > On Mon, Feb 27, 2012 at 07:58:39PM +0100, Felix Fietkau wrote:
> >> The approach of this change is flawed, as it triggers tx status processing
> >> from more callsites, yet the chips only have one global tx status queue.
> >> Subsequent patches will properly fix the issue that this one tried to address.
> >>
> >> Signed-off-by: Felix Fietkau <[email protected]>
> >
> > How flawed is the current approach? That commit looks like it has been
> > around for months already. Does it really _need_ to be fixed in 3.3?
> It only slightly reduces the severity of the issue it tried to address,
> while adding some ugly race conditions. It is not possible to properly
> fix the issue without a revert of that commit, so I really would like to
> see this patch series in 3.3.
I am happy to take this series for 3.4, but it seems quite late
for 3.3. We have been living with it for a while already.
John
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
On 2012-03-05 8:33 PM, John W. Linville wrote:
> On Wed, Feb 29, 2012 at 07:24:44PM +0100, Felix Fietkau wrote:
>> On 2012-02-29 7:08 PM, John W. Linville wrote:
>> > On Mon, Feb 27, 2012 at 07:58:39PM +0100, Felix Fietkau wrote:
>> >> The approach of this change is flawed, as it triggers tx status processing
>> >> from more callsites, yet the chips only have one global tx status queue.
>> >> Subsequent patches will properly fix the issue that this one tried to address.
>> >>
>> >> Signed-off-by: Felix Fietkau <[email protected]>
>> >
>> > How flawed is the current approach? That commit looks like it has been
>> > around for months already. Does it really _need_ to be fixed in 3.3?
>> It only slightly reduces the severity of the issue it tried to address,
>> while adding some ugly race conditions. It is not possible to properly
>> fix the issue without a revert of that commit, so I really would like to
>> see this patch series in 3.3.
>
> I am happy to take this series for 3.4, but it seems quite late
> for 3.3. We have been living with it for a while already.
OK, no problem. I'm using bleeding edge anyway ;)
- Felix