[PATCH] ath6kl: Avoid taking semaphore in TCMD event report handling func

[PATCH] ath6kl: Avoid taking semaphore in TCMD event report handling func

[rmani]

From: Raja Mani <rmani@qca.qualcomm.com>

ath6kl_tm_rx_report() func takes ar->sem and sends tcmd to the chip
and then waits for wake_up event from ath6kl_tm_rx_report_event() with timeout.

In the current case, When tcmd report is reached to the host,
ath6kl_tm_rx_report_event() func tries to take the same semaphore (ar->sem)
which is already taken in ath6kl_tm_rx_report().

Due to this, ath6kl_tm_rx_report_event() func always fails to update
tcmd report in the local buffer and sends wake_up event to ath6kl_tm_rx_report().
So, the timeout will happen in ath6kl_tm_rx_report() always and
then it will release ar->sem. Now ath6kl_tm_rx_report_event() will
get a chance to update tcmd report in the local buffer.

There is no need of taking ar->sem in ath6kl_tm_rx_report_event(), we can go ahead
and update the local buffer and send wake_up event to ath6kl_tm_rx_report().
In this way, we will get tcmd report (in the user space) in the first time itself.

Signed-off-by: Raja Mani <rmani@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath6kl/testmode.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/testmode.c b/drivers/net/wireless/ath/ath6kl/testmode.c
index 381eb66..971ea61 100644
--- a/drivers/net/wireless/ath/ath6kl/testmode.c
+++ b/drivers/net/wireless/ath/ath6kl/testmode.c
@@ -43,16 +43,11 @@ static const struct nla_policy ath6kl_tm_policy[ATH6KL_TM_ATTR_MAX + 1] = {
 
 void ath6kl_tm_rx_report_event(struct ath6kl *ar, void *buf, size_t buf_len)
 {
-	if (down_interruptible(&ar->sem))
-		return;
-
 	kfree(ar->tm.rx_report);
 
 	ar->tm.rx_report = kmemdup(buf, buf_len, GFP_KERNEL);
 	ar->tm.rx_report_len = buf_len;
 
-	up(&ar->sem);
-
 	wake_up(&ar->event_wq);
 }
 
-- 
1.7.0.4

Re: [PATCH] ath6kl: Avoid taking semaphore in TCMD event report handling func

[Kalle Valo]

On 09/06/2011 02:05 PM, rmani@qca.qualcomm.com wrote:
> From: Raja Mani <rmani@qca.qualcomm.com>
> 
> ath6kl_tm_rx_report() func takes ar->sem and sends tcmd to the chip
> and then waits for wake_up event from ath6kl_tm_rx_report_event() with timeout.
> 
> In the current case, When tcmd report is reached to the host,
> ath6kl_tm_rx_report_event() func tries to take the same semaphore (ar->sem)
> which is already taken in ath6kl_tm_rx_report().
> 
> Due to this, ath6kl_tm_rx_report_event() func always fails to update
> tcmd report in the local buffer and sends wake_up event to ath6kl_tm_rx_report().
> So, the timeout will happen in ath6kl_tm_rx_report() always and
> then it will release ar->sem. Now ath6kl_tm_rx_report_event() will
> get a chance to update tcmd report in the local buffer.

This makes sense. I remember seeing something similar myself.

> There is no need of taking ar->sem in ath6kl_tm_rx_report_event(), we can go ahead
> and update the local buffer and send wake_up event to ath6kl_tm_rx_report().
> In this way, we will get tcmd report (in the user space) in the first time itself.

To me that looks racy. What will then prevent concurrent access to
ar->tm.rx_report?

Wouldn't it be better to release semaphore beforing calling
wait_event_interruptible_timeout() and then take it again after the
event has happened?

Kalle