Re: Re: [PATCH] IRQ: Add a check for uninitialized irqs.

From: Raghavendra D Prabhu <raghu.prabhu13@gmail.com>
To: Paul Gortmaker <paul.gortmaker@windriver.com>
Cc: jeremy@goop.org, mingo@redhat.com, x86@kernel.org,
	Thomas Gleixner <tglx@linutronix.de>,
	"H. Peter Anvin" <hpa@zytor.com>,
	liu chuansheng <chuansheng.liu@intel.com>,
	Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re:  Re: [PATCH] IRQ: Add a check for uninitialized irqs.
Date: Sat, 15 Sep 2012 08:25:02 +0530	[thread overview]
Message-ID: <20120915025502.GB58426@Archie.local> (raw)
In-Reply-To: <504DEE1D.3040704@windriver.com>

[-- Attachment #1: Type: text/plain, Size: 2798 bytes --]

Hi,

* On Mon, Sep 10, 2012 at 09:41:49AM -0400, Paul Gortmaker <paul.gortmaker@windriver.com> wrote:
>On 12-09-09 03:25 PM, raghu.prabhu13@gmail.com wrote:
>> From: Raghavendra D Prabhu <rprabhu@wnohang.net>
>>
>> Errors like "do_IRQ: 0.84 No Irq handler for vector (irq -1)" have been reported
>> earlier filling up the console/logs. So this adds a condition to check for
>
>Reported by who, and where, and on what hardware, and under what
>circumstances?

I have seen it reported in many places like 
https://bugzilla.redhat.com/show_bug.cgi?id=225399 and 
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/480997

>
>You are adding another if statement to the hot path of do_IRQ just
>to mask some random unknown symptom. instead of trying to understand
>(and then solve) what the real breakage is.  Which is not the right
>approach.

I just realized that handler_irq checks for < NR_IRQS where the 
underflow will fail and will return. So this patch is not required.

However,  I think we need to fix the unsigned / int mixup by 
assigning vector_irq[..] to NR_IRQS+1  as guard value and 
changing unsigned to signed as in patch.
>
>Paul.
>--
>
>> uninitialized irqs so that it exits early and doesn't proceed further. Also, irq
>> is made a signed integer, since if it is not mapped to a vector, it will be
>> assigned -1 which will be UINT_MAX otherwise.
>>
>> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>> ---
>>  arch/x86/kernel/irq.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
>> index d44f782..ddba63a 100644
>> --- a/arch/x86/kernel/irq.c
>> +++ b/arch/x86/kernel/irq.c
>> @@ -184,21 +184,24 @@ unsigned int __irq_entry do_IRQ(struct pt_regs *regs)
>>
>>  	/* high bit used in ret_from_ code  */
>>  	unsigned vector = ~regs->orig_ax;
>> -	unsigned irq;
>> +	int irq;
>>
>>  	irq_enter();
>>  	exit_idle();
>>
>> -	irq = __this_cpu_read(vector_irq[vector]);
>> +	if (unlikely((irq = __this_cpu_read(vector_irq[vector])) == -1)) {
>> +		pr_emerg_ratelimited("IRQ handler not setup for vector %u", vector);
>> +		goto err_exit;
>> +	}
>>
>>  	if (!handle_irq(irq, regs)) {
>>  		ack_APIC_irq();
>>
>> -		if (printk_ratelimit())
>> -			pr_emerg("%s: %d.%d No irq handler for vector (irq %d)\n",
>> -				__func__, smp_processor_id(), vector, irq);
>> +		pr_emerg_ratelimited("%s: %d.%d No irq handler for vector (irq %u)\n",
>> +			__func__, smp_processor_id(), vector, irq);
>>  	}
>>
>> +err_exit:
>>  	irq_exit();
>>
>>  	set_irq_regs(old_regs);
>>
>

Regards,
-- 
Raghavendra Prabhu
GPG Id : 0xD72BE977
Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
www: wnohang.net

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]