From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S268986AbUHZO3O (ORCPT ); Thu, 26 Aug 2004 10:29:14 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S269029AbUHZO27 (ORCPT ); Thu, 26 Aug 2004 10:28:59 -0400 Received: from cantor.suse.de ([195.135.220.2]:18826 "EHLO Cantor.suse.de") by vger.kernel.org with ESMTP id S268986AbUHZO1A (ORCPT ); Thu, 26 Aug 2004 10:27:00 -0400 Date: Thu, 26 Aug 2004 16:27:00 +0200 Message-ID: From: Takashi Iwai To: Russell King Cc: Andrew Morton , linux-kernel@vger.kernel.org, thomas@undata.org Subject: Re: [PATCH] Fix shared interrupt handling of SA_INTERRUPT and SA_SAMPLE_RANDOM In-Reply-To: <20040826151832.G21364@flint.arm.linux.org.uk> References: <20040824204508.3b31449f.akpm@osdl.org> <20040825134112.5aefaf8e.akpm@osdl.org> <20040826150404.D21364@flint.arm.linux.org.uk> User-Agent: Wanderlust/2.10.1 (Watching The Wheels) SEMI/1.14.5 (Awara-Onsen) FLIM/1.14.5 (Demachiyanagi) APEL/10.6 MULE XEmacs/21.4 (patch 15) (Security Through Obscurity) (i386-suse-linux) MIME-Version: 1.0 (generated by SEMI 1.14.5 - "Awara-Onsen") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org At Thu, 26 Aug 2004 15:18:32 +0100, Russell King wrote: > > On Thu, Aug 26, 2004 at 04:10:54PM +0200, Takashi Iwai wrote: > > At Thu, 26 Aug 2004 15:04:04 +0100, > > Russell King wrote: > > > > > > On Thu, Aug 26, 2004 at 02:50:52PM +0200, Takashi Iwai wrote: > > > > At Wed, 25 Aug 2004 13:41:12 -0700, > > > > Andrew Morton wrote: > > > > > > > > > > Takashi Iwai wrote: > > > > > > > > > > > > Anyway, suppressing the unnecessary call of add_interrupt_randomness() > > > > > > should be still valid. The reduced patch is below. > > > > (snip) > > > > > > > > > > Shouldn't that be `if (ret == IRQ_HANDLED)'? > > > > > > > > Yes, it's more strict. > > > > > > I don't think so. Look at what's going on. If "ret" is IRQ_HANDLED > > > all well and fine. However, look at how "retval" is being used: > > > > > > static void __report_bad_irq(int irq, irq_desc_t *desc, irqreturn_t action_ret) > > > { > > > ... > > > if (action_ret != IRQ_HANDLED && action_ret != IRQ_NONE) { > > > printk(KERN_ERR "irq event %d: bogus return value %x\n", > > > irq, action_ret); > > > } else { > > > printk(KERN_ERR "irq %d: nobody cared!\n", irq); > > > } > > > > > > So, we're looking to see not only if a handler returned IRQ_HANDLED, > > > but also if a handler returned _some other value_ other than IRQ_HANDLED > > > or IRQ_NONE. > > > > But obviously any other value is invalid as shown above, so we > > shouldn't take it seriously as the correct return value for > > triggering add_random_interrupt(). > > Point is: you're killing the method for detecting when IRQ handlers > return bogus return codes since you only look at them when they > respond with IRQ_HANDLED. > > So, you're disabling the "bogus return value" check. Completely. No, the return value from handle_IRQ_event won't be changed at all by my patch. asmlinkage int handle_IRQ_event(unsigned int irq, ... { ... do { ret = action->handler(irq, action->dev_id, regs); ==> if (ret == IRQ_HANDLED) ==> status |= action->flags; ==> retval |= ret; action = action->next; } while (action); if (status & SA_SAMPLE_RANDOM) add_interrupt_randomness(irq); local_irq_disable(); return retval; } The patch adds the additional check for another variable 'status' which is used only for checking SA_SAMPLE_RANDOM. Takashi