[3/9] IR: replace spinlock with mutex.
Commit Message
Some handlers (lirc for example) allocates memory on initialization,
doing so in atomic context is cumbersome.
Fixes warning about sleeping function in atomic context.
Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
---
drivers/media/IR/ir-raw-event.c | 28 ++++++++++++++--------------
1 files changed, 14 insertions(+), 14 deletions(-)
Comments
Em 28-07-2010 12:14, Maxim Levitsky escreveu:
> Some handlers (lirc for example) allocates memory on initialization,
> doing so in atomic context is cumbersome.
> Fixes warning about sleeping function in atomic context.
You should not replace it by a mutex, as the decoding code may happen during
IRQ time on several drivers.
If lirc is allocating memory, it should be using GFP_ATOMIC to avoid sleeping.
Cheers,
Mauro
--
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 Wed, 2010-07-28 at 13:03 -0300, Mauro Carvalho Chehab wrote:
> Em 28-07-2010 12:14, Maxim Levitsky escreveu:
> > Some handlers (lirc for example) allocates memory on initialization,
> > doing so in atomic context is cumbersome.
> > Fixes warning about sleeping function in atomic context.
>
> You should not replace it by a mutex, as the decoding code may happen during
> IRQ time on several drivers.
I though decoding code is run by a work queue?
I don't see any atomic codepath here...
>
> If lirc is allocating memory, it should be using GFP_ATOMIC to avoid sleeping.
If its really not possible, I can make lirc use GFP_ATOMIC. a bit ugly,
but should work.
Best regards,
--
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 Wed, Jul 28, 2010 at 07:32:58PM +0300, Maxim Levitsky wrote:
> On Wed, 2010-07-28 at 13:03 -0300, Mauro Carvalho Chehab wrote:
> > Em 28-07-2010 12:14, Maxim Levitsky escreveu:
> > > Some handlers (lirc for example) allocates memory on initialization,
> > > doing so in atomic context is cumbersome.
> > > Fixes warning about sleeping function in atomic context.
> >
> > You should not replace it by a mutex, as the decoding code may happen during
> > IRQ time on several drivers.
> I though decoding code is run by a work queue?
Yeah, it is. (INIT_WORK(&ir->raw->rx_work, ir_raw_event_work); in
ir_raw_event_register).
> I don't see any atomic codepath here...
I think the ir_raw_event_store variants are the only things that are run
from an interrupt context, and none of them touch ir_raw_handler_lock.
That lock is advertised as being for the protection of ir_raw_handler_list
and ir_raw_client_list, which are primarily manipulated by
register/unregister functions, and we just lock them when doing actual IR
decode work (via said work queue) so we don't feed raw IR somewhere that
we shouldn't. I think Maxim is correct here, we should be okay with
changing this to a mutex, unless I'm missing something else.
Em 28-07-2010 14:43, Jarod Wilson escreveu:
> On Wed, Jul 28, 2010 at 07:32:58PM +0300, Maxim Levitsky wrote:
>> On Wed, 2010-07-28 at 13:03 -0300, Mauro Carvalho Chehab wrote:
>>> Em 28-07-2010 12:14, Maxim Levitsky escreveu:
>>>> Some handlers (lirc for example) allocates memory on initialization,
>>>> doing so in atomic context is cumbersome.
>>>> Fixes warning about sleeping function in atomic context.
>>>
>>> You should not replace it by a mutex, as the decoding code may happen during
>>> IRQ time on several drivers.
>> I though decoding code is run by a work queue?
>
> Yeah, it is. (INIT_WORK(&ir->raw->rx_work, ir_raw_event_work); in
> ir_raw_event_register).
>
>> I don't see any atomic codepath here...
>
> I think the ir_raw_event_store variants are the only things that are run
> from an interrupt context, and none of them touch ir_raw_handler_lock.
> That lock is advertised as being for the protection of ir_raw_handler_list
> and ir_raw_client_list, which are primarily manipulated by
> register/unregister functions, and we just lock them when doing actual IR
> decode work (via said work queue) so we don't feed raw IR somewhere that
> we shouldn't. I think Maxim is correct here, we should be okay with
> changing this to a mutex, unless I'm missing something else.
You're probably right. The previous code used to do this at IRQ time, but a latter
patch changed it to happen via a workqueue.
So, I'm OK with this patch.
Cheers,
Mauro.
--
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
@@ -13,7 +13,7 @@
*/
#include <linux/workqueue.h>
-#include <linux/spinlock.h>
+#include <linux/mutex.h>
#include <linux/sched.h>
#include "ir-core-priv.h"
@@ -24,7 +24,7 @@
static LIST_HEAD(ir_raw_client_list);
/* Used to handle IR raw handler extensions */
-static DEFINE_SPINLOCK(ir_raw_handler_lock);
+static DEFINE_MUTEX(ir_raw_handler_lock);
static LIST_HEAD(ir_raw_handler_list);
static u64 available_protocols;
@@ -41,10 +41,10 @@ static void ir_raw_event_work(struct work_struct *work)
container_of(work, struct ir_raw_event_ctrl, rx_work);
while (kfifo_out(&raw->kfifo, &ev, sizeof(ev)) == sizeof(ev)) {
- spin_lock(&ir_raw_handler_lock);
+ mutex_lock(&ir_raw_handler_lock);
list_for_each_entry(handler, &ir_raw_handler_list, list)
handler->decode(raw->input_dev, ev);
- spin_unlock(&ir_raw_handler_lock);
+ mutex_unlock(&ir_raw_handler_lock);
raw->prev_ev = ev;
}
}
@@ -150,9 +150,9 @@ u64
ir_raw_get_allowed_protocols()
{
u64 protocols;
- spin_lock(&ir_raw_handler_lock);
+ mutex_lock(&ir_raw_handler_lock);
protocols = available_protocols;
- spin_unlock(&ir_raw_handler_lock);
+ mutex_unlock(&ir_raw_handler_lock);
return protocols;
}
@@ -180,12 +180,12 @@ int ir_raw_event_register(struct input_dev *input_dev)
return rc;
}
- spin_lock(&ir_raw_handler_lock);
+ 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)
if (handler->raw_register)
handler->raw_register(ir->raw->input_dev);
- spin_unlock(&ir_raw_handler_lock);
+ mutex_unlock(&ir_raw_handler_lock);
return 0;
}
@@ -200,12 +200,12 @@ void ir_raw_event_unregister(struct input_dev *input_dev)
cancel_work_sync(&ir->raw->rx_work);
- spin_lock(&ir_raw_handler_lock);
+ mutex_lock(&ir_raw_handler_lock);
list_del(&ir->raw->list);
list_for_each_entry(handler, &ir_raw_handler_list, list)
if (handler->raw_unregister)
handler->raw_unregister(ir->raw->input_dev);
- spin_unlock(&ir_raw_handler_lock);
+ mutex_unlock(&ir_raw_handler_lock);
kfifo_free(&ir->raw->kfifo);
kfree(ir->raw);
@@ -220,13 +220,13 @@ int ir_raw_handler_register(struct ir_raw_handler *ir_raw_handler)
{
struct ir_raw_event_ctrl *raw;
- spin_lock(&ir_raw_handler_lock);
+ mutex_lock(&ir_raw_handler_lock);
list_add_tail(&ir_raw_handler->list, &ir_raw_handler_list);
if (ir_raw_handler->raw_register)
list_for_each_entry(raw, &ir_raw_client_list, list)
ir_raw_handler->raw_register(raw->input_dev);
available_protocols |= ir_raw_handler->protocols;
- spin_unlock(&ir_raw_handler_lock);
+ mutex_unlock(&ir_raw_handler_lock);
return 0;
}
@@ -236,13 +236,13 @@ void ir_raw_handler_unregister(struct ir_raw_handler *ir_raw_handler)
{
struct ir_raw_event_ctrl *raw;
- spin_lock(&ir_raw_handler_lock);
+ mutex_lock(&ir_raw_handler_lock);
list_del(&ir_raw_handler->list);
if (ir_raw_handler->raw_unregister)
list_for_each_entry(raw, &ir_raw_client_list, list)
ir_raw_handler->raw_unregister(raw->input_dev);
available_protocols &= ~ir_raw_handler->protocols;
- spin_unlock(&ir_raw_handler_lock);
+ mutex_unlock(&ir_raw_handler_lock);
}
EXPORT_SYMBOL(ir_raw_handler_unregister);