linux-kernel - Re: [RFC PATCH] irq: handle private interrupt registration

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]

Message-Id: <20100601152652.9296c5d0.akpm@linux-foundation.org>
Date:	Tue, 1 Jun 2010 15:26:52 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	adharmap@...eaurora.org
Cc:	tglx@...utronix.de,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Trilok Soni <soni.trilok@...il.com>,
	Pavel Machek <pavel@....cz>,
	Brian Swetland <swetland@...gle.com>,
	Joonyoung Shim <jy0922.shim@...sung.com>,
	m.szyprowski@...sung.com, t.fujak@...sung.com,
	kyungmin.park@...sung.com, David Brownell <david-b@...bell.net>,
	Daniel Ribeiro <drwyrm@...il.com>, arve@...roid.com,
	Barry Song <21cnbao@...il.com>,
	Russell King <linux@....linux.org.uk>,
	Bryan Huntsman <bryanh@...cinc.com>,
	Iliyan Malchev <malchev@...gle.com>,
	Michael Buesch <mb@...sch.de>,
	Bruno Premont <bonbons@...ux-vserver.org>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] irq: handle private interrupt registration

On Wed, 26 May 2010 13:29:54 -0700
adharmap@...eaurora.org wrote:

> From: Abhijeet Dharmapurikar <adharmap@...eaurora.org>
> 
> The current code fails to register a handler for the same irq
> without taking in to account that it could be a per cpu interrupt.
> If the IRQF_PERCPU flag is set, enable the interrupt on that cpu
> and return success.
> 
> Change-Id: I748b3aa08d794342ad74cbd0bb900cc599f883a6
> Signed-off-by: Abhijeet Dharmapurikar <adharmap@...eaurora.org>
> ---
> 
> On systems with an interrupt controller that supports
> private interrupts per core, it is not possible to call 
> request_irq/setup_irq from multiple cores for the same irq. This is because
> the second+ invocation of __setup_irq checks if the previous
> hndler had a IRQ_SHARED flag set and errors out if not. 
> 
> The current irq handling code doesnt take in to account what cpu it 
> is executing on.  Usually the local interrupt controller registers are banked 
> per cpu a.k.a. a cpu can enable its local interrupt by writing to its banked 
> registers.
> 
> One way to get around this problem is to call the setup_irq on a single cpu 
> while other cpus simply enable their private interrupts by writing to their 
> banked registers
> 
> For eg. code in arch/arm/time/smp_twd.c
> 	/* Make sure our local interrupt controller has this enabled */
> 	local_irq_save(flags);
> 	get_irq_chip(clk->irq)->unmask(clk->irq);
> 	local_irq_restore(flags);
> 
> This looks like a hacky way to get local interrupts working on 
> multiple cores.
> 
> The patch adds a check for PERCPU flag in __setup_irq - if an handler is 
> present it simply enables that interrupt for that core and returns 0.
> 
> ...
>  
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -683,6 +683,37 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>  	old_ptr = &desc->action;
>  	old = *old_ptr;
>  	if (old) {
> +#if defined(CONFIG_IRQ_PER_CPU)
> +		/* All handlers must agree on per-cpuness */
> +		if ((old->flags & IRQF_PERCPU) !=
> +			(new->flags & IRQF_PERCPU))
> +			goto mismatch;
> +
> +		if (old->flags & IRQF_PERCPU) {
> +			/* the chip must have been set for this interrupt*/
> +			if (!(desc->status & IRQ_NOAUTOEN)) {
> +				desc->depth = 0;
> +				desc->status &= ~IRQ_DISABLED;
> +				desc->chip->startup(irq);
> +			} else
> +				/* Undo nested disables: */
> +				desc->depth = 1;
> +
> +			spin_unlock_irqrestore(&desc->lock, flags);

The rest of the code uses raw_spin_unlock_irqrestore().

I don't know _why_ is uses this.  There are no code comments, and the
239007b8440abff689632f50cdf0f2b9e895b534 changelog is:

: genirq: Convert irq_desc.lock to raw_spinlock
:    
: Convert locks which cannot be sleeping locks in preempt-rt to
: raw_spinlocks.

which is pathetically useless.

But I suppose we should ignorantly copy it and hope we're not screwing
something up.

> +			if (new->thread)
> +				wake_up_process(new->thread);
> +			return 0;
> +		}
> +#endif
> +
> +		/* they are the same types and same handler
> +		 * perhaps it is a private cpu interrupt
> +		 */
> +		if (old->flags == new->flags
> +			&& old->handler == new->handler)
> +			setup_affinity(irq, desc);
> +			return 0;

And this appears to have forgotten to undo the lock altogether, which
makes one wonder about the testing coverage.

It also embeds a `return' statement deep inside a huge and complex
function, which is invariably bad.

And in so doing it bypasses the register_irq_proc() and
register_handler_proc() calls.  I have no way of knowing whether that
was deliberate or whether it was a bug.  If it was deliberate then some
code and'/or changelog commentary is needed, so that others don't think
that it is a bug too.


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