[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 <maximlevitsky@gmail.com>
---
 drivers/media/IR/ir-raw-event.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)
  

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 <maximlevitsky@gmail.com>
> ---
>  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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html