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 for Android: free password hash cracker in your pocket

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

Message-ID: <20090323185618.GA11432@infradead.org>
Date:	Mon, 23 Mar 2009 14:56:18 -0400
From:	Christoph Hellwig <hch@...radead.org>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>,
	Peter Zijlstra <peterz@...radead.org>,
	Christoph Hellwig <hch@...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

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/