Thanks for picking this up Peter! I am still a week out, at least, before I could work on this!
Gregg Wonderly!
Sent from my iPhone
> On Apr 18, 2023, at 6:03 PM, Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
>
> Peter Seiderer <ps.report@xxxxxxx> writes:
>
>> Hello Toke,
>>
>>> On Fri, 14 Apr 2023 00:17:04 +0200, Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
>>>
>>> Peter Seiderer <ps.report@xxxxxxx> writes:
>>>
>>>> Hello Gregg, Toke,
>>>>
>>>> On Thu, 30 Mar 2023 08:44:25 -0500, Gregg Wonderly <greggwonderly@xxxxxxxxxxxxxx> wrote:
>>>>
>>>>> I have not tested this. I am in the middle of testing on this machine of many other things and building a kernel right now is not on my timeline. Note that I have a magic 6 constant in there. I derived this from dividing 32 by the bit mask 0x1f width of 5. But looking further at this, it seems like chk_dbg should actually be a u64, and dma_dbg_4 and dma_dbg_5 placed into that value as a continuous 64bit value. But again, I don’t know if there are 2 bits at the top of dma_dbg_4 and 3 bits at the bottom of dma_dbg_5 that go together. This needs to be fixed by someone with the time and the knowledge of what’s going on in the hardware.
>>>>
>>>> The comment from drivers/net/wireless/ath/ath9k/ar9003_hw.c only some
>>>> lines above seems to support your reasoning (for the '6' constant, not
>>>> for the packaging into an u64 - bit 30-31 unused):
>>>>
>>>> 1073 /*
>>>> 1074 * MAC HW hang check
>>>> 1075 * =================
>>>> 1076 *
>>>> 1077 * Signature: dcu_chain_state is 0x6 and dcu_complete_state is 0x1.
>>>> 1078 *
>>>> 1079 * The state of each DCU chain (mapped to TX queues) is available from these
>>>> 1080 * DMA debug registers:
>>>> 1081 *
>>>> 1082 * Chain 0 state : Bits 4:0 of AR_DMADBG_4
>>>> 1083 * Chain 1 state : Bits 9:5 of AR_DMADBG_4
>>>> 1084 * Chain 2 state : Bits 14:10 of AR_DMADBG_4
>>>> 1085 * Chain 3 state : Bits 19:15 of AR_DMADBG_4
>>>> 1086 * Chain 4 state : Bits 24:20 of AR_DMADBG_4
>>>> 1087 * Chain 5 state : Bits 29:25 of AR_DMADBG_4
>>>> 1088 * Chain 6 state : Bits 4:0 of AR_DMADBG_5
>>>> 1089 * Chain 7 state : Bits 9:5 of AR_DMADBG_5
>>>> 1090 * Chain 8 state : Bits 14:10 of AR_DMADBG_5
>>>> 1091 * Chain 9 state : Bits 19:15 of AR_DMADBG_5
>>>> 1092 *
>>>> 1093 * The DCU chain state "0x6" means "WAIT_FRDONE" - wait for TX frame to be done.
>>>> 1094 */
>>>>
>>>> But with the same/similar bug some lines below (dma_dbg_chain is AR_DMADBG_4
>>>> for queue < 6 and AR_DMADBG_5 above):
>>>>
>>>> 1112 dcu_chain_state = (dma_dbg_chain >> (5 * queue)) & 0x1f;
>>>
>>> Okay, here is a patch fixing both places; could one of you please test
>>> it?
>>
>> Sorry for the delayed answer..., did prepare already a similar patch (but
>> without the optimization of moving out the dbg_reg/offset out of the for-
>> loop in ath9k_hw_verify_hang) and tested it via some additional debug
>> output....
>>
>> In IBSS mode with iperf running in both directions all 1 to 3 hours
>> ar9003_hw_detect_mac_hang() triggers the additional check/call
>> to ath9k_hw_verify_hang() but always without real hang outcome...
>
> Great, thanks!
>
>> Some (minor) style questions below...
>>
>>>
>>> -Toke
>>>
>>> diff --git a/drivers/net/wireless/ath/ath9k/ar9003_hw.c b/drivers/net/wireless/ath/ath9k/ar9003_hw.c
>>> index 4f27a9fb1482..2e8570baabf6 100644
>>> --- a/drivers/net/wireless/ath/ath9k/ar9003_hw.c
>>> +++ b/drivers/net/wireless/ath/ath9k/ar9003_hw.c
>>> @@ -1099,17 +1099,22 @@ static bool ath9k_hw_verify_hang(struct ath_hw *ah, unsigned int queue)
>>> {
>>> u32 dma_dbg_chain, dma_dbg_complete;
>>> u8 dcu_chain_state, dcu_complete_state;
>>> + unsigned int dbg_reg, offset;
>>> int i;
>>>
>>> - for (i = 0; i < NUM_STATUS_READS; i++) {
>>> - if (queue < 6)
>>> - dma_dbg_chain = REG_READ(ah, AR_DMADBG_4);
>>> - else
>>> - dma_dbg_chain = REG_READ(ah, AR_DMADBG_5);
>>> + if (queue < 6) {
>>> + dbg_reg = AR_DMADBG_4;
>>> + offset = queue;
>>
>> Or calculate the 'real' offset here:
>>
>> offset = queue * 5;
>>
>>> + } else {
>>> + dbg_reg = AR_DMADBG_5;
>>> + offset = queue - 6;
>>
>> offset = (queue - 6) * 5;
>>> + }
>>>
>>> + for (i = 0; i < NUM_STATUS_READS; i++) {
>>> + dma_dbg_chain = REG_READ(ah, dbg_reg);
>>> dma_dbg_complete = REG_READ(ah, AR_DMADBG_6);
>>>
>>> - dcu_chain_state = (dma_dbg_chain >> (5 * queue)) & 0x1f;
>>> + dcu_chain_state = (dma_dbg_chain >> (5 * offset)) & 0x1f;
>>
>> And a slightly simpler calculation here:
>>
>> dcu_chain_state = (dma_dbg_chain >> offset) & 0x1f;
>
> Sure, SGTM.
>
>> Or alternative (without offset var) solution:
>>
>> dcu_chain_state = (dma_dbg_chain >> (5 * (queue % 6))) & 0x1f;
>
> Was trying to avoid the divide (in %) by defining the offset above
> (probably a useless optimisation, but, well :)).
>
>> Do you prefer to convert your suggestion into a complete patch/commit or
>> should I update mine (incorporating the optimization of moving out the
>> dbg_reg/offset out of the for-loop) and send to the mailing list?
>
> I mostly wrote that because I wasn't sure any of y'all would send a
> patch; so sure, please go ahead and submit a proper one :)
>
> -Toke
