Re: [PATCH V2 06/14] irqdomain: Don't set type when mapping an IRQ

[Jon Hunter]

On 22/04/16 09:22, Marc Zyngier wrote:
> Hi Jon,
> 
> On 21/04/16 16:45, Jon Hunter wrote:
>>
>> On 20/04/16 12:03, Jon Hunter wrote:
>>> Some IRQ chips, such as GPIO controllers or secondary level interrupt
>>> controllers, may require require additional runtime power management
>>> control to ensure they are accessible. For such IRQ chips, it makes sense
>>> to enable the IRQ chip when interrupts are requested and disabled them
>>> again once all interrupts have been freed.
>>>
>>> When mapping an IRQ, the IRQ type settings are read and then programmed.
>>> The mapping of the IRQ happens before the IRQ is requested and so the
>>> programming of the type settings occurs before the IRQ is requested. This
>>> is a problem for IRQ chips that require additional power management
>>> control because they may not be accessible yet. Therefore, when mapping
>>> the IRQ, don't program the type settings, just save them and then program
>>> these saved settings when the IRQ is requested (so long as if they are not
>>> overridden via the call to request the IRQ).
>>>
>>> Add a stub function for irq_domain_free_irqs() to avoid any compilation
>>> errors when CONFIG_IRQ_DOMAIN_HIERARCHY is not selected.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh@xxxxxxxxxx>
>>> ---
>>>  include/linux/irqdomain.h |  3 +++
>>>  kernel/irq/irqdomain.c    | 17 +++++++++++++----
>>>  2 files changed, 16 insertions(+), 4 deletions(-)
>>
>> [snip]
>>  
>>> -	/* Set type if specified and different than the current one */
>>> -	if (type != IRQ_TYPE_NONE &&
>>> -	    type != irq_get_trigger_type(virq))
>>> -		irq_set_irq_type(virq, type);
>>> +	irq_data = irq_get_irq_data(virq);
>>> +	if (!irq_data) {
>>> +		if (irq_domain_is_hierarchy(domain))
>>> +			irq_domain_free_irqs(virq, 1);
>>> +		else
>>> +			irq_dispose_mapping(virq);
>>> +		return 0;
>>> +	}
>>> +
>>> +	/* Store trigger type */
>>> +	irqd_set_trigger_type(irq_data, type);
>>> +
>>
>> I appear to have missed another place for saving the irq type
>> which I had change in this version. Next time I will triple
>> check! Should have been ...
>>
>>
>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
>> index 2aed04396210..fc66876d1965 100644
>> --- a/include/linux/irqdomain.h
>> +++ b/include/linux/irqdomain.h
>> @@ -440,6 +440,9 @@ static inline int irq_domain_alloc_irqs(struct irq_domain *domain,
>>         return -1;
>>  }
>>  
>> +static inline void irq_domain_free_irqs(unsigned int virq,
>> +                                       unsigned int nr_irqs) { }
>> +
>>  static inline bool irq_domain_is_hierarchy(struct irq_domain *domain)
>>  {
>>         return false;
>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> index 3d6ef5527b71..46ecf5d468b2 100644
>> --- a/kernel/irq/irqdomain.c
>> +++ b/kernel/irq/irqdomain.c
>> @@ -569,6 +569,7 @@ static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
>>  unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>  {
>>         struct irq_domain *domain;
>> +       struct irq_data *irq_data;
>>         irq_hw_number_t hwirq;
>>         unsigned int type = IRQ_TYPE_NONE;
>>         int virq;
>> @@ -619,7 +620,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>                  * it now and return the interrupt number.
>>                  */
>>                 if (IRQ_TYPE_NONE == irq_get_trigger_type(virq)) {
>> -                       irq_set_irq_type(virq, type);
>> +                       irq_data = irq_get_irq_data(virq);
>> +                       if (!irq_data)
>> +                               return 0;
> 
> Can you point out in which circumstances irq_data can be NULL? If we can
> lookup the interrupt in the domain, it'd better exist somewhere. Or am I
> missing something obvious?

To be honest, I was not 100% sure if this could be possible if we find
an existing mapping or if there were any paths that could race. So if we
can guarantee that it is not NULL here, I can drop this and simplify the
change.

By the way, is there any sort of reference count for mapping an IRQ so
that if irq_create_fwspec_mapping() is called more than once for an IRQ,
it is not freed on the first call to irq_dispose_mapping()? I assume
there is but could not see where this is handled.

Cheers
Jon