[PATCH] ath6kl: Use a mutex_lock to avoid race in diabling and handling irq

[PATCH] ath6kl: Use a mutex_lock to avoid race in diabling and handling irq

[Vasanthakumar Thiagarajan]

Currently this race is handled but in a messy way an atomic
variable is being checked in a loop which sleeps upto ms
in every iteration. Remove this logic and use a mutex
to make sure irq is not disabled when irq handling is in
progress.

Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath6kl/sdio.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/sdio.c b/drivers/net/wireless/ath/ath6kl/sdio.c
index 278a9f3..8b36904 100644
--- a/drivers/net/wireless/ath/ath6kl/sdio.c
+++ b/drivers/net/wireless/ath/ath6kl/sdio.c
@@ -49,11 +49,13 @@ struct ath6kl_sdio {
 	/* scatter request list head */
 	struct list_head scat_req;
 
+	/* Avoids disabling irq while the interrupts being handled */
+	struct mutex mtx_irq;
+
 	spinlock_t scat_lock;
 	bool scatter_enabled;
 
 	bool is_disabled;
-	atomic_t irq_handling;
 	const struct sdio_device_id *id;
 	struct work_struct wr_async_work;
 	struct list_head wr_asyncq;
@@ -460,8 +462,7 @@ static void ath6kl_sdio_irq_handler(struct sdio_func *func)
 	ath6kl_dbg(ATH6KL_DBG_SDIO, "irq\n");
 
 	ar_sdio = sdio_get_drvdata(func);
-	atomic_set(&ar_sdio->irq_handling, 1);
-
+	mutex_lock(&ar_sdio->mtx_irq);
 	/*
 	 * Release the host during interrups so we can pick it back up when
 	 * we process commands.
@@ -470,7 +471,7 @@ static void ath6kl_sdio_irq_handler(struct sdio_func *func)
 
 	status = ath6kl_hif_intr_bh_handler(ar_sdio->ar);
 	sdio_claim_host(ar_sdio->func);
-	atomic_set(&ar_sdio->irq_handling, 0);
+	mutex_unlock(&ar_sdio->mtx_irq);
 	WARN_ON(status && status != -ECANCELED);
 }
 
@@ -578,17 +579,14 @@ static void ath6kl_sdio_irq_disable(struct ath6kl *ar)
 
 	sdio_claim_host(ar_sdio->func);
 
-	/* Mask our function IRQ */
-	while (atomic_read(&ar_sdio->irq_handling)) {
-		sdio_release_host(ar_sdio->func);
-		schedule_timeout(HZ / 10);
-		sdio_claim_host(ar_sdio->func);
-	}
+	mutex_lock(&ar_sdio->mtx_irq);
 
 	ret = sdio_release_irq(ar_sdio->func);
 	if (ret)
 		ath6kl_err("Failed to release sdio irq: %d\n", ret);
 
+	mutex_unlock(&ar_sdio->mtx_irq);
+
 	sdio_release_host(ar_sdio->func);
 }
 
@@ -1253,6 +1251,7 @@ static int ath6kl_sdio_probe(struct sdio_func *func,
 	spin_lock_init(&ar_sdio->scat_lock);
 	spin_lock_init(&ar_sdio->wr_async_lock);
 	mutex_init(&ar_sdio->dma_buffer_mutex);
+	mutex_init(&ar_sdio->mtx_irq);
 
 	INIT_LIST_HEAD(&ar_sdio->scat_req);
 	INIT_LIST_HEAD(&ar_sdio->bus_req_freeq);
-- 
1.7.0.4

Re: [PATCH] ath6kl: Use a mutex_lock to avoid race in diabling and handling irq

[David Miller]

From: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>
Date: Wed, 4 Jan 2012 15:57:19 +0530

> Currently this race is handled but in a messy way an atomic
> variable is being checked in a loop which sleeps upto ms
> in every iteration. Remove this logic and use a mutex
> to make sure irq is not disabled when irq handling is in
> progress.
> 
> Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>

So you guys have time to code up patches like this but not enough
time to fix the build failure properly that exists in -next for
some time now?

This kind of stuff really drives me nuts, fix your priorities.  You
guys added a build regression to the tree, fix that first.

Re: [PATCH] ath6kl: Use a mutex_lock to avoid race in diabling and handling irq

[Kalle Valo]

On 01/04/2012 12:27 PM, Vasanthakumar Thiagarajan wrote:
> Currently this race is handled but in a messy way an atomic
> variable is being checked in a loop which sleeps upto ms
> in every iteration. Remove this logic and use a mutex
> to make sure irq is not disabled when irq handling is in
> progress.
> 
> Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>

Thanks, applied.

There is one race, though. It's possible that the irq handler is
executed once after the interrupts are disabled. Do we care about that?

AFAICS this race was before your patch so I'll take your patch anyway.

Kalle

Re: [PATCH] ath6kl: Use a mutex_lock to avoid race in diabling and handling irq

[Vasanthakumar Thiagarajan]

On Mon, Jan 09, 2012 at 04:22:40PM +0200, Kalle Valo wrote:
> On 01/04/2012 12:27 PM, Vasanthakumar Thiagarajan wrote:
> > Currently this race is handled but in a messy way an atomic
> > variable is being checked in a loop which sleeps upto ms
> > in every iteration. Remove this logic and use a mutex
> > to make sure irq is not disabled when irq handling is in
> > progress.
> > 
> > Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>
> 
> Thanks, applied.
> 
> There is one race, though. It's possible that the irq handler is
> executed once after the interrupts are disabled. Do we care about that?

I don't think so, we read interrupt status only when the interrupts
are enabled by checking dev->irq_en_reg.int_status_en in
proc_pending_irqs(), so it is already taken care.

Vasanth