Re: [PATCH 13/16] wil6210: ignore HALP ICR if already handled
On 2018-11-06 12:04, Kalle Valo wrote:
Maya Erez 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
[...]
--- 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(>halp.comp);
+ if (atomic_read(>halp.handle_icr)) {
+ /* no need to handle HALP ICRs until next vote */
+ atomic_set(>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
Re: [PATCH 13/16] wil6210: ignore HALP ICR if already handled
Maya Erez 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
[...]
> --- 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(>halp.comp);
> + if (atomic_read(>halp.handle_icr)) {
> + /* no need to handle HALP ICRs until next vote */
> + atomic_set(>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.
--
Kalle Valo
[PATCH 13/16] wil6210: ignore HALP ICR if already handled
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
---
drivers/net/wireless/ath/wil6210/interrupt.c | 10 +++---
drivers/net/wireless/ath/wil6210/main.c | 2 ++
drivers/net/wireless/ath/wil6210/wil6210.h | 1 +
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/wil6210/interrupt.c
b/drivers/net/wireless/ath/wil6210/interrupt.c
index 5d287a8..a58fccb 100644
--- 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(>halp.comp);
+ if (atomic_read(>halp.handle_icr)) {
+ /* no need to handle HALP ICRs until next vote */
+ atomic_set(>halp.handle_icr, 0);
+ wil_dbg_irq(wil, "irq_misc: HALP IRQ invoked\n");
+ wil6210_mask_halp(wil);
+ complete(>halp.comp);
+ }
}
wil->isr_misc = isr;
diff --git a/drivers/net/wireless/ath/wil6210/main.c
b/drivers/net/wireless/ath/wil6210/main.c
index 5303867..5c89c01 100644
--- a/drivers/net/wireless/ath/wil6210/main.c
+++ b/drivers/net/wireless/ath/wil6210/main.c
@@ -1922,6 +1922,8 @@ void wil_halp_vote(struct wil6210_priv *wil)
if (++wil->halp.ref_cnt == 1) {
reinit_completion(>halp.comp);
+ /* mark to IRQ context to handle HALP ICR */
+ atomic_set(>halp.handle_icr, 1);
wil6210_set_halp(wil);
rc = wait_for_completion_timeout(>halp.comp, to_jiffies);
if (!rc) {
diff --git a/drivers/net/wireless/ath/wil6210/wil6210.h
b/drivers/net/wireless/ath/wil6210/wil6210.h
index 0f3be3ff..1897d8c 100644
--- a/drivers/net/wireless/ath/wil6210/wil6210.h
+++ b/drivers/net/wireless/ath/wil6210/wil6210.h
@@ -791,6 +791,7 @@ struct wil_halp {
struct mutexlock; /* protect halp ref_cnt */
unsigned intref_cnt;
struct completion comp;
+ atomic_thandle_icr;
};
struct wil_blob_wrapper {
--
1.9.1
