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]

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

On Thu, Jan 28, 2016 at 12:49:37PM +0100, Thomas Gleixner 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?
>

In general, agreed. In this case though I think they get some grace - my
devices are all in an FPGA which only has one interrupt line to the SoC.

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

Right, there's some redundancy here.

>
>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'.

This 'fix' is what makes me hit the problem - but even without it I
think the problem still exists.

It seems like in principle two drivers ought to be able to do

	request_irq(irq, handler, IRQF_SHARED | IRQF_TRIGGER_HIGH, ...);

and

	request_irq(irq, handler, IRQF_SHARED, ...);

without the latter call failing. Or do you disagree?

-Brian

>
>Thanks,
>
>	tglx
>