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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
