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

[Marc Zyngier]

On 22/04/16 09:48, Jon Hunter wrote:
> 
> 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.

No, you're right. It could very well race against irq_dispose_mapping
(and the disassociate method doesn't provide enough guarantee).
Unfortunately, most of the irqdomain code still relies on this race not
happening. How to fix this is still a bit unclear to me.

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

There is none, unfortunately.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...