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

[Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>]

On 11/11/2015 12:13 PM, Jon Hunter wrote:
> 
> On 10/11/15 18:07, Lars-Peter Clausen wrote:
>> 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.
> 
> Yes, I agree that this does sound a bit odd, but ...
> 
>>> For example, during OF boot:
>>>
>>> [a]  irq_create_of_mapping()
>>>     - irq_create_fwspec_mapping()
>>>       - irq_set_irq_type()
> 
> The above means that if someone calls of_irq_get() (or
> platform_get_irq()), before request_irq(), then this will call
> irq_create_of_mapping() and hence, call irq_set_irq_type. So should
> irq_create_fwspec_mapping() be setting the type in the first place? I
> can see it is convenient to do it here.

In general there is another option - save OF-flags and pass them to
__setup_irq() where they can be processed.

> 
>>> or
[b]
>>> 	irq_set_irq_type(irq, IRQ_TYPE_LEVEL_HIGH);
>>> 	irq_set_chained_handler(irq, mx31ads_expio_irq_handler);

option: add "flag" parameter to irq_set_chained_handler

>>>
>>> or
[c]
>>> 	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().
> 
> Right. May be we can look at the various entry points to the chip
> operators to get a feel for which public APIs need to be handled.


Seems yes. But we need to be very careful with this, some of functions could be
called recursively (nested), like:
[d]
static int pcf857x_irq_set_wake(struct irq_data *data, unsigned int on)
{
	...
	error = irq_set_irq_wake(gpio->irq_parent, on);


Personally, I have nothing against irq_pm_(get|put) :) and thought about similar things
when tried to solve the same problem for omap-gpio driver.
But :(, I have to fall back to irq_bus_lock/sync_unlock, because of [a,b,c] - all above
APIs surrounded by chip_bus_lock/chip_bus_sync_unlock. ([d] - I've not hit it just because
I was lucky).


-- 
regards,
-grygorii