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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 23 Mar 2009 20:10:22 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Christoph Hellwig <hch@...radead.org>
cc:	LKML <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>,
	Peter Zijlstra <peterz@...radead.org>,
	Arjan van de Veen <arjan@...radead.org>,
	Jon Masters <jonathan@...masters.org>,
	Sven Dietrich <sdietrich@...e.de>,
	David Brownell <david-b@...bell.net>
Subject: Re: [patch 1/2] genirq: add threaded interrupt handler support

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