> --- a/drivers/net/wireless/ath/ath9k/mac.c
[...]
> +bool ath9k_hw_abort_tx_dma(struct ath_hw *ah)
[...]
> + for (q = 0; q < AR_NUM_QCU; q++) {
> + for (i = 1000; i > 0; i--) {
> + if (!ath9k_hw_numtxpending(ah, q))
> + break;
> +
> + udelay(5);
> + }
> + }
> + if (!i)
> + return false;
Awesome-looking patch set.
What?s the return value of this function supposed to mean? As written, it only returns false there?s anything left pending on the final queue. Notably, if the inner loop ran for all 999 iterations and ath9k_hw_numtxpending never returned false, the code above would effectively ignore that condition.
I?m less concerned about the return value (because it actually seems that the function could be declared void, you don?t use the return value anywhere) than I am about the REG_CLR_BIT and REG_WRITE operations that follow. It seems that you might be either missing those in cases where they should happen, or performing them in cases where they shouldn?t. Did you mean to leave AR_Q_TXD set when something is left pending on the final hardware queue?
On 2011-03-10 5:21 AM, Mark Mentovai wrote:
>> --- a/drivers/net/wireless/ath/ath9k/mac.c
> [...]
>> +bool ath9k_hw_abort_tx_dma(struct ath_hw *ah)
> [...]
>> + for (q = 0; q < AR_NUM_QCU; q++) {
>> + for (i = 1000; i > 0; i--) {
>> + if (!ath9k_hw_numtxpending(ah, q))
>> + break;
>> +
>> + udelay(5);
>> + }
>> + }
>> + if (!i)
>> + return false;
>
> Awesome-looking patch set.
>
> What?s the return value of this function supposed to mean? As
> written, it only returns false there?s anything left pending on the
> final queue. Notably, if the inner loop ran for all 999 iterations
> and ath9k_hw_numtxpending never returned false, the code above would
> effectively ignore that condition.
Makes sense
> I?m less concerned about the return value (because it actually seems
> that the function could be declared void, you don?t use the return
> value anywhere) than I am about the REG_CLR_BIT and REG_WRITE
> operations that follow. It seems that you might be either missing
> those in cases where they should happen, or performing them in cases
> where they shouldn?t. Did you mean to leave AR_Q_TXD set when
> something is left pending on the final hardware queue?
I'll drop the return code and make the clearing of AR_Q_TXD and the
other bits unconditional.
- Felix