linux-kernel - Re: [PATCH] genirq: fix trigger flags check for shared irqs

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]

Date:	Thu, 28 Jan 2016 09:58:08 -0600
From:	Rob Herring <robh+dt@...nel.org>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	Brian Starkey <brian.starkey@....com>,
	LKML <linux-kernel@...r.kernel.org>,
	Marc Zyngier <marc.zyngier@....com>,
	Tomasz Figa <tomasz.figa@...il.com>,
	Frank Rowand <frowand.list@...il.com>,
	Grant Likely <grant.likely@...aro.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH] genirq: fix trigger flags check for shared irqs

On Thu, Jan 28, 2016 at 5:49 AM, Thomas Gleixner <tglx@...utronix.de> wrote:
> On Thu, 28 Jan 2016, Brian Starkey wrote:
>> I've got a few devices on the same interrupt line. One driver does
>
> Just for the record: When will hardware folks finally understand that shared
> interrupt lines are a nightmare?
>
>> 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.
>
> So that commit does:
>
>    r->flags = IORESOURCE_IRQ | irqd_get_trigger_type(irq_get_irq_data(irq));
>
> which reads the current setting of the interrupt line.
>
> Now we pass exactly that to request_irq(). So first irq_of_parse_and_map()
> configures the interrupt type when mapping it and then hands in the same type
> information when requesting the irq.
>
> I have no idea what the purpose of this is and the changelog of that commit is
> completely useless, sigh!
>
> I've cc'ed the author and the device tree folks. Perhaps are they able to
> explain what this commit tries to 'fix'.

It's certainly not clear what driver was being fixed. I think the
intention was to provide the trigger flags from the DT to drivers in a
standard, non-DT specific way. The implementation of it certainly
seems convoluted. The usecase I can think of is drivers may need the
trigger information for cases where the device can support different
trigger modes/polarity. The DT says which trigger mode to use and the
device and irqchip need to be programmed to match. IIRC, SMSC ethernet
chips frequently used on ARM boards do this.

Rob