linux-kernel - Re: [RFC PATCH 1/2] genirq: Add runtime resume/suspend support for IRQ chips

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:	Tue, 10 Nov 2015 19:07:01 +0100
From:	Lars-Peter Clausen <lars@...afoo.de>
To:	Grygorii Strashko <grygorii.strashko@...com>,
	Jon Hunter <jonathanh@...dia.com>,
	Thomas Gleixner <tglx@...utronix.de>
CC:	Jason Cooper <jason@...edaemon.net>,
	Marc Zyngier <marc.zyngier@....com>,
	Stephen Warren <swarren@...dotorg.org>,
	Thierry Reding <thierry.reding@...il.com>,
	Kevin Hilman <khilman@...nel.org>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	LKML <linux-kernel@...r.kernel.org>, linux-tegra@...r.kernel.org,
	Soren Brinkmann <soren.brinkmann@...inx.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Alexandre Courbot <gnurou@...il.com>
Subject: Re: [RFC PATCH 1/2] genirq: Add runtime resume/suspend support for
 IRQ chips

On 11/10/2015 05:47 PM, Grygorii Strashko wrote:
[...]
>> I was trying to simplify matters by placing the resume call in
>> __setup_irq() as opposed to requested_threaded_irq(). However, the would
>> mean the resume is inside the bus_lock and may be I should not assume
>> that I can sleep here.
>>
>>> Can you folks please agree on something which is correct and complete?
>>
>> Soren I am happy to defer to your patch and drop this. My only comment
>> would be what about the request_percpu_irq() path in your patch?
>>
> 
> I have the same comment here as I asked Soren:
> 1) There are no restrictions to call irq set_irq_type() whenever,
> as result HW can be accessed before request_x_irq()/__setup_irq().
> And this is used quite widely now :(
> 

Changing the configuration of a resource that is not owned seems to be
fairly broken. In the worst case this will overwrite the configuration that
was set by owner of the resource.

Especially those that call irq_set_irq_type() directly before request_irq(),
given that you supply the trigger type to request_irq() which will make sure
that there are no conflicts and the configure.

This is a bit like calling gpio_set_direction() before you call
gpio_request(), which will also have PM issues.

> For example, during OF boot:
> 
> [a]  irq_create_of_mapping()
>    - irq_create_fwspec_mapping()
>      - irq_set_irq_type()
> 
> or 
> 	irq_set_irq_type(irq, IRQ_TYPE_LEVEL_HIGH);
> 	irq_set_chained_handler(irq, mx31ads_expio_irq_handler);
> 
> or
> 	irq_set_irq_type(alarm_irq, IRQ_TYPE_EDGE_BOTH);
> 	err = devm_request_irq(&pdev->dev, alarm_irq, fan_alarm_irq_handler,
> (there are ~200 occurrences of irq set_irq_type in Kernel)
> 
> 2) if i'm not wrong, the same is valid for irq_set_irq_wake() and irq_set_affinity()
> 
> I'm not saying all these code is correct, but that what's now in kernel :(
> I've tried to test Soren's patch with omap-gpio and immediately hit case [a] :.( 

All functions for which are part of the public API and for which it is legal
to call them without calling request_irq() (or similar) first will need to
have pm_get()/pm_put().
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/