Re: [patch 1/2] genirq: add threaded interrupt handler support

[Christoph Hellwig]

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/