From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935064AbcA1LKs (ORCPT ); Thu, 28 Jan 2016 06:10:48 -0500 Received: from foss.arm.com ([217.140.101.70]:59400 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934802AbcA1LKp (ORCPT ); Thu, 28 Jan 2016 06:10:45 -0500 Date: Thu, 28 Jan 2016 11:10:42 +0000 From: Brian Starkey To: Thomas Gleixner Cc: linux-kernel@vger.kernel.org, marc.zyngier@arm.com Subject: Re: [PATCH] genirq: fix trigger flags check for shared irqs Message-ID: <20160128111041.GB12884@e106950-lin.cambridge.arm.com> References: <96c068ece0f5d01fc85bdbd9e0798f68180eebb8.1453204950.git.brian.starkey@arm.com> <20160128095955.GA12884@e106950-lin.cambridge.arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jan 28, 2016 at 11:29:21AM +0100, Thomas Gleixner wrote: >Brian, > >On Thu, 28 Jan 2016, Brian Starkey wrote: >> On Tue, Jan 26, 2016 at 09:45:32PM +0100, Thomas Gleixner wrote: >> > On Tue, 26 Jan 2016, Brian Starkey wrote: >> > >> > > For shared interrupts, if one requester passes in any IRQF_TRIGGER_* >> > > flags whilst another doesn't, __setup_irq() can erroneously fail. >> > > >> > > The no-flags case should be treated as "already configured", so change >> > > __setup_irq() to only check that the flags match if any have been >> > > provided. >> > >> > What happens if that "already configured", i.e. the default setting, is >> > conflicting with the newly requested interrupt? >> > >> > I rather prefer the failure than the resulting silent wreckage. >> > >> >> Yes, I agree that would be best avoided. It seems to me that this case >> is actually handled a bit lower down: >> >> } else if (new->flags & IRQF_TRIGGER_MASK) { >> unsigned int nmsk = new->flags & IRQF_TRIGGER_MASK; >> unsigned int omsk = irq_settings_get_trigger_mask(desc); >> >> if (nmsk != omsk) >> /* hope the handler works with current trigger mode >> */ >> pr_warning("irq %d uses trigger mode %u; requested >> %u\n", >> irq, nmsk, omsk); >> } >> >> Perhaps that should be louder/fatal? > >Perhaps. So what's the actual problem case you are trying to solve? I've got a few devices on the same interrupt line. One driver does something along these lines: res = platform_get_resource(dev, IORESOURCE_IRQ, 0); flags = (res->flags & IRQF_TRIGGER_MASK) | IRQF_SHARED; request_irq(res->start, handler, flags, "name", dev); This seems pretty reasonable. The problem is since 4a43d686fe33: of/irq: Pass trigger type in IRQ resource flags[1] the trigger type information from device-tree is in res->flags. So when the other drivers don't pass in any flags, they fail the check in __setup_irq(). Changing the former driver to remove the flags doesn't seem right, and adding flags to the latter would imply adding flags to _every_ driver, which is an awful lot to change - and I'm not sure it would be possible and/or effective in all cases. Cheers, -Brian [1]http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4a43d686fe336cc0e955c4400ba4d3fcff788786 > >Thanks, > > tglx >