[PATCH 04/13] IR: fix locking in ir_raw_event_work
It is prefectly possible to have ir_raw_event_work
running concurently on two cpus, thus we must protect
it from that situation.
Just switch to a thread that we wake up as soon as we have data.
This also ensures that this thread doesn't run unnessesarly.
Signed-off-by: Maxim Levitsky
---
drivers/media/IR/ir-core-priv.h |2 +-
drivers/media/IR/ir-raw-event.c | 42 --
2 files changed, 32 insertions(+), 12 deletions(-)
diff --git a/drivers/media/IR/ir-core-priv.h b/drivers/media/IR/ir-core-priv.h
index dc26e2b..84c7a9a 100644
--- a/drivers/media/IR/ir-core-priv.h
+++ b/drivers/media/IR/ir-core-priv.h
@@ -32,7 +32,7 @@ struct ir_raw_handler {
struct ir_raw_event_ctrl {
struct list_headlist; /* to keep track of raw
clients */
- struct work_struct rx_work;/* for the rx decoding
workqueue */
+ struct task_struct *thread;
struct kfifokfifo; /* fifo for the
pulse/space durations */
ktime_t last_event; /* when last event
occurred */
enum raw_event_type last_type; /* last event type */
diff --git a/drivers/media/IR/ir-raw-event.c b/drivers/media/IR/ir-raw-event.c
index 9d5c029..d0c18db 100644
--- a/drivers/media/IR/ir-raw-event.c
+++ b/drivers/media/IR/ir-raw-event.c
@@ -12,9 +12,10 @@
* GNU General Public License for more details.
*/
-#include
+#include
#include
#include
+#include
#include "ir-core-priv.h"
/* Define the max number of pulse/space transitions to buffer */
@@ -33,20 +34,30 @@ static u64 available_protocols;
static struct work_struct wq_load;
#endif
-static void ir_raw_event_work(struct work_struct *work)
+static int ir_raw_event_thread(void *data)
{
struct ir_raw_event ev;
struct ir_raw_handler *handler;
- struct ir_raw_event_ctrl *raw =
- container_of(work, struct ir_raw_event_ctrl, rx_work);
+ struct ir_raw_event_ctrl *raw = (struct ir_raw_event_ctrl *)data;
+
+ while (!kthread_should_stop()) {
+ try_to_freeze();
- while (kfifo_out(&raw->kfifo, &ev, sizeof(ev)) == sizeof(ev)) {
mutex_lock(&ir_raw_handler_lock);
- list_for_each_entry(handler, &ir_raw_handler_list, list)
- handler->decode(raw->input_dev, ev);
+
+ while (kfifo_out(&raw->kfifo, &ev, sizeof(ev)) == sizeof(ev)) {
+ list_for_each_entry(handler, &ir_raw_handler_list, list)
+ handler->decode(raw->input_dev, ev);
+ raw->prev_ev = ev;
+ }
+
mutex_unlock(&ir_raw_handler_lock);
- raw->prev_ev = ev;
+
+ set_current_state(TASK_INTERRUPTIBLE);
+ schedule();
}
+
+ return 0;
}
/**
@@ -141,7 +152,7 @@ void ir_raw_event_handle(struct input_dev *input_dev)
if (!ir->raw)
return;
- schedule_work(&ir->raw->rx_work);
+ wake_up_process(ir->raw->thread);
}
EXPORT_SYMBOL_GPL(ir_raw_event_handle);
@@ -170,7 +181,7 @@ int ir_raw_event_register(struct input_dev *input_dev)
return -ENOMEM;
ir->raw->input_dev = input_dev;
- INIT_WORK(&ir->raw->rx_work, ir_raw_event_work);
+
ir->raw->enabled_protocols = ~0;
rc = kfifo_alloc(&ir->raw->kfifo, sizeof(s64) * MAX_IR_EVENT_SIZE,
GFP_KERNEL);
@@ -180,6 +191,15 @@ int ir_raw_event_register(struct input_dev *input_dev)
return rc;
}
+ ir->raw->thread = kthread_run(ir_raw_event_thread, ir->raw,
+ "rc%u", (unsigned int)ir->devno);
+
+ if (IS_ERR(ir->raw->thread)) {
+ kfree(ir->raw);
+ ir->raw = NULL;
+ return PTR_ERR(ir->raw->thread);
+ }
+
mutex_lock(&ir_raw_handler_lock);
list_add_tail(&ir->raw->list, &ir_raw_client_list);
list_for_each_entry(handler, &ir_raw_handler_list, list)
@@ -198,7 +218,7 @@ void ir_raw_event_unregister(struct input_dev *input_dev)
if (!ir->raw)
return;
- cancel_work_sync(&ir->raw->rx_work);
+ kthread_stop(ir->raw->thread);
mutex_lock(&ir_raw_handler_lock);
list_del(&ir->raw->list);
--
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/13] IR: fix locking in ir_raw_event_work
On Thu, 2010-07-29 at 22:42 -0400, Andy Walls wrote: > On Fri, 2010-07-30 at 05:17 +0300, Maxim Levitsky wrote: > > It is prefectly possible to have ir_raw_event_work > > running concurently on two cpus, thus we must protect > > it from that situation. > > Yup, the work is marked as not pending (and hence reschedulable) just > before the work handler is run. > > > > Maybe better solution is to ditch the workqueue at all > > and use good 'ol thread per receiver, and just wake it up... > > I suppose you could also use a single threaded workqueue instead of a > mutex, and let a bit test provide exclusivity. With the mutex, when the > second thread finally obtains the lock, there will likely not be > anything for it to do. Mutex there is for another reason, to protect against decoder insert/removal. However, I think its best just to use a bare kthread for the purpose of this. Best regards, Maxim Levitsky -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/13] IR: fix locking in ir_raw_event_work
On Fri, 2010-07-30 at 05:17 +0300, Maxim Levitsky wrote:
> It is prefectly possible to have ir_raw_event_work
> running concurently on two cpus, thus we must protect
> it from that situation.
Yup, the work is marked as not pending (and hence reschedulable) just
before the work handler is run.
> Maybe better solution is to ditch the workqueue at all
> and use good 'ol thread per receiver, and just wake it up...
I suppose you could also use a single threaded workqueue instead of a
mutex, and let a bit test provide exclusivity. With the mutex, when the
second thread finally obtains the lock, there will likely not be
anything for it to do.
Regards,
Andy
> Signed-off-by: Maxim Levitsky
> ---
> drivers/media/IR/ir-raw-event.c |7 +--
> 1 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/IR/ir-raw-event.c b/drivers/media/IR/ir-raw-event.c
> index 9d5c029..4098748 100644
> --- a/drivers/media/IR/ir-raw-event.c
> +++ b/drivers/media/IR/ir-raw-event.c
> @@ -40,13 +40,16 @@ static void ir_raw_event_work(struct work_struct *work)
> struct ir_raw_event_ctrl *raw =
> container_of(work, struct ir_raw_event_ctrl, rx_work);
>
> + mutex_lock(&ir_raw_handler_lock);
> +
> while (kfifo_out(&raw->kfifo, &ev, sizeof(ev)) == sizeof(ev)) {
> - mutex_lock(&ir_raw_handler_lock);
> list_for_each_entry(handler, &ir_raw_handler_list, list)
> handler->decode(raw->input_dev, ev);
> - mutex_unlock(&ir_raw_handler_lock);
> raw->prev_ev = ev;
> }
> +
> + mutex_unlock(&ir_raw_handler_lock);
> +
> }
>
> /**
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 04/13] IR: fix locking in ir_raw_event_work
It is prefectly possible to have ir_raw_event_work
running concurently on two cpus, thus we must protect
it from that situation.
Maybe better solution is to ditch the workqueue at all
and use good 'ol thread per receiver, and just wake it up...
Signed-off-by: Maxim Levitsky
---
drivers/media/IR/ir-raw-event.c |7 +--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/media/IR/ir-raw-event.c b/drivers/media/IR/ir-raw-event.c
index 9d5c029..4098748 100644
--- a/drivers/media/IR/ir-raw-event.c
+++ b/drivers/media/IR/ir-raw-event.c
@@ -40,13 +40,16 @@ static void ir_raw_event_work(struct work_struct *work)
struct ir_raw_event_ctrl *raw =
container_of(work, struct ir_raw_event_ctrl, rx_work);
+ mutex_lock(&ir_raw_handler_lock);
+
while (kfifo_out(&raw->kfifo, &ev, sizeof(ev)) == sizeof(ev)) {
- mutex_lock(&ir_raw_handler_lock);
list_for_each_entry(handler, &ir_raw_handler_list, list)
handler->decode(raw->input_dev, ev);
- mutex_unlock(&ir_raw_handler_lock);
raw->prev_ev = ev;
}
+
+ mutex_unlock(&ir_raw_handler_lock);
+
}
/**
--
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
