Re: [PATCH 13/16] wil6210: ignore HALP ICR if already handled

[merez@codeaurora.org]

On 2018-11-06 12:04, Kalle Valo wrote:
> Maya Erez <merez@codeaurora.org> writes:
> 
>> HALP ICR is set as long as the FW should stay awake.
>> To prevent its multiple handling the driver masks this IRQ bit.
>> However, if there is a different MISC ICR before the driver clears
>> this bit, there is a risk of race condition between HALP mask and
>> unmask. This race leads to HALP timeout, in case it is mistakenly
>> masked.
>> Add an atomic flag to indicate if HALP ICR should be handled.
>> 
>> Signed-off-by: Maya Erez <merez@codeaurora.org>
> 
> [...]
> 
>> --- a/drivers/net/wireless/ath/wil6210/interrupt.c
>> +++ b/drivers/net/wireless/ath/wil6210/interrupt.c
>> @@ -575,10 +575,14 @@ static irqreturn_t wil6210_irq_misc(int irq, 
>> void *cookie)
>>  	}
>> 
>>  	if (isr & BIT_DMA_EP_MISC_ICR_HALP) {
>> -		wil_dbg_irq(wil, "irq_misc: HALP IRQ invoked\n");
>> -		wil6210_mask_halp(wil);
>>  		isr &= ~BIT_DMA_EP_MISC_ICR_HALP;
>> -		complete(&wil->halp.comp);
>> +		if (atomic_read(&wil->halp.handle_icr)) {
>> +			/* no need to handle HALP ICRs until next vote */
>> +			atomic_set(&wil->halp.handle_icr, 0);
> 
> atomic_read() followed by atomic_set() is IMHO not really atomic :) I
> would assume there's a function to reset the variable really in atomic
> way.

Actually it shouldn't be atomic :-)
I'll remove it.

-- 
Maya Erez
Qualcomm Israel, Inc. on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project