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

[Thomas Gleixner <tglx@linutronix.de>]

Christoph,

On Mon, 23 Mar 2009, Christoph Hellwig wrote:

> 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.

Yep, got lost in the restructuring.

> > +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 :)

Sigh. I was staring at this construct for a while because it looked
strange :)
 
> > +	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.

Thought about that already, but did not come around to implement it.
 
> >  /**
> > - *	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.

Thanks for the review ! 

       tglx