lkml.org 
[lkml]   [2009]   [Mar]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [patch 1/2] genirq: add threaded interrupt handler support
I like the API and concept of the patch a lot.

some minor comments:

> + switch (ret) {
> + case IRQ_WAKE_THREAD:
> + /*
> + * Wake up the handler thread for this
> + * action. In case the thread crashed and was
> + * killed we just pretend that we handled the
> + * interrupt. The hardirq handler above has
> + * disabled the device interrupt, so no irq
> + * storm is lurking.
> + */

We probably wants some sort of error check to catch drivers returning
IRQ_WAKE_THREAD without actually defining a threaded handler here.

> +static inline int irq_thread_should_run(struct irqaction *action)
> +{
> + return test_and_clear_bit(IRQTF_RUNTHREAD, &action->thread_flags);
> +}
> +
> +static int irq_wait_for_interrupt(struct irqaction *action)
> +{
> + while (!kthread_should_stop()) {
> + set_current_state(TASK_INTERRUPTIBLE);
> + if (irq_thread_should_run(action)) {

Does adding the helper for one use really help readability?

> + __set_current_state(TASK_RUNNING);
> + return 0;
> + } else
> + schedule();

No need for the else here :)

> + if (new->thread_fn) {
> + struct task_struct *t;
> +
> + t = kthread_create(irq_thread, new, "irq/%d-%s", irq,
> + new->name);
> + if (IS_ERR(t))
> + return PTR_ERR(t);
> + /*
> + * We keep the reference to the task struct even if
> + * the thread dies to avoid that the interrupt code
> + * references an already freed task_struct.
> + */
> + get_task_struct(t);
> + new->thread = t;
> + wake_up_process(t);
> + }

We should really introduce a refcounted variant of the kthread helpers.
Not an argument against the patch, just a public mental note.

> /**
> - * request_irq - allocate an interrupt line
> + * request_threaded_irq - allocate an interrupt line
> * @irq: Interrupt line to allocate
> - * @handler: Function to be called when the IRQ occurs
> + * @handler: Function to be called when the IRQ occurs.
> + * Primary handler for threaded interrupts
> + * @thread_fn: Function called from the irq handler thread
> + * If NULL, no irq thread is created

The indentation is messed up here.



\
 
 \ /
  Last update: 2009-03-23 19:59    [W:0.089 / U:0.600 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site