Messages in this thread | | | Date | Mon, 23 Mar 2009 14:56:18 -0400 | From | Christoph Hellwig <> | Subject | Re: [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.
| |