Re: [PATCH 06/15] irqdomain: Ensure type settings match for an existing mapping

[Jon Hunter]

On 17/03/16 14:19, Jon Hunter wrote:
> When mapping an IRQ, if a mapping already exists, then we simply return
> the virtual IRQ number. However, we do not check that the type settings
> for the existing mapping match those for the mapping that is about to be
> created. It may be unlikely that the type settings would not match, but
> check for this and don't return a valid IRQ mapping if the type settings
> do not match.
> 
> WARN if the type return by irq_domain_translate() has bits outside the
> sense mask set and then clear these bits. If these bits are not cleared
> then this will cause the comparision of the type settings for an
> existing mapping to fail with that of the new mapping even if the sense
> bit themselves match. The reason being is that the existing type
> settings are read by calling irq_get_trigger_type() which will clear
> any bits outside the sense mask. This will allow us to detect irqchips
> that are not correctly clearing these bits and fix them.
> 
> Signed-off-by: Jon Hunter <jonathanh@xxxxxxxxxx>
> ---
>  kernel/irq/irqdomain.c | 59 ++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 43 insertions(+), 16 deletions(-)
> 
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 3a519a01118b..0ea285baa619 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -549,6 +549,13 @@ static int irq_domain_translate(struct irq_domain *d,
>  				     fwspec->param, fwspec->param_count,
>  				     hwirq, type);
>  
> +	/*
> +	 * WARN if the irqchip returns a type with bits
> +	 * outside the sense mask set and clear these bits.
> +	 */
> +	if (WARN_ON(*type & ~IRQ_TYPE_SENSE_MASK))
> +		*type &= IRQ_TYPE_SENSE_MASK;
> +
>  	/* If domain has no translation, then we assume interrupt line */
>  	*hwirq = fwspec->param[0];
>  	return 0;
> @@ -570,6 +577,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>  {
>  	struct irq_domain *domain;
>  	irq_hw_number_t hwirq;
> +	unsigned int cur_type = IRQ_TYPE_NONE;
>  	unsigned int type = IRQ_TYPE_NONE;
>  	int virq;
>  
> @@ -592,23 +600,42 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>  	if (irq_domain_translate(domain, fwspec, &hwirq, &type))
>  		return 0;
>  
> -	if (irq_domain_is_hierarchy(domain)) {
> -		/*
> -		 * If we've already configured this interrupt,
> -		 * don't do it again, or hell will break loose.
> -		 */
> -		virq = irq_find_mapping(domain, hwirq);
> -		if (virq)
> -			return virq;
> -
> -		virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
> -		if (virq <= 0)
> -			return 0;
> +	/*
> +	 * If we've already configured this interrupt,
> +	 * don't do it again, or hell will break loose.
> +	 */
> +	virq = irq_find_mapping(domain, hwirq);
> +	if (!virq) {
> +		if (irq_domain_is_hierarchy(domain)) {
> +			virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE,
> +						     fwspec);
> +			if (virq <= 0)
> +				return 0;
> +		} else {
> +			/* Create mapping */
> +			virq = irq_create_mapping(domain, hwirq);
> +			if (!virq)
> +				return virq;
> +		}
>  	} else {
> -		/* Create mapping */
> -		virq = irq_create_mapping(domain, hwirq);
> -		if (!virq)
> -			return virq;
> +		cur_type = irq_get_trigger_type(virq);
> +	}
> +
> +	/*
> +	 * If the trigger type is not specified or matches the current
> +	 * trigger type then we are done so return the interrupt number.
> +	 */
> +	if (type == IRQ_TYPE_NONE || type == cur_type)
> +		return virq;
> +
> +	/*
> +	 * If the trigger type is already set and does
> +	 * not match this interrupt, then return 0.
> +	 */
> +	if (cur_type != IRQ_TYPE_NONE) {
> +		pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n",
> +			hwirq, of_node_full_name(to_of_node(fwspec->fwnode)));
> +		return 0;

If we created a new mapping and return here, then this will leak the
mapping. I will fix this. Sorry should have caught this before!

Jon